Retain invalid span links with attributes or trace state

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.
    *
-   * <p>Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span
-   * context}.
+   * <p>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}.
    *
    * <p>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.
    *
-   * <p>Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span
-   * context}.
+   * <p>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.
    *
    * <p>Callers should prefer to add links before starting the span via {@link
    * SpanBuilder#addLink(SpanContext, Attributes)} if possible.

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.
    *
-   * <p>Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span
-   * context}.
+   * <p>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.
    *
-   * <p>Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span
-   * context}.
+   * <p>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}.

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 extends Message> T parse(T prototype, Marshaler marshaler) {
     byte[] serialized = toByteArray(marshaler);

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.");

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(

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<Arguments> 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<Arguments> 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