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.

Sign up for GitHub

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

Jump to bottom

Panic due to unwrap in mapped range vector in query planning #6344

Closed
owen-d opened this issue Jun 8, 2022 · 2 comments · Fixed by #6348
Closed

Panic due to unwrap in mapped range vector in query planning #6344

owen-d opened this issue Jun 8, 2022 · 2 comments · Fixed by #6348
Assignees
@chaudum
Labels
component/logql

Comments

@owen-d
Copy link
Member

owen-d commented Jun 8, 2022

There's a panic @cstyan is looking into in the range-splitting code. When handling a rate(foo | unwrap), we map it to (count_over_time(split1) + count_over_time(split2) ..etc ) / duration, but count_over_time doesn't support unwrap! There's also a little nit that it doesn't support extrapolated rates: https://github.com/grafana/loki/blob/main/pkg/logql/range_vector.go#L236

What if we changed the optimization to the following:
(rate(split1) * split1_duration + rate(split2) * split2_duration ..etc) / total_duration

cc @cyriltovena @slim-bean @ssncferreira @chaudum
@chaudum chaudum self-assigned this Jun 9, 2022
chaudum added a commit that referenced this issue Jun 9, 2022
@chaudum
Fix panic in instant query splitting when using unwrapped rate
3f76172 
The range aggregation `rate()` supports both log ranges and unwrapped
ranges, e.g.

`rate({app="foo"} [$__interval])`
and
`rate({app="foo"} | unwrap bar [$__interval])`

Since `rate()` was split into multiple `count_over_time()` over total
duration, but `count_over_time()` does not support `unwrap`, unwrapped
rate queries caused panics.

This fix changes the splitting of `rate({app="foo"} | unwrap bar [$__interval]`
into multiple `sum_over_time()` over total duration.

Fixes #6344

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum
Copy link
Contributor

chaudum commented Jun 9, 2022

We have the possibility to generically split the rate() to also support rate(foo | unwrap bar):

(rate(split1) * split1_duration ++ rate(split2) * split2_duration ++ ... ++ rate(splitN) * splitN_duration) / total_duration

Or, handle the unwrapped rate aggregation separately and optimize like that:

(sum_over_time(split1) + sum_over_time(split2) ++ ... ++ sum_over_time(splitN)) / total_duration

The latter is more efficient, because we don't need to calculate the rate first and then multiply by the time again.

@chaudum chaudum mentioned this issue Jun 9, 2022
Merged
4 tasks
@chaudum
Copy link
Contributor

chaudum commented Jun 9, 2022

@owen-d can you elaborate on what you mean with:

There's also a little nit that it doesn't support extrapolated rates: https://github.com/grafana/loki/blob/main/pkg/logql/range_vector.go#L236

@chaudum chaudum mentioned this issue Jun 9, 2022
Open
chaudum added a commit that referenced this issue Jun 19, 2022
@chaudum
Fix panic in instant query splitting when using unwrapped rate
2c5931e 
The range aggregation `rate()` supports both log ranges and unwrapped
ranges, e.g.

`rate({app="foo"} [$__interval])`
and
`rate({app="foo"} | unwrap bar [$__interval])`

Since `rate()` was split into multiple `count_over_time()` over total
duration, but `count_over_time()` does not support `unwrap`, unwrapped
rate queries caused panics.

This fix changes the splitting of `rate({app="foo"} | unwrap bar [$__interval]`
into multiple `sum_over_time()` over total duration.

Fixes #6344

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
slim-bean pushed a commit that referenced this issue Jun 22, 2022
@chaudum @ssncferreira
Fix panic in instant query splitting when using unwrapped rate (#6348)
798677a 
* Fix panic in instant query splitting when using unwrapped rate

The range aggregation `rate()` supports both log ranges and unwrapped
ranges, e.g.

`rate({app="foo"} [$__interval])`
and
`rate({app="foo"} | unwrap bar [$__interval])`

Since `rate()` was split into multiple `count_over_time()` over total
duration, but `count_over_time()` does not support `unwrap`, unwrapped
rate queries caused panics.

This fix changes the splitting of `rate({app="foo"} | unwrap bar [$__interval]`
into multiple `sum_over_time()` over total duration.

Fixes #6344

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

* Add tests

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

* Integrate review feedback

Co-authored-by: Susana Ferreira <susana.ferreira@grafana.com>

Co-authored-by: Susana Ferreira <susana.ferreira@grafana.com>
grafanabot pushed a commit that referenced this issue Jun 30, 2022
@chaudum @grafanabot
Fix panic in instant query splitting when using unwrapped rate (#6348)
f24c9bd 
* Fix panic in instant query splitting when using unwrapped rate

The range aggregation `rate()` supports both log ranges and unwrapped
ranges, e.g.

`rate({app="foo"} [$__interval])`
and
`rate({app="foo"} | unwrap bar [$__interval])`

Since `rate()` was split into multiple `count_over_time()` over total
duration, but `count_over_time()` does not support `unwrap`, unwrapped
rate queries caused panics.

This fix changes the splitting of `rate({app="foo"} | unwrap bar [$__interval]`
into multiple `sum_over_time()` over total duration.

Fixes #6344

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

* Add tests

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

* Integrate review feedback

Co-authored-by: Susana Ferreira <susana.ferreira@grafana.com>

Co-authored-by: Susana Ferreira <susana.ferreira@grafana.com>
(cherry picked from commit 798677a)
ssncferreira pushed a commit that referenced this issue Jun 30, 2022
@grafanabot @chaudum
Fix panic in instant query splitting when using unwrapped rate (#6348) (
df8edd6 
#6557)

* Fix panic in instant query splitting when using unwrapped rate

The range aggregation `rate()` supports both log ranges and unwrapped
ranges, e.g.

`rate({app="foo"} [$__interval])`
and
`rate({app="foo"} | unwrap bar [$__interval])`

Since `rate()` was split into multiple `count_over_time()` over total
duration, but `count_over_time()` does not support `unwrap`, unwrapped
rate queries caused panics.

This fix changes the splitting of `rate({app="foo"} | unwrap bar [$__interval]`
into multiple `sum_over_time()` over total duration.

Fixes #6344

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

* Add tests

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

* Integrate review feedback

Co-authored-by: Susana Ferreira <susana.ferreira@grafana.com>

Co-authored-by: Susana Ferreira <susana.ferreira@grafana.com>
(cherry picked from commit 798677a)

Co-authored-by: Christian Haudum <christian.haudum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chaudum chaudum

Labels
component/logql
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix panic in instant query splitting when using unwrapped rate grafana/loki
2 participants
@chaudum @owen-d