-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Enable sharding for avg_over_time. #10373
Enable sharding for avg_over_time. #10373
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Left a nit, but lgtm
@@ -894,6 +903,19 @@ func (r *LogRange) Walk(f WalkFn) { | |||
r.Left.Walk(f) | |||
} | |||
|
|||
// WithoutUnwrap returns a copy of the log range without the unwrap statement. | |||
func (r *LogRange) WithoutUnwrap() (*LogRange, error) { | |||
left, err := Clone(r.Left) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be more extensible if we Clone(self)
then set the unwrap to nil. This ensures that any future fields are propagated through this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it returns and unexpected unwrap
error. I'll take a look tomorrow.
Co-authored-by: Owen Diehl <ow.diehl@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!
pkg/logql/shardmapper_test.go
Outdated
downstream<sum(avg_over_time({job=~"myapps.*"}|="stats"|jsonbusy="utilization"|unwrapbusy[5m])),shard=0_of_2> | ||
++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct? unwrapbusy
missing a space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and jsonbusy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We remove all white spaces in the tests. I've copied the expected string and adapted. Will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
countOverTimeSelector, err := expr.Left.WithoutUnwrap() | ||
if err != nil { | ||
return nil, 0, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for something like avg(avg_over_time({job=~"myapps.*"} |= "stats" | json busy="utilization" | unwrap busy [5m]))
is the unwrap part of the left
? And then the inner avg_over_time
is the right
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean in parsing? It has its own field 🙈
What this PR does / why we need it:
The range aggregation
avg_over_time
is simply shardable if there's no label reduction.If there is one it can be express as
Note that
sum by
,sum_over_time
andcount_over_time
are shardable once more.Checklist
CONTRIBUTING.md
guide (required)CHANGELOG.md
updatedadd-to-release-notes
labeldocs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PR