Attribute expressions by aaron-steinfeld · Pull Request #116 · hypertrace/gateway-service

aaron-steinfeld · 2021-12-30T03:06:34Z

Description

Replaced usages of column identifiers throughout with attribute expressions to support subpaths. Sorry for the large PR, difficult to add support piecemeal without breaking things. Requires QS release once hypertrace/query-service#132 is reviewed and merged..
Ref issue : hypertrace/hypertrace-ui#1099

Testing

Existing tests pass and verified E2E.

codecov · 2022-01-04T21:40:49Z

Codecov Report

Merging #116 (307a5d1) into main (913ccfa) will increase coverage by 0.65%.
The diff coverage is 82.28%.

@@             Coverage Diff              @@
##               main     #116      +/-   ##
============================================
+ Coverage     76.95%   77.60%   +0.65%     
- Complexity     1115     1133      +18     
============================================
  Files           106      106              
  Lines          5077     5158      +81     
  Branches        436      426      -10     
============================================
+ Hits           3907     4003      +96     
+ Misses          981      972       -9     
+ Partials        189      183       -6

Flag	Coverage Δ
unit	`77.60% <82.28%> (+0.65%)`	⬆️

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

Impacted Files	Coverage Δ
...hypertrace/gateway/service/GatewayServiceImpl.java	`16.84% <0.00%> (+0.09%)`	⬆️
...g/hypertrace/gateway/service/span/SpanService.java	`9.43% <0.00%> (-0.28%)`	⬇️
...ypertrace/gateway/service/trace/TracesService.java	`11.50% <0.00%> (-0.54%)`	⬇️
.../common/datafetcher/EntityInteractionsFetcher.java	`46.25% <24.24%> (-1.10%)`	⬇️
...rters/EntityServiceAndGatewayServiceConverter.java	`34.92% <63.63%> (+1.31%)`	⬆️
...ice/common/util/MetricAggregationFunctionUtil.java	`65.71% <63.63%> (+7.57%)`	⬆️
...common/converters/QueryAndGatewayDtoConverter.java	`86.94% <75.00%> (-0.52%)`	⬇️
.../gateway/service/common/util/ExpressionReader.java	`78.48% <75.86%> (-7.73%)`	⬇️
...on/datafetcher/EntityDataServiceEntityFetcher.java	`95.00% <89.74%> (+3.00%)`	⬆️
.../common/datafetcher/QueryServiceEntityFetcher.java	`87.81% <96.55%> (+1.62%)`	⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 913ccfa...307a5d1. Read the comment docs.

kotharironak · 2022-01-05T07:25:11Z

+        AttributeExpression.newBuilder().setAttributeId(columnIdentifier.getColumnName());
+
+    Optional.of(columnIdentifier.getAlias())
+        .filter(Predicate.not(String::isEmpty))


Curious Q: Did we observe any issue as prior we were not checking for an empty string? If we are doing this here, do we need to check if not empty in the above function - convertToQueryAttributeExpression (line 295/296)?

On 295, we do this check in a more formal way via hasAlias(). This works because on the new GW AttributeExpression message, alias is optional and thus generates the presence check. On the existing column identifier message being used here, because it is not optional the only way to check if it's set is comparing it to the default value (empty string).

Prior to this change, when going from one col identifier to another, the check wasn't needed more or less for the same reason. The QS version of alias didn't support the presence check leaving no difference between an unset alias and one set to an empty string. Now, if we propagated the empty string col identifier alias into attribute expression, .hasAlias() would incorrectly return true.

A bit long winded, but does that make sense?

Hmm!! Got it.

kotharironak · 2022-01-05T07:32:56Z

+        .orElse(false);
+  }
+
+  private static Optional<String> getSelectionAttributeId(Expression expression) {


should we rename this getSelectionAttributeId -> getAttributeId? The first time, I was reading, I read something to do with the selection clause. What do you think?

What I was trying to convey here (and I think is missing from getAttributeId) is that this only extracts attribute id from a col identifier or attribute expression, rather than an order by, function etc. I had taken to lumping the two of them together in naming as "selection", but I can see how that would be unclear. Any ideas that could better convey that distinction?

I see it's a bit tricky as the two names are very different 'columnIdentifier' and 'attributeExpression'. Not able to come up with a good name so that it is readable in the above context. Maybe any of the below (if not we can use the same name for now).

<get|fetch|extract>AttributeIdFromColumnOrAttributeExpression
getAttributeIdFromAttributeSelection - As we are using, somewhere attributeSelection which refers to Column/AttributeExpression

I buy getAttributeIdFromAttributeSelection - will update to that.

kotharironak · 2022-01-05T09:19:25Z

-            expression ->
-                !mappedEntityIdAttributeIds.contains(
-                    expression.getColumnIdentifier().getColumnName()))
+    entitiesRequest


Curious Q: Why did we remove this filter !mappedEntityIdAttributeIds? Does this mean we will add those selections twice?

The reason it was removed was that we were dropping alias information here - if the caller requested an id attribute by a different alias, the existing implementation was removing that. I had figured that duplicating a selection wouldn't have any significant impact, and wasn't worth the complexity.

