From 5c788f0ecdfa8872494ab9ec95421aef2efd93b6 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 8 Oct 2026 12:01:54 -0700 Subject: [PATCH] Retain invalid span links with attributes or trace state Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e115342e-020d-4c4b-92ad-4cb6b5c06b2d --- .../java/io/opentelemetry/api/trace/Span.java | 10 +- .../opentelemetry/api/trace/SpanBuilder.java | 10 +- .../traces/TraceRequestMarshalerTest.java | 31 ++++ .../io/opentelemetry/sdk/trace/SdkSpan.java | 8 +- .../sdk/trace/SdkSpanBuilder.java | 7 +- .../sdk/trace/SdkSpanBuilderTest.java | 132 +++++++++++++++++- .../opentelemetry/sdk/trace/SdkSpanTest.java | 113 +++++++++++++++ 7 files changed, 296 insertions(+), 15 deletions(-) diff --git a/api/all/src/main/java/io/opentelemetry/api/trace/Span.java b/api/all/src/main/java/io/opentelemetry/api/trace/Span.java index fbd08bedcce..b9f6b137819 100644 --- a/api/all/src/main/java/io/opentelemetry/api/trace/Span.java +++ b/api/all/src/main/java/io/opentelemetry/api/trace/Span.java @@ -455,8 +455,9 @@ default Span recordException(Throwable exception) { * operations, where a single batch handler processes multiple requests from different traces or * the same trace. * - *

Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span - * context}. + *

