diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fe16f3a8bd..6359cc3ff0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## master / unreleased +* [BUGFIX] Ingester: Fix `cortex_ingester_ingestion_delay_seconds` native histogram losing ~86% of observations by setting `NativeHistogramMinResetDuration` to 1h instead of the bare integer `1` (interpreted as 1ns). #7731 * [FEATURE] Engine: Add `-querier.selector-batch-size` and `-ruler.selector-batch-size` flags to configure series batching in the Thanos promQL engine. 0 disables batching. #7763 * [CHANGE] Querier: Make query time range configurations per-tenant: `query_ingesters_within`, `query_store_after`, and `shuffle_sharding_ingesters_lookback_period`. Uses `model.Duration` instead of `time.Duration` to support serialization but has minimum unit of 1ms (nanoseconds/microseconds not supported). #7160 * [CHANGE] Cache: Setting `-blocks-storage.bucket-store.metadata-cache.bucket-index-content-ttl` to 0 will disable the bucket-index cache. #7446 diff --git a/pkg/ingester/metrics.go b/pkg/ingester/metrics.go index 3ad21faad6d..54a573da1e6 100644 --- a/pkg/ingester/metrics.go +++ b/pkg/ingester/metrics.go @@ -88,6 +88,20 @@ type ingesterMetrics struct { unoptimizedRegexRejectedTotal *prometheus.CounterVec } +// ingestionDelaySecondsHistogramOpts defines the options for the +// cortex_ingester_ingestion_delay_seconds native histogram. NativeHistogramMinResetDuration +// must be a real duration (time.Hour): a bare integer literal is interpreted as +// nanoseconds, which resets the native histogram on essentially every scrape and +// discards the vast majority of observations (see cortexproject/cortex#7731). +var ingestionDelaySecondsHistogramOpts = prometheus.HistogramOpts{ + Name: "cortex_ingester_ingestion_delay_seconds", + Help: "Delay in seconds between sample ingestion time and sample timestamp.", + NativeHistogramBucketFactor: 1.1, + NativeHistogramMaxBucketNumber: 100, + NativeHistogramMinResetDuration: 1 * time.Hour, + Buckets: []float64{1, 5, 10, 30, 60, 120, 300, 600}, // 1s, 5s, 10s, 30s, 1m, 2m, 5m, 10m +} + func newIngesterMetrics(r prometheus.Registerer, createMetricsConflictingWithTSDB bool, activeSeriesEnabled bool, @@ -151,14 +165,7 @@ func newIngesterMetrics(r prometheus.Registerer, NativeHistogramMinResetDuration: 1 * time.Hour, Buckets: prometheus.ExponentialBuckets(1, 2, 10), // 1 to 512 buckets }, []string{"user"}), - ingestionDelaySeconds: promauto.With(r).NewHistogramVec(prometheus.HistogramOpts{ - Name: "cortex_ingester_ingestion_delay_seconds", - Help: "Delay in seconds between sample ingestion time and sample timestamp.", - NativeHistogramBucketFactor: 1.1, - NativeHistogramMaxBucketNumber: 100, - NativeHistogramMinResetDuration: 1, - Buckets: []float64{1, 5, 10, 30, 60, 120, 300, 600}, // 1s, 5s, 10s, 30s, 1m, 2m, 5m, 10m - }, []string{"user"}), + ingestionDelaySeconds: promauto.With(r).NewHistogramVec(ingestionDelaySecondsHistogramOpts, []string{"user"}), oooLabelsTotal: promauto.With(r).NewCounterVec(prometheus.CounterOpts{ Name: "cortex_ingester_out_of_order_labels_total", Help: "The total number of out of order label found per user.", diff --git a/pkg/ingester/metrics_test.go b/pkg/ingester/metrics_test.go index af09d07e2a3..5a43e420b93 100644 --- a/pkg/ingester/metrics_test.go +++ b/pkg/ingester/metrics_test.go @@ -3,6 +3,7 @@ package ingester import ( "bytes" "testing" + "time" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" @@ -1298,3 +1299,14 @@ func populateTSDBMetrics(base float64) *prometheus.Registry { return r } + +// TestIngestionDelaySecondsHistogramResetDuration is a regression test for +// cortexproject/cortex#7731: ingestionDelaySeconds was registered with +// NativeHistogramMinResetDuration: 1, where 1 is interpreted as 1 nanosecond +// (the field is a time.Duration). That reset the native histogram on essentially +// every scrape and discarded ~86% of observations. The reset duration must be a +// real hour-long window. +func TestIngestionDelaySecondsHistogramResetDuration(t *testing.T) { + require.Equal(t, time.Hour, ingestionDelaySecondsHistogramOpts.NativeHistogramMinResetDuration, + "NativeHistogramMinResetDuration must be time.Hour, not a bare integer (interpreted as nanoseconds)") +} diff --git a/pkg/util/request_tracker/request_extractor.go b/pkg/util/request_tracker/request_extractor.go index cbd9f31e8fe..7675d026f8b 100644 --- a/pkg/util/request_tracker/request_extractor.go +++ b/pkg/util/request_tracker/request_extractor.go @@ -83,7 +83,10 @@ func trimStringByBytes(str string, size int) string { bytesStr := []byte(str) trimIndex := len(bytesStr) if size < len(bytesStr) { - for !utf8.RuneStart(bytesStr[size]) { + // Scan backwards to a rune boundary. Bound the scan at size > 0: if the + // string has no rune start (e.g. only UTF-8 continuation bytes) the loop + // must not underflow past zero, which would panic on bytesStr[-1]. + for size > 0 && !utf8.RuneStart(bytesStr[size]) { size-- } trimIndex = size diff --git a/pkg/util/request_tracker/request_tracker_test.go b/pkg/util/request_tracker/request_tracker_test.go index 13ef0a802cd..9842b33ec2f 100644 --- a/pkg/util/request_tracker/request_tracker_test.go +++ b/pkg/util/request_tracker/request_tracker_test.go @@ -191,3 +191,20 @@ func TestRangedQueryExtractorMultiByteTruncation(t *testing.T) { assert.True(t, utf8.Valid(entry), "entry should be valid UTF-8") }) } + +// TestTrimForJsonMarshalContinuationBytes reproduces cortexproject/cortex#7729: +// when the string consists only of UTF-8 continuation bytes (0x80-0xBF) there is +// no rune start to scan back to, so the backwards scan in trimStringByBytes +// underflows past zero and panics with index out of range [-1]. The fix bounds +// the scan at size > 0. +func TestTrimForJsonMarshalContinuationBytes(t *testing.T) { + // 1200 continuation bytes (0x80) with a truncation size below the length. + continuation := strings.Repeat("\x80", 1200) + + require.NotPanics(t, func() { + out := trimForJsonMarshal(continuation, 900) + // Result must always be valid UTF-8 (no partial runes). + assert.True(t, utf8.ValidString(out), "result should be valid UTF-8") + assert.LessOrEqual(t, len(out), len(continuation)) + }) +}