Repository navigation
Added support to query labels on entity schema - #101
Conversation
Codecov Report
@@ Coverage Diff @@
## main #101 +/- ##
============================================
- Coverage 22.22% 21.67% -0.56%
Complexity 67 67
============================================
Files 64 64
Lines 1615 1661 +46
Branches 50 54 +4
============================================
+ Hits 359 360 +1
- Misses 1248 1293 +45
Partials 8 8
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| EntityRequest request, | ||
| EntitiesResponse response, | ||
| BaselineEntitiesResponse baselineResponse, | ||
| Map<String, String> labelsMap) { |
There was a problem hiding this comment.
I am guessing the value in the map is the label key. Instead, the value in the map should be the entire server Label response
There was a problem hiding this comment.
it is to map. Since the response is not per entity, I had converted it once and passed it as an argument.
There was a problem hiding this comment.
anyways the logic is now moved to converter
| return EMPTY_LABEL_RESULT_SET; | ||
| } | ||
| Value value = | ||
| entity.getAttributeOrDefault(request.labelsAttributeRequest().attribute().id(), null); |
There was a problem hiding this comment.
Just wondering, should we use something like
A similar example case happens for tying up log event with a span https://github.com/hypertrace/hypertrace-core-graphql/blob/e6a3738f459a45bd45eb73ebd8f326de23dac2d0/hypertrace-core-graphql-span-schema/src/main/java/org/hypertrace/core/graphql/span/dao/SpanLogEventResponseConverter.java#L41
There was a problem hiding this comment.
as we discussed this is probably not needed
| org.hypertrace.gateway.service.v1.entity.Entity entity, | ||
| EntityRequest request, | ||
| Map<String, String> labelsMap) { | ||
| if (labelsMap.isEmpty()) { |
There was a problem hiding this comment.
I don't think there is a need for this check anyways, since it would be automatically taken care of as we are streaming labelsMap on L151
There was a problem hiding this comment.
to return a static empty result set instead of creating an empty result set each time.
This comment has been minimized.
This comment has been minimized.
skjindal93
left a comment
skjindal93
left a comment
There was a problem hiding this comment.
looks good. Can you please unit tests mostly for the request and response converters?
This comment has been minimized.
This comment has been minimized.
| EntityRequest request, | ||
| EntitiesResponse response, | ||
| BaselineEntitiesResponse baselineResponse, | ||
| Optional<GetLabelsResponse> labelsResponse) { |
There was a problem hiding this comment.
Usually you don't want to pass optionals as arguments (more of a return mechanism, since an optional argument is better represented by an overload). Sometimes it can be a pain, but here we could just as easily provide an empty response.
| implementation("org.hypertrace.core.graphql:hypertrace-core-graphql-attribute-store") | ||
| implementation("org.hypertrace.core.graphql:hypertrace-core-graphql-deserialization") | ||
| implementation("org.hypertrace.core.graphql:hypertrace-core-graphql-rx-utils") | ||
| implementation("org.hypertrace.config.service:labels-config-service-api") |
| labelResultSet)); | ||
| } | ||
|
|
||
| private LabelResultSet buildLabelResultSet( |
There was a problem hiding this comment.
This seems like repetition with the label's own response building - the labels in an entity response are no different than the stand alone label api. The responsibility for building labels should be delegated to the existing label converter (LabelResponseConverter should implemented the Converter interface and bind it so this service can inject a Converter<GetLabelsResponse, LabelResultSet>.
The real difference comes in the fact that you want to split up that result set and map it back to entities that requested each label. That use case is identical to what we do with entity joining, so looking at it may be helpful - it collects the entity IDs from an arbitrary source, fetches them together, then maps the result back on to the original requests.
Anyway, no matter how you do it - the label schema should be responsible for everything from building the label request all the way through mapping it back to the graphql label object. The cardinal rule we're breaking here is that no other schema should be aware of the underlying implementation of labels (that is, implementation("org.hypertrace.config.service:labels-config-service-api") cannot be a dependency of this schema).
There was a problem hiding this comment.
Let me look in to this. We didn't go joiner route as we are not fetching label ids based on entities labels. We are fetching all and then filtering based on respective entity's labels.
There was a problem hiding this comment.
server-side vs in-memory filtering is a bit of an implementation detail - it's more about separating out the actual join glue (and deduping it since we'll want to join in other spots like explorer, too). Anyway, the big thing is keeping the server side of labels encapsulated within the label schema.
| serviceConfig.getGatewayServiceHost(), serviceConfig.getGatewayServicePort())) | ||
| .withCallCredentials(credentials); | ||
|
|
||
| this.labelConfigServiceStub = |
There was a problem hiding this comment.
commented above - delegate to the other schema, rather than implementing a separate dependency to the label service.
| EdgeSetGroupRequest incomingEdgeRequests = new EmptyEdgeSetGroupRequest(); | ||
| EdgeSetGroupRequest outgoingEdgeRequests = new EmptyEdgeSetGroupRequest(); | ||
| boolean includeInactive = true; // When joining we want the entity regardless of time range | ||
| Optional<LabelRequest> labelRequest = Optional.empty(); |
There was a problem hiding this comment.
labels can't be fetched here?
There was a problem hiding this comment.
Let me pick it up in next PR
| return this.argumentDeserializer.deserializePrimitive(arguments, SpaceArgument.class); | ||
| } | ||
|
|
||
| private Optional<LabelRequest> buildLabelRequest( |
There was a problem hiding this comment.
would suggest pulling this outside - a boolean that turns into an optional empty could just as easily (and with more clarity) be done above.
| false, | ||
| false)); | ||
| false, | ||
| Optional.empty())); |
There was a problem hiding this comment.
also should be supported here.
There was a problem hiding this comment.
Will pick it up in next PR
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
aaron-steinfeld
left a comment
aaron-steinfeld
left a comment
There was a problem hiding this comment.
mostly nits - only two things I think we need to address before merge is logging the errors.
This comment has been minimized.
This comment has been minimized.
| return entity -> { | ||
| Value labelAttributeValue = | ||
| entity.getAttributeOrDefault( | ||
| request.labelRequest().get().labelIdArrayAttributeRequest().attribute().id(), null); |
There was a problem hiding this comment.
nit: this is the same behavior as just a regular .get
aaron-steinfeld
left a comment
aaron-steinfeld
left a comment
There was a problem hiding this comment.
Looks like I can't even remove my own approval now without requesting changes. great job github. Code looks fine, but can we get tests added for the new stuff?
|
Will add tests in a separate PR |
No description provided.