Skip to content

implementation for span derived primary tags #11358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Sam-Maya wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

implementation for span derived primary tags #11358

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
07df666
implement mvp for span derived primary tags
Sam-Maya May 12, 2026
4597764
add max keys and switch from list to set
Sam-Maya May 13, 2026
983ae32
add cardinality control
Sam-Maya May 13, 2026
4b981d7
add protection against long chars
Sam-Maya May 13, 2026
68fea6a
simple cardinality control
Sam-Maya May 15, 2026
b6a04dc
optimize tag handling
Sam-Maya May 15, 2026
9bfd57f
fix CI: resize health-metrics dedupe buffer and correct config schema
Sam-Maya May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dd-trace-api/src/main/java/datadog/trace/api/config/GeneralConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ public final class GeneralConfig {
public static final String TRACER_METRICS_MAX_PENDING = "trace.tracer.metrics.max.pending";
public static final String TRACER_METRICS_IGNORED_RESOURCES =
"trace.tracer.metrics.ignored.resources";
public static final String TRACE_STATS_ADDITIONAL_TAGS = "trace.stats.additional.tags";

public static final String AZURE_APP_SERVICES = "azure.app.services";
public static final String INTERNAL_EXIT_ON_FAILURE = "trace.internal.exit.on.failure";
Expand Down
1 change: 1 addition & 0 deletions ...-core/src/jmh/java/datadog/trace/common/metrics/ConflatingMetricsAggregatorBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public class ConflatingMetricsAggregatorBenchmark {
new ConflatingMetricsAggregator(
new WellKnownTags("", "", "", "", "", ""),
Collections.emptySet(),
Collections.emptyList(),
featuresDiscovery,
HealthMetrics.NO_OP,
new NullSink(),
Expand Down
44 changes: 43 additions & 1 deletion dd-trace-core/src/main/java/datadog/trace/common/metrics/ConflatingMetricsAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ public final class ConflatingMetricsAggregator implements MetricsAggregator, Eve
Pair.of(
DDCaches.newFixedSizeCache(512),
value -> UTF8BytesString.create(key + ":" + value));
private static final DDCache<
String, Pair<DDCache<String, UTF8BytesString>, Function<String, UTF8BytesString>>>
ADDITIONAL_TAG_VALUES_CACHE = DDCaches.newFixedSizeCache(64);
private static final Function<
String, Pair<DDCache<String, UTF8BytesString>, Function<String, UTF8BytesString>>>
ADDITIONAL_TAG_VALUES_CACHE_ADDER =

@dougqh dougqh May 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hindsight, I wish we hadn't chosen to concatenate key + ":" + value in the payload. For keeping memory down, separate fields would probably have been better.

key ->
Pair.of(
DDCaches.newFixedSizeCache(512),
value -> UTF8BytesString.create(key + ":" + value));

@dougqh dougqh May 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as is this degenerates into hot allocation when a tag has high cardinality. In high cardinality situation, we need to avoid the concatenation before it happens.

Right now, we're still paying the allocation cost and undoing the benefit of the cache.

@dougqh dougqh May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to verify, I ran this through Claude as well...

Vulnerabilities / Misconfiguration risk

  1. No per-value cardinality cap (acknowledged in PR description)
    The MAX_ADDITIONAL_TAG_KEYS = 10 cap protects against the number of configured keys. It does nothing about the number of unique values per key. A single misconfiguration —
    DD_TRACE_STATS_ADDITIONAL_TAGS=user_id or request_id or trace_id — gives the customer an unbounded MetricKey set, unbounded keys map growth, and an oversized payload to the
    agent. The PR explicitly defers cardinality protection ("requires an HLL/bounded-set estimator"), which is reasonable for MVP — but in its current form this config is a foot-gun
    and should not be enabled by default, which it isn't. Two suggestions:
  • Add a log.warn (one-shot, at startup) when additionalTagKeys is non-empty, calling out the cardinality risk and pointing at the future
    DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT config.
  • Consider a stop-gap "max unique MetricKeys before we stop adding" guard rather than waiting for the full HLL implementation.
  1. Tag values are not length-bounded before being interned and serialized
    UTF8BytesString.create(key + ":" + value) will faithfully serialize whatever the customer set on the span tag. If application code stuffs a JSON body or a stack trace into a tag
    named in DD_TRACE_STATS_ADDITIONAL_TAGS, the per-value cache entry, the MetricKey, and the msgpack payload all carry it. This is pre-existing for peer tags too, but again is
    amplified here because the user controls which keys feed this path. A value.length() < N guard with a one-shot warn would be cheap insurance.

Overhead
5. Cache footprint scales with MAX_ADDITIONAL_TAG_KEYS × per-key-cache-size
ADDITIONAL_TAG_VALUES_CACHE is newFixedSizeCache(64) (plenty) but each entry holds an inner newFixedSizeCache(512). With 10 configured keys that's up to 10×512 = 5,120 cached
UTF8BytesString entries plus the lambda closures, vs. the existing peer-tags cache of similar size. Acceptable, but worth a comment near ADDITIONAL_TAG_VALUES_CACHE explaining
the bound (mirrors the existing comment on PEER_TAGS_CACHE).

@dougqh dougqh May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also worth noting, that the cache is still unbounded in terms of bytes. That has bit us with SQL statements previously.

And I guess part of what Claude is pointing out is that tags may also contain complicated objects, we might need to filter those out otherwise allocation could explode if misconfigured. Although, that was definitely a pre-existing issue.

@Sam-Maya Sam-Maya May 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added basic cardinality control and will add a char limit to the string values, would that address all your concerns here?

@dougqh dougqh May 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm experimenting with a revised approach.

I think the simplest solution might be to combine the cardinality tracking and the caching into a single class.

In hindsight, I'd argue that DDCache isn't a great fit for metrics. DDCache works well with low cardinality, but doesn't work well in situations where cardinality can be high.

In a strict sense, that's okay because the GC will be able to reclaim the memory, so memory isn't unbounded. However, a solution that deals with high cardinality more gracefully would be preferable.

private static final CharSequence SYNTHETICS_ORIGIN = "synthetics";

private static final Set<String> ELIGIBLE_SPAN_KINDS_FOR_METRICS =
Expand All @@ -93,6 +103,7 @@ public final class ConflatingMetricsAggregator implements MetricsAggregator, Eve
new HashSet<>(Arrays.asList(SPAN_KIND_CLIENT, SPAN_KIND_PRODUCER, SPAN_KIND_CONSUMER)));

private final Set<String> ignoredResources;
private final List<String> additionalTagKeys;
private final MessagePassingQueue<Batch> batchPool;
private final ConcurrentHashMap<MetricKey, Batch> pending;
private final ConcurrentHashMap<MetricKey, MetricKey> keys;
Expand All @@ -115,6 +126,7 @@ public ConflatingMetricsAggregator(
this(
config.getWellKnownTags(),
config.getMetricsIgnoredResources(),
config.getTraceStatsAdditionalTags(),
sharedCommunicationObjects.featuresDiscovery(config),
healthMetrics,
new OkHttpSink(
Expand All @@ -132,6 +144,7 @@ public ConflatingMetricsAggregator(
ConflatingMetricsAggregator(
WellKnownTags wellKnownTags,
Set<String> ignoredResources,
List<String> additionalTagKeys,
DDAgentFeaturesDiscovery features,
HealthMetrics healthMetric,
Sink sink,
Expand All @@ -141,6 +154,7 @@ public ConflatingMetricsAggregator(
this(
wellKnownTags,
ignoredResources,
additionalTagKeys,
features,
healthMetric,
sink,
Expand All @@ -154,6 +168,7 @@ public ConflatingMetricsAggregator(
ConflatingMetricsAggregator(
WellKnownTags wellKnownTags,
Set<String> ignoredResources,
List<String> additionalTagKeys,
DDAgentFeaturesDiscovery features,
HealthMetrics healthMetric,
Sink sink,
Expand All @@ -164,6 +179,7 @@ public ConflatingMetricsAggregator(
boolean includeEndpointInMetrics) {
this(
ignoredResources,
additionalTagKeys,
features,
healthMetric,
sink,
Expand All @@ -177,6 +193,7 @@ public ConflatingMetricsAggregator(

ConflatingMetricsAggregator(
Set<String> ignoredResources,
List<String> additionalTagKeys,
DDAgentFeaturesDiscovery features,
HealthMetrics healthMetric,
Sink sink,
Expand All @@ -187,6 +204,8 @@ public ConflatingMetricsAggregator(
TimeUnit timeUnit,
boolean includeEndpointInMetrics) {
this.ignoredResources = ignoredResources;
this.additionalTagKeys =
additionalTagKeys == null ? Collections.emptyList() : additionalTagKeys;

@sarahchen6 sarahchen6 May 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also add a MAX_ADDITIONAL_TAG_KEYS value limiting the number of tag keys that customers can set to prevent "infinite" tag keys and possible exploitation(? - not sure if exploitation is possible here but a limit and warning once you hit the limit seems safer)

@Sam-Maya Sam-Maya May 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only allow 4 unique primary tag keys in the UI so if a user is setting more than that they are aggregating their stats on an additional dimension only for that value to get discarded in our stats pipeline.
IIRC we do increase the amount of keys a customer can set but its rare, a max value of 10 I think makes sense here. It provides protection against the extreme while leaving some room for customers with an increased key count.

this.includeEndpointInMetrics = includeEndpointInMetrics;
this.inbox = Queues.mpscArrayQueue(queueSize);
this.batchPool = Queues.spmcArrayQueue(maxAggregates);
Expand Down Expand Up @@ -350,7 +369,8 @@ private boolean publish(CoreSpan<?> span, boolean isTopLevel, CharSequence spanK
getPeerTags(span, spanKind.toString()),
httpMethod,
httpEndpoint,
grpcStatusCode);
grpcStatusCode,
getAdditionalTags(span));
MetricKey key = keys.putIfAbsent(newKey, newKey);
if (null == key) {
key = newKey;
Expand Down Expand Up @@ -413,6 +433,28 @@ private List<UTF8BytesString> getPeerTags(CoreSpan<?> span, String spanKind) {
return Collections.emptyList();
}

private List<UTF8BytesString> getAdditionalTags(CoreSpan<?> span) {

@dougqh dougqh May 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have high allocation from creating MetricKey-s just to perform Map look-ups.
Producing an intermediate list with UTF8BytesStrings is going to compound that problem significantly.

@Sam-Maya Sam-Maya May 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied how we interact with peer tags, to keep behavior consistent. What do you propose I do differently?

if (additionalTagKeys.isEmpty()) {
return Collections.emptyList();
}
List<UTF8BytesString> result = null;
for (String tagKey : additionalTagKeys) {
Object value = span.unsafeGetTag(tagKey);
if (value == null) {
continue;
}
Pair<DDCache<String, UTF8BytesString>, Function<String, UTF8BytesString>> cacheAndCreator =
ADDITIONAL_TAG_VALUES_CACHE.computeIfAbsent(tagKey, ADDITIONAL_TAG_VALUES_CACHE_ADDER);
UTF8BytesString formatted =
cacheAndCreator.getLeft().computeIfAbsent(value.toString(), cacheAndCreator.getRight());
if (result == null) {
result = new ArrayList<>(additionalTagKeys.size());
}
result.add(formatted);
}
return result == null ? Collections.emptyList() : result;
}

private static boolean isSynthetic(CoreSpan<?> span) {
return span.getOrigin() != null && SYNTHETICS_ORIGIN.equals(span.getOrigin().toString());
}
Expand Down
13 changes: 11 additions & 2 deletions dd-trace-core/src/main/java/datadog/trace/common/metrics/MetricKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public final class MetricKey {
private final UTF8BytesString httpMethod;
private final UTF8BytesString httpEndpoint;
private final UTF8BytesString grpcStatusCode;
private final List<UTF8BytesString> additionalTags;

public MetricKey(
CharSequence resource,
Expand All @@ -53,7 +54,8 @@ public MetricKey(
List<UTF8BytesString> peerTags,
CharSequence httpMethod,
CharSequence httpEndpoint,
CharSequence grpcStatusCode) {
CharSequence grpcStatusCode,
List<UTF8BytesString> additionalTags) {
this.resource = null == resource ? EMPTY : utf8(RESOURCE_CACHE, resource);
this.service = null == service ? EMPTY : utf8(SERVICE_CACHE, service);
this.serviceSource = null == serviceSource ? null : utf8(SERVICE_SOURCE_CACHE, serviceSource);
Expand All @@ -68,6 +70,7 @@ public MetricKey(
this.httpEndpoint = httpEndpoint == null ? null : utf8(HTTP_ENDPOINT_CACHE, httpEndpoint);
this.grpcStatusCode =
grpcStatusCode == null ? null : utf8(GRPC_STATUS_CODE_CACHE, grpcStatusCode);
this.additionalTags = additionalTags == null ? Collections.emptyList() : additionalTags;

int tmpHash = 0;
tmpHash = HashingUtils.addToHash(tmpHash, this.isTraceRoot);
Expand All @@ -83,6 +86,7 @@ public MetricKey(
tmpHash = HashingUtils.addToHash(tmpHash, this.httpEndpoint);
tmpHash = HashingUtils.addToHash(tmpHash, this.httpMethod);
tmpHash = HashingUtils.addToHash(tmpHash, this.grpcStatusCode);
tmpHash = HashingUtils.addToHash(tmpHash, this.additionalTags);
this.hash = tmpHash;
}

Expand Down Expand Up @@ -146,6 +150,10 @@ public UTF8BytesString getGrpcStatusCode() {
return grpcStatusCode;
}

public List<UTF8BytesString> getAdditionalTags() {
return additionalTags;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand All @@ -166,7 +174,8 @@ public boolean equals(Object o) {
&& Objects.equals(serviceSource, metricKey.serviceSource)
&& Objects.equals(httpMethod, metricKey.httpMethod)
&& Objects.equals(httpEndpoint, metricKey.httpEndpoint)
&& Objects.equals(grpcStatusCode, metricKey.grpcStatusCode);
&& Objects.equals(grpcStatusCode, metricKey.grpcStatusCode)
&& additionalTags.equals(metricKey.additionalTags);
}
return false;
}
Expand Down
10 changes: 9 additions & 1 deletion dd-trace-core/src/main/java/datadog/trace/common/metrics/SerializingMetricWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public final class SerializingMetricWriter implements MetricWriter {
private static final byte[] IS_TRACE_ROOT = "IsTraceRoot".getBytes(ISO_8859_1);
private static final byte[] SPAN_KIND = "SpanKind".getBytes(ISO_8859_1);
private static final byte[] PEER_TAGS = "PeerTags".getBytes(ISO_8859_1);
private static final byte[] ADDITIONAL_METRIC_TAGS = "AdditionalMetricTags".getBytes(ISO_8859_1);
private static final byte[] HTTP_METHOD = "HTTPMethod".getBytes(ISO_8859_1);
private static final byte[] HTTP_ENDPOINT = "HTTPEndpoint".getBytes(ISO_8859_1);
private static final byte[] GRPC_STATUS_CODE = "GRPCStatusCode".getBytes(ISO_8859_1);
Expand Down Expand Up @@ -149,7 +150,7 @@ public void add(MetricKey key, AggregateMetric aggregate) {
final boolean hasServiceSource = key.getServiceSource() != null;
final boolean hasGrpcStatusCode = key.getGrpcStatusCode() != null;
final int mapSize =
15
16

@sarahchen6 sarahchen6 May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A future improvement can be to make this conditional such as hasServiceSource and hasHttpMethod below. Otherwise, most customers will always get an empty additional tag and thus an unnecessarily large payload

+ (hasServiceSource ? 1 : 0)
+ (hasHttpMethod ? 1 : 0)
+ (hasHttpEndpoint ? 1 : 0)
Expand Down Expand Up @@ -189,6 +190,13 @@ public void add(MetricKey key, AggregateMetric aggregate) {
writer.writeUTF8(peerTag);
}

writer.writeUTF8(ADDITIONAL_METRIC_TAGS);
final List<UTF8BytesString> additionalTags = key.getAdditionalTags();
writer.startArray(additionalTags.size());
for (UTF8BytesString tag : additionalTags) {
writer.writeUTF8(tag);
}

if (hasServiceSource) {
writer.writeUTF8(SERVICE_SOURCE);
writer.writeUTF8(key.getServiceSource());
Expand Down
4 changes: 2 additions & 2 deletions dd-trace-core/src/test/groovy/datadog/trace/common/metrics/AggregateMetricTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class AggregateMetricTest extends DDSpecification {
given:
AggregateMetric aggregate = new AggregateMetric().recordDurations(3, new AtomicLongArray(0L, 0L, 0L | ERROR_TAG | TOP_LEVEL_TAG))

Batch batch = new Batch().reset(new MetricKey("foo", "bar", "qux", null, "type", 0, false, true, "corge", [UTF8BytesString.create("grault:quux")], null, null, null))
Batch batch = new Batch().reset(new MetricKey("foo", "bar", "qux", null, "type", 0, false, true, "corge", [UTF8BytesString.create("grault:quux")], null, null, null, null))
batch.add(0L, 10)
batch.add(0L, 10)
batch.add(0L, 10)
Expand Down Expand Up @@ -140,7 +140,7 @@ class AggregateMetricTest extends DDSpecification {
def "consistent under concurrent attempts to read and write"() {
given:
AggregateMetric aggregate = new AggregateMetric()
MetricKey key = new MetricKey("foo", "bar", "qux", null, "type", 0, false, true, "corge", [UTF8BytesString.create("grault:quux")], null, null, null)
MetricKey key = new MetricKey("foo", "bar", "qux", null, "type", 0, false, true, "corge", [UTF8BytesString.create("grault:quux")], null, null, null, null)
BlockingDeque<Batch> queue = new LinkedBlockingDeque<>(1000)
ExecutorService reader = Executors.newSingleThreadExecutor()
int writerCount = 10
Expand Down
Loading
Loading