Add without aggregator modifier. #1376
Conversation
| groupingKey := model.SignatureForLabels(sample.Metric.Metric, grouping...) | ||
| var withoutMetric metric.Metric | ||
| if without { | ||
| withoutMetric = *sample.Metric.Copy() |
There was a problem hiding this comment.
For by we build the metric in the if condition above. Doing it here for without makes things inconsistent and hard to read.
Also metric.Metric is already the copy-on-write version. So this line can essentially be dropped.
There was a problem hiding this comment.
The problem is we need this version in two places, so I didn't want to duplicate the code.
Dropped the line.
|
|
||
| result := map[uint64]*groupedAggregation{} | ||
|
|
||
| for _, sample := range vec { | ||
| groupingKey := model.SignatureForLabels(sample.Metric.Metric, grouping...) | ||
| withoutMetric := sample.Metric |
This has the advantage that the user doesn't need to list all labels they want to keep (as with "by") but without having to worry about inconsistent labels as when there's only one time series (as with "keeping_common"). Almost all aggregation should use this rather than the existing two options as it's much less error prone and easier to maintain due to not having to always add in "job" plus whatever other common job-level labels you have like "region".
|
👍 |
Add without aggregator modifier.
|
I realize I'm late to the party, but if this does what I think it does I would like to propose calling it |
|
(just putting this out there before this is in a release) |
|
"over" sounds more like what "by" does by default. I'd consider removing labels to be a key part of aggregation. |
This has the advantage that the user doesn't need
to list all labels they want to keep (as with "by")
but without having to worry about inconsistent labels
as when there's only one time series (as with "keeping_common").
Almost all aggregation should use this rather than the existing
two options as it's much less error prone and easier to maintain
due to not having to always add in "job" plus whatever other common
job-level labels you have like "region".
@fabxc In terms of aggregation, this should make us feature complete.