Ingester: fix cortex_ingester_ingestion_delay_seconds losing most observations - #7744
Conversation
…ervations The histogram was registered with NativeHistogramMinResetDuration: 1, an untyped constant that Go implicitly converts to time.Duration(1), i.e. 1 nanosecond, instead of the intended 1 hour used by every other native histogram in this file. When the native histogram's bucket count exceeds NativeHistogramMaxBucketNumber (100), client_golang's limitBuckets() first tries maybeReset(), which fully resets the histogram (wiping both native and classic bucket counts, keeping only the latest observation) if at least NativeHistogramMinResetDuration has elapsed since the last reset. With an effectively-zero duration, that condition is satisfied on virtually every call, so instead of gracefully reducing resolution (bucket width doubling / zero bucket widening), the histogram repeatedly self-resets and silently drops the large majority of observations. In a 100k-sample simulation this loses ~86% of observations, corrupting both _count/_sum and the classic le="600" bucket that operators alert on for ingestion lag. Fix it to 1 * time.Hour, matching every other histogram in this file. Fixes cortexproject#7731 Signed-off-by: ankit090701 <ankitanku090701@gmail.com>
Signed-off-by: ankit090701 <ankitanku090701@gmail.com>
| * [BUGFIX] Ring: Fix DynamoDB KV CAS not retrying on transactional conditional check failures. `TransactWriteItems` reports condition failures as `TransactionCanceledException` with a `ConditionalCheckFailed` cancellation reason, which was not recognized as retryable, so any concurrent ring update conflict (e.g. many ingesters joining during a rolling update) failed immediately instead of re-reading and retrying. `TransactionConflict` cancellation reasons are also treated as retryable. #7706 | ||
| * [BUGFIX] Distributor: Return HTTP 499 (Client Closed Request) instead of 500 when a remote-write or OTLP push is canceled by the client, so client-side cancellations are no longer counted as server-side errors. #7717 | ||
| * [BUGFIX] Querier: Fix gRPC `codes.Canceled` errors being mapped to HTTP 500 instead of 499 when a client cancels a query. #7738 | ||
| * [BUGFIX] Ingester: Fix `cortex_ingester_ingestion_delay_seconds` losing the large majority of observations. `NativeHistogramMinResetDuration` was set to an untyped `1` (1 nanosecond) instead of `1 * time.Hour`, causing the native histogram to fully reset instead of gracefully reducing resolution every time it exceeded its bucket limit. #7744 |
There was a problem hiding this comment.
I think we can skip it. The PR that introduced this bug is not released yet.
| // so instead of gracefully reducing resolution (bucket width doubling / zero | ||
| // bucket widening), the histogram silently drops the vast majority of prior | ||
| // observations on every bucket-limit breach. | ||
| func TestIngestionDelaySecondsHistogram_DoesNotLoseObservationsOnNativeBucketLimit(t *testing.T) { |
There was a problem hiding this comment.
Umm do we need this test? I think it is an obvious bug when defining the metric so maybe fine to omit it.
There was a problem hiding this comment.
Fair to question — happy to drop it if you'd still rather not have it after this. My case for keeping it: the typo itself (1 vs 1 * time.Hour) is obvious once you're looking right at it, but the consequence isn't — I didn't expect "wrong reset duration" to mean "the histogram fully resets and throws away 86% of observations on every bucket-limit breach" until I actually read maybeReset() in the vendored client. Without a test pinning that behavior, a future refactor of this metric block (or someone copying it as a template for a new histogram, which is how the other ~60 correct instances of 1 * time.Hour presumably multiplied in the first place) could reintroduce the exact same bug and nothing would catch it — the type system doesn't, since an untyped 1 silently converts to time.Duration(1).
That said, it's a genuinely fine call to make either way, and I don't want to hold up the PR over it — let me know and I'll remove it if you still think it's not worth keeping.
- Drop the CHANGELOG entry: the bug was introduced in cortexproject#7443, which was merged after the latest release (v1.21.1, 2026-06-04) and has never shipped, so there's nothing for users to be informed about fixing. - Fix check-modernize lint failure: use `for range numObservations` instead of `for i := 0; i < numObservations; i++` in the new test, since the loop index was never used. Signed-off-by: ankit090701 <ankitanku090701@gmail.com>
SungJin1212
left a comment
SungJin1212
left a comment
There was a problem hiding this comment.
Thanks for fixing it
I also don't think the test is necessary, but either way I'm fine.
3 participants
What this PR does:
cortex_ingester_ingestion_delay_seconds(added in #7443) is registered withNativeHistogramMinResetDuration: 1. Since the field is atime.Duration, the untyped constant1means 1 nanosecond, not 1 hour — every other native histogram inpkg/ingester/metrics.go(and repo-wide, ~60 other occurrences) usestime.Hour.Root cause, verified by reading the vendored
client_golangsource (vendor/github.com/prometheus/client_golang/prometheus/histogram.go):NativeHistogramMaxBucketNumber(100 here),limitBuckets()callsmaybeReset()first.maybeReset()fully resets the histogram (resetCounts()zeroessumBits,count, and all classic bucket counts, plus native buckets — it keeps only the single latest observation) ifnow - lastResetTime >= NativeHistogramMinResetDuration.This is not confined to the native representation:
resetCounts()also zeroes the classicBucketscounts, sohistogram_quantile()over the classic buckets and_count/_sumare equally corrupted.Fix: change
NativeHistogramMinResetDuration: 1toNativeHistogramMinResetDuration: 1 * time.Hour, matching every other histogram in the file.Testing:
Added
TestIngestionDelaySecondsHistogram_DoesNotLoseObservationsOnNativeBucketLimitinpkg/ingester/metrics_test.go, which observes 500 widely-spread values (forcing the native bucket count past the 100 limit) into the realingesterMetrics.ingestionDelaySecondsbuilt bynewIngesterMetrics, then asserts the histogram'sSampleCountstill equals 500.NativeHistogramMinResetDuration: 1): the test fails,SampleCountgets stuck at 100 (repeated full resets right at the bucket-limit boundary) instead of 500.SampleCountis exactly 500 (bucket count is reduced by merging/widening, not by discarding samples).I verified this by reverting only the one-line fix (keeping the new test) with
git stashand confirming the test fails exactly as described, then restoring the fix and confirming it passes.go vet ./pkg/ingester/...is clean and the fullpkg/ingestersuite (go test -tags "netgo slicelabels" ./pkg/ingester/...) passes.Which issue(s) this PR fixes:
Fixes #7731
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags(No user-facing flags/config changed, so no
v1-guarantees.mdupdate needed. Per @yeya24's review, the CHANGELOG entry was removed: #7443, which introduced this metric, hasn't shipped in a release yet (latest release v1.21.1 predates it), so there's nothing for a "bugfix" note to inform users about.)