From 9bcaffdf9021c50b9c66dc4a3daafd4d08370308 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 23 Oct 2026 15:06:57 -0400 Subject: [PATCH] Take the span's tag lock once in setTagsFrom SpanData is what a TagContributor or TagExtractor may read and set on a span: the setTag overloads (by name and by id) and getTag, with the span's tag semantics (a null value removes the tag, the interceptor runs), plus setResourceName, setError and setMeasured. AgentSpan extends it with covariant returns, and the contributor and extractor now take a SpanData. DDSpanContext is a SpanData too: its setTag family returns this. Its setTagsFrom takes the tag lock once and hands the contributor a HeldSpanData that lives only for that call: it forwards to new package-private *HoldingLock variants of setTag, removeTag and getTag (the same bodies without the synchronized), and asserts the lock is still held. Contributor call sites are monomorphic by design, so the wrapper should be scalar-replaced. The public setters are unchanged. SetTagsFromBenchmark compares a jdbc span's nine tags one by one, through a contributor and through an extractor, on a reused and a fresh span. Co-Authored-By: Claude Opus 5.5 --- .../trace/core/SetTagsFromBenchmark.java | 147 +++++ .../main/java/datadog/trace/core/DDSpan.java | 26 + .../datadog/trace/core/DDSpanContext.java | 510 ++++++++++++++++-- .../trace/core/DDSpanSetTagsFromTest.java | 147 +++++ .../instrumentation/api/AgentSpan.java | 3 +- .../instrumentation/api/SpanData.java | 59 ++ .../instrumentation/api/TagContributor.java | 11 +- .../instrumentation/api/TagExtractor.java | 9 +- 8 files changed, 867 insertions(+), 45 deletions(-) create mode 100644 dd-trace-core/src/jmh/java/datadog/trace/core/SetTagsFromBenchmark.java create mode 100644 dd-trace-core/src/test/java/datadog/trace/core/DDSpanSetTagsFromTest.java create mode 100644 internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanData.java diff --git a/dd-trace-core/src/jmh/java/datadog/trace/core/SetTagsFromBenchmark.java b/dd-trace-core/src/jmh/java/datadog/trace/core/SetTagsFromBenchmark.java new file mode 100644 index 00000000000..f7fc315872e --- /dev/null +++ b/dd-trace-core/src/jmh/java/datadog/trace/core/SetTagsFromBenchmark.java @@ -0,0 +1,147 @@ +package datadog.trace.core; + +import static java.util.concurrent.TimeUnit.NANOSECONDS; + +import datadog.trace.api.KnownTags; +import datadog.trace.bootstrap.instrumentation.api.SpanData; +import datadog.trace.bootstrap.instrumentation.api.TagContributor; +import datadog.trace.bootstrap.instrumentation.api.TagExtractor; +import datadog.trace.bootstrap.instrumentation.api.Tags; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +/** + * Measures {@code DDSpan.setTagsFrom}, which takes the span's tag lock once for a whole batch, + * against the same tags set one {@code setTag} call (and one lock) at a time. + * + *

The batch is a jdbc span's nine tags: eight known tags by id, among them an int ({@code + * peer.port}) and an intercepted tag ({@code span.kind}), and one custom tag. They come from a + * {@code DbInfo}-style POJO, the case {@link TagContributor} is for, or through a {@link + * TagExtractor}. + * + *

+ * + *

