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..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() != DDTraceId.ZERO) { + 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 bc05a8753a4..7912fefcd6a 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.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 f40b47a89d4..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,6 +13,8 @@ public class DD64bTraceId extends DDTraceId { public static final DD64bTraceId MAX = new DD64bTraceId(-1, "18446744073709551615"); // All bits set + // 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; private String str; // cache for string representation @@ -59,23 +62,27 @@ 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) { + // Reuse cached singletons rather than allocating: -1 -> MAX, 0 -> ZERO_ID. + if (id == -1) { return MAX; + } else if (id == 0) { + return ZERO_ID; } else { return new DD64bTraceId(id, 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; - if (!(o instanceof DD64bTraceId)) return false; - DD64bTraceId ddId = (DD64bTraceId) o; - return this.id == ddId.id; + 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; } @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 7ed5be915cc..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 @@ -1,7 +1,5 @@ package datadog.trace.api; -import datadog.trace.api.internal.util.LongStringUtils; - /** * Class encapsulating the id used for TraceIds. * @@ -9,11 +7,15 @@ * 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); + // 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 */ - public static final DDTraceId ONE = from(1); + /** Convenience constant used from tests. */ + public static final DDTraceId ONE = new DDTraceIdConstant(1, "1"); /** * Creates a new {@link DD64bTraceId 64-bit TraceId} from the given {@code long} interpreted as @@ -24,7 +26,8 @@ public abstract class DDTraceId { * @return A new {@link DDTraceId} instance. */ public static DDTraceId from(long id) { - return DD64bTraceId.from(id); + // Zero maps to the ZERO constant so callers can still compare against it. + return id == 0 ? ZERO : DD64bTraceId.from(id); } /** @@ -36,7 +39,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; } /** @@ -52,7 +56,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; } /** @@ -102,4 +110,14 @@ public static DDTraceId fromHex(String s) throws NumberFormatException { * 0 for 64-bit {@link DDTraceId} only. */ public abstract long toHighOrderLong(); + + /** + * 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 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 new file mode 100644 index 00000000000..3fca1426d82 --- /dev/null +++ b/dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java @@ -0,0 +1,73 @@ +package datadog.trace.api; + +import datadog.trace.api.internal.util.LongStringUtils; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + +/** + * 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}. + * + *

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; + 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 + @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(); + 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/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..7cf1f7561bf --- /dev/null +++ b/dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java @@ -0,0 +1,54 @@ +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("00000000000000000000000000000001", DDTraceId.ONE.toHexStringPadded(32)); + assertEquals(1L, DDTraceId.ONE.toLong()); + assertEquals(0L, DDTraceId.ONE.toHighOrderLong()); + } + + @Test + 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 3820b17c256..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,6 +1,7 @@ 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; @@ -175,10 +176,13 @@ void failParsingIllegal128BitIdHexadecimalStringRepresentationFromPartialString( @Test void checkZeroConstantInitialization() { DDTraceId zero = DDTraceId.ZERO; - DDTraceId fromZero = DDTraceId.from(0); assertNotNull(zero); - assertSame(fromZero, zero); + 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 73e3c2063e0..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() == DDTraceId.ZERO - ? 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 5afc104c675..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 && !DDTraceId.ZERO.equals(traceId)) { + 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 || DDTraceId.ZERO.equals(traceId) + 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 6723dfdbc8c..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 != DDTraceId.ZERO + 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 f2396d83706..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 @@ -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,8 @@ 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) { + // 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())); } 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/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..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() != DDTraceId.ZERO && getSpanId() != DDSpanId.ZERO; + return getTraceId().isValid() && getSpanId() != DDSpanId.ZERO; } @Override