Implementations should record links with an {@linkplain SpanContext#isValid() invalid span + * context} if its {@link TraceState} is nonempty. Implementations may ignore calls with an + * invalid span context and an empty {@link TraceState}. * *

Callers should prefer to add links before starting the span via {@link * SpanBuilder#addLink(SpanContext)} if possible. @@ -480,8 +481,9 @@ default Span addLink(SpanContext spanContext) { * operations, where a single batch handler processes multiple requests from different traces or * the same trace. * - *

Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span - * context}. + *

Implementations should record links with an {@linkplain SpanContext#isValid() invalid span + * context} if the attributes or its {@link TraceState} are nonempty. Implementations may ignore + * calls with an invalid span context when both are empty. * *

Callers should prefer to add links before starting the span via {@link * SpanBuilder#addLink(SpanContext, Attributes)} if possible. diff --git a/api/all/src/main/java/io/opentelemetry/api/trace/SpanBuilder.java b/api/all/src/main/java/io/opentelemetry/api/trace/SpanBuilder.java index 28fae81f0dc..dbd258cdaf0 100644 --- a/api/all/src/main/java/io/opentelemetry/api/trace/SpanBuilder.java +++ b/api/all/src/main/java/io/opentelemetry/api/trace/SpanBuilder.java @@ -148,8 +148,9 @@ public interface SpanBuilder { * operations, where a single batch handler processes multiple requests from different traces or * the same trace. * - *

Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span - * context}. + *

Implementations should record links with an {@linkplain SpanContext#isValid() invalid span + * context} if its {@link TraceState} is nonempty. Implementations may ignore calls with an + * invalid span context and an empty {@link TraceState}. * * @param spanContext the context of the linked {@code Span}. * @return this. @@ -163,8 +164,9 @@ public interface SpanBuilder { * operations, where a single batch handler processes multiple requests from different traces or * the same trace. * - *

Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span - * context}. + *

Implementations should record links with an {@linkplain SpanContext#isValid() invalid span + * context} if the attributes or its {@link TraceState} are nonempty. Implementations may ignore + * calls with an invalid span context when both are empty. * * @param spanContext the context of the linked {@code Span}. * @param attributes the attributes of the {@code Link}. diff --git a/exporters/otlp/common/src/test/java/io/opentelemetry/exporter/internal/otlp/traces/TraceRequestMarshalerTest.java b/exporters/otlp/common/src/test/java/io/opentelemetry/exporter/internal/otlp/traces/TraceRequestMarshalerTest.java index 7fd9a84400e..bd9c1cb4344 100644 --- a/exporters/otlp/common/src/test/java/io/opentelemetry/exporter/internal/otlp/traces/TraceRequestMarshalerTest.java +++ b/exporters/otlp/common/src/test/java/io/opentelemetry/exporter/internal/otlp/traces/TraceRequestMarshalerTest.java @@ -505,6 +505,37 @@ void toProtoSpanLink_WithAttributes(MarshalerSource marshalerSource) { .build()); } + @ParameterizedTest + @EnumSource(MarshalerSource.class) + void toProtoSpanLink_WithInvalidContext(MarshalerSource marshalerSource) { + SpanContext context = + SpanContext.createFromRemoteParent( + TraceId.getInvalid(), + SpanId.getInvalid(), + TraceFlags.getSampled(), + TraceState.builder().put("vendor", "value").build()); + assertThat(context.isValid()).isFalse(); + assertThat( + parse( + Span.Link.getDefaultInstance(), + marshalerSource.create( + LinkData.create(context, Attributes.of(stringKey("message.id"), "123"), 2)))) + .isEqualTo( + Span.Link.newBuilder() + .setTraceId(ByteString.copyFrom(new byte[16])) + .setSpanId(ByteString.copyFrom(new byte[8])) + .setFlags( + (TraceFlags.getSampled().asByte() & 0xff) | SpanFlags.getParentIsRemoteMask()) + .setTraceState("vendor=value") + .addAttributes( + KeyValue.newBuilder() + .setKey("message.id") + .setValue(AnyValue.newBuilder().setStringValue("123").build()) + .build()) + .setDroppedAttributesCount(1) + .build()); + } + @SuppressWarnings("unchecked") private static T parse(T prototype, Marshaler marshaler) { byte[] serialized = toByteArray(marshaler); diff --git a/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java b/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java index b67ac3d403b..ef3d2254c0b 100644 --- a/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java +++ b/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java @@ -515,19 +515,23 @@ public ReadWriteSpan updateName(String name) { @Override public Span addLink(SpanContext spanContext, Attributes attributes) { - if (spanContext == null || !spanContext.isValid()) { + if (spanContext == null) { return this; } if (attributes == null) { attributes = Attributes.empty(); } + if (!spanContext.isValid() && attributes.isEmpty() && spanContext.getTraceState().isEmpty()) { + return this; + } LinkData link = LinkData.create( spanContext, AttributeUtil.applyAttributesLimit( attributes, spanLimits.getMaxNumberOfAttributesPerLink(), - spanLimits.getMaxAttributeValueLength())); + spanLimits.getMaxAttributeValueLength()), + attributes.size()); synchronized (lock) { if (!isModifiableByCurrentThread()) { logger.log(Level.FINE, "Calling addLink() on an ended Span."); diff --git a/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpanBuilder.java b/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpanBuilder.java index 050067db815..1305e5c5210 100644 --- a/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpanBuilder.java +++ b/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpanBuilder.java @@ -98,7 +98,7 @@ public SpanBuilder setSpanKind(SpanKind spanKind) { @Override public SpanBuilder addLink(SpanContext spanContext) { - if (spanContext == null || !spanContext.isValid()) { + if (spanContext == null || (!spanContext.isValid() && spanContext.getTraceState().isEmpty())) { return this; } addLink(LinkData.create(spanContext)); @@ -107,12 +107,15 @@ public SpanBuilder addLink(SpanContext spanContext) { @Override public SpanBuilder addLink(SpanContext spanContext, Attributes attributes) { - if (spanContext == null || !spanContext.isValid()) { + if (spanContext == null) { return this; } if (attributes == null) { attributes = Attributes.empty(); } + if (!spanContext.isValid() && attributes.isEmpty() && spanContext.getTraceState().isEmpty()) { + return this; + } int totalAttributeCount = attributes.size(); addLink( LinkData.create( diff --git a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanBuilderTest.java b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanBuilderTest.java index c1ad4626ccb..157435100c5 100644 --- a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanBuilderTest.java +++ b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanBuilderTest.java @@ -15,6 +15,7 @@ import static io.opentelemetry.api.common.AttributeKey.stringKey; import static io.opentelemetry.api.common.AttributeKey.valueKey; import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; import static java.util.stream.Collectors.joining; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; @@ -29,12 +30,14 @@ import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.api.trace.StatusCode; import io.opentelemetry.api.trace.TraceFlags; +import io.opentelemetry.api.trace.TraceId; import io.opentelemetry.api.trace.TraceState; import io.opentelemetry.api.trace.TracerProvider; import io.opentelemetry.context.Context; import io.opentelemetry.context.ContextKey; import io.opentelemetry.context.Scope; import io.opentelemetry.internal.testing.slf4j.SuppressLogger; +import io.opentelemetry.sdk.common.CompletableResultCode; import io.opentelemetry.sdk.trace.data.LinkData; import io.opentelemetry.sdk.trace.data.SpanData; import io.opentelemetry.sdk.trace.samplers.Sampler; @@ -45,10 +48,14 @@ import java.util.List; import java.util.concurrent.TimeUnit; import java.util.stream.IntStream; +import java.util.stream.Stream; import javax.annotation.Nullable; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatchers; import org.mockito.Mock; @@ -92,7 +99,10 @@ void addLink() { SdkSpan span = (SdkSpan) spanBuilder.startSpan(); try { - assertThat(span.toSpanData().getLinks()).hasSize(2); + assertThat(span.toSpanData().getLinks()) + .containsExactly( + LinkData.create(sampledSpanContext), LinkData.create(sampledSpanContext)); + assertThat(span.toSpanData().getTotalRecordedLinks()).isEqualTo(2); } finally { span.end(); } @@ -113,6 +123,119 @@ void addLink_invalid() { } } + @ParameterizedTest + @MethodSource("invalidLinkArguments") + void addLink_invalidWithMetadata( + boolean hasTraceState, @Nullable Attributes attributes, boolean recorded) { + SpanContext context = + SpanContext.create( + TraceId.getInvalid(), + SpanId.getInvalid(), + TraceFlags.builder().setSampled(true).setRandomTraceId(true).build(), + hasTraceState + ? TraceState.builder().put("vendor", "value").build() + : TraceState.getDefault()); + + SdkSpan span = (SdkSpan) sdkTracer.spanBuilder(SPAN_NAME).addLink(context).startSpan(); + span.end(); + assertThat(span.toSpanData().getLinks()) + .containsExactlyElementsOf( + hasTraceState ? singletonList(LinkData.create(context)) : emptyList()); + assertThat(span.toSpanData().getTotalRecordedLinks()).isEqualTo(hasTraceState ? 1 : 0); + + span = (SdkSpan) sdkTracer.spanBuilder(SPAN_NAME).addLink(context, attributes).startSpan(); + span.end(); + assertThat(span.toSpanData().getLinks()) + .containsExactlyElementsOf( + recorded + ? singletonList( + LinkData.create(context, attributes == null ? Attributes.empty() : attributes)) + : emptyList()); + assertThat(span.toSpanData().getTotalRecordedLinks()).isEqualTo(recorded ? 1 : 0); + } + + private static Stream invalidLinkArguments() { + Attributes attributes = Attributes.of(stringKey("message.id"), "123"); + return Stream.of( + Arguments.argumentSet("attributes", false, attributes, true), + Arguments.argumentSet("trace state", true, Attributes.empty(), true), + Arguments.argumentSet("attributes and trace state", true, attributes, true), + Arguments.argumentSet("flags only", false, Attributes.empty(), false), + Arguments.argumentSet("null attributes", false, null, false), + Arguments.argumentSet("trace state and null attributes", true, null, true)); + } + + @ParameterizedTest + @MethodSource("linkLimits") + void addLink_invalidWithMetadataLimitsAndSampling(int maxLinks, int maxAttributes) { + Sampler sampler = Mockito.spy(Sampler.alwaysOn()); + Mockito.when(mockedSpanProcessor.shutdown()).thenReturn(CompletableResultCode.ofSuccess()); + try (SdkTracerProvider provider = + SdkTracerProvider.builder() + .setSampler(sampler) + .addSpanProcessor(mockedSpanProcessor) + .setSpanLimits( + SpanLimits.builder() + .setMaxNumberOfLinks(maxLinks) + .setMaxNumberOfAttributesPerLink(maxAttributes) + .setMaxAttributeValueLength(3) + .build()) + .build()) { + SpanContext invalid = SpanContext.getInvalid(); + SpanContext withTraceState = + SpanContext.create( + TraceId.getInvalid(), + SpanId.getInvalid(), + TraceFlags.getDefault(), + TraceState.builder().put("vendor", "value").build()); + Attributes attributes = Attributes.of(stringKey("key0"), "value", stringKey("key1"), "other"); + SdkSpan span = + (SdkSpan) + provider + .get("test") + .spanBuilder(SPAN_NAME) + .addLink(null, attributes) + .addLink(invalid) + .addLink(invalid, attributes) + .addLink(sampledSpanContext) + .addLink(withTraceState) + .addLink(withTraceState, attributes) + .startSpan(); + span.end(); + SpanData spanData = span.toSpanData(); + assertThat(spanData.getLinks()) + .containsExactlyElementsOf( + maxLinks == 0 + ? emptyList() + : Arrays.asList( + LinkData.create( + invalid, + maxAttributes == 0 + ? Attributes.empty() + : Attributes.of(stringKey("key0"), "val"), + 2), + LinkData.create(sampledSpanContext), + LinkData.create(withTraceState))); + assertThat(spanData.getTotalRecordedLinks()).isEqualTo(4); + Mockito.verify(sampler) + .shouldSample( + ArgumentMatchers.any(), + ArgumentMatchers.anyString(), + ArgumentMatchers.eq(SPAN_NAME), + ArgumentMatchers.eq(SpanKind.INTERNAL), + ArgumentMatchers.eq(Attributes.empty()), + ArgumentMatchers.eq(spanData.getLinks())); + } + } + + private static Stream linkLimits() { + return Stream.of( + Arguments.argumentSet("zero links and attributes", 0, 0), + Arguments.argumentSet("zero links", 0, 1), + Arguments.argumentSet("zero attributes", 3, 0), + Arguments.argumentSet("limited links and attributes", 3, 1)); + } + @Test void truncateLink() { int maxNumberOfLinks = 8; @@ -247,8 +370,11 @@ void addLinkSpanContextAttributes_nullContext() { @Test void addLinkSpanContextAttributes_nullAttributes() { - assertThatCode(() -> sdkTracer.spanBuilder(SPAN_NAME).addLink(sampledSpanContext, null)) - .doesNotThrowAnyException(); + SdkSpan span = + (SdkSpan) sdkTracer.spanBuilder(SPAN_NAME).addLink(sampledSpanContext, null).startSpan(); + span.end(); + assertThat(span.toSpanData().getLinks()).containsExactly(LinkData.create(sampledSpanContext)); + assertThat(span.toSpanData().getTotalRecordedLinks()).isEqualTo(1); } @Test diff --git a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanTest.java b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanTest.java index ccec5eba657..93d11865044 100644 --- a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanTest.java +++ b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanTest.java @@ -37,6 +37,7 @@ import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.api.trace.StatusCode; import io.opentelemetry.api.trace.TraceFlags; +import io.opentelemetry.api.trace.TraceId; import io.opentelemetry.api.trace.TraceState; import io.opentelemetry.context.Context; import io.opentelemetry.sdk.common.InstrumentationScopeInfo; @@ -1030,6 +1031,7 @@ void addLink() { // The 5th attribute key should be omitted due to attribute limits. Can't predict // which of the 5 is dropped. assertThat(link.getAttributes().size()).isEqualTo(4); + assertThat(link.getTotalAttributeCount()).isEqualTo(5); }); } finally { span.end(); @@ -1044,6 +1046,117 @@ void addLink_InvalidArgs() { assertThatCode(() -> span.addLink(null, null)).doesNotThrowAnyException(); assertThatCode(() -> span.addLink(SpanContext.getInvalid(), Attributes.empty())) .doesNotThrowAnyException(); + span.addLink(null, Attributes.of(stringKey("message.id"), "123")); + span.addLink(SpanContext.getInvalid(), null); + assertThat(span.toSpanData().getLinks()).containsExactly(link); + assertThat(span.toSpanData().getTotalRecordedLinks()).isEqualTo(1); + span.addLink(spanContext, null); + span.end(); + assertThat(span.toSpanData().getLinks()).containsExactly(link, LinkData.create(spanContext)); + assertThat(span.toSpanData().getTotalRecordedLinks()).isEqualTo(2); + } + + @ParameterizedTest + @MethodSource("invalidLinkArguments") + void addLink_invalidWithMetadata( + boolean hasTraceState, @Nullable Attributes attributes, boolean recorded) { + SpanContext context = + SpanContext.create( + TraceId.getInvalid(), + SpanId.getInvalid(), + TraceFlags.builder().setSampled(true).setRandomTraceId(true).build(), + hasTraceState + ? TraceState.builder().put("vendor", "value").build() + : TraceState.getDefault()); + SdkSpan span = createTestSpan(SpanKind.INTERNAL); + span.addLink(context); + assertThat(span.toSpanData().getLinks()) + .containsExactlyElementsOf( + hasTraceState ? Arrays.asList(link, LinkData.create(context)) : singletonList(link)); + assertThat(span.toSpanData().getTotalRecordedLinks()).isEqualTo(hasTraceState ? 2 : 1); + span.end(); + span.addLink(context); + assertThat(span.toSpanData().getTotalRecordedLinks()).isEqualTo(hasTraceState ? 2 : 1); + + span = createTestSpan(SpanKind.INTERNAL); + span.addLink(context, attributes); + span.end(); + span.addLink(context, attributes); + assertThat(span.toSpanData().getLinks()) + .containsExactlyElementsOf( + recorded + ? Arrays.asList( + link, + LinkData.create(context, attributes == null ? Attributes.empty() : attributes)) + : singletonList(link)); + assertThat(span.toSpanData().getTotalRecordedLinks()).isEqualTo(recorded ? 2 : 1); + } + + private static Stream invalidLinkArguments() { + Attributes attributes = Attributes.of(stringKey("message.id"), "123"); + return Stream.of( + Arguments.argumentSet("attributes", false, attributes, true), + Arguments.argumentSet("trace state", true, Attributes.empty(), true), + Arguments.argumentSet("attributes and trace state", true, attributes, true), + Arguments.argumentSet("flags only", false, Attributes.empty(), false), + Arguments.argumentSet("null attributes", false, null, false), + Arguments.argumentSet("trace state and null attributes", true, null, true)); + } + + @ParameterizedTest + @MethodSource("linkLimits") + void addLink_invalidWithMetadataLimits(int maxLinks, int maxAttributes) { + SdkSpan span = + createTestSpan( + SpanKind.INTERNAL, + SpanLimits.builder() + .setMaxNumberOfLinks(maxLinks) + .setMaxNumberOfAttributesPerLink(maxAttributes) + .setMaxAttributeValueLength(3) + .build(), + parentSpanId, + null, + Collections.emptyList(), + ExceptionAttributeResolver.getDefault()); + SpanContext invalid = SpanContext.getInvalid(); + SpanContext withTraceState = + SpanContext.create( + TraceId.getInvalid(), + SpanId.getInvalid(), + TraceFlags.getDefault(), + TraceState.builder().put("vendor", "value").build()); + Attributes attributes = Attributes.of(stringKey("key0"), "value", stringKey("key1"), "other"); + span.addLink(null, attributes); + span.addLink(invalid); + span.addLink(invalid, attributes); + span.addLink(spanContext); + span.addLink(withTraceState); + span.addLink(withTraceState, attributes); + span.end(); + + SpanData spanData = span.toSpanData(); + assertThat(spanData.getLinks()) + .containsExactlyElementsOf( + maxLinks == 0 + ? Collections.emptyList() + : Arrays.asList( + LinkData.create( + invalid, + maxAttributes == 0 + ? Attributes.empty() + : Attributes.of(stringKey("key0"), "val"), + 2), + link, + LinkData.create(withTraceState))); + assertThat(spanData.getTotalRecordedLinks()).isEqualTo(4); + } + + private static Stream linkLimits() { + return Stream.of( + Arguments.argumentSet("zero links and attributes", 0, 0), + Arguments.argumentSet("zero links", 0, 1), + Arguments.argumentSet("zero attributes", 3, 0), + Arguments.argumentSet("limited links and attributes", 3, 1)); } @Test