fix: keep late observations out of subsequent collection buffers - #2471
Open
zeitlinger wants to merge 4 commits into
Open
zeitlinger wants to merge 4 commits into
zeitlinger wants to merge 4 commits into
Conversation
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
zeitlinger
requested review from
dhoard,
fstab and
jaydeluca
as code owners
September 15, 2026 11:37
Contributor
Benchmark resultsBenchmark run succeeded for
Prometheus Java Client BenchmarksRun Information
Comparison with base
Results for PR headCounterBenchmark
HistogramBenchmark
HistogramTextFormatBenchmark
TextFormatUtilBenchmark
Allocation per operationJMH GC profiler
Raw ResultsNotes
Benchmark Descriptions
|
jaydeluca
reviewed
Sep 15, 2026
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
jaydeluca
approved these changes
Sep 15, 2026
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Member
Author
|
The reviewed hook plumbing caused a measurable JIT regression: I split the ticket/active-bit hot path from the generation/lock path. The resulting A same-host paired JMH run (JDK 25.0.3, 3 forks, 3x3s warmup, 5x3s measurement, classic histogram methods) improved the reviewed head by +11.3% per-thread and +15.0% single-thread (34,074 vs 30,626 ops/s; 9,278 vs 8,063 ops/s). This is a smaller local sample than CI, so a fresh CI benchmark is still needed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem and proof
A late appender can increment its stripe during collection A, then read
activeGenerationonly after A has completed and collection B has started.B already includes that observation in
expectedCount. If the appender puts it into B'sbuffer, B waits for an observation that will only be replayed after B's wait ends.
This deterministically causes the five-second collection timeout.
The old late-appender test paused after reading the generation, missing this window.
Moving the existing injected pause hook before that read, without the fix, reproduced:
The production hook remains a no-op; the test forces a scheduling window that real threads can hit.
This has the same failure signature as the histogram failures observed in
#2468 and
#2441.
The reproduction proves this race exists, not which interleaving occurred in any particular CI run.
Fix
appendLock.current generation's starting count. It completes through the existing direct observation path.
with no new per-collection allocation or additional fast-path lock.
Regression coverage
Strengthen the late-appender test to force the previously untested window and capture worker
exceptions through futures, with bounded waits and executor cleanup.
Cover both normal collection and a preceding reset. Each case also verifies that a genuinely new
observation buffers normally, stays out of the current snapshot, replays exactly once, and appears
in the following collection.
Validation
BufferTest,HistogramTest, andSummaryTest— 50 tests passed.HistogramTest.testObserveMultithreaded, repeated 50 times with each of-XX:ActiveProcessorCount=1,2, and4— 450 successful test executions on JDK 25.mise run lint:fix— passed; formatter changes retained.mise run build -- -DskipITs=true— passed. Docker-backed integration tests skipped becauseDocker is unavailable locally.
mise run test— full unit-test suite passed../mvnw verify -pl prometheus-metrics-core -am— passed, including coverage checks.git diff --check— passed.Kept separate from the benchmark-only PR.
Current scope after update onto main
The original production fix is already present on
mainthrough #2441 and #2468. This PR now contains the remaining merge-update and review hardening: explicit coverage for both generation handoff windows, fail-closed absolute cutoffs, clearer test seams, and non-masking executor cleanup assertions. The historical race reproduction and validation context above are retained.