From 2d28b7459964477a9c38de084763644bcdb25611 Mon Sep 17 00:00:00 2001 From: Saumya Shah Date: Thu, 10 Apr 2025 10:53:00 +0530 Subject: [PATCH 1/2] fix: handle analyze returning nil gracefully Signed-off-by: Saumya Shah --- pkg/api/query/grpc.go | 3 +++ pkg/api/query/v1.go | 17 +++++++---------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/api/query/grpc.go b/pkg/api/query/grpc.go index 99f1b8262a..0d58b18485 100644 --- a/pkg/api/query/grpc.go +++ b/pkg/api/query/grpc.go @@ -273,6 +273,9 @@ func extractQueryStats(qry promql.Query) *querypb.QueryStats { } if explQry, ok := qry.(engine.ExplainableQuery); ok { analyze := explQry.Analyze() + if analyze == nil { + return stats + } stats.SamplesTotal = analyze.TotalSamples() stats.PeakSamples = analyze.PeakSamples() } diff --git a/pkg/api/query/v1.go b/pkg/api/query/v1.go index 72e0af5017..22091ce654 100644 --- a/pkg/api/query/v1.go +++ b/pkg/api/query/v1.go @@ -407,13 +407,16 @@ func (qapi *QueryAPI) getQueryExplain(query promql.Query) (*engine.ExplainOutput return eq.Explain(), nil } return nil, &api.ApiError{Typ: api.ErrorBadData, Err: errors.Errorf("Query not explainable")} - } func (qapi *QueryAPI) parseQueryAnalyzeParam(r *http.Request, query promql.Query) (queryTelemetry, error) { if r.FormValue(QueryAnalyzeParam) == "true" || r.FormValue(QueryAnalyzeParam) == "1" { if eq, ok := query.(engine.ExplainableQuery); ok { - return processAnalysis(eq.Analyze()), nil + if analyze := eq.Analyze(); analyze == nil { + return queryTelemetry{}, errors.Errorf("Query: %v not analyzable", query) + } else { + return processAnalysis(analyze), nil + } } return queryTelemetry{}, errors.Errorf("Query not analyzable; change engine to 'thanos'") } @@ -530,7 +533,6 @@ func (qapi *QueryAPI) queryExplain(r *http.Request) (interface{}, []error, *api. var qErr error qry, qErr = qapi.queryCreate.makeInstantQuery(ctx, engineParam, queryable, remoteEndpoints, planOrQuery{query: queryStr}, queryOpts, ts) return qErr - }); err != nil { return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}, func() {} } @@ -638,14 +640,13 @@ func (qapi *QueryAPI) query(r *http.Request) (interface{}, []error, *api.ApiErro var qErr error qry, qErr = qapi.queryCreate.makeInstantQuery(ctx, engineParam, queryable, remoteEndpoints, planOrQuery{query: queryStr}, queryOpts, ts) return qErr - }); err != nil { return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}, func() {} } analysis, err := qapi.parseQueryAnalyzeParam(r, qry) if err != nil { - return nil, nil, apiErr, func() {} + return nil, nil, &api.ApiError{Typ: api.ErrorExec, Err: err}, func() {} } if err := tracing.DoInSpanWithErr(ctx, "query_gate_ismyturn", qapi.gate.Start); err != nil { @@ -813,7 +814,6 @@ func (qapi *QueryAPI) queryRangeExplain(r *http.Request) (interface{}, []error, var qErr error qry, qErr = qapi.queryCreate.makeRangeQuery(ctx, engineParam, queryable, remoteEndpoints, planOrQuery{query: queryStr}, queryOpts, start, end, step) return qErr - }); err != nil { return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}, func() {} } @@ -946,14 +946,13 @@ func (qapi *QueryAPI) queryRange(r *http.Request) (interface{}, []error, *api.Ap var qErr error qry, qErr = qapi.queryCreate.makeRangeQuery(ctx, engineParam, queryable, remoteEndpoints, planOrQuery{query: queryStr}, queryOpts, start, end, step) return qErr - }); err != nil { return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}, func() {} } analysis, err := qapi.parseQueryAnalyzeParam(r, qry) if err != nil { - return nil, nil, apiErr, func() {} + return nil, nil, &api.ApiError{Typ: api.ErrorExec, Err: err}, func() {} } if err := tracing.DoInSpanWithErr(ctx, "query_gate_ismyturn", qapi.gate.Start); err != nil { @@ -964,7 +963,6 @@ func (qapi *QueryAPI) queryRange(r *http.Request) (interface{}, []error, *api.Ap var res *promql.Result tracing.DoInSpan(ctx, "range_query_exec", func(ctx context.Context) { res = qry.Exec(ctx) - }) beforeRange := time.Now() if res.Err != nil { @@ -1145,7 +1143,6 @@ func (qapi *QueryAPI) series(r *http.Request) (interface{}, []error, *api.ApiErr nil, query.NoopSeriesStatsReporter, ).Querier(timestamp.FromTime(start), timestamp.FromTime(end)) - if err != nil { return nil, nil, &api.ApiError{Typ: api.ErrorExec, Err: err}, func() {} } From b8e767b4fd940d2e24312e401733ca503b2e56e6 Mon Sep 17 00:00:00 2001 From: Saumya Shah Date: Thu, 10 Apr 2025 11:36:09 +0530 Subject: [PATCH 2/2] update CHANGELOG.md Signed-off-by: Saumya Shah --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76e28707bb..94fc46f0b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re ### Removed ### Fixed +- [#8199](https://github.com/thanos-io/thanos/pull/8199) Query: handle panics or nil pointer dereference in querier gracefully when query analyze returns nil ## [v0.38.0](https://github.com/thanos-io/thanos/tree/release-0.38) - 03.04.2025