From 0e15d6c6bf9704501cacef7fad80b8a0a4fb78a3 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Fri, 29 May 2026 22:46:29 -0400 Subject: [PATCH 1/3] Fix DDTraceId/DD64bTraceId class-init deadlock at the source DD64bTraceId is a subclass of DDTraceId, so the JVM must initialize DDTraceId before DD64bTraceId. DDTraceId. in turn initialized DD64bTraceId by building its ZERO/ONE constants via DD64bTraceId.from(), a circular initialization dependency. When the two classes were first touched concurrently from opposite ends -- the service-discovery task (muteTracing() -> blackholeSpan() -> DDTraceId.ZERO) racing the application's first span (IdGenerationStrategy.generateTraceId() -> DD64bTraceId.from()) -- each thread held one class-init lock and waited for the other, hanging trace creation. This surfaced as recurring 30s LogInjectionSmokeTest timeouts in CI (latent since #9705 added the scheduled muteTracing task). Break the cycle at its source while keeping DDTraceId.ZERO/ONE as public fields (preserving the API restored in #5021): ZERO/ONE are now instances of a private DDTraceId subtype (a sibling of DD64bTraceId), so DDTraceId. no longer references the subclass. Replace the fragile "== DDTraceId.ZERO" identity checks with a value-based DDTraceId.isZero(). Those identity checks relied on every zero id being the single ZERO instance; isZero() recognizes a zero id of any concrete type, so the factories need not route 0 to the singleton and the propagation codecs no longer mishandle a zero parsed via the direct 64-bit factories. Add a forked regression test that initializes the two classes concurrently from opposite ends (deadlocks without the fix), plus isZero() coverage across the DDTraceId subtypes. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../spark/AbstractDatadogSparkListener.java | 2 +- dd-trace-api/build.gradle.kts | 1 + .../java/datadog/trace/api/DD64bTraceId.java | 9 +- .../java/datadog/trace/api/DDTraceId.java | 88 ++++++++++++++++++- .../DDTraceIdClinitDeadlockForkedTest.java | 88 +++++++++++++++++++ .../trace/api/DDTraceIdConstantsTest.java | 40 +++++++++ .../java/datadog/trace/api/DDTraceIdTest.java | 7 +- .../java/datadog/trace/core/CoreTracer.java | 2 +- .../core/propagation/ContextInterpreter.java | 4 +- .../core/propagation/DatadogHttpCodec.java | 2 +- .../trace/core/propagation/XRayHttpCodec.java | 6 +- .../datadog/trace/api/TraceIdIsZeroTest.java | 40 +++++++++ .../instrumentation/api/AgentSpan.java | 2 +- 13 files changed, 271 insertions(+), 20 deletions(-) create mode 100644 dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdClinitDeadlockForkedTest.java create mode 100644 dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java create mode 100644 dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java diff --git a/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java b/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java index 817aa1f0d84..c1826f9e548 100644 --- a/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java +++ b/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java @@ -444,7 +444,7 @@ private void addDatabricksSpecificTags( AgentSpanContext parentContext = new DatabricksParentContext(databricksJobId, databricksJobRunId, databricksTaskRunId); - if (parentContext.getTraceId() != DDTraceId.ZERO) { + if (!parentContext.getTraceId().isZero()) { if (withParentContext) { builder.asChildOf(parentContext); } else { diff --git a/dd-trace-api/build.gradle.kts b/dd-trace-api/build.gradle.kts index bc05a8753a4..fc4924ba271 100644 --- a/dd-trace-api/build.gradle.kts +++ b/dd-trace-api/build.gradle.kts @@ -16,6 +16,7 @@ val excludedClassesCoverage by extra( "datadog.trace.api.DDTags", "datadog.trace.api.DDTraceApiInfo", "datadog.trace.api.DDTraceId", + "datadog.trace.api.DDTraceId.ConstantId", "datadog.trace.api.EventTracker", "datadog.trace.api.GlobalTracer*", "datadog.trace.api.PropagationStyle", diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java index f40b47a89d4..958e45f93f9 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java @@ -59,11 +59,10 @@ public static DD64bTraceId fromHex(String s) throws NumberFormatException { } static DD64bTraceId create(long id, String str) { - // ZERO constant is created and stored by the parent class as part of its API contract - // But initialized by this 64-bit child class. Ensures uniqueness of ZERO once created. - if (id == 0 && ZERO != null) { - return (DD64bTraceId) ZERO; - } else if (id == -1) { + // -1 (all bits set) reuses the MAX singleton. 0 is not special-cased: DDTraceId.ZERO is a + // sibling type (not a DD64bTraceId), so it cannot be returned here; a zero 64-bit id is simply + // a DD64bTraceId(0), and callers detect zero via DDTraceId.isZero() rather than by identity. + if (id == -1) { return MAX; } else { return new DD64bTraceId(id, str); diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java index 7ed5be915cc..03ec9e2538a 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java @@ -9,11 +9,25 @@ * The string representations are either kept from parsing, or generated on demand and cached. */ public abstract class DDTraceId { - /** Invalid TraceId value used to denote no TraceId. */ - public static final DDTraceId ZERO = from(0); + /** + * Invalid TraceId value used to denote no TraceId. + * + *

This is an instance of a private {@link DDTraceId} subtype (a sibling of {@link + * DD64bTraceId}), not a {@code DD64bTraceId}, and that is deliberate. {@code DD64bTraceId} is a + * subclass of {@code DDTraceId}, so the JVM must initialize {@code DDTraceId} before {@code + * DD64bTraceId}. If this constant were built via {@code DD64bTraceId.from(0)} (as it once was), + * {@code DDTraceId.} would initialize {@code DD64bTraceId} while holding the {@code + * DDTraceId} init lock, and two threads first touching the classes from opposite ends would + * deadlock on the two class-initialization locks. Building it from a sibling type keeps {@code + * DDTraceId.} free of any reference to the subclass. + * + *

