fix: keep late observations out of subsequent collection buffers - #2471
Merged
Merged
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. |
zeitlinger
pushed a commit
that referenced
this pull request
Sep 16, 2026
🤖 I have created a release *beep* *boop* --- ## [1.9.0](v1.8.0...v1.9.0) (2026-09-16) ### Features * support metric name filtering in OpenTelemetry exporter ([#2344](#2344)) ([9b0ede8](9b0ede8)) ### Bug Fixes * avoid protobuf debug reflection in native images ([#2251](#2251)) ([7f899e7](7f899e7)) * bound HTTPServer request resources ([#2333](#2333)) ([33ec556](33ec556)) * bound observation buffering during collection ([#2336](#2336)) ([43788f5](43788f5)) * bound scrape query parameters ([#2334](#2334)) ([27e1912](27e1912)) * **ci:** skip benchmark report for skipped runs ([#2422](#2422)) ([40eddb0](40eddb0)) * clarify benchmark regression report verdicts ([#2394](#2394)) ([e5fa067](e5fa067)) * **deps:** update dependency com.google.guava:guava to v33.7.0-jre ([#2387](#2387)) ([bf0db49](bf0db49)) * **deps:** update dependency io.dropwizard.metrics:metrics-core to v4.2.40 ([#2432](#2432)) ([dd88326](dd88326)) * **deps:** update dependency io.dropwizard.metrics5:metrics-core to v5.0.8 ([#2433](#2433)) ([42f3c8a](42f3c8a)) * **deps:** update dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha to v2.29.0-alpha ([#2235](#2235)) ([cf9f702](cf9f702)) * **deps:** update dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha to v2.30.0-alpha ([#2328](#2328)) ([1ca2716](1ca2716)) * **deps:** update dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha to v2.30.0-alpha ([#2330](#2330)) ([07623c1](07623c1)) * **deps:** update dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha to v2.31.0-alpha ([#2401](#2401)) ([6c26619](6c26619)) * **deps:** update dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha to v2.31.0-alpha ([#2402](#2402)) ([ac0d68a](ac0d68a)) * **deps:** update dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha to v2.31.1-alpha ([#2409](#2409)) ([5eea652](5eea652)) * **deps:** update dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha to v2.31.1-alpha ([#2410](#2410)) ([0bcef89](0bcef89)) * **deps:** update dependency org.apache.tomcat.embed:tomcat-embed-core to v11.0.23 ([#2241](#2241)) ([a017f80](a017f80)) * **deps:** update dependency org.apache.tomcat.embed:tomcat-embed-core to v11.0.24 ([#2294](#2294)) ([63967bd](63967bd)) * **deps:** update dependency org.apache.tomcat.embed:tomcat-embed-core to v11.0.25 ([#2389](#2389)) ([92f8344](92f8344)) * **deps:** update dependency org.apache.tomcat.embed:tomcat-embed-core to v11.0.26 ([#2477](#2477)) ([05146c0](05146c0)) * **deps:** update dependency org.springframework.boot:spring-boot-starter-parent to v4.1.1 ([#2399](#2399)) ([a0b0880](a0b0880)) * **deps:** update jetty monorepo to v12.1.11 ([#2279](#2279)) ([4dc54da](4dc54da)) * **deps:** update jetty monorepo to v12.1.12 ([#2371](#2371)) ([08967e0](08967e0)) * **deps:** update jetty monorepo to v12.1.13 ([#2459](#2459)) ([b217f05](b217f05)) * **deps:** update junit-framework monorepo to v6.1.2 ([#2300](#2300)) ([5966d1d](5966d1d)) * **deps:** update junit-framework monorepo to v6.1.3 ([#2374](#2374)) ([d1ade52](d1ade52)) * **deps:** update otel.instrumentation.version ([#2236](#2236)) ([158230d](158230d)) * **deps:** update protobuf ([#2400](#2400)) ([e2db1ed](e2db1ed)) * **deps:** update protobuf ([#2438](#2438)) ([8ad6fa8](8ad6fa8)) * **deps:** update protobuf to v4.35.1 ([#2221](#2221)) ([cf17073](cf17073)) * disable micrometer compat build cache ([#2457](#2457)) ([6a40eda](6a40eda)) * drop +Inf bound from OpenTelemetry classic histogram boundaries ([#2458](#2458)) ([a3bce9a](a3bce9a)) * **exposition:** export internal package for OSGi resolution ([#2415](#2415)) ([28b503d](28b503d)) * **httpserver:** make scrape error responses secure and configurable ([f6d9df5](f6d9df5)) * include counter names in negative value errors ([#2315](#2315)) ([ea8f935](ea8f935)) * include license files in release source jars ([#2250](#2250)) ([08cf925](08cf925)), closes [#2216](#2216) * keep late observations out of subsequent collection buffers ([#2471](#2471)) ([d78b149](d78b149)) * keep PR title check required after rebases ([#2414](#2414)) ([e3d4c3b](e3d4c3b)) * prevent buffer stripe index overflow ([#2331](#2331)) ([b6cd000](b6cd000)) * redact invalid configuration values ([#2335](#2335)) ([7e7e533](7e7e533)) * show uncertainty in benchmark comparisons ([#2476](#2476)) ([398d087](398d087)) * stabilize OpenTelemetry exporter builder API ([#2257](#2257)) ([09e6e2d](09e6e2d)) * Summary quantiles collapsing for targeted quantiles with 2*epsilon >= 1-quantile ([#2396](#2396)) ([9c7479f](9c7479f)) * update component-prefixed action tags ([#2419](#2419)) ([4acf481](4acf481)) ### Performance Improvements * skip snapshot rebuild in mergeDuplicates when names are unique ([#2441](#2441)) ([8722230](8722230)) ### Documentation * add API design guideline to contributing docs ([#2350](#2350)) ([23ae29a](23ae29a)) * document scrape query limits in request API ([#2391](#2391)) ([ba8f5eb](ba8f5eb)) * document semantic PR title guidance ([#2318](#2318)) ([5e813a0](5e813a0)) --- > [!IMPORTANT] > Close and reopen this PR to trigger CI checks. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.