Skip to content

[bugfix] fix avg_over_time & compare queries panic with max series #5016

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
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[bugfix] fix avg_over_time & compare queries panic with max series #5016

wants to merge 5 commits into from
+404 −13

Conversation

ie-pham
Copy link
Contributor

@ie-pham ie-pham commented Apr 16, 2025 •
edited
Loading

What this PR does:

  1. Max series PR introduced a bug for avg_over_time metrics queries. The metrics calculation relies on a pair of series per actual value and when the max series limit truncated the series, in some occurrences, one of the pair is no longer in the resulting time series - leading to a panic during aggregations.
  2. Truncating the series caused issues for the frontend because it expects at least one _total series per key (attribute key). This PR add different truncating logic for compare queries.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@ie-pham ie-pham marked this pull request as ready for review April 16, 2025 23:34
mdisibio
mdisibio reviewed Apr 17, 2025
View reviewed changes
modules/frontend/combiner/metrics_query_range.go Outdated
@@ -58,7 +58,9 @@ func NewQueryRange(req *tempopb.QueryRangeRequest, maxSeriesLimit int) (Combiner
if combiner.MaxSeriesReached() {
// Truncating the final response because even if we bail as soon as len(resp.Series) >= maxSeries
// it's possible that the last response pushed us over the max series limit.
resp.Series = resp.Series[:maxSeries]
if len(resp.Series) > maxSeries {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this also need to check && maxSeries > 0 to handle the case of unlimited? Not sure if there is a case where we could get combiner.MaxSeriesReached() and maxSeries==0.

@ie-pham ie-pham added the type/bug Something isn't working label Apr 17, 2025
@ie-pham ie-pham requested a review from mdisibio April 17, 2025 21:42
@ie-pham ie-pham changed the title [bugfix] fix avg_over_time panic with max series [bugfix] fix avg_over_time & compare queries panic with max series Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott is a code owner

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@yvrhdn yvrhdn Awaiting requested review from yvrhdn yvrhdn is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

@electron0zero electron0zero Awaiting requested review from electron0zero electron0zero is a code owner

@stoewer stoewer Awaiting requested review from stoewer stoewer is a code owner

@javiermolinar javiermolinar Awaiting requested review from javiermolinar javiermolinar is a code owner

@carles-grafana carles-grafana Awaiting requested review from carles-grafana carles-grafana is a code owner

@knylander-grafana knylander-grafana Awaiting requested review from knylander-grafana

@mdisibio mdisibio Awaiting requested review from mdisibio mdisibio is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
type/bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ie-pham @mdisibio