Run with {@code -prof gc} to check that the batch's short-lived {@code SpanData} is + * scalar-replaced: the batched arms should allocate no more than the one-by-one arms. + */ +@State(Scope.Thread) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 5, time = 1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(NANOSECONDS) +@Fork(value = 3, jvmArgsAppend = "-DTEST_LOG_LEVEL=warn") +public class SetTagsFromBenchmark { + /** A jdbc span's connection facts, contributed as tags: the shape TagContributor is for. */ + static final class DbInfo implements TagContributor { + final String type = "postgresql"; + final String pool = "HikariPool-1"; + final String user = "app"; + final String instance = "petclinic"; + final String host = "db.internal"; + final int port = 54321; // outside Integer's box cache, like a real port + final String operation = "SELECT"; + final String statement = "SELECT * FROM owners WHERE id = ?"; + + @Override + public void addTo(SpanData span) { + span.setTag(KnownTags.SPAN_KIND_ID, Tags.SPAN_KIND_CLIENT); + span.setTag(KnownTags.COMPONENT_ID, "java-jdbc-prepared_statement"); + span.setTag(KnownTags.DB_TYPE_ID, type); + span.setTag(KnownTags.DB_POOL_NAME_ID, pool); + span.setTag(KnownTags.DB_USER_ID, user); + span.setTag(KnownTags.DB_INSTANCE_ID, instance); + span.setTag(KnownTags.PEER_HOSTNAME_ID, host); + span.setTag(KnownTags.PEER_PORT_ID, port); + span.setTag(KnownTags.DB_OPERATION_ID, operation); + span.setTag("db.statement.custom", statement); + } + } + + /** The same writes from a foreign source, through an extractor held in a constant. */ + static final class DbInfoExtractor implements TagExtractor { + static final DbInfoExtractor INSTANCE = new DbInfoExtractor(); + + @Override + public void extract(DbInfo info, SpanData span) { + info.addTo(span); + } + } + + static final DbInfo DB_INFO = new DbInfo(); + + private CoreTracer tracer; + private DDSpan span; + + @Setup + public void setup(Blackhole blackhole) { + tracer = + CoreTracer.builder().writer(new TraceShapeBenchmark.SizeRecordingWriter(blackhole)).build(); + span = (DDSpan) tracer.startSpan("java-jdbc", "database.query"); + } + + @TearDown + public void tearDown() { + span.finish(); + tracer.close(); + } + + /** The batch one setTag at a time: DDSpan is a SpanData too, so the same addTo applies. */ + private static void oneByOne(DDSpan span) { + DB_INFO.addTo(span); + } + + @Benchmark + public Object reused_oneByOne() { + oneByOne(span); + return span; + } + + @Benchmark + public Object reused_contributor() { + return span.setTagsFrom(DB_INFO); + } + + @Benchmark + public Object reused_extractor() { + return span.setTagsFrom(DB_INFO, DbInfoExtractor.INSTANCE); + } + + @Benchmark + public Object fresh_oneByOne() { + DDSpan fresh = (DDSpan) tracer.startSpan("java-jdbc", "database.query"); + oneByOne(fresh); + fresh.finish(); + return fresh; + } + + @Benchmark + public Object fresh_contributor() { + DDSpan fresh = (DDSpan) tracer.startSpan("java-jdbc", "database.query"); + fresh.setTagsFrom(DB_INFO); + fresh.finish(); + return fresh; + } + + @Benchmark + public Object fresh_extractor() { + DDSpan fresh = (DDSpan) tracer.startSpan("java-jdbc", "database.query"); + fresh.setTagsFrom(DB_INFO, DbInfoExtractor.INSTANCE); + fresh.finish(); + return fresh; + } +} diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java index e15a7ed49cb..df38b71aed7 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java @@ -22,6 +22,7 @@ import datadog.trace.api.TagMap; import datadog.trace.api.TraceConfig; import datadog.trace.api.debugger.DebuggerConfigBridge; +import datadog.trace.api.function.StrategyConsumer; import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.internal.VisibleForTesting; @@ -37,6 +38,8 @@ import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities; import datadog.trace.bootstrap.instrumentation.api.SpanPrototype; import datadog.trace.bootstrap.instrumentation.api.SpanWrapper; +import datadog.trace.bootstrap.instrumentation.api.TagContributor; +import datadog.trace.bootstrap.instrumentation.api.TagExtractor; import datadog.trace.core.util.StackTraces; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.ArrayList; @@ -589,6 +592,29 @@ public DDSpan setTag(final long tagId, final Object value) { return this; } + /** + * Applies {@code extractor} holding the span's tag lock once for the whole call, instead of once + * per {@code setTag}; the extractor sees the span's usual tag semantics. Kept small so it inlines + * and each call site sees its exact extractor type. + */ + @Override + @StrategyConsumer + public DDSpan setTagsFrom(final T source, final TagExtractor extractor) { + if (source != null) { + context.setTagsFrom(source, extractor); + } + return this; + } + + /** Like {@link #setTagsFrom(Object, TagExtractor)}, for a {@link TagContributor}. */ + @Override + public DDSpan setTagsFrom(final TagContributor contributor) { + if (contributor != null) { + context.setTagsFrom(contributor); + } + return this; + } + @Override public AgentSpan setAllTags(Map map) { context.setAllTags(map); diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java index 79aa8468b1b..d7c39873de9 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java @@ -27,6 +27,7 @@ import datadog.trace.api.cache.DDCaches; import datadog.trace.api.config.TracerConfig; import datadog.trace.api.datastreams.PathwayContext; +import datadog.trace.api.function.NoEscape; import datadog.trace.api.gateway.BlockResponseFunction; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; @@ -38,10 +39,14 @@ import datadog.trace.bootstrap.instrumentation.api.AppendableSpanLinks; import datadog.trace.bootstrap.instrumentation.api.Baggage; import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData; +import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; import datadog.trace.bootstrap.instrumentation.api.ProfilerContext; import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration; import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities; +import datadog.trace.bootstrap.instrumentation.api.SpanData; import datadog.trace.bootstrap.instrumentation.api.SpanPrototype; +import datadog.trace.bootstrap.instrumentation.api.TagContributor; +import datadog.trace.bootstrap.instrumentation.api.TagExtractor; import datadog.trace.bootstrap.instrumentation.api.Tags; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; import datadog.trace.core.propagation.PropagationTags; @@ -74,7 +79,12 @@ * the associated Span instance */ public class DDSpanContext - implements AgentSpanContext, RequestContext, TraceSegment, ProfilerContext, SpanStorage { + implements AgentSpanContext, + RequestContext, + TraceSegment, + ProfilerContext, + SpanStorage, + SpanData { private static final Logger log = LoggerFactory.getLogger(DDSpanContext.class); public static final String PRIORITY_SAMPLING_KEY = "_sampling_priority_v1"; @@ -672,10 +682,12 @@ public boolean isMeasured() { return measured; } - public void setMeasured(boolean measured) { + @Override + public DDSpanContext setMeasured(boolean measured) { if (measured != this.measured) { this.measured = measured; } + return this; } public boolean isTopLevel() { @@ -1429,9 +1441,10 @@ private boolean isPresent(TagMap.EntryReader entry) { * @param tag The tag name. * @param value The nullable tag value. */ - public void setTag(final String tag, final Object value) { + @Override + public DDSpanContext setTag(final String tag, final Object value) { if (null == tag) { - return; + return this; } final long tagId = KnownTagCodec.keyOf(tag); if (tagId != 0) { @@ -1443,11 +1456,13 @@ public void setTag(final String tag, final Object value) { customTagsForWrite().set(tag, value); // already known to be custom } } + return this; } - public void setTag(final String tag, final String value) { + @Override + public DDSpanContext setTag(final String tag, final String value) { if (null == tag) { - return; + return this; } final long tagId = KnownTagCodec.keyOf(tag); if (tagId != 0) { @@ -1459,6 +1474,7 @@ public void setTag(final String tag, final String value) { customTagsForWrite().set(tag, value); // already known to be custom } } + return this; } /* @@ -1466,39 +1482,44 @@ public void setTag(final String tag, final String value) { * KnownTagCodec.INTERCEPTED test folds, leaving only the split-by-tags table load; an id set never * takes the custom-tag path. An id that names no known tag is ignored, like a null tag. */ - public void setTag(final long tagId, final Object value) { + @Override + public DDSpanContext setTag(final long tagId, final Object value) { if (!KnownTagCodec.isKnown(tagId)) { - return; + return this; } if (null == value) { removeTag(tagId); - return; + return this; } if (!tagInterceptor.needsIntercept(tagId) || !tagInterceptor.interceptTag(this, tagId, value)) { synchronized (knownTags) { knownTags.set(tagId, value); } } + return this; } - public void setTag(final long tagId, final CharSequence value) { + @Override + public DDSpanContext setTag(final long tagId, final CharSequence value) { if (!KnownTagCodec.isKnown(tagId)) { - return; + return this; } if (null == value) { removeTag(tagId); - return; + return this; } if (!tagInterceptor.needsIntercept(tagId) || !tagInterceptor.interceptTag(this, tagId, value)) { synchronized (knownTags) { knownTags.set(tagId, value); } } + return this; } - public void setTag(final long tagId, final boolean value) { + @Override + public DDSpanContext setTag(final long tagId, final boolean value) { if (!KnownTagCodec.isKnown(tagId)) { - return; + return this; } if (tagInterceptor.needsIntercept(tagId)) { this.setBox(tagId, value); @@ -1507,11 +1528,13 @@ public void setTag(final long tagId, final boolean value) { knownTags.set(tagId, value); } } + return this; } - public void setTag(final long tagId, final int value) { + @Override + public DDSpanContext setTag(final long tagId, final int value) { if (!KnownTagCodec.isKnown(tagId)) { - return; + return this; } if (tagInterceptor.needsIntercept(tagId)) { this.setBox(tagId, value); @@ -1520,11 +1543,13 @@ public void setTag(final long tagId, final int value) { knownTags.set(tagId, value); } } + return this; } - public void setTag(final long tagId, final long value) { + @Override + public DDSpanContext setTag(final long tagId, final long value) { if (!KnownTagCodec.isKnown(tagId)) { - return; + return this; } if (tagInterceptor.needsIntercept(tagId)) { this.setBox(tagId, value); @@ -1533,11 +1558,13 @@ public void setTag(final long tagId, final long value) { knownTags.set(tagId, value); } } + return this; } - public void setTag(final long tagId, final float value) { + @Override + public DDSpanContext setTag(final long tagId, final float value) { if (!KnownTagCodec.isKnown(tagId)) { - return; + return this; } if (tagInterceptor.needsIntercept(tagId)) { this.setBox(tagId, value); @@ -1546,11 +1573,13 @@ public void setTag(final long tagId, final float value) { knownTags.set(tagId, value); } } + return this; } - public void setTag(final long tagId, final double value) { + @Override + public DDSpanContext setTag(final long tagId, final double value) { if (!KnownTagCodec.isKnown(tagId)) { - return; + return this; } if (tagInterceptor.needsIntercept(tagId)) { this.setBox(tagId, value); @@ -1559,11 +1588,13 @@ public void setTag(final long tagId, final double value) { knownTags.set(tagId, value); } } + return this; } - public void setTag(TagMap.EntryReader entry) { + @Override + public DDSpanContext setTag(TagMap.EntryReader entry) { if (entry == null) { - return; + return this; } // the interceptor prechecks the entry, to avoid boxing @@ -1572,6 +1603,7 @@ public void setTag(TagMap.EntryReader entry) { routePut(entry); } } + return this; } /* @@ -1594,9 +1626,10 @@ private void setBox(long tagId, Object box) { } } - public void setTag(final String tag, final boolean value) { + @Override + public DDSpanContext setTag(final String tag, final boolean value) { if (null == tag) { - return; + return this; } final long tagId = KnownTagCodec.keyOf(tag); if (tagId != 0) { @@ -1607,11 +1640,13 @@ public void setTag(final String tag, final boolean value) { customTagsForWrite().set(tag, value); // already known to be custom } } + return this; } - public void setTag(final String tag, final int value) { + @Override + public DDSpanContext setTag(final String tag, final int value) { if (null == tag) { - return; + return this; } final long tagId = KnownTagCodec.keyOf(tag); if (tagId != 0) { @@ -1622,11 +1657,13 @@ public void setTag(final String tag, final int value) { customTagsForWrite().set(tag, value); // already known to be custom } } + return this; } - public void setTag(final String tag, final long value) { + @Override + public DDSpanContext setTag(final String tag, final long value) { if (null == tag) { - return; + return this; } final long tagId = KnownTagCodec.keyOf(tag); if (tagId != 0) { @@ -1637,11 +1674,13 @@ public void setTag(final String tag, final long value) { customTagsForWrite().set(tag, value); // already known to be custom } } + return this; } - public void setTag(final String tag, final float value) { + @Override + public DDSpanContext setTag(final String tag, final float value) { if (null == tag) { - return; + return this; } final long tagId = KnownTagCodec.keyOf(tag); if (tagId != 0) { @@ -1652,11 +1691,13 @@ public void setTag(final String tag, final float value) { customTagsForWrite().set(tag, value); // already known to be custom } } + return this; } - public void setTag(final String tag, final double value) { + @Override + public DDSpanContext setTag(final String tag, final double value) { if (null == tag) { - return; + return this; } final long tagId = KnownTagCodec.keyOf(tag); if (tagId != 0) { @@ -1667,6 +1708,404 @@ public void setTag(final String tag, final double value) { customTagsForWrite().set(tag, value); // already known to be custom } } + return this; + } + + /* + * The *HoldingLock variants: the same tag semantics as setTag / removeTag / getTag (a null value + * removes, the tag interceptor runs), for a caller that already holds the knownTags lock, so a + * batch takes the lock once. Only HeldSpanData calls them, during setTagsFrom. + */ + + void setTagHoldingLock(final String tag, final Object value) { + if (null == tag) { + return; + } + final long tagId = KnownTagCodec.keyOf(tag); + if (tagId != 0) { + setTagHoldingLock(tagId, value); + } else if (null == value) { + removeTagHoldingLock(tag); + } else if (!tagInterceptor.interceptCustomTag(this, tag, value)) { + customTagsForWrite().set(tag, value); // already known to be custom + } + } + + void setTagHoldingLock(final String tag, final String value) { + if (null == tag) { + return; + } + final long tagId = KnownTagCodec.keyOf(tag); + if (tagId != 0) { + setTagHoldingLock(tagId, value); + } else if (null == value) { + removeTagHoldingLock(tag); + } else if (!tagInterceptor.interceptCustomTag(this, tag, value)) { + customTagsForWrite().set(tag, value); // already known to be custom + } + } + + void setTagHoldingLock(final long tagId, final Object value) { + if (!KnownTagCodec.isKnown(tagId)) { + return; + } + if (null == value) { + removeTagHoldingLock(tagId); + return; + } + if (!tagInterceptor.needsIntercept(tagId) || !tagInterceptor.interceptTag(this, tagId, value)) { + knownTags.set(tagId, value); + } + } + + void setTagHoldingLock(final long tagId, final CharSequence value) { + if (!KnownTagCodec.isKnown(tagId)) { + return; + } + if (null == value) { + removeTagHoldingLock(tagId); + return; + } + if (!tagInterceptor.needsIntercept(tagId) || !tagInterceptor.interceptTag(this, tagId, value)) { + knownTags.set(tagId, value); + } + } + + void setTagHoldingLock(final long tagId, final boolean value) { + if (!KnownTagCodec.isKnown(tagId)) { + return; + } + if (tagInterceptor.needsIntercept(tagId)) { + this.setBoxHoldingLock(tagId, value); + } else { + knownTags.set(tagId, value); + } + } + + void setTagHoldingLock(final long tagId, final int value) { + if (!KnownTagCodec.isKnown(tagId)) { + return; + } + if (tagInterceptor.needsIntercept(tagId)) { + this.setBoxHoldingLock(tagId, value); + } else { + knownTags.set(tagId, value); + } + } + + void setTagHoldingLock(final long tagId, final long value) { + if (!KnownTagCodec.isKnown(tagId)) { + return; + } + if (tagInterceptor.needsIntercept(tagId)) { + this.setBoxHoldingLock(tagId, value); + } else { + knownTags.set(tagId, value); + } + } + + void setTagHoldingLock(final long tagId, final float value) { + if (!KnownTagCodec.isKnown(tagId)) { + return; + } + if (tagInterceptor.needsIntercept(tagId)) { + this.setBoxHoldingLock(tagId, value); + } else { + knownTags.set(tagId, value); + } + } + + void setTagHoldingLock(final long tagId, final double value) { + if (!KnownTagCodec.isKnown(tagId)) { + return; + } + if (tagInterceptor.needsIntercept(tagId)) { + this.setBoxHoldingLock(tagId, value); + } else { + knownTags.set(tagId, value); + } + } + + void setTagHoldingLock(TagMap.EntryReader entry) { + if (entry == null) { + return; + } + + // the interceptor prechecks the entry, to avoid boxing + if (!tagInterceptor.interceptTag(this, entry)) { + routePut(entry); + } + } + + void setTagHoldingLock(final String tag, final boolean value) { + if (null == tag) { + return; + } + final long tagId = KnownTagCodec.keyOf(tag); + if (tagId != 0) { + setTagHoldingLock(tagId, value); + } else if (!tagInterceptor.needsInterceptCustomTag(tag) + || !tagInterceptor.interceptCustomTag(this, tag, value)) { + customTagsForWrite().set(tag, value); // already known to be custom + } + } + + void setTagHoldingLock(final String tag, final int value) { + if (null == tag) { + return; + } + final long tagId = KnownTagCodec.keyOf(tag); + if (tagId != 0) { + setTagHoldingLock(tagId, value); + } else if (!tagInterceptor.needsInterceptCustomTag(tag) + || !tagInterceptor.interceptCustomTag(this, tag, value)) { + customTagsForWrite().set(tag, value); // already known to be custom + } + } + + void setTagHoldingLock(final String tag, final long value) { + if (null == tag) { + return; + } + final long tagId = KnownTagCodec.keyOf(tag); + if (tagId != 0) { + setTagHoldingLock(tagId, value); + } else if (!tagInterceptor.needsInterceptCustomTag(tag) + || !tagInterceptor.interceptCustomTag(this, tag, value)) { + customTagsForWrite().set(tag, value); // already known to be custom + } + } + + void setTagHoldingLock(final String tag, final float value) { + if (null == tag) { + return; + } + final long tagId = KnownTagCodec.keyOf(tag); + if (tagId != 0) { + setTagHoldingLock(tagId, value); + } else if (!tagInterceptor.needsInterceptCustomTag(tag) + || !tagInterceptor.interceptCustomTag(this, tag, value)) { + customTagsForWrite().set(tag, value); // already known to be custom + } + } + + void setTagHoldingLock(final String tag, final double value) { + if (null == tag) { + return; + } + final long tagId = KnownTagCodec.keyOf(tag); + if (tagId != 0) { + setTagHoldingLock(tagId, value); + } else if (!tagInterceptor.needsInterceptCustomTag(tag) + || !tagInterceptor.interceptCustomTag(this, tag, value)) { + customTagsForWrite().set(tag, value); // already known to be custom + } + } + + void removeTagHoldingLock(String tag) { + if (tagEquals(tag, Tags.SPAN_KIND)) { + // Clear the cached ordinal; the tag store still needs to be updated below. + spanKindOrdinal = SPAN_KIND_UNSET; + } + routeRemoval(tag); + } + + void removeTagHoldingLock(long tagId) { + if (!KnownTagCodec.isKnown(tagId)) { + return; + } + if (tagId == KnownTags.SPAN_KIND_ID) { + spanKindOrdinal = SPAN_KIND_UNSET; + } + knownTags.remove(tagId); + } + + private void setBoxHoldingLock(long tagId, Object box) { + if (!tagInterceptor.interceptTag(this, tagId, box)) { + knownTags.set(tagId, box); + } + } + + Object getTagHoldingLock(final String key) { + switch (key) { + case DDTags.THREAD_ID: + return threadId; + case DDTags.THREAD_NAME: + // maintain previously observable type of the thread name :| + return threadName.toString(); + case Tags.HTTP_STATUS: + case KnownTags.HTTP_STATUS_CODE_OTEL_NAME: + return 0 == httpStatusCode ? null : (int) httpStatusCode; + case Tags.SPAN_KIND: + return getSpanKindString(); + default: + Object value = unsafeGetTag(key); + // maintain previously observable type of http url :| + return value == null ? null : Tags.HTTP_URL.equals(key) ? value.toString() : value; + } + } + + @Override + public DDSpanContext setResourceName(final CharSequence resourceName) { + setResourceName(resourceName, ResourceNamePriorities.DEFAULT); + return this; + } + + @Override + public DDSpanContext setError(final boolean error) { + setErrorFlag(error, ErrorPriorities.DEFAULT); + return this; + } + + /** + * Applies {@code extractor} to {@code source} holding the tag lock once for the whole call: the + * extractor's writes go through a {@link HeldSpanData}, which keeps the span's tag semantics but + * doesn't lock again. Kept small so it inlines and each call site sees its exact extractor type. + */ + void setTagsFrom(final T source, final TagExtractor extractor) { + synchronized (knownTags) { + extractor.extract(source, new HeldSpanData(this)); + } + } + + /** {@link #setTagsFrom(Object, TagExtractor)} for a {@link TagContributor}. */ + void setTagsFrom(final TagContributor contributor) { + synchronized (knownTags) { + contributor.addTo(new HeldSpanData(this)); + } + } + + /** + * The span as a {@link SpanData} for the length of one {@code setTagsFrom} call, which holds the + * tag lock throughout: tag writes and reads skip relocking. It must not outlive the call; with + * the contributor's call site monomorphic, it is scalar-replaced. + */ + @NoEscape + static final class HeldSpanData implements SpanData { + private final DDSpanContext span; + + HeldSpanData(DDSpanContext span) { + this.span = span; + } + + /** The span, checking (with assertions on) that this is still inside its setTagsFrom call. */ + private DDSpanContext span() { + assert Thread.holdsLock(span.knownTags) : "HeldSpanData used outside its setTagsFrom call"; + return span; + } + + @Override + public SpanData setTag(String key, Object value) { + span().setTagHoldingLock(key, value); + return this; + } + + @Override + public SpanData setTag(String key, String value) { + span().setTagHoldingLock(key, value); + return this; + } + + @Override + public SpanData setTag(String key, boolean value) { + span().setTagHoldingLock(key, value); + return this; + } + + @Override + public SpanData setTag(String key, int value) { + span().setTagHoldingLock(key, value); + return this; + } + + @Override + public SpanData setTag(String key, long value) { + span().setTagHoldingLock(key, value); + return this; + } + + @Override + public SpanData setTag(String key, float value) { + span().setTagHoldingLock(key, value); + return this; + } + + @Override + public SpanData setTag(String key, double value) { + span().setTagHoldingLock(key, value); + return this; + } + + @Override + public SpanData setTag(long tagId, Object value) { + span().setTagHoldingLock(tagId, value); + return this; + } + + @Override + public SpanData setTag(long tagId, CharSequence value) { + span().setTagHoldingLock(tagId, value); + return this; + } + + @Override + public SpanData setTag(long tagId, boolean value) { + span().setTagHoldingLock(tagId, value); + return this; + } + + @Override + public SpanData setTag(long tagId, int value) { + span().setTagHoldingLock(tagId, value); + return this; + } + + @Override + public SpanData setTag(long tagId, long value) { + span().setTagHoldingLock(tagId, value); + return this; + } + + @Override + public SpanData setTag(long tagId, float value) { + span().setTagHoldingLock(tagId, value); + return this; + } + + @Override + public SpanData setTag(long tagId, double value) { + span().setTagHoldingLock(tagId, value); + return this; + } + + @Override + public SpanData setTag(TagMap.EntryReader entry) { + span().setTagHoldingLock(entry); + return this; + } + + @Override + public Object getTag(String key) { + return span().getTagHoldingLock(key); + } + + @Override + public SpanData setResourceName(CharSequence resourceName) { + span().setResourceName(resourceName); + return this; + } + + @Override + public SpanData setError(boolean error) { + span().setError(error); + return this; + } + + @Override + public SpanData setMeasured(boolean measured) { + span().setMeasured(measured); + return this; + } } void setAllTags(final TagMap map) { @@ -1752,7 +2191,8 @@ void unsafeSetTag(final String tag, final double value) { routeSet(tag, value); } - Object getTag(final String key) { + @Override + public Object getTag(final String key) { switch (key) { case DDTags.THREAD_ID: return threadId; diff --git a/dd-trace-core/src/test/java/datadog/trace/core/DDSpanSetTagsFromTest.java b/dd-trace-core/src/test/java/datadog/trace/core/DDSpanSetTagsFromTest.java new file mode 100644 index 00000000000..a6c1a573bd4 --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/core/DDSpanSetTagsFromTest.java @@ -0,0 +1,147 @@ +package datadog.trace.core; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.trace.api.DDTags; +import datadog.trace.api.KnownTags; +import datadog.trace.bootstrap.instrumentation.api.SpanData; +import datadog.trace.bootstrap.instrumentation.api.TagContributor; +import datadog.trace.bootstrap.instrumentation.api.TagExtractor; +import datadog.trace.bootstrap.instrumentation.api.Tags; +import datadog.trace.common.writer.ListWriter; +import java.util.TreeMap; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * {@code DDSpan.setTagsFrom}: the contributor or extractor runs holding the span's tag lock once, + * through a short-lived {@link SpanData} that keeps the span's tag semantics. + */ +class DDSpanSetTagsFromTest extends DDCoreJavaSpecification { + private CoreTracer tracer; + + @BeforeEach + void setup() { + tracer = tracerBuilder().writer(new ListWriter()).build(); + } + + @AfterEach + void tearDown() { + tracer.close(); + } + + private DDSpan start() { + return (DDSpan) tracer.startSpan("test-instr", "tags-from.op"); + } + + /** + * A non-String value, shared so both spans hold the same object (StringBuilder has no equals). + */ + private static final Object OBJECT_VALUE = new StringBuilder("built"); + + /** The same writes either way: known tags by id and by name, custom tags, every value type. */ + private static void writeTags(SpanData span) { + span.setTag(KnownTags.HTTP_ROUTE_ID, "/users/{id}"); + span.setTag(KnownTags.HTTP_STATUS_CODE_ID, 503); + span.setTag(Tags.COMPONENT, "jdbc"); + span.setTag("custom.string", "value"); + span.setTag("custom.object", OBJECT_VALUE); + span.setTag("custom.bool", true); + span.setTag("custom.int", 7); + span.setTag("custom.long", 8L); + span.setTag("custom.float", 1.5f); + span.setTag("custom.double", 2.5d); + span.setTag("custom.removed", "gone"); + span.setTag("custom.removed", (Object) null); + } + + @Test + void aContributorSetsWhatTheSameSetTagCallsWouldSet() { + DDSpan oneByOne = start(); + DDSpan batched = start(); + try { + writeTags(oneByOne); + TagContributor contributor = DDSpanSetTagsFromTest::writeTags; + assertSame(batched, batched.setTagsFrom(contributor)); + + // compared as plain maps: TagMap.equals isn't Map content equality + assertEquals(new TreeMap<>(oneByOne.getTags()), new TreeMap<>(batched.getTags())); + assertNull(batched.getTag("custom.removed")); + } finally { + oneByOne.finish(); + batched.finish(); + } + } + + @Test + void anExtractorSeesTheSpanTagSemanticsIncludingInterception() { + DDSpan span = start(); + try { + TagExtractor extractor = + (resource, target) -> { + target.setTag(DDTags.RESOURCE_NAME, resource); // intercepted: becomes the resource + target.setTag(Tags.SPAN_KIND, Tags.SPAN_KIND_CLIENT); // intercepted: the cached kind + target.setTag(KnownTags.HTTP_ROUTE_ID, "/orders"); + // reads inside the batch see the batch's own writes + target.setTag("custom.route.seen", String.valueOf(target.getTag(Tags.HTTP_ROUTE))); + }; + span.setTagsFrom("GET /orders", extractor); + + assertEquals("GET /orders", span.getResourceName().toString()); + assertEquals(Tags.SPAN_KIND_CLIENT, span.getSpanKindString()); + assertEquals("/orders", span.getTag("custom.route.seen")); + } finally { + span.finish(); + } + } + + @Test + void theSpanLevelSettersWorkInsideTheBatch() { + DDSpan span = start(); + try { + span.setTagsFrom( + (TagContributor) + target -> target.setResourceName("batched").setError(true).setMeasured(true)); + + assertEquals("batched", span.getResourceName().toString()); + assertTrue(span.isError()); + assertTrue(span.isMeasured()); + } finally { + span.finish(); + } + } + + @Test + void aNullSourceOrContributorIsSkipped() { + DDSpan span = start(); + try { + TagExtractor extractor = (source, target) -> target.setTag("custom.ran", true); + assertSame(span, span.setTagsFrom(null, extractor)); + assertSame(span, span.setTagsFrom((TagContributor) null)); + assertFalse(span.getTags().containsKey("custom.ran")); + } finally { + span.finish(); + } + } + + @Test + void theBatchsSpanDataMustNotOutliveTheCall() { + DDSpan span = start(); + try { + AtomicReference kept = new AtomicReference<>(); + span.setTagsFrom((TagContributor) kept::set); + + // assertions are on in tests, so a write outside the lock is caught + assertThrows(AssertionError.class, () -> kept.get().setTag("custom.late", "late")); + } finally { + span.finish(); + } + } +} diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java index 5e3918f123c..388d311c7b2 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java @@ -27,7 +27,8 @@ public interface AgentSpan ImplicitContextKeyed, Context, IGSpanInfo, - WithAgentSpan { + WithAgentSpan, + SpanData { /** * Extracts the span from the current {@link Context}. diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanData.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanData.java new file mode 100644 index 00000000000..df5d10c93a4 --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanData.java @@ -0,0 +1,59 @@ +package datadog.trace.bootstrap.instrumentation.api; + +import datadog.trace.api.TagMap; + +/** + * What a {@link TagContributor} or {@link TagExtractor} may read and set on a span: its tags, with + * the span's full tag semantics (a null value removes the tag, and the tag interceptor runs), and + * the few span-level fields that aren't yet tags. + * + *

The intercepted counterpart of {@link datadog.trace.api.SpanStorage}, which loads and stores + * raw. It uses the span's own vocabulary, {@code getTag}/{@code setTag}, so contributor code reads + * like span code. Unlike OpenTelemetry's {@code SpanData}, it is not a snapshot: writes go to the + * live span. + * + *

{@link AgentSpan#setTagsFrom} may hand its contributor a short-lived {@code SpanData} that + * holds the span's tag lock for the whole call, so a run of {@code setTag} calls takes the lock + * once. Such an instance is valid only during that call: don't keep it. + */ +public interface SpanData { + SpanData setTag(String key, Object value); + + SpanData setTag(String key, String value); + + SpanData setTag(String key, boolean value); + + SpanData setTag(String key, int value); + + SpanData setTag(String key, long value); + + SpanData setTag(String key, float value); + + SpanData setTag(String key, double value); + + /** Sets a known tag by its {@code KnownTags.*_ID}; an id that names no known tag is ignored. */ + SpanData setTag(long tagId, Object value); + + SpanData setTag(long tagId, CharSequence value); + + SpanData setTag(long tagId, boolean value); + + SpanData setTag(long tagId, int value); + + SpanData setTag(long tagId, long value); + + SpanData setTag(long tagId, float value); + + SpanData setTag(long tagId, double value); + + /** A null entry leaves the tags unchanged. */ + SpanData setTag(TagMap.EntryReader entry); + + Object getTag(String key); + + SpanData setResourceName(CharSequence resourceName); + + SpanData setError(boolean error); + + SpanData setMeasured(boolean measured); +} diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagContributor.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagContributor.java index 637600b4203..1bc2c8b9c62 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagContributor.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagContributor.java @@ -13,12 +13,13 @@ * List} sink — that re-megamorphizes and recreates the decorator problem. Prefer * stateless implementations. * - *

Targets {@link AgentSpan} for now (so it can also drive span-level state — resource name, - * error, measured — that is not yet expressed as tags). As those span fields migrate into the tag - * model, this surface narrows toward a future {@code addTo(TagMap)}. See {@link TagExtractor} for - * the extrinsic counterpart (foreign objects we do not own). Apply it with {@link + *

Targets {@link SpanData}: the span's tags with full tag semantics, plus the span-level state + * (resource name, error, measured) that is not yet expressed as tags. {@link + * AgentSpan#setTagsFrom(TagContributor)} may hand it a short-lived {@code SpanData} that holds the + * span's tag lock for the whole call; don't keep it past {@code addTo}. See {@link TagExtractor} + * for the extrinsic counterpart (foreign objects we do not own). Apply it with {@link * AgentSpan#setTagsFrom(TagContributor)}. */ public interface TagContributor { - void addTo(AgentSpan span); + void addTo(SpanData span); } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagExtractor.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagExtractor.java index a62e10836b7..edc78c7e045 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagExtractor.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/TagExtractor.java @@ -23,9 +23,10 @@ * a non-capturing lambda constant is accepted but is the weaker, speculative shape. The consumer, * {@link AgentSpan#setTagsFrom(Object, TagExtractor)}, is small so it inlines and each call site * sees the exact extractor type. Don't hold extractors in a field or collection of the abstract - * type and dispatch from there. Targets {@link AgentSpan} so it can also drive span-level state - * (resource name, error, status) during the transition; that surface narrows as those fields - * migrate into the tag model. + * type and dispatch from there. Targets {@link SpanData}: the span's tags with full tag semantics, + * plus the span-level state (resource name, error, measured) not yet expressed as tags. {@link + * AgentSpan#setTagsFrom(Object, TagExtractor)} may hand it a short-lived {@code SpanData} that + * holds the span's tag lock for the whole call; don't keep it past {@code extract}. * *

Apply it with {@link AgentSpan#setTagsFrom(Object, TagExtractor)}. * @@ -34,5 +35,5 @@ @Strategy @FunctionalInterface public interface TagExtractor { - void extract(T source, AgentSpan span); + void extract(T source, SpanData span); }