feat: adds support for group-by queries on the backend #1244
Conversation
size-limit report 📦
|
Codecov Report
@@ Coverage Diff @@
## main #1244 +/- ##
==========================================
- Coverage 69.87% 69.66% -0.20%
==========================================
Files 104 110 +6
Lines 3401 3579 +178
Branches 893 953 +60
==========================================
+ Hits 2376 2493 +117
- Misses 1022 1083 +61
Partials 3 3
Continue to review full report at Codecov.
|
|
also works with rideshare example:
nvm makes sense since its render endpoint |
petethepig
requested review from
kolesnikovae and
eh-am
and removed request for
kolesnikovae
| // TODO: make this a part of the query | ||
| GroupBy string |
There was a problem hiding this comment.
Maybe we should make this a slice, like in the most of query languages. I anticipate that many users will find this useful. Another very handy operator is without, which makes query engine to ignore the specified labels. I'm not sure if we need this (and, especially, in the first version), just a note.
Also, I wonder if we're still going to maintain similarity to PromQL. I think that it may make sense to go our own way. Otherwise, we should follow their semantics (e.g there group by is an aggregation operator and does slightly different things).
There was a problem hiding this comment.
Good points. Will make it a slice in the future, it should be backward-compatible with the HTTP API (e.g groupBy=foo,bar. Same with without, (e.g groupBy=!foo).
Eventually we might add it to flameQL too. I think it's too early for these decisions tho. At this point my goal with this current design is to make it so that we can add all these additional features in the future, but not to actually add them.
webapp/javascript/services/render.ts
Outdated
| @@ -39,6 +39,7 @@ export async function renderSingle( | |||
| z.object({ timeline: TimelineSchema }) | |||
| ) | |||
| .merge(z.object({ telemetry: z.object({}).passthrough().optional() })) | |||
| .merge(z.object({ groups: z.object({}) })) | |||
There was a problem hiding this comment.
This should be optional no?
There was a problem hiding this comment.
Yeah that's a good question. I'm not super sure. I made backend always return something for groups. Is there some other considerations I should be aware of? E.g is this relevant for flamegraph.com?
There was a problem hiding this comment.
Not relevant for flamegraph.com, it only runs here/cloud.
| @@ -108,7 +112,16 @@ func (s *Storage) Get(ctx context.Context, gi *GetInput) (*GetOutput, error) { | |||
| aggregationType = averageAggregationType | |||
| } | |||
|
|
|||
| timelineKey := "*" | |||
There was a problem hiding this comment.
It's for the case when there's no value for a certain key:value pair. E.g if somebody ingested data at app{foo="bar"}, and you're grouping by baz. in this example baz is empty, so the data written to app{foo="bar"} becomes *.
When data is present, e.g when data is written to app{baz="bazz"}, it goes into bazz bucket.
Ideally we should come up with something better than * I think.
To test: