Add span-derived primary tags (CSS v1.3.0)

[dougqh]

What Does This Do

Implements span-derived primary tags (Client-Side Stats v1.3.0): users configure DD_TRACE_STATS_ADDITIONAL_TAGS to extract matching span tag values as additional aggregation dimensions on ClientGroupedStats.AdditionalMetricTags.

Motivation

From the RFC...

Users may want to aggregate APM statistics by custom dimensions beyond the standard primary tag (typically env). For example, a user might want stats grouped by region, tenant_id, or other business-relevant span tags. This enables more granular visibility into service performance across different segments.

Configuration

This feature is experimental and must be explicitly opted in.

Environment Variable	System Property	Default	Description
DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED	dd.trace.experimental.features.enabled	(none)	Required to enable this feature. Set to DD_TRACE_STATS_ADDITIONAL_TAGS (comma-separate to enable multiple experimental features).
DD_TRACE_STATS_ADDITIONAL_TAGS	dd.trace.stats.additional.tags	(none)	Comma-separated span tag keys to use as additional aggregation dimensions. Max 4 keys; excess dropped at startup with a warn log.
DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT	dd.trace.stats.additional.tags.cardinality.limit	100	Per-key distinct-value cap per reporting cycle. Values beyond the cap are reported as tracer_blocked_value.

Minimal configuration to enable:

DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED=DD_TRACE_STATS_ADDITIONAL_TAGS
DD_TRACE_STATS_ADDITIONAL_TAGS=region,tenant_id

Additional Notes

Design

Wire format: new AdditionalMetricTags field on ClientGroupedStats, emitted as repeated string of "<key>:<value>" entries (mirrors PeerTags). Schema-ordered (alphabetical by key); null slots skipped; field omitted when empty, so unconfigured deployments pay zero payload overhead.

Cardinality protection — three caps, matching the .NET implementation:

• MAX_ADDITIONAL_TAG_KEYS = 4 — configured-key count cap; excess keys dropped at startup with a warn log.
• Per-value length cap (200 chars) — values exceeding this are replaced with tracer_blocked_value.
• DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT (default 100) — per-key distinct-value cap per reporting cycle; further new values collapse to the "<key>:tracer_blocked_value" sentinel.

Threading: all canonicalization (UTF8BytesString interning, cardinality limiting) runs on the aggregator thread. The producer path captures raw String values into a String[] parallel to the schema — no sync on the hot path.

Tests

Migrated SerializingMetricWriterTest from Spock/Groovy to JUnit 5 Java and folded the additional-tags coverage into its shared ValidatingSink — one msgpack-decode harness instead of two.

Benchmark

AdditionalTagsMetricsBenchmark.publish (JMH, Java 17, 8 threads, @Fork(1)):

limitsEnabled	throughput
false	22.3M ± 0.7M ops/s
true	6.9M ± 1.4M ops/s

The true arm is slower because it records more — over-cardinality values collapse into the tracer_blocked_value sentinel — not because limiting itself is expensive.

🤖 Generated with Claude Code

[dougqh]

This is bit off to the side. But since I was adding the convenience to LongHashingUtils, I wanted to update HashingUtils, too

[dougqh]

Convenience added to make AggregateTable / AggregateEntry a bit cleaner

[mhdatie]

Curious, why do we not use GET accessor annotations?

[dougqh]

Do you mean something like Lombok?
We use to, but Lombok does more than we want.

The main one being encouraging the use builders - which are an anti-pattern in dd-trace-java from an allocation & CPU perspective.

[mhdatie]

Why do we pass the blocked variable when the method name is onTagCardinalityBlocked?

[mhdatie]

I see, so this reports the number of blocked values per cardinality budget on each reset / flush of reporting cycles. Could budget be renamed to blockedCount or tracerBlockedValueCount?

[dougqh]

Yes, blocked is a count. Maybe rename to numBlocked? Or blockedCount?

[mhdatie]

Sure whatever makes it more readable. Upon reading it initially I thought it was a 0/1 boolean

[dougqh]

[Claude] Done — renamed to numBlocked consistently across all three reset loops: PropertyHandlers, PeerTagSchema, and AdditionalTagsSchema (commit de9849d).

[mhdatie]

Curious, why do we choose to run benchmarks via annotations instead of programmatically via OptionsBuilder?

[dougqh]

Mostly, that's just what we've done historically.

Although in other places, I'm pushing for minimum number of threads & forks, so allocation translates into an obvious throughput hit. That was missed in a lot of places previously.

[mhdatie]

Although in other places, I'm pushing for minimum number of threads & forks, so allocation translates into an obvious throughput hit

Makes sense to me - is this work recorded somewhere? If not, we can create a ticket and expand on it later

[mhdatie]

My question came from a re-usability standpoint in case we plan to pass configuration there unless there is a way to read config with annotations

[dougqh]

Normally, you'd do that through some command line flags to jmh.
Our gradle build currently makes that slightly annoying.

I fixed that in isolated place where I was frequently changing the benchmark set.
It would be good to generalized that support into a Gradle plug-in.

[mhdatie]

Our gradle build currently makes that slightly annoying.
...I fixed that in isolated place where I was frequently changing the benchmark set.

I could use more context - why it makes it annoying and where was the isolated place so one can compare how that looks compared to a Gradle plug-in support?

And do you think that would improve our benchmark use if we move to a standardized Gradle support where it would support minimum number of threads & forks?

[dougqh]

I'm often wanting to run just a subset of benchmarks related to the changes that I'm making rather than the full benchmark set. Our gradle wasn't allowing for that.

And sometimes, I also want to run spot checks for those I use less forks, so it is nice to have the ability to override.

I wouldn't Gradle to prevent me from doing either of those things. Although, if I had to use a force override flag that would be fine with me.

[mhdatie]

I have created a ticket with the help of Claude - please make the necessary adjustments if your ask isn't clear in there or if the information is incorrect https://datadoghq.atlassian.net/browse/APMLP-1614

[mhdatie]

Why is this schema class designed to have a HealthMetrics property when the aggregator and PeerTagSchema (for example) rely on methods which accept a health metrics instance? Or for example not allowing TagCardinalityHandler#reset to accept a health metric instance (each handler is reporting its own blocked cardinality anyways).

It's primarily a design question - not recommending a better design here. I want to understand the rationale behind it or whether I'm missing an important design piece 🙇 .

[dougqh]

Actually, I think that's worth cleaning up.

PeerTagSchema worked the same way originally, but then ended up being decoupled because the config can be updated dynamically and some weird build interactions with Graal native image.

I think it does make to change AdditionalTagsSchema to work the same way -- even it isn't strictly necessary here.

[dougqh]

[Claude] Good question — and it's now addressed. AdditionalTagsSchema no longer holds a HealthMetrics reference. The field and constructor parameter have been removed; HealthMetrics is now passed directly to resetHandlers(HealthMetrics), matching the existing PeerTagSchema.resetHandlers(HealthMetrics) pattern. The from() factory and EMPTY singleton are simplified accordingly (commit de9849d).

[mhdatie]

I wanted to add that we could leverage adding this design choice to the Agents.md file so that future extensions to the schema can follow it

[dougqh]

Yes, I'd also like to start to put design docs into the repo, but I'm not quite sure how others feel about that.

And I do think at some point we need to revisit HealthMetrics on a few levels.

My usual stance is that I like to bake the design choice into the API.
Similar to the perf toolbox, I want to give people an easy way to do the right thing.

Right now, I'm doing some managing of local counters to avoid high overhead from HealthMetrics. That would be a good thing to document in the perf rubric.

But ideally, the API would make it easy to create a per-use counter and then flush it periodically.

[mhdatie]

Left a few questions for learning purposes - not planning on approving or blocking the merge.

And in case this is missed since it's a collapsed conversation #11402 (comment)

[mhdatie]

Left one clarification comment but otherwise this generally looks good to me as far as my understanding of the change goes.

[sarahchen6]

First pass looks good, left some comments from me and Codex!

Codex also recommended adding a bit more test coverage, specifically for:

- CardinalityHandlerTest.java: direct maxValueLength, blocked count, and reset tests.
- AdditionalTagsSchemaTest.java: per-key isolation and resetHandlers() health-metric
  reporting.
- AggregateTableAdditionalTagsTest.java: verify oversized/over-cardinality values
  collapse into the same aggregate.
- ClientStatsAggregatorTest.groovy: one full publish/report/reset-cycle test proving
  collapse and health metrics through the production path.

I think it wouldn't hurt, but up to you!

[sarahchen6]

since we're using experimentalFeaturesEnabled, we should update https://github.com/DataDog/dd-trace-java/blob/master/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java#L297 and its corresponding =all test as well to include TRACE_STATS_ADDITIONAL_TAGS

[dougqh]

From Claude:

Added DD_TRACE_STATS_ADDITIONAL_TAGS to the =all set (DEFAULT_TRACE_EXPERIMENTAL_FEATURES_ENABLED) so experimental=all picks it up — a17ce32. There wasn't an existing =all test to extend; happy to add one if you'd like coverage there.

[sarahchen6]

the test is here: https://github.com/DataDog/dd-trace-java/blob/master/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy#L2248 and likely why CI is failing

[datadog-official]

Bits has a CI fix ready

🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready

Updated internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy so the DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED=all expectation includes DD_TRACE_STATS_ADDITIONAL_TAGS, matching the expanded default experimental feature set.

Commit fix to this PR

---

View in Datadog | Reviewed commit a7ebd82 · Any feedback? Reach out in #deveng-pr-agent

[AlexeyKuznetsov-DD]

minor: Just curious why ++hitCount (and others)? Why not hitCount++?

[dougqh]

That's a stylistic quirk of mine from C++ where it used to make a difference performance-wise. I can change it if you want.

[AlexeyKuznetsov-DD]

Q: What if length of values will be less than n ? Or it is impossible?

[dougqh]

The design of the Schemas is that the tags and values are the same length by construction.

That said, I do think checking values reads more clearly (and optimizes better), so I've updated the code accordingly.

[AlexeyKuznetsov-DD]

Just curious why order swapped?

[AlexeyKuznetsov-DD]

Would it make sense to put any description why 100 and 200 selected?

[AlexeyKuznetsov-DD]

Just curious if we can start flooding in logs?

[AlexeyKuznetsov-DD]

All those package visibility is for testing? I recall we have an annotation @VisibleForTesting

[dougqh]

The fields are not just visible for testing, but also for use in the code itself (e.g. AggregateEntry.Canonical calls handlers.resource.register(...) etc.).

I prefer package-visible fields to introducing package-visible accessors everywhere.