chore | adding the support of edge filters in edges #167

aman-bansal · 2022-11-10T03:19:36Z

No description provided.

github-actions · 2022-11-10T03:22:24Z

Test Results

25 tests ±0 25 ✔️ ±0 13s ⏱️ -3s
11 suites ±0   0 💤 ±0
11 files ±0   0 ❌ ±0

Results for commit 32c4d8e. ± Comparison against base commit cd79b26.

♻️ This comment has been updated with latest results.

codecov · 2022-11-10T03:24:45Z

Codecov Report

Merging #167 (32c4d8e) into main (cd79b26) will decrease coverage by 0.48%.
The diff coverage is 2.00%.

@@             Coverage Diff              @@
##               main     #167      +/-   ##
============================================
- Coverage     22.84%   22.36%   -0.49%     
  Complexity       75       75              
============================================
  Files            65       68       +3     
  Lines          1742     1784      +42     
  Branches         53       54       +1     
============================================
+ Hits            398      399       +1     
- Misses         1335     1376      +41     
  Partials          9        9

Flag	Coverage Δ
unit	`22.36% <2.00%> (-0.49%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...trace/graphql/entity/dao/EdgesDataFetcherImpl.java	`0.00% <0.00%> (ø)`
...phql/entity/dao/GatewayServiceEntityConverter.java	`0.00% <ø> (ø)`
...GatewayServiceEntityInteractionRequestBuilder.java	`0.00% <0.00%> (ø)`
...e/graphql/entity/dao/IncomingEdgesDataFetcher.java	`0.00% <0.00%> (ø)`
...e/graphql/entity/dao/OutgoingEdgesDataFetcher.java	`0.00% <0.00%> (ø)`
...ace/graphql/entity/request/EdgeRequestBuilder.java	`0.00% <0.00%> (ø)`
...phql/entity/joiner/DefaultEntityJoinerBuilder.java	`88.61% <100.00%> (+0.09%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

aaron-steinfeld

Assuming this was not ready to review yet. Please mark it as draft in that case and re-request review when ready.

...l-entity-schema/src/main/java/org/hypertrace/graphql/entity/dao/GatewayServiceEntityDao.java

aaron-steinfeld · 2022-11-15T15:44:59Z

...ql-entity-schema/src/main/java/org/hypertrace/graphql/entity/request/EdgeRequestBuilder.java

      TimeRangeArgument timeRange,
      Optional<String> space,
-      Map<String, Set<SelectedField>> edgesByType,
+      Stream<SelectedField> edgeSetFields,


This looks to be off. There are potentially multiple edge requests that each can have different fields and filters - hence the previous map of type to fields. We're splitting them up later here, but we're not doing the same thing for filters.

...ql-entity-schema/src/main/java/org/hypertrace/graphql/entity/request/EdgeRequestBuilder.java

aaron-steinfeld · 2022-11-15T15:52:16Z

...ql-entity-schema/src/main/java/org/hypertrace/graphql/entity/request/EdgeRequestBuilder.java


+  private List<FilterArgument> getFilters(Set<SelectedField> selectedFields) {
+    return selectedFields.stream()
+        .map(


referred to in other comment, but this is combining the filter arguments from each independent edge request (each field here is one edge request like incomingEdges(type: X)).

This I have already discussed with @skjindal93 . With this PR we are adding two limitations in the case of using edge filters,

If using edge filters, we can only have one entityType incoming/outgoing edge

In the case of different filters for each selected field. They will be merged into one AND filter.

we are introducing this to solve the deliverable first. Immediately after this we will make the changes to remove above two limitations. Currently, no one is using multiple entity types interactions, so it won't break anything. also old behaviour is still intact.

The reason we didnt solve this is because it will need big change in gateway service where we will have to make interactions request as an array of request rather than one single filter which it is right now. https://github.com/hypertrace/gateway-service/blob/main/gateway-service-api/src/main/proto/org/hypertrace/gateway/service/v1/entities.proto#L41 this is the filter i am referring to

I'm not sure I agree here, but we can discuss further.

Currently, no one is using multiple entity types interactions, so it won't break anything

Confused here - the main application flow uses multiple entity types? It shows services with downstream services and downstream backends.

The reason we didnt solve this is because it will need big change in gateway service where we will have to make interactions request as an array of request rather than one single filter which it is right now.

We already handle multiple entity types through a filter, is this different? You'd need to just change how you build the filter to OR across selection types and then AND for each selection type.

Confused here - the main application flow uses multiple entity types? It shows services with downstream services and downstream backends.

It won't break the current workflows. Those api will work. These limitations only come into the picture when filters are being used.

We don't want to leave an api in this broken state though - so if this is going to impact how we implement things, we should be considering that now.

We were just planning to do it in iterations. Have a basic application flow working with limitations (not breaking the existing application flow), and then refactor it next to make it perfect. (because, refactoring would take considerable amount of time otherwise, causing delays for Third Party APIs)

We are definitely planning to pick up the refactor as the next task

It shouldn't require a big refactor though - that's the part that's confusing me. The refactor would just be in the new code that's being written in these 2 PRs, right? I can definitely be mistaken, but aren't we talking a handful of lines (since we already have the infrastructure to separate out the requests then merge them for the existing code)?

If we choose not to however, can we at least have this error out if we receive an unsupported request? My bigger concern is that certain calls will just result in success but have inaccurate results (because filters get merged that shouldn't be).

We already handle multiple entity types through a filter, is this different? You'd need to just change how you build the filter to OR across selection types and then AND for each selection type.

This won't work actually if I am understanding it correctly. Consider this for an example, let's say we query for two edges like this

incomingEdge (scope: SERVICE, filter: {dataType in "dataTypeIds1"}) incomingEdge (scope: API, filter: {dataType in "dataTypeIds2"} }

Now the interaction request to the gateway service will be like this

filter AND [ { fromEntityType in SERVICE, API} { dataType in dataTypeIds1 } OR { dataType in dataTypeIds2 } ]

Now this will return wrong results for both service and api. Since each filter corresponding to the edge is specific to that interaction we cannot do it in a single query. We will need to change the API contract and make gateway service to run each edge query separately. For now, we have made the code error out in case of above-mentioned limitations

Now this will return wrong results for both service and api. Since each filter corresponding to the edge is specific to that interaction we cannot do it in a single query

I was actually suggesting:

filter OR [ {{ fromEntityType = SERVICE} AND { dataType in dataTypeIds1 }} {{ fromEntityType = API} AND { dataType in dataTypeIds2 }} ]

Would that be problematic?

aman-bansal · 2022-11-15T16:03:18Z

Assuming this was not ready to review yet. Please mark it as draft in that case and re-request review when ready.

I don't think i can convert the PR in draft state now. I have added WIP marker on top. Will ask for review once i am done

aaron-steinfeld · 2022-11-15T16:12:15Z

Assuming this was not ready to review yet. Please mark it as draft in that case and re-request review when ready.

I don't think i can convert the PR in draft state now. I have added WIP marker on top. Will ask for review once i am done

It's kind of hidden -

…aphql into aman/add_filter

skjindal93

looks good

...ql-entity-schema/src/main/java/org/hypertrace/graphql/entity/request/EdgeRequestBuilder.java

...-entity-schema/src/main/java/org/hypertrace/graphql/entity/dao/OutgoingEdgesDataFetcher.java

...-entity-schema/src/main/java/org/hypertrace/graphql/entity/dao/IncomingEdgesDataFetcher.java

Co-authored-by: sanket-mundra <[email protected]>

chore | adding the support of edge filters in edges

f370fb5

aman-bansal requested a review from a team as a code owner November 10, 2022 03:19

starting to implement gql query

e99f6b6

aaron-steinfeld reviewed Nov 14, 2022

View reviewed changes

...l-entity-schema/src/main/java/org/hypertrace/graphql/entity/dao/GatewayServiceEntityDao.java Outdated Show resolved Hide resolved

renaming things

75f6247

aaron-steinfeld reviewed Nov 15, 2022

View reviewed changes

aman-bansal changed the title ~~chore | adding the support of edge filters in edges~~ [WIP] chore | adding the support of edge filters in edges Nov 15, 2022

aman-bansal marked this pull request as draft November 15, 2022 16:18

aman-bansal added 4 commits November 17, 2022 11:23

adding the custom data fetcher

e0d1fe0

Merge branch 'main' into aman/add_filter

b59fe7a

refactor a bit

1ccc1f0

Merge branch 'aman/add_filter' of github.com:hypertrace/hypertrace-gr…

db2ef52

…aphql into aman/add_filter

aman-bansal marked this pull request as ready for review November 17, 2022 05:57

aman-bansal added 2 commits November 17, 2022 11:49

erroring out

97f4849

making it pretty

16511fd

skjindal93 reviewed Nov 17, 2022

View reviewed changes

...ql-entity-schema/src/main/java/org/hypertrace/graphql/entity/request/EdgeRequestBuilder.java Outdated Show resolved Hide resolved

aaron-steinfeld reviewed Nov 17, 2022

View reviewed changes

refactor

e2dee52

aman-bansal changed the title ~~[WIP] chore | adding the support of edge filters in edges~~ chore | adding the support of edge filters in edges Nov 18, 2022

adding comment

32c4d8e

skjindal93 approved these changes Nov 18, 2022

View reviewed changes

aman-bansal merged commit 009e884 into main Nov 18, 2022

aman-bansal deleted the aman/add_filter branch November 18, 2022 06:11

skjindal93 pushed a commit that referenced this pull request Jun 21, 2024

Update gradle locks (#167)

c8de1d3

Co-authored-by: sanket-mundra <[email protected]>

chore | adding the support of edge filters in edges #167

chore | adding the support of edge filters in edges #167

Uh oh!

Conversation

aman-bansal commented Nov 10, 2022

Uh oh!

github-actions bot commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

codecov bot commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aaron-steinfeld left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aman-bansal Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aman-bansal commented Nov 15, 2022

Uh oh!

aaron-steinfeld commented Nov 15, 2022

Uh oh!

skjindal93 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 10, 2022 •

edited

Loading

codecov bot commented Nov 10, 2022 •

edited

Loading

aman-bansal Nov 15, 2022 •

edited

Loading