To test whether an id is zero, prefer {@link #isZero()} over {@code == DDTraceId.ZERO}: a + * zero id parsed via the 64-bit factories is a distinct instance, not this singleton. + */ + public static final DDTraceId ZERO = new ConstantId(0, "0"); - /** Convenience constant used from tests */ - public static final DDTraceId ONE = from(1); + /** Convenience constant used from tests. See {@link #ZERO} for why this is a sibling type. */ + public static final DDTraceId ONE = new ConstantId(1, "1"); /** * Creates a new {@link DD64bTraceId 64-bit TraceId} from the given {@code long} interpreted as @@ -102,4 +116,70 @@ public static DDTraceId fromHex(String s) throws NumberFormatException { * 0 for 64-bit {@link DDTraceId} only. */ public abstract long toHighOrderLong(); + + /** + * Returns whether this {@link DDTraceId} is zero, the value used to denote no/invalid TraceId. + * + *

Prefer this over {@code == DDTraceId.ZERO}: it is value-based, so it recognizes a zero id + * regardless of its concrete type or how it was created (e.g. a zero parsed via the 64-bit + * factories, which is a distinct instance from the {@link #ZERO} singleton). + * + * @return {@code true} if both the high- and low-order 64 bits are zero. + */ + public boolean isZero() { + return toHighOrderLong() == 0 && toLong() == 0; + } + + /** + * Minimal concrete {@link DDTraceId} backing the {@link #ZERO} and {@link #ONE} constants. It is + * a sibling of {@link DD64bTraceId} (it extends {@link DDTraceId} directly), so constructing + * these constants in {@code DDTraceId.} never initializes {@code DD64bTraceId} (which + * would create a class-initialization deadlock; see {@link #ZERO}). It represents a 64-bit id and + * formats identically to the equivalent {@link DD64bTraceId}. + */ + private static final class ConstantId extends DDTraceId { + private final long id; + private final String str; + private String hexStr; // cache for hex string representation + + private ConstantId(long id, String str) { + this.id = id; + this.str = str; + } + + @Override + public String toString() { + return this.str; + } + + @Override + public String toHexString() { + String hexStr = this.hexStr; + // This race condition is intentional and benign. + // The worst that can happen is that an identical value is produced and written into the + // field. + if (hexStr == null) { + this.hexStr = hexStr = LongStringUtils.toHexStringPadded(this.id, 32); + } + return hexStr; + } + + @Override + public String toHexStringPadded(int size) { + if (size > 16) { + return toHexString(); + } + return LongStringUtils.toHexStringPadded(this.id, size); + } + + @Override + public long toLong() { + return this.id; + } + + @Override + public long toHighOrderLong() { + return 0; + } + } } diff --git a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdClinitDeadlockForkedTest.java b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdClinitDeadlockForkedTest.java new file mode 100644 index 00000000000..e838ef75177 --- /dev/null +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdClinitDeadlockForkedTest.java @@ -0,0 +1,88 @@ +package datadog.trace.api; + +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.fail; + +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +/** + * Regression test for the {@code DDTraceId} <-> {@code DD64bTraceId} class-initialization + * deadlock. + * + *

{@code DD64bTraceId} is a subclass of {@code DDTraceId}, so the JVM must initialize {@code + * DDTraceId} before {@code DD64bTraceId}. The bug was that {@code DDTraceId.} in turn + * initialized {@code DD64bTraceId} by building its {@code ZERO}/{@code ONE} constants via {@code + * DD64bTraceId.from(...)}. When the two classes were first touched concurrently from opposite ends + * (one thread initializing {@code DDTraceId}, another initializing {@code DD64bTraceId}), each + * thread held one class-initialization lock and waited for the other, hanging trace creation. This + * surfaced as 30s {@code LogInjectionSmokeTest} timeouts in CI. + * + *

{@code DDTraceId.ZERO}/{@code ONE} are now instances of a private sibling type (not {@code + * DD64bTraceId}), so {@code DDTraceId.} no longer references {@code DD64bTraceId} and the + * cycle is gone. This test initializes the two classes for the first time concurrently from + * opposite ends and asserts neither thread hangs. + * + *

Runs forked ({@code forkEvery = 1}) so it gets a fresh JVM in which these classes have not yet + * been initialized by another test. Without the fix it deadlocks and fails via the join check (and + * the {@code @Timeout} backstop); with the fix it completes immediately. + */ +class DDTraceIdClinitDeadlockForkedTest { + + @Test + @Timeout(value = 60, unit = SECONDS) // backstop; the join below is the primary guard + void traceIdClassPairInitializesConcurrentlyWithoutDeadlock() throws Exception { + final ClassLoader cl = getClass().getClassLoader(); + final CyclicBarrier barrier = new CyclicBarrier(2); + final AtomicReference error = new AtomicReference<>(); + + // One thread enters via the superclass (mirrors blackholeSpan() -> DDTraceId.ZERO), the other + // via the subclass (mirrors IdGenerationStrategy.generateTraceId() -> DD64bTraceId.from()). + Thread viaSuper = + new Thread( + () -> { + try { + barrier.await(); + Class.forName("datadog.trace.api.DDTraceId", true, cl); + } catch (Throwable t) { + error.compareAndSet(null, t); + } + }, + "init-DDTraceId"); + Thread viaSub = + new Thread( + () -> { + try { + barrier.await(); + Class.forName("datadog.trace.api.DD64bTraceId", true, cl); + } catch (Throwable t) { + error.compareAndSet(null, t); + } + }, + "init-DD64bTraceId"); + // Daemon so a deadlock cannot block forked-JVM shutdown. + viaSuper.setDaemon(true); + viaSub.setDaemon(true); + + viaSuper.start(); + viaSub.start(); + viaSuper.join(SECONDS.toMillis(15)); + viaSub.join(SECONDS.toMillis(15)); + + if (viaSuper.isAlive() || viaSub.isAlive()) { + fail( + "DDTraceId/DD64bTraceId class-initialization deadlock: DDTraceId. must not " + + "reference DD64bTraceId (init-DDTraceId.alive=" + + viaSuper.isAlive() + + ", init-DD64bTraceId.alive=" + + viaSub.isAlive() + + ")."); + } + if (error.get() != null) { + throw new AssertionError( + "Unexpected error during concurrent class initialization", error.get()); + } + } +} diff --git a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java new file mode 100644 index 00000000000..f65bc74853d --- /dev/null +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java @@ -0,0 +1,40 @@ +package datadog.trace.api; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +/** Behavior of the {@link DDTraceId#ZERO}/{@link DDTraceId#ONE} sibling constants. */ +class DDTraceIdConstantsTest { + + @Test + void zeroAndOneFormatLikeTheEquivalentDD64bTraceId() { + assertEquals("0", DDTraceId.ZERO.toString()); + assertEquals("00000000000000000000000000000000", DDTraceId.ZERO.toHexString()); + assertEquals("0000000000000000", DDTraceId.ZERO.toHexStringPadded(16)); + assertEquals("00000000000000000000000000000000", DDTraceId.ZERO.toHexStringPadded(32)); + assertEquals(0L, DDTraceId.ZERO.toLong()); + assertEquals(0L, DDTraceId.ZERO.toHighOrderLong()); + + assertEquals("1", DDTraceId.ONE.toString()); + assertEquals("00000000000000000000000000000001", DDTraceId.ONE.toHexString()); + assertEquals("0000000000000001", DDTraceId.ONE.toHexStringPadded(16)); + assertEquals(1L, DDTraceId.ONE.toLong()); + assertEquals(0L, DDTraceId.ONE.toHighOrderLong()); + } + + @Test + void isZeroReflectsTheValue() { + assertTrue(DDTraceId.ZERO.isZero()); + assertTrue(DDTraceId.from(0).isZero()); + assertTrue(DDTraceId.from("0").isZero()); + assertTrue(DDTraceId.fromHex("0").isZero()); + assertTrue(DD64bTraceId.from(0).isZero()); + + assertFalse(DDTraceId.ONE.isZero()); + assertFalse(DDTraceId.from(1).isZero()); + assertFalse(DD64bTraceId.from(42).isZero()); + } +} diff --git a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java index 3820b17c256..2aa58c07b22 100644 --- a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java @@ -2,8 +2,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; -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 org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -175,10 +175,11 @@ void failParsingIllegal128BitIdHexadecimalStringRepresentationFromPartialString( @Test void checkZeroConstantInitialization() { DDTraceId zero = DDTraceId.ZERO; - DDTraceId fromZero = DDTraceId.from(0); assertNotNull(zero); - assertSame(fromZero, zero); + assertTrue(zero.isZero()); + // from(0) is a zero-valued id but, by design, no longer the ZERO singleton; use isZero(). + assertTrue(DDTraceId.from(0).isZero()); } private static String leftPadWithZeros(String value, int size) { diff --git a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java index 73e3c2063e0..bc892e8b5c4 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java @@ -2025,7 +2025,7 @@ protected static final DDSpanContext buildSpanContext( propagationTags = extractedContext.getPropagationTags(); } else if (resolvedParentContext != null) { traceId = - resolvedParentContext.getTraceId() == DDTraceId.ZERO + resolvedParentContext.getTraceId().isZero() ? tracer.idGenerationStrategy.generateTraceId() : resolvedParentContext.getTraceId(); parentSpanId = resolvedParentContext.getSpanId(); diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java index 5afc104c675..17a78262f47 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java @@ -250,7 +250,7 @@ public ContextInterpreter reset(TraceConfig traceConfig) { protected TagContext build() { if (valid) { - if (fullContext && !DDTraceId.ZERO.equals(traceId)) { + if (fullContext && !traceId.isZero()) { if (propagationTags == null) { propagationTags = propagationTagsFactory.empty(); } @@ -305,7 +305,7 @@ private TagContext.HttpHeaders getHeaders() { } private int samplingPriorityOrDefault(DDTraceId traceId, int samplingPriority) { - return samplingPriority == PrioritySampling.UNSET || DDTraceId.ZERO.equals(traceId) + return samplingPriority == PrioritySampling.UNSET || traceId.isZero() ? defaultSamplingPriority() : samplingPriority; } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java index 6723dfdbc8c..fe8c046934f 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java @@ -234,7 +234,7 @@ private long extractEndToEndStartTime(String value) { private void restore128bTraceId() { long highOrderBits; // Check if the low-order 64 bits of the TraceId, and propagation tags were parsed - if (traceId != DDTraceId.ZERO + if (!traceId.isZero() && propagationTags != null && (highOrderBits = propagationTags.getTraceIdHighOrderBits()) != 0) { // Restore the 128-bit TraceId diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java index f2396d83706..2f485ee230f 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java @@ -12,7 +12,6 @@ import datadog.trace.api.DD64bTraceId; import datadog.trace.api.DDSpanId; import datadog.trace.api.DDTags; -import datadog.trace.api.DDTraceId; import datadog.trace.api.TraceConfig; import datadog.trace.api.TracePropagationStyle; import datadog.trace.api.sampling.PrioritySampling; @@ -212,7 +211,10 @@ static void handleXRayTraceHeader(ContextInterpreter interpreter, String value) } String part = value.substring(startPart, endPart).trim(); if (part.startsWith(ROOT_PREFIX)) { - if (interpreter.traceId == null || interpreter.traceId == DDTraceId.ZERO) { + // isZero() (not == DDTraceId.ZERO): DD64bTraceId.fromHex below returns a non-singleton + // zero-valued id, so a zero Root must still be treated as "unset" and overwritten by a + // later valid Root in the same header. + if (interpreter.traceId == null || interpreter.traceId.isZero()) { interpreter.traceId = DD64bTraceId.fromHex(part.substring(ROOT_PREAMBLE + TRACE_ID_PADDING.length())); } diff --git a/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java b/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java new file mode 100644 index 00000000000..1e809dac66a --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java @@ -0,0 +1,40 @@ +package datadog.trace.api; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.trace.core.propagation.B3TraceId; +import org.junit.jupiter.api.Test; + +/** + * {@link DDTraceId#isZero()} across every {@link DDTraceId} subtype, including the {@code ZERO}/ + * {@code ONE} sibling constants and {@link B3TraceId} (defined in dd-trace-core). + * + *

{@code isZero()} is the value-based replacement for the old {@code == DDTraceId.ZERO} identity + * checks, so it must recognize a zero id regardless of concrete type or how it was created (a zero + * parsed via the 64-bit factories is a distinct instance from the {@code ZERO} singleton). + */ +class TraceIdIsZeroTest { + + @Test + void zeroValuedIdsOfEveryTypeAreZero() { + assertTrue(DDTraceId.ZERO.isZero()); + assertTrue(DDTraceId.from(0).isZero()); + assertTrue(DDTraceId.from("0").isZero()); + assertTrue(DDTraceId.fromHex("0").isZero()); + assertTrue(DD64bTraceId.from(0).isZero()); + assertTrue(DD128bTraceId.from(0, 0).isZero()); + assertTrue(B3TraceId.fromHex("0").isZero()); + } + + @Test + void nonZeroIdsAreNotZero() { + assertFalse(DDTraceId.ONE.isZero()); + assertFalse(DDTraceId.from(1).isZero()); + assertFalse(DD64bTraceId.from(42).isZero()); + assertFalse(DD128bTraceId.from(0, 1).isZero()); + // High-order bits set, low-order zero: still not a zero TraceId. + assertFalse(DD128bTraceId.from(7, 0).isZero()); + assertFalse(B3TraceId.fromHex("2a").isZero()); + } +} 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 99c90b53b30..07a75d774bd 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 @@ -62,7 +62,7 @@ static AgentSpan fromSpanContext(AgentSpanContext spanContext) { * @return {@code true} if the span is considered valid, {@code false} otherwise. */ default boolean isValid() { - return getTraceId() != DDTraceId.ZERO && getSpanId() != DDSpanId.ZERO; + return !getTraceId().isZero() && getSpanId() != DDSpanId.ZERO; } @Override From b94d3d83084631c71bd8d9cdb770cdf5c625ec98 Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 1 Jun 2026 09:27:04 -0400 Subject: [PATCH 2/3] Address review: isValid() over isZero(), restore zero cache, extract constant - Rename DDTraceId.isZero() to value-based isValid() (OTel-aligned; mcculls); flip the boolean at all 7 production call sites. - Restore a cached zero singleton in DD64bTraceId so from(0)/fromHex("0") do not allocate per call (mcculls). - Extract the ZERO/ONE backing type to a dedicated DDTraceIdConstant class (PerfectSlayer); update the JaCoCo coverage exclusion. - Preserve equals/hashCode: ZERO/ONE stay value-equal (both directions) to the equivalent DD64bTraceId, matching pre-PR behavior. - Trim verbose non-test comments (mcculls, PerfectSlayer). - Parameterize and rename TraceIdIsZeroTest -> TraceIdIsValidTest (PerfectSlayer). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../spark/AbstractDatadogSparkListener.java | 2 +- dd-trace-api/build.gradle.kts | 2 +- .../java/datadog/trace/api/DD64bTraceId.java | 21 +++- .../java/datadog/trace/api/DDTraceId.java | 105 +++++------------- .../datadog/trace/api/DDTraceIdConstant.java | 78 +++++++++++++ .../trace/api/DDTraceIdConstantsTest.java | 34 ++++-- .../java/datadog/trace/api/DDTraceIdTest.java | 11 +- .../java/datadog/trace/core/CoreTracer.java | 6 +- .../core/propagation/ContextInterpreter.java | 4 +- .../core/propagation/DatadogHttpCodec.java | 2 +- .../trace/core/propagation/XRayHttpCodec.java | 7 +- .../datadog/trace/api/TraceIdIsValidTest.java | 46 ++++++++ .../datadog/trace/api/TraceIdIsZeroTest.java | 40 ------- .../instrumentation/api/AgentSpan.java | 2 +- 14 files changed, 209 insertions(+), 151 deletions(-) create mode 100644 dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java create mode 100644 dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsValidTest.java delete mode 100644 dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java diff --git a/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java b/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java index c1826f9e548..6f729f321d4 100644 --- a/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java +++ b/dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java @@ -444,7 +444,7 @@ private void addDatabricksSpecificTags( AgentSpanContext parentContext = new DatabricksParentContext(databricksJobId, databricksJobRunId, databricksTaskRunId); - if (!parentContext.getTraceId().isZero()) { + if (parentContext.getTraceId().isValid()) { if (withParentContext) { builder.asChildOf(parentContext); } else { diff --git a/dd-trace-api/build.gradle.kts b/dd-trace-api/build.gradle.kts index fc4924ba271..7912fefcd6a 100644 --- a/dd-trace-api/build.gradle.kts +++ b/dd-trace-api/build.gradle.kts @@ -16,7 +16,7 @@ val excludedClassesCoverage by extra( "datadog.trace.api.DDTags", "datadog.trace.api.DDTraceApiInfo", "datadog.trace.api.DDTraceId", - "datadog.trace.api.DDTraceId.ConstantId", + "datadog.trace.api.DDTraceIdConstant", "datadog.trace.api.EventTracker", "datadog.trace.api.GlobalTracer*", "datadog.trace.api.PropagationStyle", diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java index 958e45f93f9..f575fcab453 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java @@ -12,6 +12,10 @@ public class DD64bTraceId extends DDTraceId { public static final DD64bTraceId MAX = new DD64bTraceId(-1, "18446744073709551615"); // All bits set + // Cached zero singleton so create()/from() don't allocate for every zero id. Initialized with + // this subclass (not in DDTraceId.), so it does not reintroduce the init deadlock. It is + // value-equal to DDTraceId.ZERO. + private static final DD64bTraceId ZERO_ID = new DD64bTraceId(0, "0"); private final long id; private String str; // cache for string representation @@ -59,11 +63,13 @@ public static DD64bTraceId fromHex(String s) throws NumberFormatException { } static DD64bTraceId create(long id, String str) { - // -1 (all bits set) reuses the MAX singleton. 0 is not special-cased: DDTraceId.ZERO is a - // sibling type (not a DD64bTraceId), so it cannot be returned here; a zero 64-bit id is simply - // a DD64bTraceId(0), and callers detect zero via DDTraceId.isZero() rather than by identity. + // Reuse cached singletons rather than allocating: -1 (all bits set) is MAX, 0 is ZERO_ID. + // ZERO_ID is a DD64bTraceId, not DDTraceId.ZERO (a sibling type), but is value-equal to it; + // callers detect zero via DDTraceId.isValid() rather than by identity. if (id == -1) { return MAX; + } else if (id == 0) { + return ZERO_ID; } else { return new DD64bTraceId(id, str); } @@ -72,9 +78,12 @@ static DD64bTraceId create(long id, String str) { @Override public boolean equals(Object o) { if (this == o) return true; - if (!(o instanceof DD64bTraceId)) return false; - DD64bTraceId ddId = (DD64bTraceId) o; - return this.id == ddId.id; + // Value-equal to the DDTraceIdConstant backing DDTraceId.ZERO/ONE (also a 64-bit id), so the + // ZERO/ONE constants keep comparing equal to the equivalent DD64bTraceId as they did when they + // were themselves DD64bTraceId instances. + if (o instanceof DD64bTraceId) return this.id == ((DD64bTraceId) o).id; + if (o instanceof DDTraceIdConstant) return this.id == ((DDTraceIdConstant) o).toLong(); + return false; } @Override diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java index 03ec9e2538a..f62d0e5d4ed 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java @@ -1,7 +1,5 @@ package datadog.trace.api; -import datadog.trace.api.internal.util.LongStringUtils; - /** * Class encapsulating the id used for TraceIds. * @@ -10,24 +8,21 @@ */ public abstract class DDTraceId { /** - * Invalid TraceId value used to denote no TraceId. + * Invalid TraceId value used to denote no/unset TraceId. * - *

This is an instance of a private {@link DDTraceId} subtype (a sibling of {@link - * DD64bTraceId}), not a {@code DD64bTraceId}, and that is deliberate. {@code DD64bTraceId} is a - * subclass of {@code DDTraceId}, so the JVM must initialize {@code DDTraceId} before {@code - * DD64bTraceId}. If this constant were built via {@code DD64bTraceId.from(0)} (as it once was), - * {@code DDTraceId.} would initialize {@code DD64bTraceId} while holding the {@code - * DDTraceId} init lock, and two threads first touching the classes from opposite ends would - * deadlock on the two class-initialization locks. Building it from a sibling type keeps {@code - * DDTraceId.} free of any reference to the subclass. + *

Backed by {@link DDTraceIdConstant}, a sibling of {@link DD64bTraceId}, not by {@code + * DD64bTraceId} itself. {@code DD64bTraceId} is a subclass, so building this via {@code + * DD64bTraceId.from(0)} (as it once was) made {@code DDTraceId.} initialize the subclass + * while holding the {@code DDTraceId} init lock; two threads touching the classes from opposite + * ends then deadlocked. The sibling type keeps {@code DDTraceId.} free of the subclass. * - *

To test whether an id is zero, prefer {@link #isZero()} over {@code == DDTraceId.ZERO}: a - * zero id parsed via the 64-bit factories is a distinct instance, not this singleton. + *

Use {@link #isValid()} rather than {@code == DDTraceId.ZERO}: a zero id parsed via the + * 64-bit factories is a distinct instance, not this constant. */ - public static final DDTraceId ZERO = new ConstantId(0, "0"); + public static final DDTraceId ZERO = new DDTraceIdConstant(0, "0"); /** Convenience constant used from tests. See {@link #ZERO} for why this is a sibling type. */ - public static final DDTraceId ONE = new ConstantId(1, "1"); + public static final DDTraceId ONE = new DDTraceIdConstant(1, "1"); /** * Creates a new {@link DD64bTraceId 64-bit TraceId} from the given {@code long} interpreted as @@ -38,7 +33,9 @@ public abstract class DDTraceId { * @return A new {@link DDTraceId} instance. */ public static DDTraceId from(long id) { - return DD64bTraceId.from(id); + // Normalize a zero id to the ZERO constant so callers comparing against DDTraceId.ZERO keep + // working. DD64bTraceId.from keeps its own cached zero singleton for the 64-bit-specific path. + return id == 0 ? ZERO : DD64bTraceId.from(id); } /** @@ -50,7 +47,8 @@ public static DDTraceId from(long id) { * @throws NumberFormatException If the given {@link String} does not represent a valid number. */ public static DDTraceId from(String s) throws NumberFormatException { - return DD64bTraceId.create(LongStringUtils.parseUnsignedLong(s), s); + DD64bTraceId id = DD64bTraceId.from(s); + return id.toLong() == 0 ? ZERO : id; } /** @@ -66,7 +64,11 @@ public static DDTraceId fromHex(String s) throws NumberFormatException { if (s == null) { throw new NumberFormatException("s cannot be null"); } - return s.length() > 16 ? DD128bTraceId.fromHex(s) : DD64bTraceId.fromHex(s); + if (s.length() > 16) { + return DD128bTraceId.fromHex(s); + } + DD64bTraceId id = DD64bTraceId.fromHex(s); + return id.toLong() == 0 ? ZERO : id; } /** @@ -118,68 +120,15 @@ public static DDTraceId fromHex(String s) throws NumberFormatException { public abstract long toHighOrderLong(); /** - * Returns whether this {@link DDTraceId} is zero, the value used to denote no/invalid TraceId. + * Returns whether this is a valid (non-zero) {@link DDTraceId}; a zero id denotes no/unset + * TraceId. * - *

Prefer this over {@code == DDTraceId.ZERO}: it is value-based, so it recognizes a zero id - * regardless of its concrete type or how it was created (e.g. a zero parsed via the 64-bit - * factories, which is a distinct instance from the {@link #ZERO} singleton). + *

Value-based, aligning with OpenTelemetry: it recognizes a zero id of any concrete type, + * including a zero parsed via the 64-bit factories (a distinct instance from {@link #ZERO}). * - * @return {@code true} if both the high- and low-order 64 bits are zero. + * @return {@code true} if the high- or low-order 64 bits are non-zero. */ - public boolean isZero() { - return toHighOrderLong() == 0 && toLong() == 0; - } - - /** - * Minimal concrete {@link DDTraceId} backing the {@link #ZERO} and {@link #ONE} constants. It is - * a sibling of {@link DD64bTraceId} (it extends {@link DDTraceId} directly), so constructing - * these constants in {@code DDTraceId.} never initializes {@code DD64bTraceId} (which - * would create a class-initialization deadlock; see {@link #ZERO}). It represents a 64-bit id and - * formats identically to the equivalent {@link DD64bTraceId}. - */ - private static final class ConstantId extends DDTraceId { - private final long id; - private final String str; - private String hexStr; // cache for hex string representation - - private ConstantId(long id, String str) { - this.id = id; - this.str = str; - } - - @Override - public String toString() { - return this.str; - } - - @Override - public String toHexString() { - String hexStr = this.hexStr; - // This race condition is intentional and benign. - // The worst that can happen is that an identical value is produced and written into the - // field. - if (hexStr == null) { - this.hexStr = hexStr = LongStringUtils.toHexStringPadded(this.id, 32); - } - return hexStr; - } - - @Override - public String toHexStringPadded(int size) { - if (size > 16) { - return toHexString(); - } - return LongStringUtils.toHexStringPadded(this.id, size); - } - - @Override - public long toLong() { - return this.id; - } - - @Override - public long toHighOrderLong() { - return 0; - } + public boolean isValid() { + return toHighOrderLong() != 0 || toLong() != 0; } } diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java new file mode 100644 index 00000000000..760aa3607f4 --- /dev/null +++ b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java @@ -0,0 +1,78 @@ +package datadog.trace.api; + +import datadog.trace.api.internal.util.LongStringUtils; + +/** + * Concrete {@link DDTraceId} backing the {@link DDTraceId#ZERO} and {@link DDTraceId#ONE} + * constants. + * + *

It extends {@link DDTraceId} directly, so it is a sibling of {@link DD64bTraceId} rather than + * a {@code DD64bTraceId}. That keeps {@code DDTraceId.} free of any reference to the + * subclass (see {@link DDTraceId#ZERO} for why that matters). It represents a 64-bit id and is + * value-equal to the equivalent {@link DD64bTraceId}. + * + *

Invariant: this type must only ever be initialized as a consequence of {@code + * DDTraceId.} constructing the constants below. It is {@code final} and package-private, + * and the only place it is instantiated is {@link DDTraceId}. {@code instanceof}/cast (used in + * {@link #equals}) do not trigger class initialization, so they are safe. Do not add a static + * member access or any other path that could initialize this class independently of {@code + * DDTraceId}: that would let a thread hold this class's init lock while waiting for {@code + * DDTraceId}, reintroducing the very class-initialization deadlock this design removes. + */ +final class DDTraceIdConstant extends DDTraceId { + private final long id; + private final String str; + private String hexStr; // cache for hex string representation + + DDTraceIdConstant(long id, String str) { + this.id = id; + this.str = str; + } + + @Override + public String toString() { + return this.str; + } + + @Override + public String toHexString() { + String hexStr = this.hexStr; + // This race condition is intentional and benign. + // The worst that can happen is that an identical value is produced and written into the field. + if (hexStr == null) { + this.hexStr = hexStr = LongStringUtils.toHexStringPadded(this.id, 32); + } + return hexStr; + } + + @Override + public String toHexStringPadded(int size) { + if (size > 16) { + return toHexString(); + } + return LongStringUtils.toHexStringPadded(this.id, size); + } + + @Override + public long toLong() { + return this.id; + } + + @Override + public long toHighOrderLong() { + return 0; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o instanceof DD64bTraceId) return this.id == ((DD64bTraceId) o).toLong(); + if (o instanceof DDTraceIdConstant) return this.id == ((DDTraceIdConstant) o).id; + return false; + } + + @Override + public int hashCode() { + return (int) (this.id ^ (this.id >>> 32)); + } +} diff --git a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java index f65bc74853d..7cf1f7561bf 100644 --- a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java @@ -21,20 +21,34 @@ void zeroAndOneFormatLikeTheEquivalentDD64bTraceId() { assertEquals("1", DDTraceId.ONE.toString()); assertEquals("00000000000000000000000000000001", DDTraceId.ONE.toHexString()); assertEquals("0000000000000001", DDTraceId.ONE.toHexStringPadded(16)); + assertEquals("00000000000000000000000000000001", DDTraceId.ONE.toHexStringPadded(32)); assertEquals(1L, DDTraceId.ONE.toLong()); assertEquals(0L, DDTraceId.ONE.toHighOrderLong()); } @Test - void isZeroReflectsTheValue() { - assertTrue(DDTraceId.ZERO.isZero()); - assertTrue(DDTraceId.from(0).isZero()); - assertTrue(DDTraceId.from("0").isZero()); - assertTrue(DDTraceId.fromHex("0").isZero()); - assertTrue(DD64bTraceId.from(0).isZero()); - - assertFalse(DDTraceId.ONE.isZero()); - assertFalse(DDTraceId.from(1).isZero()); - assertFalse(DD64bTraceId.from(42).isZero()); + void isValidReflectsTheValue() { + assertFalse(DDTraceId.ZERO.isValid()); + assertFalse(DDTraceId.from(0).isValid()); + assertFalse(DDTraceId.from("0").isValid()); + assertFalse(DDTraceId.fromHex("0").isValid()); + assertFalse(DD64bTraceId.from(0).isValid()); + + assertTrue(DDTraceId.ONE.isValid()); + assertTrue(DDTraceId.from(1).isValid()); + assertTrue(DD64bTraceId.from(42).isValid()); + } + + @Test + void constantsAreValueEqualToTheEquivalentDD64bTraceId() { + // ZERO/ONE used to be DD64bTraceId instances; they are now a sibling type but must stay + // value-equal (both directions, with matching hashCode) to the equivalent DD64bTraceId. + assertEquals(DDTraceId.ZERO, DD64bTraceId.from(0)); + assertEquals(DD64bTraceId.from(0), DDTraceId.ZERO); + assertEquals(DDTraceId.ZERO.hashCode(), DD64bTraceId.from(0).hashCode()); + + assertEquals(DDTraceId.ONE, DD64bTraceId.from(1)); + assertEquals(DD64bTraceId.from(1), DDTraceId.ONE); + assertEquals(DDTraceId.ONE.hashCode(), DD64bTraceId.from(1).hashCode()); } } diff --git a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java index 2aa58c07b22..ea27fee6142 100644 --- a/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java @@ -1,9 +1,10 @@ package datadog.trace.api; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +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 org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -177,9 +178,11 @@ void checkZeroConstantInitialization() { DDTraceId zero = DDTraceId.ZERO; assertNotNull(zero); - assertTrue(zero.isZero()); - // from(0) is a zero-valued id but, by design, no longer the ZERO singleton; use isZero(). - assertTrue(DDTraceId.from(0).isZero()); + assertFalse(zero.isValid()); + // The public DDTraceId factories normalize a zero id back to the ZERO constant. + assertSame(zero, DDTraceId.from(0)); + assertSame(zero, DDTraceId.from("0")); + assertSame(zero, DDTraceId.fromHex("0")); } private static String leftPadWithZeros(String value, int size) { diff --git a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java index bc892e8b5c4..80e6138791d 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java @@ -2025,9 +2025,9 @@ protected static final DDSpanContext buildSpanContext( propagationTags = extractedContext.getPropagationTags(); } else if (resolvedParentContext != null) { traceId = - resolvedParentContext.getTraceId().isZero() - ? tracer.idGenerationStrategy.generateTraceId() - : resolvedParentContext.getTraceId(); + resolvedParentContext.getTraceId().isValid() + ? resolvedParentContext.getTraceId() + : tracer.idGenerationStrategy.generateTraceId(); parentSpanId = resolvedParentContext.getSpanId(); samplingPriority = resolvedParentContext.getSamplingPriority(); endToEndStartTime = 0; diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java index 17a78262f47..eba03efceaa 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java @@ -250,7 +250,7 @@ public ContextInterpreter reset(TraceConfig traceConfig) { protected TagContext build() { if (valid) { - if (fullContext && !traceId.isZero()) { + if (fullContext && traceId.isValid()) { if (propagationTags == null) { propagationTags = propagationTagsFactory.empty(); } @@ -305,7 +305,7 @@ private TagContext.HttpHeaders getHeaders() { } private int samplingPriorityOrDefault(DDTraceId traceId, int samplingPriority) { - return samplingPriority == PrioritySampling.UNSET || traceId.isZero() + return samplingPriority == PrioritySampling.UNSET || !traceId.isValid() ? defaultSamplingPriority() : samplingPriority; } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java index fe8c046934f..72b0e1859ef 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java @@ -234,7 +234,7 @@ private long extractEndToEndStartTime(String value) { private void restore128bTraceId() { long highOrderBits; // Check if the low-order 64 bits of the TraceId, and propagation tags were parsed - if (!traceId.isZero() + if (traceId.isValid() && propagationTags != null && (highOrderBits = propagationTags.getTraceIdHighOrderBits()) != 0) { // Restore the 128-bit TraceId diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java index 2f485ee230f..8e7e3ce7c56 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java @@ -211,10 +211,9 @@ static void handleXRayTraceHeader(ContextInterpreter interpreter, String value) } String part = value.substring(startPart, endPart).trim(); if (part.startsWith(ROOT_PREFIX)) { - // isZero() (not == DDTraceId.ZERO): DD64bTraceId.fromHex below returns a non-singleton - // zero-valued id, so a zero Root must still be treated as "unset" and overwritten by a - // later valid Root in the same header. - if (interpreter.traceId == null || interpreter.traceId.isZero()) { + // Value-based check, not == DDTraceId.ZERO: a zero Root parsed here is a non-singleton + // id that must still count as unset so a later valid Root can replace it. + if (interpreter.traceId == null || !interpreter.traceId.isValid()) { interpreter.traceId = DD64bTraceId.fromHex(part.substring(ROOT_PREAMBLE + TRACE_ID_PADDING.length())); } diff --git a/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsValidTest.java b/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsValidTest.java new file mode 100644 index 00000000000..67d1b6e4add --- /dev/null +++ b/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsValidTest.java @@ -0,0 +1,46 @@ +package datadog.trace.api; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import datadog.trace.core.propagation.B3TraceId; +import java.util.stream.Stream; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * {@link DDTraceId#isValid()} across every {@link DDTraceId} subtype, including the {@code ZERO}/ + * {@code ONE} sibling constants and {@link B3TraceId} (defined in dd-trace-core). + * + *

{@code isValid()} is value-based: a zero id is invalid regardless of its concrete type or how + * it was created (a zero parsed via the 64-bit factories is a distinct instance from {@code ZERO}). + */ +class TraceIdIsValidTest { + + static Stream traceIds() { + return Stream.of( + // Zero-valued ids of every type are invalid. A label is supplied because several distinct + // cases share the same toString() ("0"), so the value alone would not name them uniquely. + Arguments.of("ZERO constant", DDTraceId.ZERO, false), + Arguments.of("DDTraceId.from(0)", DDTraceId.from(0), false), + Arguments.of("DDTraceId.from(\"0\")", DDTraceId.from("0"), false), + Arguments.of("DDTraceId.fromHex(\"0\")", DDTraceId.fromHex("0"), false), + Arguments.of("DD64bTraceId.from(0)", DD64bTraceId.from(0), false), + Arguments.of("DD128bTraceId.from(0, 0)", DD128bTraceId.from(0, 0), false), + Arguments.of("B3TraceId.fromHex(\"0\")", B3TraceId.fromHex("0"), false), + // Non-zero ids are valid. + Arguments.of("ONE constant", DDTraceId.ONE, true), + Arguments.of("DDTraceId.from(1)", DDTraceId.from(1), true), + Arguments.of("DD64bTraceId.from(42)", DD64bTraceId.from(42), true), + Arguments.of("DD128bTraceId.from(0, 1)", DD128bTraceId.from(0, 1), true), + // High-order bits set, low-order zero: still a valid TraceId. + Arguments.of("DD128bTraceId.from(7, 0)", DD128bTraceId.from(7, 0), true), + Arguments.of("B3TraceId.fromHex(\"2a\")", B3TraceId.fromHex("2a"), true)); + } + + @ParameterizedTest(name = "{0} isValid={2}") + @MethodSource("traceIds") + void isValidReflectsTheValue(String label, DDTraceId traceId, boolean expectedValid) { + assertEquals(expectedValid, traceId.isValid(), label); + } +} diff --git a/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java b/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java deleted file mode 100644 index 1e809dac66a..00000000000 --- a/dd-trace-core/src/test/java/datadog/trace/api/TraceIdIsZeroTest.java +++ /dev/null @@ -1,40 +0,0 @@ -package datadog.trace.api; - -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import datadog.trace.core.propagation.B3TraceId; -import org.junit.jupiter.api.Test; - -/** - * {@link DDTraceId#isZero()} across every {@link DDTraceId} subtype, including the {@code ZERO}/ - * {@code ONE} sibling constants and {@link B3TraceId} (defined in dd-trace-core). - * - *

{@code isZero()} is the value-based replacement for the old {@code == DDTraceId.ZERO} identity - * checks, so it must recognize a zero id regardless of concrete type or how it was created (a zero - * parsed via the 64-bit factories is a distinct instance from the {@code ZERO} singleton). - */ -class TraceIdIsZeroTest { - - @Test - void zeroValuedIdsOfEveryTypeAreZero() { - assertTrue(DDTraceId.ZERO.isZero()); - assertTrue(DDTraceId.from(0).isZero()); - assertTrue(DDTraceId.from("0").isZero()); - assertTrue(DDTraceId.fromHex("0").isZero()); - assertTrue(DD64bTraceId.from(0).isZero()); - assertTrue(DD128bTraceId.from(0, 0).isZero()); - assertTrue(B3TraceId.fromHex("0").isZero()); - } - - @Test - void nonZeroIdsAreNotZero() { - assertFalse(DDTraceId.ONE.isZero()); - assertFalse(DDTraceId.from(1).isZero()); - assertFalse(DD64bTraceId.from(42).isZero()); - assertFalse(DD128bTraceId.from(0, 1).isZero()); - // High-order bits set, low-order zero: still not a zero TraceId. - assertFalse(DD128bTraceId.from(7, 0).isZero()); - assertFalse(B3TraceId.fromHex("2a").isZero()); - } -} 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 07a75d774bd..2f5721bd83e 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 @@ -62,7 +62,7 @@ static AgentSpan fromSpanContext(AgentSpanContext spanContext) { * @return {@code true} if the span is considered valid, {@code false} otherwise. */ default boolean isValid() { - return !getTraceId().isZero() && getSpanId() != DDSpanId.ZERO; + return getTraceId().isValid() && getSpanId() != DDSpanId.ZERO; } @Override From ec20933f49d0283b54a5ee390df4df5f9c68348b Mon Sep 17 00:00:00 2001 From: Brian Marks Date: Mon, 1 Jun 2026 10:21:49 -0400 Subject: [PATCH 3/3] Simplify comments; suppress SpotBugs cross-type equals false positive - Trim the verbose DDTraceId/DD64bTraceId/DDTraceIdConstant/XRayHttpCodec comments to terse navigation aids (review feedback). - Suppress EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS on the two cross-type equals methods (intentional value-equality between ZERO/ONE and the equivalent DD64bTraceId); fixes the check_base SpotBugs failure. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../java/datadog/trace/api/DD64bTraceId.java | 17 +++++----- .../java/datadog/trace/api/DDTraceId.java | 31 ++++++------------- .../datadog/trace/api/DDTraceIdConstant.java | 23 ++++++-------- .../trace/core/propagation/XRayHttpCodec.java | 3 +- 4 files changed, 28 insertions(+), 46 deletions(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java index f575fcab453..ba7a53e952d 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java @@ -1,6 +1,7 @@ package datadog.trace.api; import datadog.trace.api.internal.util.LongStringUtils; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; /** * Class encapsulating the unsigned 64 bit id used for Traceids. @@ -12,9 +13,7 @@ public class DD64bTraceId extends DDTraceId { public static final DD64bTraceId MAX = new DD64bTraceId(-1, "18446744073709551615"); // All bits set - // Cached zero singleton so create()/from() don't allocate for every zero id. Initialized with - // this subclass (not in DDTraceId.), so it does not reintroduce the init deadlock. It is - // value-equal to DDTraceId.ZERO. + // Cached zero so create()/from() don't allocate per zero id. Value-equal to DDTraceId.ZERO. private static final DD64bTraceId ZERO_ID = new DD64bTraceId(0, "0"); private final long id; @@ -63,9 +62,7 @@ public static DD64bTraceId fromHex(String s) throws NumberFormatException { } static DD64bTraceId create(long id, String str) { - // Reuse cached singletons rather than allocating: -1 (all bits set) is MAX, 0 is ZERO_ID. - // ZERO_ID is a DD64bTraceId, not DDTraceId.ZERO (a sibling type), but is value-equal to it; - // callers detect zero via DDTraceId.isValid() rather than by identity. + // Reuse cached singletons rather than allocating: -1 -> MAX, 0 -> ZERO_ID. if (id == -1) { return MAX; } else if (id == 0) { @@ -76,12 +73,14 @@ static DD64bTraceId create(long id, String str) { } @Override + @SuppressFBWarnings( + value = "EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS", + justification = + "DDTraceIdConstant is a sibling type backing ZERO/ONE; equal by 64-bit value.") public boolean equals(Object o) { if (this == o) return true; - // Value-equal to the DDTraceIdConstant backing DDTraceId.ZERO/ONE (also a 64-bit id), so the - // ZERO/ONE constants keep comparing equal to the equivalent DD64bTraceId as they did when they - // were themselves DD64bTraceId instances. if (o instanceof DD64bTraceId) return this.id == ((DD64bTraceId) o).id; + // Also value-equal to the DDTraceIdConstant backing ZERO/ONE (both 64-bit ids). if (o instanceof DDTraceIdConstant) return this.id == ((DDTraceIdConstant) o).toLong(); return false; } diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java index f62d0e5d4ed..0955529756e 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java @@ -7,21 +7,14 @@ * The string representations are either kept from parsing, or generated on demand and cached. */ public abstract class DDTraceId { - /** - * Invalid TraceId value used to denote no/unset TraceId. - * - *

Backed by {@link DDTraceIdConstant}, a sibling of {@link DD64bTraceId}, not by {@code - * DD64bTraceId} itself. {@code DD64bTraceId} is a subclass, so building this via {@code - * DD64bTraceId.from(0)} (as it once was) made {@code DDTraceId.} initialize the subclass - * while holding the {@code DDTraceId} init lock; two threads touching the classes from opposite - * ends then deadlocked. The sibling type keeps {@code DDTraceId.} free of the subclass. - * - *

Use {@link #isValid()} rather than {@code == DDTraceId.ZERO}: a zero id parsed via the - * 64-bit factories is a distinct instance, not this constant. - */ + // ZERO/ONE use DDTraceIdConstant (a sibling of DD64bTraceId) rather than DD64bTraceId so that + // initializing DDTraceId never initializes its subclass, which would deadlock. Test for zero with + // isValid(), not == ZERO. + + /** Invalid TraceId value used to denote no/unset TraceId. */ public static final DDTraceId ZERO = new DDTraceIdConstant(0, "0"); - /** Convenience constant used from tests. See {@link #ZERO} for why this is a sibling type. */ + /** Convenience constant used from tests. */ public static final DDTraceId ONE = new DDTraceIdConstant(1, "1"); /** @@ -33,8 +26,7 @@ public abstract class DDTraceId { * @return A new {@link DDTraceId} instance. */ public static DDTraceId from(long id) { - // Normalize a zero id to the ZERO constant so callers comparing against DDTraceId.ZERO keep - // working. DD64bTraceId.from keeps its own cached zero singleton for the 64-bit-specific path. + // Zero maps to the ZERO constant so callers can still compare against it. return id == 0 ? ZERO : DD64bTraceId.from(id); } @@ -120,13 +112,10 @@ public static DDTraceId fromHex(String s) throws NumberFormatException { public abstract long toHighOrderLong(); /** - * Returns whether this is a valid (non-zero) {@link DDTraceId}; a zero id denotes no/unset - * TraceId. - * - *

Value-based, aligning with OpenTelemetry: it recognizes a zero id of any concrete type, - * including a zero parsed via the 64-bit factories (a distinct instance from {@link #ZERO}). + * Returns whether this is a valid (non-zero) {@link DDTraceId}; zero denotes no/unset TraceId. + * Value-based, so it recognizes a zero id of any concrete type. * - * @return {@code true} if the high- or low-order 64 bits are non-zero. + * @return {@code true} if any bit is non-zero. */ public boolean isValid() { return toHighOrderLong() != 0 || toLong() != 0; diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java index 760aa3607f4..3fca1426d82 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java @@ -1,23 +1,15 @@ package datadog.trace.api; import datadog.trace.api.internal.util.LongStringUtils; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; /** - * Concrete {@link DDTraceId} backing the {@link DDTraceId#ZERO} and {@link DDTraceId#ONE} - * constants. + * Backs {@link DDTraceId#ZERO} and {@link DDTraceId#ONE}. A 64-bit id that is a sibling of {@link + * DD64bTraceId} (it extends {@link DDTraceId} directly) so initializing {@code DDTraceId} never + * initializes its subclass; value-equal to the equivalent {@link DD64bTraceId}. * - *

It extends {@link DDTraceId} directly, so it is a sibling of {@link DD64bTraceId} rather than - * a {@code DD64bTraceId}. That keeps {@code DDTraceId.} free of any reference to the - * subclass (see {@link DDTraceId#ZERO} for why that matters). It represents a 64-bit id and is - * value-equal to the equivalent {@link DD64bTraceId}. - * - *

Invariant: this type must only ever be initialized as a consequence of {@code - * DDTraceId.} constructing the constants below. It is {@code final} and package-private, - * and the only place it is instantiated is {@link DDTraceId}. {@code instanceof}/cast (used in - * {@link #equals}) do not trigger class initialization, so they are safe. Do not add a static - * member access or any other path that could initialize this class independently of {@code - * DDTraceId}: that would let a thread hold this class's init lock while waiting for {@code - * DDTraceId}, reintroducing the very class-initialization deadlock this design removes. + *

Only ever initialize this through {@link DDTraceId}'s constants. Initializing it independently + * (e.g. a static-member access) would bring back the class-initialization deadlock. */ final class DDTraceIdConstant extends DDTraceId { private final long id; @@ -64,6 +56,9 @@ public long toHighOrderLong() { } @Override + @SuppressFBWarnings( + value = "EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS", + justification = "DD64bTraceId is a sibling type; ZERO/ONE are equal to it by 64-bit value.") public boolean equals(Object o) { if (this == o) return true; if (o instanceof DD64bTraceId) return this.id == ((DD64bTraceId) o).toLong(); diff --git a/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java b/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java index 8e7e3ce7c56..a838f27d475 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java @@ -211,8 +211,7 @@ static void handleXRayTraceHeader(ContextInterpreter interpreter, String value) } String part = value.substring(startPart, endPart).trim(); if (part.startsWith(ROOT_PREFIX)) { - // Value-based check, not == DDTraceId.ZERO: a zero Root parsed here is a non-singleton - // id that must still count as unset so a later valid Root can replace it. + // isValid(), not == ZERO: a parsed zero Root must still count as unset. if (interpreter.traceId == null || !interpreter.traceId.isValid()) { interpreter.traceId = DD64bTraceId.fromHex(part.substring(ROOT_PREAMBLE + TRACE_ID_PADDING.length()));