feat: Add spanId attribute for span-log join query #60
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #60 +/- ##
=========================================
Coverage 60.86% 60.86%
Complexity 233 233
=========================================
Files 78 78
Lines 1183 1183
Branches 33 33
=========================================
Hits 720 720
Misses 435 435
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
...an-schema/src/main/java/org/hypertrace/core/graphql/span/dao/SpanLogEventRequestBuilder.java
Outdated
Show resolved
Hide resolved
| .concatMapSingle( | ||
| logEventsResponseVar -> this.convert(attributeRequests, logEventsResponseVar)) | ||
| .collect(Collectors.groupingBy(logEvent -> (String) logEvent.attribute(foreignIdAttribute))) | ||
| logEventsResponseVar -> |
There was a problem hiding this comment.
What's the motivation for the change here? Since we were assuming the attribute existed in the response map and now we're guaranteeing it, the old logic would have also worked, right?
There was a problem hiding this comment.
If the original request for logs didn't have the spanId attribute we shouldn't return it back either, I guess. So, this part needed changes to filter out the spanId returned if it wasn't requested.
There was a problem hiding this comment.
based on the way the response object is designed, it's safe to return extra data (because it's not a programmatic access but instead via the schema, we already know at request time what values of the map will be read by the serializer - that's how we generated the requested attributes)
There was a problem hiding this comment.
yeah right spanId filtering would be automatically done, the problem was building the mapping of spanId -> List<LogEvent>. Currently the code was first converting LogEvent (gateway) -> LogEvent (gql) and then mapping spanId to logEvents, but now it is possible that spanId attribute would be lost after conversion.
There was a problem hiding this comment.
got it - because the attribute map conversion is based on the requested rather than returned data, and we don't save the extra attribute into the request. makes sense.
...an-schema/src/main/java/org/hypertrace/core/graphql/span/dao/SpanLogEventRequestBuilder.java
Outdated
Show resolved
Hide resolved
...an-schema/src/main/java/org/hypertrace/core/graphql/span/dao/SpanLogEventRequestBuilder.java
Outdated
Show resolved
Hide resolved
...an-schema/src/main/java/org/hypertrace/core/graphql/span/dao/SpanLogEventRequestBuilder.java
Outdated
Show resolved
Hide resolved
...schema/src/main/java/org/hypertrace/core/graphql/span/dao/SpanLogEventResponseConverter.java
Outdated
Show resolved
Hide resolved
...schema/src/main/java/org/hypertrace/core/graphql/span/dao/SpanLogEventResponseConverter.java
Outdated
Show resolved
Hide resolved
...chema/src/test/java/org/hypertrace/core/graphql/span/dao/SpanLogEventRequestBuilderTest.java
Outdated
Show resolved
Hide resolved
...chema/src/test/java/org/hypertrace/core/graphql/span/dao/SpanLogEventRequestBuilderTest.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
| .concatMapSingle( | ||
| logEventsResponseVar -> this.convert(attributeRequests, logEventsResponseVar)) | ||
| .collect(Collectors.groupingBy(logEvent -> (String) logEvent.attribute(foreignIdAttribute))) | ||
| logEventsResponseVar -> |
There was a problem hiding this comment.
got it - because the attribute map conversion is based on the requested rather than returned data, and we don't save the extra attribute into the request. makes sense.
hypertrace/hypertrace#224