Migrate TracerHealthMetrics onto the Accumulator primitive by dougqh · Pull Request #12383 · DataDog/dd-trace-java

dougqh · 2026-09-02T13:14:16Z

Summary

Trial migration of TracerHealthMetrics onto Accumulator<E> (Add Accumulator: a striped long counter primitive as an alternative to LongAdder #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<TracerHealthMetric>, 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.

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)
/techdebt — clean, no fixable debt (this branch is itself a debt-removal commit)
/perf-review — 1 flag-as-measure finding (SEV-3, non-blocking): capturing lambdas passed to Accumulator.update() at 8 call sites are a different shape from the shipped AccumulatorBenchmark's non-capturing case; worth a JFR allocation check before merge but not required

🤖 Generated with Claude Code

datadog-datadog-prod-us1 · 2026-09-02T13:29:28Z

🎯 Code Coverage (details)
• Patch Coverage: 94.12%
• Overall Coverage: 57.09% (-0.18%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 97041a6 | Docs | View more details | Give us feedback!}

dd-octo-sts · 2026-09-02T13:42:00Z

🟢 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.81 s	14.68 s	[+0.1%; +1.7%] (maybe worse)
startup:insecure-bank:tracing:Agent	13.63 s	13.65 s	[-0.8%; +0.5%] (no difference)
startup:petclinic:appsec:Agent	16.96 s	16.32 s	[-0.7%; +8.5%] (no difference)
startup:petclinic:iast:Agent	16.90 s	16.98 s	[-1.4%; +0.5%] (no difference)
startup:petclinic:profiling:Agent	16.76 s	16.82 s	[-1.5%; +0.8%] (no difference)
startup:petclinic:sca:Agent	16.70 s	16.61 s	[-0.4%; +1.5%] (no difference)
startup:petclinic:tracing:Agent	16.24 s	16.21 s	[-0.9%; +1.3%] (no difference)

Commit: 97041a6a · 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 · 2026-09-02T14:13:07Z

-
-  private final LongAdder statsAggregateDropped = new LongAdder();
-  private final LongAdder statsInboxFull = new LongAdder();
+  private final Accumulator<TracerHealthMetric> counters =


Can we call this metricAccumulator instead?

Renamed to metricAccumulator.

dougqh · 2026-09-02T14:13:47Z