That said, in the QS version of the entity fetcher, I believe I ended up with a better solution where I dropped dupe requests and used the aliases at the end to remap them back ( https://github.com/hypertrace/gateway-service/pull/116/files/ea0965d95e98c7caaa83dea5501dc17437362189#diff-11defa5d01d0ff51c801f8ea113e9b9100328b20e0d3073b3faae6bb38c31626R241-R246 ) so let me revisit this and see if that's feasible here.

Followed up with the same solution here.

kotharironak · 2022-01-05T09:20:41Z

+        .map(
+            expression ->
+                Map.entry(
+                    ExpressionReader.getSelectionResultName(expression).orElseThrow(),


Can we rename this as ExpressionReader.getSelectionAttributeId -> ExpressionReader.getAttributeId?

Started discussion in https://github.com/hypertrace/gateway-service/pull/116/files/ea0965d95e98c7caaa83dea5501dc17437362189#r778870420

kotharironak · 2022-01-05T09:35:24Z

+    if (selectionResultNames.containsKey(FROM_ENTITY_ID_ATTRIBUTE_ID)) {
      interaction.putAttribute(
-          FROM_ENTITY_ID_ATTRIBUTE_ID,
+          selectionResultNames.get(FROM_ENTITY_ID_ATTRIBUTE_ID),


Prior, the alias was not used, was that taken care of some other place, or did this change fix the case of not handling alias?

Correct, aliases were not working here before this change (well more accurately, graphql was using attribute IDs as aliases, so they matched).

kotharironak · 2022-01-05T11:09:22Z

   */
-  public EntitiesRequest transformFilter(
-      EntitiesRequest originalRequest, EntitiesRequestContext context) {
+  public EntitiesRequest process(EntitiesRequest originalRequest, EntitiesRequestContext context) {


Q: Any specific reason to rename the method as we have one more down transformFilter for TraceRequest? Are we planning to rename it as well in future?

The reason I renamed it is this wasn't just transforming the filter, it was also modifying other parts of the request (selections), so the name "transformFilter" felt misleading. Since the class is called RequestPreProcessor, "process" seemed an apt name, but open to other suggestions.

In the meantime, will rename the trace version too.

Trace version renamed.

kotharironak · 2022-01-05T11:47:31Z

+
+  private Map<String, AttributeMetadata> getAttributeMetadataByResultName(
+      UpdateEntityRequest updateRequest, UpdateExecutionContext updateExecutionContext) {
+    return updateRequest.getSelectionList().stream()


Looks like AttributeMetadataUtil.remapAttributeMetadataByResultKey can be reused here?

AttributeMetadataUtil.remapAttributeMetadataByResultKey(updateRequest.getSelectionList(), updateExecutionContext.getAttributeMetadata())

Thanks! I had implemented this multiple times before I realized it was identical and moved it to the shared util, must have missed fixing this one up.

kotharironak · 2022-01-05T11:56:04Z

    filterBuilder.setOperator(Operator.AND);
-    List<String> groupByColumns = groupByColumnList(originalRequest);
+    Map<String, Expression> groupBySelectionExpressionsByKey =
+        groupByExpressionByKey(originalRequest);


should we rename groupByExpressionByKey -> groupByExpressionByResultKey and groupBySelectionExpressionsByKey -> groupBySelectionExpressionsByResultKey?

or

groupByExpressionByResultName and groupBySelectionExpressionsByResultName?

Sure, will use result name for consistency.

kotharironak

overall, lgtm. Have only a few minor comments which can be taken care of as follow-up if we are doing.

It seems to me that the test for alias changes are covered in the existing tests - attribute having an alias, not having - both in JSON files and in EnityServiceTest asserts for map change. So, scan one more time that if we haven't missed something.

github-actions · 2022-01-05T18:26:29Z

Unit Test Results

  57 files ±0   57 suites ±0 8s ⏱️ -1s
333 tests ±0 333 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 0753952. ± Comparison against base commit 913ccfa.

aaron-steinfeld added 3 commits December 17, 2021 09:54

chore: temp commit for testing

f17f3b4

fix: last minute cleanup

36e6c80

refactor: remove debug logging

28c513b

aaron-steinfeld requested review from findingrish, kotharironak and sarthak77 December 30, 2021 03:06

aaron-steinfeld added 2 commits January 4, 2022 16:32

Merge remote-tracking branch 'origin/main' into attribute-expressions

3978099

chore: update deps

ea0965d

This comment has been minimized.

Sign in to view

aaron-steinfeld marked this pull request as ready for review January 4, 2022 22:06

aaron-steinfeld requested a review from a team January 4, 2022 22:06

kotharironak reviewed Jan 5, 2022

View reviewed changes

kotharironak previously approved these changes Jan 5, 2022

View reviewed changes

chore: renaming and fixing eds entity fetcher to dedupe

4af437a

aaron-steinfeld dismissed kotharironak’s stale review via 4af437a January 5, 2022 15:28

This comment has been minimized.

Sign in to view

refactor: rename attribute id util

307a5d1

This comment has been minimized.

Sign in to view

kotharironak approved these changes Jan 5, 2022

View reviewed changes

aaron-steinfeld merged commit 0753952 into main Jan 5, 2022

aaron-steinfeld deleted the attribute-expressions branch January 5, 2022 18:24

Conversation

aaron-steinfeld commented Dec 30, 2021 • edited by sarthak77 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Uh oh!

This comment has been minimized.

codecov Bot commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kotharironak Jan 5, 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!

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!

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!

kotharironak Jan 5, 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!

kotharironak left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

github-actions Bot commented Jan 5, 2022

Unit Test Results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aaron-steinfeld commented Dec 30, 2021 •

edited by sarthak77

Loading

codecov Bot commented Jan 4, 2022 •

edited

Loading

kotharironak Jan 5, 2022 •

edited

Loading

kotharironak Jan 5, 2022 •

edited

Loading