Migrate TracerHealthMetrics onto the Accumulator primitive

[dougqh]

What Does This Do

Trial migration of TracerHealthMetrics onto Accumulator<E> (#12351), the concrete case cited in that PR's review discussion as the motivating ceremony.

• Replaces ~49 LongAdder fields and the hand-rolled previousCounts[]/countIndex delta-tracking (with its ArrayIndexOutOfBoundsException safety-net catch) in Flush.run() with a single Accumulator<Metric> (nested TracerHealthMetrics.Metric enum), using accumulateAndReset() for the periodic drain.
• summary() reads a live value (storedTotal.plus(counters.sum())) that never races the periodic Flush drain, using the primitive's non-destructive sum()/Counts.plus().
• Adds reusable StatsDCounterKey/StatsDCountReporter glue in metrics-api, decoupled from internal-api via a ToLongFunction<E> accessor.
• HealthMetricsTest and MetricsReliabilityTest pass unmodified — same statsd.count(...) call shape per flush, same summary() labels.

Motivation

Accumulator (#12351) had no real caller yet, and review pushed back on whether the abstraction earns its keep versus the status quo (LongAdder + hand-rolled delta tracking). Rather than keep arguing in the abstract, this wires it into TracerHealthMetrics — the concrete case already cited in that discussion — to settle the question on working code:

• Ergonomics: eliminates the previousCounts/countIndex hand-tracking ceremony (Accumulator.accumulateAndReset() already returns the delta since the last drain) and the 49 individual reportIfChanged call sites, for a 187-line (28%) reduction in TracerHealthMetrics (656 → 469 lines vs. pre-migration at 77964b3996), despite now inlining all 54 Metric constants that previously lived in a separate file.
• Performance: see benchmark results below — no longer a speed-for-ergonomics trade.

Additional Notes

Benchmark results (TracerHealthMetricsBenchmark, direct measurement of real call sites)

Accumulator was rewritten after this trial started to a lock-free AtomicLongArray-striped design (see #12351). Under that design, the new implementation is now at parity with, or measurably faster than, the legacy LongAdder baseline on every single-call-site benchmark, including under Threads.MAX contention. Confirmed stable across JDK 17 and JDK 25 (point estimates agree almost exactly between JVMs).

Apple M1 Max, 10 CPUs - macOS/aarch64 - JDK 17 (Zulu) / JDK 25 (Zulu)
Benchmark                       New (JDK17/25)  Legacy (JDK17/25)  Ratio
onCreateSpan_lowContention        0.007 / 0.007    0.007 / 0.007    1.0x / 1.0x
onCreateSpan_highContention       0.009 / 0.009    0.010 / 0.010    0.9x / 0.9x
onFailedPublish_lowContention     0.007 / 0.007    0.008 / 0.008    0.9x / 0.9x
onFailedPublish_highContention    0.010 / 0.010    0.011 / 0.012    0.9x / 0.8x
onPartialPublish_lowContention    0.007 / 0.007    0.008 / 0.008    0.9x / 0.9x
onPartialPublish_highContention   0.009 / 0.010    0.012 / 0.012    0.75x / 0.8x
onSend_lowContention              0.009 / 0.009    0.012 / 0.013    0.75x / 0.7x
onSend_highContention             0.013 / 0.013    0.020 / 0.022    0.65x / 0.6x
summaryWhileWriting_write         0.008 / 0.008    0.009 / 0.008    0.9x / 1.0x
summaryWhileWriting_read          1.835 / 1.833    0.705 / 0.720    2.6x / 2.55x
(all figures us/op, avgt; JDK17 / JDK25)

The only remaining cost is the diagnostic summary() read (walking all 54 stripes non-destructively, ~2.5-2.6x legacy) — well below the periodic 30s-default Flush cadence and the ad hoc/diagnostic calls that trigger it, so not disqualifying.

Note: AccumulatorBenchmark's own javadoc (on #12351) went through two corrections. A width-8, per-thread-distributed write-side comparison was added (the single-counter benchmarks were the degenerate worst case for the longAdderGroup baseline), and a Fork(5)/15-sample re-run showed the drain-side "regression" from an earlier correction (13.357 us/op, ~5.5x worse) was itself a correlated anomaly across two low-sample runs — the verified number is 2.746 us/op, roughly at parity with (and in this reading faster than) longAdderGroup's noisy 4.770 ± 1.795 us/op. Neither correction changes this PR's real-call-site numbers above, which were re-measured directly.

Test plan

• ./gradlew :dd-trace-core:test --tests "datadog.trace.core.monitor.HealthMetricsTest" — 40/40 passing, no test-file changes
• ./gradlew :dd-trace-core:test --tests "datadog.trace.common.metrics.MetricsReliabilityTest" — passing, no test-file changes
• ./gradlew :products:metrics:metrics-api:test --tests "datadog.metrics.api.statsd.StatsDCountReporterTest" — new tests passing
• ./gradlew :internal-api:test --tests "datadog.trace.util.AccumulatorTest" — passing (on base branch)
• ./gradlew :dd-trace-core:jmh -Pjmh.includes=TracerHealthMetricsBenchmark — run on both JDK 17 and JDK 25, results above
• /techdebt — clean, no fixable debt (this branch is itself a debt-removal commit)
• /perf-review — 1 flag-as-measure finding (SEV-3, non-blocking, now stale — predates the update() removal, no capturing-lambda call sites remain on this branch)

Contributor Checklist

• Format the title according to the contribution guidelines
• Assign the type: and (comp: or inst:) labels in addition to any other useful labels
• Avoid using close, fix, or any linking keywords when referencing an issue
• Update the CODEOWNERS file on source file addition, migration, or deletion
• Update public documentation with any new configuration flags or behaviors
• Once approved, use merge queue to merge the PR

Jira ticket: APMLP-1779

🤖 Generated with Claude Code

[dd-octo-sts]

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite	Status
Startup	🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results

Scenario	Candidate	master	Δ (95% CI of mean)
startup:insecure-bank:iast:Agent	14.78 s	14.68 s	[+0.0%; +1.4%] (maybe worse)
startup:insecure-bank:tracing:Agent	13.67 s	13.67 s	[-0.9%; +0.9%] (no difference)
startup:petclinic:appsec:Agent	16.94 s	16.65 s	[+1.0%; +2.6%] (maybe worse)
startup:petclinic:iast:Agent	16.91 s	17.02 s	[-1.5%; +0.2%] (no difference)
startup:petclinic:profiling:Agent	16.29 s	16.77 s	[-7.4%; +1.7%] (no difference)
startup:petclinic:sca:Agent	16.85 s	16.50 s	[+1.2%; +2.9%] (significantly worse)
startup:petclinic:tracing:Agent	16.12 s	15.79 s	[-2.1%; +6.2%] (no difference)

Commit: c5b2c72f · CI Pipeline · Benchmarking Platform UI

---

Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

[dougqh]

Can we call this metricAccumulator instead?

[dougqh]

Renamed to metricAccumulator.

[dougqh]

Maybe we should provide an API for creating a Counts directly. This feels a little awkward to me.

I'm also wondering if Counts should handle thread-safety, but I'm torn on that.

[dougqh]

Added Accumulator.Counts.zero(E[] values) (an all-zero Counts sized for the enum, no scratch Accumulator/sum() needed) and switched storedTotal's seeding to it. Left Counts itself as a plain immutable value type for now since you flagged you're torn on the thread-safety question -- happy to revisit if you land on an answer there.

[dougqh]

To keep the porting simple, I think let's just do inline increments here. I know that means taking the lock multiple times, but so be it.

[dougqh]

Done -- onPublish now increments the priority-specific counter inline per switch case and adds ENQUEUED_SPANS separately, no update() lambda.

[dougqh]

I think I'd prefer either just do inline increments. Or if we can still coarsen and use, accumulator.update, but do inline in each switch case.

Right now, we're creating a capturing lambda which is something that we want to avoid.

[dougqh]

Same treatment here -- onFailedPublish now does inline per-case add/inc pairs instead of building droppedSpansMetric/droppedTracesMetric locals and closing over them in a single update() lambda.

[dougqh]

Hmm, this case is slightly annoying.
We end up capturing the trace.
I guess we can a contextual variation on update that uses BiConsumer<C, Stripe> to solve that.

[dougqh]

Added exactly the overload you sketched: Accumulator.update(C context, BiConsumer<C, Stripe<E>> mutator) on dougqh/accumulator-primitive (so it's shared with anyone else pulling in the primitive). Wired it into onFailedSerialize (passes trace.size() instead of capturing trace) and, for consistency, onPartialPublish too (passes numberOfDroppedSpans). Javadoc on the new overload flags the one caveat: a primitive context gets boxed, so it's a real allocation trade against the capturing lambda it replaces rather than a free win -- worth it here since it kills the capture, but not a blanket recommendation.

[dougqh]

I'm wondering it a better option is take a Counts directly.
I'm also pondering whether Accumulator or Counts should provide a way to get the corresponding enum elements to reduce the amount of ceremony.

[dougqh]

Done both ways: Accumulator/Counts now remember the enum's values() array from construction, so Counts.values() lets a caller iterate its own keys -- StatsDCountReporter.report(StatsDClient, Counts<E>) uses that to collapse to one arg instead of TracerHealthMetric.values() + delta::get. Also added Accumulator.of(Class<E>)/Counts.zero(Class<E>) so metricAccumulator's construction reads as Accumulator.of(TracerHealthMetric.class). Landed on dougqh/accumulator-primitive (extra field on Counts, ~free since it's already a per-drain allocation) and wired in here.

[dougqh]

I think we need a way to pass primitives as context without boxing. Or as least int or long?

[dougqh]

Added Accumulator.update(long context, ObjLongConsumer<Stripe<E>> mutator) on #12351 — reuses the JDK's ObjLongConsumer instead of the generic BiConsumer, so the context is passed as a primitive long, no boxing. An int argument widens into it for free (also no boxing), so it covers both. I didn't add a separate int overload alongside it: passing a plain int literal/variable turned out ambiguous between the two (exact match for one, free widening to the other, and the differing functional-interface types block the usual most-specific tiebreak) — long alone is the simplest fix and costs nothing for the int case.

Updated onPartialPublish (and onFailedSerialize, same shape) to use it — just a lambda-arg-order swap since ObjLongConsumer#accept is (T, long) rather than (long, T).

[dougqh]

Now, that we can pass response as context we should use that.
I think we can divide into several updates to avoid capture & boxing.

At least here, I think using update is more best effort to avoid lock acquisition overhead.

[dougqh]

Restructured onSendAttempt: API_REQUESTS/FLUSHED_TRACES/FLUSHED_BYTES are now three top-level inc/add calls (independent counters, same pattern as onPublish/onFailedPublish), and response is now passed as update's context for the two response-derived checks (API_ERRORS/API_RESPONSES_OK) — since it's already a reference, that's zero boxing, and grouping just those two under one update keeps a single lock acquisition for the part where it actually helps (they both need response.status()/response.exception()). The lambda no longer captures traceCount/sizeInBytes/response together.

I kept the post-lock response.status().orElse(0) call for the non-200 statsd.incrementCounter path recomputed rather than threaded through — it's a cheap Optional unwrap, and threading it out would've meant carrying a second context value. Let me know if you'd rather see that differently.

[dougqh]

I'll probably remove this from the final PR, but for now, it shows the performance difference.

[amarziali]

Automated review — three points worth accounting for

The migration substantially reduces the bookkeeping in TracerHealthMetrics, and the accumulator's individual atomic operations appear sound.

I found three remaining concerns:

1. summary() can transiently under-report when it overlaps the two-step drain and storedTotal update.
2. An exception during StatsD reporting permanently discards the drained interval, with ambiguous partial-delivery behavior.
3. The convenience reporter overload exposes an internal-api type through the public metrics-api ABI.

The concurrency measurements cover writer-versus-summary and writer-versus-drain independently, but nothing currently exercises summary() concurrently with the complete drain-and-publish operation in TracerHealthMetrics.

Details are in the inline comments.

This was an automated, read-only review of head b2f791f.

[amarziali]

This can transiently under-report while Flush transfers counts. Java reads storedTotal before evaluating metricAccumulator.sum(). If Flush resets the accumulator and updates storedTotal between those operations, summary() combines the old stored total with the already-reset accumulator. For one pending increment it can return 0 even though the cumulative count was always 1. Please coordinate the transfer with summary(), or otherwise version the snapshot. A focused concurrency test should cover this interleaving; the current benchmarks race summaries with writers and drains with writers separately, but never summary() with the full drain-and-publish sequence.

[dougqh]

Fixed in ef15a5f: Flush.run()'s drain-and-publish and summary()'s read now share a lock (totalLock), so summary() can no longer land between the drain and the storedTotal publish. Only the periodic Flush task and the infrequent summary() call ever contend on it -- the hot inc/add write path is untouched, still going straight to metricAccumulator's lock-free stripes. This is a consumer-side fix (nothing changed in Accumulator itself -- see the reply on the corresponding #12351 thread).

[dougqh]

@/tmp/reply1.txt