Skip to content

fix(querier): fixes panic when filtering agg metrics from nil resp - #17662

Merged
ashwanthgoli merged 2 commits into
mainfrom
fix-serieshandler-panic
May 16, 2025
Merged

fix(querier): fixes panic when filtering agg metrics from nil resp#17662
ashwanthgoli merged 2 commits into
mainfrom
fix-serieshandler-panic

Conversation

@ashwanthgoli

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

/src/enterprise-logs/vendor/github.com/grafana/loki/v3/pkg/querier/handler.go:58 +0xb55 |  
github.com/grafana/loki/v3/pkg/querier.(*Handler).Do(0xc0028986d8, {0x555c0a8, 0xc000aa7a40}, {0x558c8e0, 0xc0008018f0}) |  
/src/enterprise-logs/vendor/github.com/grafana/loki/v3/pkg/querier/http.go:203 +0x569 |  
github.com/grafana/loki/v3/pkg/querier.(*QuerierAPI).SeriesHandler(_, {_, _}, _) |  
/src/enterprise-logs/vendor/github.com/grafana/loki/v3/pkg/querier/http.go:341 +0x2c |  
github.com/grafana/loki/v3/pkg/querier.(*QuerierAPI).filterAggregatedMetricsFromSeriesResp(0xc0028be370?, 0x0) |  
/usr/local/go/src/runtime/panic.go:792 +0x132

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
@ashwanthgoli
ashwanthgoli requested a review from a team as a code owner May 12, 2025 09:27
@ashwanthgoli ashwanthgoli added type/bug Somehing is not working as expected backport k253 labels May 12, 2025
Comment thread pkg/querier/http.go
@@ -199,7 +199,7 @@ func (q *QuerierAPI) SeriesHandler(ctx context.Context, req *logproto.SeriesRequ
logql.RecordSeriesQueryMetrics(ctx, utillog.Logger, req.Start, req.End, req.Groups, strconv.Itoa(status), req.GetShards(), statResult)

@sandeepsukhani sandeepsukhani May 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not going to block the PR on this, but I think we should return early here with nil check instead of doing the nil check at multiple places. Even better if we know resp would be nil only in case of err, and put an error check above at line 186 or 187 and return early.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we might not be able to return early since the stats reporting happens for failed requests too. I moved this up though to avoid checks at multiple places

@sandeepsukhani sandeepsukhani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

approved it with a non-blocking comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport k253 size/S type/bug Somehing is not working as expected

2 participants