Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: TraceQL metrics: average over time #4073

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

javiermolinar
Copy link
Contributor

What this PR does:
It implements the avg_over_time aggregation function

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

default:
// Simple addition aggregator. It adds existing values with the new sample.
f = func(existingValue float64, newValue float64) float64 { return existingValue + newValue }
f = func(b *SimpleAggregator, promLabel string, pos int, newValue float64) {
b.ss[promLabel].Values[pos] += newValue
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could also do a Kahan sum here

@@ -1175,12 +1250,21 @@ func (b *SimpleAggregator) Combine(in []*tempopb.TimeSeries) {
}

b.ss[ts.PromLabels] = existing
if b.initAvg {
Copy link
Contributor Author

@javiermolinar javiermolinar Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Struggling a bit with how to include the average in the Combiner. I think this way we allocate the extra bytes only for this case

}

func NewSimpleCombiner(req *tempopb.QueryRangeRequest, op SimpleAggregationOp) *SimpleAggregator {
l := IntervalCount(req.Start, req.End, req.Step)
var initWithNaN bool
var f func(existingValue float64, newValue float64) float64
var f func(b *SimpleAggregator, promLabel string, pos int, newValue float64)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we need to modify more than one value on each iteration (counter and compensation) this does the trick

@mdisibio
Copy link
Contributor

Not sure if I'm reading this right, but it looks like it's computing the average at each level. I was expecting raw mode to return series of sums/counts up the frontend, and then it computes the final average. Without that, it seems to be computing average of averages inside generators and queriers. We can discuss more details like the algorithm, but let's clear the overall approach first.

@javiermolinar javiermolinar marked this pull request as draft September 17, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants