Add a way to control the aggregation type for the SelectSeries API #2758
Conversation
pkg/model/series.go
Outdated
| } | ||
| return j + 1 | ||
| } | ||
|
|
||
| type TimeSeriesAggregator interface { |
There was a problem hiding this comment.
the name could be a bit confusing since we also have
I think their purpose is slightly different but putting that in the name is hard. Any thoughts @kolesnikovae?
There was a problem hiding this comment.
I don't have a strong opinion as these types are not supposed to be used in the same context. The key difference is that distributor/aggregator operates on real-time data streams, therefore we could call it e.g. StreamAggregator
pkg/model/series.go
Outdated
| } | ||
| return j + 1 | ||
| } | ||
|
|
||
| type TimeSeriesAggregator interface { |
There was a problem hiding this comment.
I don't have a strong opinion as these types are not supposed to be used in the same context. The key difference is that distributor/aggregator operates on real-time data streams, therefore we could call it e.g. StreamAggregator
pkg/model/series.go
Outdated
| func (a *avgTimeSeriesAggregator) Add(ts int64, value float64) { | ||
| a.ts = ts | ||
| a.sum += value | ||
| a.count++ | ||
| } |
There was a problem hiding this comment.
I'd like to make sure I fully understand the aggregation and its relation to grouping. I'm sorry if I got it wrong :)
Given two series (with just one value each):
{pod=a, endpoint=x}: { ts_1, 8 }
{pod=a, endpoint=y}: { ts_1, 4 }
Given a query avg (group) by pod. What would be the result: {pod=a} 12 or {pod=a} 6?
My understanding is that the aggregator gets two values at input (from seriesBuilder):
{pod=a}: { ts_1, 4 }, { ts_1, 8 }
Which then will be averaged and the result will be 6. I'm not very sure this is what users expect (despite it is literally the average value)
There was a problem hiding this comment.
It will be {pod=a} 6 and I am not sure either though summing will often make no sense as well, depending on the profile type. There are two aspects to this aggregation:
- Reducing samples for the exact same timestamp (happens in series.go, previously either "first" or "sum")
- Reducing samples within a step interval (happens in querier.go, previously always "sum")
Honestly I am not sure we need 1. Leaving duplicate samples in and letting 2 handle the aggregation would produce more reliable results (currently in 2 we will do avg of avg at times).
There was a problem hiding this comment.
Totally agree we should only do 2 to avoid avg of avg. May be only when it's not a sum.
aleks-p
force-pushed
the
feat/select-series-control-aggregation
branch
from
71f4b36
to
3d7c0de
Compare
|
I've simplified things a bit:
This shouldn't change the behavior for sums, but will make averages a bit better (no avg of avg). For the frontend part, I will hide the dropdown for now and have it make the decision for "avg" or "sum" behind the scenes. |
- perform only sum aggregation in series merger - retain duplicate samples in series merger (when sum=false) - remove "first value" aggregator
aleks-p
force-pushed
the
feat/select-series-control-aggregation
branch
from
3d7c0de
to
cc46b5f
Compare
Adds an optional "aggregation" param to the SelectSeries API, allowing to switch between "sum" (the default), "avg" and "first" (used mostly internally). Other aggregation functions can be added in the future. Right now it only impacts the time series, but I will see how much work it would be (and how expensive it is) to apply it to the flamegraph as well.
A PR in Grafana will follow where we expose this in the Explore view like this: