[dbnode] Add rpc/AggregateQuery types #1460
Conversation
| 8: optional TimeType resultTimeType = TimeType.UNIX_SECONDS | ||
| } | ||
|
|
||
| struct AggregateQueryResult { |
There was a problem hiding this comment.
TBH: intentionally avoiding making this a generic type tree. I'm sure we can do aggregation functions in the future but until it's needed it's easier to not introduce the complexity.
| // We currently only support retrieval of facets, but could extend this based on | ||
| // requirements in the future. Currently, the two predominant use-cases are: | ||
| // (1) Given a filter query (optionally), return all known tag keys matching this restriction | ||
| // (2) Given a filter query (optionally), return all know tag key+values matching this restriction |
There was a problem hiding this comment.
Should this take in an optional result filter on keys? As an example, if I'm doing a graphite query for say, foo.bar.*, I only want to list the values matching the restriction for __g2__, rather than for any tags that come after. For graphite we can do something silly like add __g3__ !=~ ".*" to soft-restrict this to the desired output, but that doesn't really translate well to arbitrary tag/values
i.e. with foo.bar.* as a query, and the following metrics:
foo.bar.biz
foo.bar.qux
foo.bar.a.b
the result should be [ "__g2__" : [biz, qux, a] ]
(sorry for using mostly graphite here, it's easier than writing out tags explicitly for the example :p)
There was a problem hiding this comment.
I like it. I'll add something equivalent to resultFilter []string where empty means no filtering
| 3: required i64 rangeEnd | ||
| 4: required string nameSpace | ||
| 5: optional i64 limit | ||
| 6: optional AggregateQueryType aggregateQueryType = AggregateQueryType.AGGREGATE_BY_TAG_NAME_VALUE |
There was a problem hiding this comment.
I don't think this should be optional; would be better for callers to explicitly state what query type is required
There was a problem hiding this comment.
Only reason to make it optional is for the case where people just curl this endpoint to see distinct tags. Seems unnecessary to make them also specify this.
That said, I agree - all the stuff calling this endpoint programmatically should totally be specifying this.
| // AggregateQueryRequest comes from our desire to aggregate on incoming data. | ||
| // We currently only support retrieval of facets, but could extend this based on | ||
| // requirements in the future. Currently, the two predominant use-cases are: | ||
| // (1) Given a filter query (optionally), return all known tag keys matching this restriction |
There was a problem hiding this comment.
This comment is a little incorrect since Query is not an optional param (the no filter case would be something like .* which is still a query, rather than a nil)
There was a problem hiding this comment.
Good catch, made Query optional
Codecov Report
@@ Coverage Diff @@
## master #1460 +/- ##
=========================================
- Coverage 70.9% 59.1% -11.8%
=========================================
Files 840 409 -431
Lines 71773 35273 -36500
=========================================
- Hits 50929 20871 -30058
+ Misses 17518 12892 -4626
+ Partials 3326 1510 -1816
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1460 +/- ##
========================================
+ Coverage 70.9% 70.9% +<.1%
========================================
Files 841 841
Lines 71891 71895 +4
========================================
+ Hits 50981 50988 +7
+ Misses 17562 17561 -1
+ Partials 3348 3346 -2
Continue to review full report at Codecov.
|
prateek
force-pushed
the
prateek/dbnode/aggregate-rpc-iface
branch
from
e35bee0 to
c80d50b
Compare
Only adding the rpc interface for some of the work related to #1453
/cc @arnikola @richardartoul