FIx for BSP benchmark aux counters (exportedSpans/droppedSpans) always reporting zero#8539
Conversation
BatchSpanProcessorMetrics.getMetric() used stringKey("dropped") to
filter metric points, but BatchSpanProcessor records the attribute with
booleanKey("dropped"). The filter never matched, so exportedSpans and
droppedSpans always returned 0 in all BSP benchmarks since open-telemetry#3017.
- Fix stringKey → booleanKey in BatchSpanProcessorMetrics
- Remove dropRatio() — ratio does not aggregate correctly across JMH
thread states
- Switch BatchSpanProcessorDroppedSpansBenchmark to Type.EVENTS to
avoid integer truncation to zero in tight-loop (millions of ops/iter
collapse per-op long counters even after the key-type fix)
- Derive numThreads from BenchmarkParams.getThreads() in @setup instead
of hardcoding in each benchmark method body
… method BenchmarkParams.getThreads() injected into @setup(Level.Iteration) correctly returns the per-method @threads(N) value. Verified empirically: ratio (exportedSpans + droppedSpans) / (primary_ops_per_sec × iteration_time) ≈ 1.0 with 5 threads. Removes the fragile manual coupling between @threads(N) and numThreads = N scattered across each benchmark method body.
…chmark BatchSpanProcessorMultiThreadBenchmark had the same two issues as the other benchmarks fixed in the previous commit: - numThreads hardcoded in each method body (fragile; same BenchmarkParams fix) - Type.OPERATIONS reports spans/s (same unit as primary, drop rate not obvious); Type.EVENTS reports absolute iteration totals with clear drop rates
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8539 +/- ##
============================================
+ Coverage 90.93% 90.97% +0.03%
+ Complexity 10209 10208 -1
============================================
Files 1013 1013
Lines 27175 27170 -5
Branches 3184 3182 -2
============================================
+ Hits 24712 24717 +5
+ Misses 1735 1729 -6
+ Partials 728 724 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add BatchSpanProcessorMetrics.dropRatio(int numIterations), adjusting AggregationPolicy.SUM (numThreads * numIterations) of Type.EVENTS. Use it in DroppedSpansBenchmark and MultiThreadBenchmark, restoring the dropRatio aux counter. CpuBenchmark stays on Type.OPERATIONS, where the drop rate is read directly and this method does not apply.
89e693c to
47db02e
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes JMH auxiliary counters in BatchSpanProcessor benchmarks so exportedSpans / droppedSpans reflect the actual processedSpans metric (instead of always reporting 0), and refactors the benchmarks to derive thread/iteration configuration from JMH BenchmarkParams rather than hardcoding it.
Changes:
- Fix
BatchSpanProcessorMetricsto filter points bybooleanKey("dropped")(matching how the metric is recorded), restoring correct exported/dropped counts. - Switch aux counter reporting to
AuxCounters.Type.EVENTSfor the drop-focused benchmarks and add a computeddropRatioaux counter. - Remove manual
numThreads = Ncoupling in benchmarks by reading thread count (and measurement iteration count) fromBenchmarkParamsduring@Setup(Level.Iteration).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sdk/trace/src/jmh/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorMultiThreadBenchmark.java | Uses BenchmarkParams for thread/iteration config; reports aux counters as EVENTS and adds dropRatio. |
| sdk/trace/src/jmh/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorMetrics.java | Fixes the attribute-key mismatch causing counters to stay at 0; updates drop-ratio calculation/docs. |
| sdk/trace/src/jmh/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorDroppedSpansBenchmark.java | Uses BenchmarkParams and switches aux counters to EVENTS with per-iteration drop ratio reporting. |
| sdk/trace/src/jmh/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorCpuBenchmark.java | Uses BenchmarkParams.getThreads() instead of hardcoding numThreads in each benchmark method. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This PR has review comments. Review suggestions, whether from maintainers or automated reviewers, aren't always correct or required. Please evaluate each comment on its merits, then make sure each thread has a clear outcome. For example, link to the commit if you applied a suggestion, explain why it wasn't applied, or ask a follow-up question. Automation flags a PR for human review once every review thread has a reply or is marked as resolved. Status across open PRs is visible on the pull request dashboard. |
TLDR
Closes issue #8538 (BatchSpanProcessorDroppedSpansBenchmark, BatchSpanProcessorMultiThreadBenchmark, and BatchSpanProcessorCpuBenchmark were reporting exportedSpans and droppedSpans auxiliary counters as 0)
AuxCounters.TypefromOPERATIONStoEVENTSinDroppedSpansBenchmarkandMultiThreadBenchmarkto fix misleading throughput-normalized counters and make drop rates and totals directly interpretable.numThreadsvariable)BatchSpanProcessorDroppedSpansBenchmark impact
Running
BatchSpanProcessorDroppedSpansBenchmarkon main produces (measured with-PjmhIterations=1 -PjmhTime=5; the benchmark default is 20s):With the fix applied (measured with
-PjmhIterations=1 -PjmhTime=5):Type.EVENTSaux counters report the total event count for the measurement iteration,not a per-second rate.
BatchSpanProcessorMultiThreadBenchmark impact
MultiThreadBenchmarkshares the sameBatchSpanProcessorMetricshelper, so it carried thesame stringKey bug — all counters reported
≈ 0on main.Before fix (all thread variants, measured with
-PjmhIterations=1 -PjmhTime=5):After fix —
Type.EVENTS+BenchmarkParams(measured with-PjmhIterations=1 -PjmhTime=5).With
Type.EVENTS, aux counters report absolute totals per iteration rather than a per-secondrate, so
dropRatio = droppedSpans / (exportedSpans + droppedSpans)is directly readable:BatchSpanProcessorCpuBenchmark impact
CpuBenchmarkkeepsType.OPERATIONS— each operation submits exactly one span andLockSupport.parkNanos(100_000)limits throughput to ~10k spans/sec per thread, so per-secondvalues are readable integers. The stringKey bug caused all counters to report
≈ 0:Before fix (all thread variants):
After fix (
Type.OPERATIONSin throughput mode reports the counter rate in the same units asthe primary metric — spans/s, not spans/op. Since each operation submits exactly one span,
exportedSpans ≈ primary_thrpt, scaling linearly with thread count):No drops are expected —
CpuBenchmarkmeasures export throughput at a controlled low rate(~10k spans/s per thread via
parkNanos), not queue saturation.Type.OPERATIONSis thenatural unit here: the per-second rate is what a CPU profiler run reports.
Details
stringKey("dropped")→booleanKey("dropped")inBatchSpanProcessorMetrics— fixes the main issueAuxCounters.Type.OPERATIONS→Type.EVENTSinBatchSpanProcessorDroppedSpansBenchmarkand
BatchSpanProcessorMultiThreadBenchmark—Type.OPERATIONSin throughput mode reportscounter / time, soexportedSpansanddroppedSpansappear inops/s. This makes it impossible to read absolute totals.Type.EVENTSreports raw iteration totals, from whichdrop ratio = droppedSpans / (exportedSpans + droppedSpans).numThreadsderived fromBenchmarkParams.getThreads()injected into@Setup(Level.Iteration)in all three benchmarks instead of being hardcoded in each benchmark method body — removes
the fragile manual coupling between
@Threads(N)andnumThreads = N(verified:getThreads()correctly returns the per-method thread count, not the JMH global default)