test: focus PR benchmarks on client_java and measure label lookups - #2468
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:12
Member
Author
|
The histogram timeout was investigated separately from this benchmark work. The forced interleaving fails on the old implementation after five seconds, then passes with the This PR remains benchmark-only. Once the correctness fix lands, update/retest this branch rather |
Contributor
Benchmark resultsBenchmark run succeeded for
Prometheus Java Client BenchmarksRun Information
Comparison with base
Results for PR headCounterBenchmark
HistogramBenchmark
HistogramTextFormatBenchmark
TextFormatUtilBenchmark
New benchmarks in PR headThese benchmarks have no base counterpart; scores are descriptive only and have no regression verdict.
Allocation per operationJMH GC profiler
Raw ResultsNotes
Benchmark Descriptions
|
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Member
Author
|
I rebased this benchmark PR additively with the focused buffer handoff fix from #2471 so its full CI can validate on the same head. Relevant core regression tests pass locally, lint and |
jaydeluca
approved these changes
Sep 15, 2026
zeitlinger
enabled auto-merge (squash)
September 15, 2026 16:06
zeitlinger
added a commit
that referenced
this pull request
Sep 15, 2026
Improve benchmark comparison reporting without changing benchmark execution or sample configuration. - Display JMH 99.9% confidence intervals beside PR and base means. - Distinguish overlapping intervals, below-threshold changes, and missing/invalid uncertainty. - Keep the verdict explicitly conservative and heuristic, not a significance test. - Add coverage for rendering, malformed data, thresholds, metadata mismatch, and the supplied overlapping example. This is separate from #2468 and does not rerun benchmarks. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
zeitlinger
disabled auto-merge
September 15, 2026 16:35
zeitlinger
added a commit
that referenced
this pull request
Sep 16, 2026
## Problem and proof A late appender can increment its stripe during collection A, then read `activeGeneration` only after A has completed and collection B has started. B already includes that observation in `expectedCount`. If the appender puts it into B's buffer, 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: ```text BufferTest.lateAppenderCountedByNextGenerationMustNotBeBufferedAgain Time elapsed: 5.037 s ExecutionException: IllegalStateException: Timed out while waiting for in-flight observations. ``` 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](#2468) and [#2441](#2441). The reproduction proves this race exists, not which interleaving occurred in any particular CI run. ## Fix - Record each stripe's absolute observation count at generation activation, under `appendLock`. - Under that same lock, reject buffering an observation whose ticket is already included in the current generation's starting count. It completes through the existing direct observation path. - Reuse a single cutoff array per Buffer: one additional long per stripe, plus array overhead, with no new per-collection allocation or additional fast-path lock. - Keep cutoffs independent of the reset-adjusted aggregate observation count. - Do not change the five-second timeout, buffer capacity, or public API. ## 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 - Before fix: deterministic timeout reproduced as above. - After fix: `BufferTest`, `HistogramTest`, and `SummaryTest` — 50 tests passed. - Temporary local JUnit launcher: both regression cases and the original `HistogramTest.testObserveMultithreaded`, repeated 50 times with each of `-XX:ActiveProcessorCount=1`, `2`, and `4` — **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 because Docker 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 `main` through [#2441](#2441) and [#2468](#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. --------- Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.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.
Summary
Benchmark-only follow-up for the release review; no production metric changes.
OpenTelemetry, Codahale, and legacy simpleclient comparisons remain in full/local/nightly runs.
Keep all client_java exposition benchmarks, including OpenMetrics.
with one-thread and four-thread variants. One invocation is one metric update.
base/head allocation deltas only when configurations match.
Related PR
Label lookup optimization #2442 is pending this
benchmark infrastructure. After this lands, update that branch and rerun the benchmarks so base and
head contain identical lookup benchmark code. The new methods in this PR itself have head-only
results and do not establish the optimization's benefit.
Validation
mise run lint:fix— passed; formatter changes retained.mise run test— passed.mise run build -- -DskipITs=true— passed.mise run buildcompiled the benchmark module, but Docker-backed integration tests randespite
-DskipTestsand failed because no Docker environment was available.OpenMetrics/Prometheus exposition cases.
JDK 25.0.3,
-Xms128m -Xmx256m. This was a smoke test on a shared development host, not acontrolled base/head performance comparison. Repeated lookup reported about 64 B/update;
cached increments were near zero.
git diff --check— passed.