-  private final LongAdder statsInboxFull = new LongAdder();
+  private final Accumulator<TracerHealthMetric> counters =
+      Accumulator.of(TracerHealthMetric.values());
+  private volatile Accumulator.Counts<TracerHealthMetric> storedTotal = counters.sum();


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.

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 · 2026-09-02T14:15:12Z

    switch (samplingPriority) {
      case USER_DROP:
-        userDropEnqueuedTraces.increment();
+        enqueuedTracesMetric = TracerHealthMetric.USER_DROP_ENQUEUED_TRACES;


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.

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

dougqh · 2026-09-02T14:17:35Z

      case USER_DROP:
-        userDropDroppedSpans.add(spanCount);
-        userDropDroppedTraces.increment();
+        droppedSpansMetric = TracerHealthMetric.USER_DROP_DROPPED_SPANS;


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.

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 · 2026-09-02T14:19:38Z

    if (trace != null) {
-      serialFailedDroppedTraces.increment();
-      serialFailedDroppedSpans.add(trace.size());
+      counters.update(


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.

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 · 2026-09-02T15:08:27Z

+public final class StatsDCountReporter {
+  private StatsDCountReporter() {}
+
+  public static <E extends Enum<E> & StatsDCounterKey> void report(


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.

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 · 2026-09-02T15:30:55Z

  public void onPartialPublish(final int numberOfDroppedSpans) {
-    partialTraces.increment();
-    samplerDropDroppedSpans.add(numberOfDroppedSpans);
+    metricAccumulator.update(


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

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 · 2026-09-02T15:50:37Z

-        statsd.incrementCounter("api.responses.total", statusTagsCache.get(status));
-      }
+    final int status = response.status().orElse(0);
+    metricAccumulator.update(


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.

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.

Replaces ~49 LongAdder fields plus the hand-rolled previousCounts[] delta-tracking in Flush.run() with a single Accumulator<TracerHealthMetric>, using accumulateAndReset() for the periodic drain and sum()/plus() to give summary() a live total that never races that drain. Adds the reusable StatsDCounterKey/StatsDCountReporter glue in metrics-api along the way. HealthMetricsTest and MetricsReliabilityTest pass unmodified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

….zero(), drop capturing lambdas from onPublish/onFailedPublish - counters -> metricAccumulator for clarity. - storedTotal is now seeded via the new Accumulator.Counts.zero(), instead of constructing a fresh Accumulator just to call sum() on it. - onPublish/onFailedPublish now increment inline per switch case instead of building metric-selecting locals and combining them in a single capturing accumulator.update() lambda -- takes the per-stripe lock twice instead of once, in exchange for no lambda capture on these paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…cals onFailedSerialize and onPartialPublish now pass their int context (trace size, dropped-span count) as an explicit parameter to Accumulator.update() instead of closing over a local, per review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ethod ref StatsDCountReporter.report(StatsDClient, Counts<E>) uses the primitive's new Counts.values() to reduce this call site to one argument instead of TracerHealthMetric.values() + delta::get. Also switch metricAccumulator's construction to Accumulator.of(TracerHealthMetric.class)/Counts.zero(class) now that those overloads exist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…s context in onSendAttempt

…ixing a pre-existing oversight

Drops the shared Tags nested class in favor of literal string tags at each enum constant, per the same reasoning as the earlier NO_TAGS removal: the array-sharing only saved a one-time classload allocation.

…ed hot path Measures the real onCreateSpan/onFailedPublish/onPartialPublish/onSend call sites (not a synthetic stand-in) plus summary()'s peek-under- concurrent-writers cost, checking AccumulatorBenchmark's raw-primitive numbers against actual production call shapes.

LegacyTracerHealthMetrics faithfully reconstructs pre-migration TracerHealthMetrics (as of 77964b3) so the new Accumulator-backed implementation can be benchmarked against it in the same run/JVM, not just argued about. Accumulator's per-call cost is 1.1-4.6x the LongAdder baseline depending on contention and counter grouping, and summary() costs ~4x more to walk; the migration's case rests on eliminating the previousCounts/countIndex ceremony and atomic multi-field updates, not on raw per-call speed.

…rray striping Benchmarking showed the AtomicLongArray design beats the realistic LongAdder-per-counter baseline by ~2 orders of magnitude on increment (the hot path) at a small cost on drain (the rare, periodic path). Per-counter atomicity is preserved via getAndSet/getAndAdd; row-wide atomicity across counters is dropped since no real caller needs it, so EmbeddingSupport, Stripe, and the update() overloads are removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…erHealthMetric as Metric The grouped update() call sites collapse to sequential inc/add calls now that Accumulator no longer offers row-wide atomicity -- none of the 6 sites relied on it as a correctness invariant. Also fold the standalone TracerHealthMetric enum into a nested TracerHealthMetrics.Metric to shorten both the type name and the file count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

dougqh · 2026-09-02T21:15:52Z

+ * combination -- several constants can share a metric name but differ by tag, mirroring the
+ * distinct {@code LongAdder} fields this enum replaces.
+ */
+enum TracerHealthMetric implements StatsDCounterKey {


I'd like to rename this Metric and make an inner class of TraceHealthMetrics.

dougqh added comp: metrics Metrics type: refactoring tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes labels Sep 2, 2026

dougqh commented Sep 2, 2026

View reviewed changes

dougqh force-pushed the dougqh/accumulator-tracerhealthmetrics branch 2 times, most recently from 47365ce to 95b9180 Compare September 2, 2026 14:36

dougqh commented Sep 2, 2026

View reviewed changes

dougqh force-pushed the dougqh/accumulator-tracerhealthmetrics branch 2 times, most recently from 6599253 to df7b5d4 Compare September 2, 2026 15:27

dougqh commented Sep 2, 2026

View reviewed changes

dougqh force-pushed the dougqh/accumulator-tracerhealthmetrics branch from df7b5d4 to daa0ae5 Compare September 2, 2026 16:28

dougqh and others added 11 commits September 2, 2026 14:15

Follow Counts.values() -> Counts.keys() rename

15280e8

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Use the boxing-free long-context update overload, and pass response a…

26e55b9

…s context in onSendAttempt

Flip TracerHealthMetric ctor arg order, drop the shared NO_TAGS instance

d9a6afc

Drop reportedInSummary; report org_guard counters in summary() too, f…

b0e4b3a

…ixing a pre-existing oversight

Make TracerHealthMetric's tags varargs and inline the tag literals

3ce265c

Drops the shared Tags nested class in favor of literal string tags at each enum constant, per the same reasoning as the earlier NO_TAGS removal: the array-sharing only saved a one-time classload allocation.

dougqh force-pushed the dougqh/accumulator-tracerhealthmetrics branch from 7157bdd to 157d441 Compare September 2, 2026 18:16

dougqh and others added 2 commits September 2, 2026 17:14

dougqh commented Sep 2, 2026

View reviewed changes

Uh oh!

Conversation

dougqh commented Sep 2, 2026

Summary

Test plan

Uh oh!

datadog-datadog-prod-us1 Bot commented Sep 2, 2026 • edited by datadog-datadog-prod-us1-2 Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dd-octo-sts Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 Java Benchmark SLOs — All performance SLOs passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougqh Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougqh Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

datadog-datadog-prod-us1 Bot commented Sep 2, 2026 •

edited by datadog-datadog-prod-us1-2 Bot

Loading

dd-octo-sts Bot commented Sep 2, 2026 •

edited

Loading

dougqh Sep 2, 2026 •

edited

Loading

dougqh Sep 2, 2026 •

edited

Loading