From d4a7c8f330c3fa85863c6629f514f1c95a30db8f Mon Sep 17 00:00:00 2001 From: Mira Radeva Date: Tue, 26 May 2026 19:20:16 +0000 Subject: [PATCH] roachtest/clusterstats: ignore PromQL info-level annotations CollectPoint and CollectInterval treated any non-empty Prometheus warnings slice as a fatal error. With Prometheus 2.53+, this slice now includes informational annotations like PromQL info: metric might not be a counter, name does not end in _total/_sum/_count/_bucket: "sys_host_disk_write_bytes" which is emitted for queries such as rate(sys_host_disk_write_bytes[1m]) and is purely a naming-convention nudge -- the query result is still valid. After the recent bump to Prometheus 2.53.5 (#170676), this broke admission-control/disk-bandwidth-limiter outright (it t.Fatals on collection errors) and dropped bandwidth samples in admission- control/{index,single-node-index}-backfill, which already log the error and continue. Extract a handlePromWarnings helper that classifies entries by the "PromQL info:" wire-format prefix: - "PromQL info: ..." entries are logged and dropped. - "PromQL warning: ..." entries, and any unprefixed entries (e.g. legacy remote-read warnings, which predate the PromQL annotation system), continue to surface as the same error string as before. Prometheus' annotations package defines exactly two sentinel error types (PromQLInfo and PromQLWarning), so the prefix check is the actual wire-format contract; the client_golang v1 API we use flattens both into a single []string with no structured alternative. Resolves: #170841 See also: #170790, #170793, #170843 Release note: None --- pkg/cmd/roachtest/clusterstats/BUILD.bazel | 1 + pkg/cmd/roachtest/clusterstats/collector.go | 44 ++++++++++-- .../roachtest/clusterstats/collector_test.go | 68 +++++++++++++++++++ 3 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 pkg/cmd/roachtest/clusterstats/collector_test.go diff --git a/pkg/cmd/roachtest/clusterstats/BUILD.bazel b/pkg/cmd/roachtest/clusterstats/BUILD.bazel index e95e91c12df9..5c3069a4830e 100644 --- a/pkg/cmd/roachtest/clusterstats/BUILD.bazel +++ b/pkg/cmd/roachtest/clusterstats/BUILD.bazel @@ -42,6 +42,7 @@ go_library( go_test( name = "clusterstats_test", srcs = [ + "collector_test.go", "exporter_test.go", "streamer_test.go", ":mock_client", # keep diff --git a/pkg/cmd/roachtest/clusterstats/collector.go b/pkg/cmd/roachtest/clusterstats/collector.go index 02504e8ccabe..0f07f09621d6 100644 --- a/pkg/cmd/roachtest/clusterstats/collector.go +++ b/pkg/cmd/roachtest/clusterstats/collector.go @@ -7,6 +7,7 @@ package clusterstats import ( "context" + "strings" "time" "github.com/cockroachdb/cockroach/pkg/roachprod/logger" @@ -95,8 +96,8 @@ func (cs *clusterStatCollector) CollectPoint( if err != nil { return nil, err } - if len(warnings) > 0 { - return nil, errors.Newf("found warnings querying prometheus: %s", warnings) + if err := handlePromWarnings(ctx, l, q, warnings); err != nil { + return nil, err } fromVec := fromVal.(model.Vector) @@ -169,8 +170,8 @@ func (cs *clusterStatCollector) CollectInterval( if err != nil { return nil, err } - if len(warnings) > 0 { - return nil, errors.Newf("found warnings querying prometheus: %s", warnings) + if err := handlePromWarnings(ctx, l, q, warnings); err != nil { + return nil, err } fromMatrixTagged := fromVal.(model.Matrix) @@ -210,3 +211,38 @@ func (cs *clusterStatCollector) CollectInterval( return result, nil } + +// handlePromWarnings classifies the annotations returned alongside a Prometheus +// query result. Prometheus annotations come in two flavors, identified by the +// prefix on the wire: +// +// - "PromQL info: ..." annotations are informational (e.g. a counter-named +// metric lint emitted since Prometheus 2.53). The query result is still +// valid; we log these and continue. +// - "PromQL warning: ..." annotations indicate a likely problem with the +// query (e.g. mismatched histogram operations) where Prometheus may have +// dropped result elements. Any other unrecognized annotation (e.g. legacy +// remote-read warnings, which predate the PromQL annotation system) is +// treated the same way to preserve the prior fail-loud behavior. +// +// The returned error, if any, matches the format used historically so existing +// callers and log scrapers see the same string. +func handlePromWarnings( + ctx context.Context, l *logger.Logger, q string, warnings promv1.Warnings, +) error { + if len(warnings) == 0 { + return nil + } + var serious promv1.Warnings + for _, w := range warnings { + if strings.HasPrefix(w, "PromQL info:") { + l.PrintfCtx(ctx, "prometheus info querying %q: %s", q, w) + continue + } + serious = append(serious, w) + } + if len(serious) == 0 { + return nil + } + return errors.Newf("found warnings querying prometheus: %s", serious) +} diff --git a/pkg/cmd/roachtest/clusterstats/collector_test.go b/pkg/cmd/roachtest/clusterstats/collector_test.go new file mode 100644 index 000000000000..4814154fdc43 --- /dev/null +++ b/pkg/cmd/roachtest/clusterstats/collector_test.go @@ -0,0 +1,68 @@ +// Copyright 2026 The Cockroach Authors. +// +// Use of this software is governed by the CockroachDB Software License +// included in the /LICENSE file. + +package clusterstats + +import ( + "context" + "testing" + + "github.com/cockroachdb/cockroach/pkg/roachprod/logger" + promv1 "github.com/prometheus/client_golang/api/prometheus/v1" + "github.com/stretchr/testify/require" +) + +func TestHandlePromWarnings(t *testing.T) { + const ( + // Sample annotations as emitted by Prometheus 2.53+. + infoNotCounter = `PromQL info: metric might not be a counter, name does not end in _total/_sum/_count/_bucket: "sys_host_disk_write_bytes" (1:6)` + warnGaugeHist = `PromQL warning: rate() applied to gauge histogram has undefined semantics` + legacyRemoteRead = `remote read failed: partial response` + ) + + tests := []struct { + name string + warnings promv1.Warnings + expectedErr string + }{ + { + name: "no warnings", + warnings: nil, + }, + { + name: "only info is silenced", + warnings: promv1.Warnings{infoNotCounter}, + }, + { + name: "warning is still fatal", + warnings: promv1.Warnings{warnGaugeHist}, + expectedErr: "found warnings querying prometheus: [" + warnGaugeHist + "]", + }, + { + name: "unprefixed annotation is treated as warning", + warnings: promv1.Warnings{legacyRemoteRead}, + expectedErr: "found warnings querying prometheus: [" + legacyRemoteRead + "]", + }, + { + name: "info filtered out, warning preserved", + warnings: promv1.Warnings{infoNotCounter, warnGaugeHist}, + expectedErr: "found warnings querying prometheus: [" + warnGaugeHist + "]", + }, + } + + l, err := (&logger.Config{}).NewLogger("") + require.NoError(t, err) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := handlePromWarnings(context.Background(), l, "q", tc.warnings) + if tc.expectedErr == "" { + require.NoError(t, err) + } else { + require.EqualError(t, err, tc.expectedErr) + } + }) + } +}