Attribute expressions #128
Conversation
Codecov Report
@@ Coverage Diff @@
## main #128 +/- ##
============================================
- Coverage 23.05% 22.80% -0.25%
Complexity 75 75
============================================
Files 65 65
Lines 1683 1732 +49
Branches 52 51 -1
============================================
+ Hits 388 395 +7
- Misses 1286 1328 +42
Partials 9 9
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.
| @@ -73,13 +76,15 @@ | |||
| GraphQlRequestContext context = request.resultSetRequest().context(); | |||
| return this.requestBuilder | |||
| .buildRequest(request) | |||
| .doOnSuccess(built -> log.warn(JsonFormat.printer().print(built))) | |||
There was a problem hiding this comment.
should we go with debug - log.debug?
There was a problem hiding this comment.
thanks - will strip this, it was only for debugging. Thought I had removed it, but I forgot to check in the removal.
| .subscribeOn(this.boundedIoScheduler) | ||
| .flatMap(serverRequest -> this.fetchAndMapEntities(context, request, serverRequest)); | ||
| } | ||
|
|
||
| private Single<EntityResultSet> fetchAndMapEntities( | ||
| GraphQlRequestContext context, EntityRequest request, EntitiesRequest serverRequest) { | ||
| return this.makeEntityRequest(context, serverRequest) | ||
| .doOnSuccess(built -> log.warn(JsonFormat.printer().print(built))) |
There was a problem hiding this comment.
here also should we move to log.debug?
There was a problem hiding this comment.
removed entirely
| return convertForArgsButNoAlias(attribute, aggregationType, Collections.emptyList()); | ||
| AttributeAssociation<AttributeExpression> attributeExpression, | ||
| AttributeModelMetricAggregationType aggregationType) { | ||
| return convertForArgsButNoAlias(attributeExpression, aggregationType, Collections.emptyList()); |
There was a problem hiding this comment.
while reading I had to look for type, so would attributeAssociation would have been a better argument name?
AttributeAssociation<AttributeExpression> attributeExpression -> AttributeAssociation<AttributeExpression> attributeAssociation?
If, we change, the same thing for appropriate places in other files too. If, we keep the same, it's fine too.
There was a problem hiding this comment.
yeah this naming's tricky. AttributeAssociation is really just a wrapper to materialize an attribute reference on another thing, attribute expression is the kernel of data that's more relevant. So I could call it attributeExpressionAssociation if you think that'd be clearer?
There was a problem hiding this comment.
I think, that would be better - attributeExpressionAssociation
There was a problem hiding this comment.
ok, that likely goes into the core code as well so will do that pass starting there.
There was a problem hiding this comment.
There was a problem hiding this comment.
Maybe there were many files, it was really hard what is what attributeExpressionAssociation or attributeExpression, but, it more clear to me. Thanks a lot for making this change.
Reviewed - hypertrace/hypertrace-core-graphql#92
There was a problem hiding this comment.
Merged and updated in this PR
| } | ||
|
|
||
| private MetricAggregationRequest requestForAggregationField( | ||
| AttributeModel attribute, SelectedField field) { | ||
| AttributeAssociation<AttributeExpression> attributeExpression, SelectedField field) { |
There was a problem hiding this comment.
I still think attributeExpression -> attributeAssociation would be more readable. It is getting confused with actual AttributeExpression and association with the model while reading. What do you think - should we rename?
| BaselinedMetricAggregationContainer metric( | ||
| @GraphQLName(MetricKeyArgument.ARGUMENT_NAME) @GraphQLNonNull String key); | ||
| default BaselinedMetricAggregationContainer metric( | ||
| @GraphQLName(MetricKeyArgument.ARGUMENT_NAME) @Nullable String key, |
There was a problem hiding this comment.
will this @Nullable String key be deprecated later?
There was a problem hiding this comment.
yep, exactly
|
lgtm, minor nit comments |
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.
Description
Add support to GQL for receiving and writing attribute Expressions. Part of hypertrace/hypertrace-ui#1099
Dependent on hypertrace/hypertrace-core-graphql#91(done)Testing
Updated UTs and manual E2E verification
Checklist: