Schema changes to get entities count for labels #122
Conversation
Codecov Report
@@ Coverage Diff @@
## main #122 +/- ##
=========================================
Coverage 23.03% 23.03%
Complexity 75 75
=========================================
Files 65 65
Lines 1680 1680
Branches 52 52
=========================================
Hits 387 387
Misses 1284 1284
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.
| @Accessors(fluent = true) | ||
| private static class DefaultLabeledEntity implements LabeledEntity { | ||
| String id; | ||
| Map<String, Object> attributeValues; |
There was a problem hiding this comment.
ah crap, I have an enormous change (oops) locally that changes how attributes work
There was a problem hiding this comment.
let me know what changes are coming, and I will update accordingly
There was a problem hiding this comment.
The relevant part here is that AttributeQueryable changes.
Object attribute(@GraphQLName(AttributeExpression.ARGUMENT_NAME) AttributeExpression expression);
and most of the impls are now holding a
Map<AttributeExpression, Object> attributeValues;
Unless I'm able to get stuff merged (and it won't be today), not much you can do to prepare for it, since don't have the new types.
| List<LabeledEntity> labeledEntities = | ||
| Optional.ofNullable(labeledEntitiesMap.get(entityType)).orElse(Collections.emptyList()); | ||
| List<LabeledEntity> labeledEntitiesWithLimit = | ||
| labeledEntities.size() > limit ? labeledEntities.subList(0, limit) : labeledEntities; |
There was a problem hiding this comment.
limits should be enforced on the request side
There was a problem hiding this comment.
An entity may hold multiple labels. I was planning to fetch entities along with labels, and reverse map from labels to entities here. Let me know if there is a better approach.
There was a problem hiding this comment.
I was thinking we'd just create a query for entities filtered by label. Using the label results, you have a list of label IDs which can become an IN filter on a single entity query (per requested type). You're likely right about the limit being enforced on the response side though, now that I think about it. We can either
- Make a request per [label, type] pair and enforce it per request
- Make a request per type, and enforce it in memory when demuxing the result
The second option seems to me like it will be significantly more performant, even if I don't love the idea that we're limiting in memory.
There was a problem hiding this comment.
I was going for second option as well
| @@ -60,5 +66,36 @@ private Label convertLabel(org.hypertrace.label.config.service.v1.Label label) { | |||
| String key; | |||
| String color; | |||
| String description; | |||
| Map<String, List<LabeledEntity>> labeledEntitiesMap; | |||
There was a problem hiding this comment.
would make sense to hold a Map<String, LabeledEntityResultSet> I think
| @Override | ||
| public LabeledEntityResultSet labeledEntities(String entityType, int limit) { | ||
| List<LabeledEntity> labeledEntities = | ||
| Optional.ofNullable(labeledEntitiesMap.get(entityType)).orElse(Collections.emptyList()); |
There was a problem hiding this comment.
If we're doing the work on the request side, this should be unreachable - can assume the map contains any requested entity types.
There was a problem hiding this comment.
Just to return empty values till implementation is done
No description provided.