From 1f74d5c84b131de3ae63553eb5a45b83fa200e26 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Tue, 1 Nov 2022 14:13:23 +0100 Subject: [PATCH 1/6] Full multiple parent support. This includes: * The Baggage union of ALL parents is used. * All parents are added as Links, in order to preserve the OpenTracing reference type as an attribute (either CHILD_OF or FOLLOWS_FROM). --- opentracing-shim/build.gradle.kts | 2 + .../opentracingshim/SpanBuilderShim.java | 149 ++++++++++++---- .../opentracingshim/SpanBuilderShimTest.java | 164 ++++++++++++++++++ 3 files changed, 284 insertions(+), 31 deletions(-) diff --git a/opentracing-shim/build.gradle.kts b/opentracing-shim/build.gradle.kts index 9947bc7f5c5..98129994fbc 100644 --- a/opentracing-shim/build.gradle.kts +++ b/opentracing-shim/build.gradle.kts @@ -12,6 +12,8 @@ dependencies { api("io.opentracing:opentracing-api") implementation(project(":semconv")) + annotationProcessor("com.google.auto.value:auto-value") + testImplementation(project(":sdk:testing")) } diff --git a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java index 5ed3baa8bf1..0e89d908543 100644 --- a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java +++ b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java @@ -10,32 +10,35 @@ import static io.opentelemetry.api.common.AttributeKey.longKey; import static io.opentelemetry.api.common.AttributeKey.stringKey; +import com.google.auto.value.AutoValue; import io.opentelemetry.api.baggage.Baggage; +import io.opentelemetry.api.baggage.BaggageBuilder; import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.api.trace.StatusCode; import io.opentelemetry.context.Context; +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; +import io.opentracing.References; import io.opentracing.Span; import io.opentracing.SpanContext; import io.opentracing.Tracer.SpanBuilder; import io.opentracing.tag.Tag; import io.opentracing.tag.Tags; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; +import javax.annotation.concurrent.Immutable; final class SpanBuilderShim extends BaseShimObject implements SpanBuilder { private final String spanName; - // The parent will be either a Span or a SpanContext. - // Inherited baggage is supported only for the main parent. - @Nullable private SpanShim parentSpan; - @Nullable private SpanContextShim parentSpanContext; + // *All* parents are saved in this list. + private List allParents = Collections.emptyList(); private boolean ignoreActiveSpan; - private final List parentLinks = new ArrayList<>(); - @SuppressWarnings("rawtypes") private final List spanBuilderAttributeKeys = new ArrayList<>(); @@ -44,6 +47,15 @@ final class SpanBuilderShim extends BaseShimObject implements SpanBuilder { private boolean error; private long startTimestampMicros; + private static final Attributes CHILD_OF_ATTR = + Attributes.of( + SemanticAttributes.OPENTRACING_REF_TYPE, + SemanticAttributes.OpentracingRefTypeValues.CHILD_OF); + private static final Attributes FOLLOWS_FROM_ATTR = + Attributes.of( + SemanticAttributes.OPENTRACING_REF_TYPE, + SemanticAttributes.OpentracingRefTypeValues.FOLLOWS_FROM); + public SpanBuilderShim(TelemetryInfo telemetryInfo, String spanName) { super(telemetryInfo); this.spanName = spanName; @@ -57,19 +69,12 @@ public SpanBuilder asChildOf(Span parent) { // TODO - Verify we handle a no-op Span SpanShim spanShim = ShimUtil.getSpanShim(parent); - - if (parentSpan == null && parentSpanContext == null) { - parentSpan = spanShim; - } else { - parentLinks.add(spanShim.getSpan().getSpanContext()); - } - - return this; + return addReference(References.CHILD_OF, spanShim.context()); } @Override public SpanBuilder asChildOf(SpanContext parent) { - return addReference(null, parent); + return addReference(References.CHILD_OF, parent); } @Override @@ -78,13 +83,30 @@ public SpanBuilder addReference(@Nullable String referenceType, SpanContext refe return this; } - // TODO - Use referenceType + ReferenceType refType; + if (References.CHILD_OF.equals(referenceType)) { + refType = ReferenceType.CHILD_OF; + } else if (References.FOLLOWS_FROM.equals(referenceType)) { + refType = ReferenceType.FOLLOWS_FROM; + } else { + // Discard references with unrecognized type. + return this; + } + SpanContextShim contextShim = ShimUtil.getContextShim(referencedContext); - if (parentSpan == null && parentSpanContext == null) { - parentSpanContext = contextShim; + // Optimization for 99% situations, when there is only one parent. + if (allParents.size() == 0) { + allParents = + Collections.singletonList( + SpanParentInfo.create( + contextShim.getSpanContext(), contextShim.getBaggage(), refType)); } else { - parentLinks.add(contextShim.getSpanContext()); + if (allParents.size() == 1) { + allParents = new ArrayList(allParents); + } + allParents.add( + SpanParentInfo.create(contextShim.getSpanContext(), contextShim.getBaggage(), refType)); } return this; @@ -186,26 +208,26 @@ public SpanBuilder withStartTimestamp(long microseconds) { @SuppressWarnings({"rawtypes", "unchecked"}) @Override public Span start() { - Baggage baggage = Baggage.empty(); + Baggage baggage = null; io.opentelemetry.api.trace.SpanBuilder builder = tracer().spanBuilder(spanName); + io.opentelemetry.api.trace.SpanContext mainParent = getMainParent(allParents); - if (ignoreActiveSpan && parentSpan == null && parentSpanContext == null) { + if (ignoreActiveSpan && mainParent == null) { builder.setNoParent(); - } else if (parentSpan != null) { - builder.setParent(Context.root().with(parentSpan.getSpan())); - baggage = ((SpanContextShim) parentSpan.context()).getBaggage(); - } else if (parentSpanContext != null) { - builder.setParent( - Context.root() - .with(io.opentelemetry.api.trace.Span.wrap(parentSpanContext.getSpanContext()))); - baggage = parentSpanContext.getBaggage(); + baggage = Baggage.empty(); + } else if (mainParent != null) { + builder.setParent(Context.root().with(io.opentelemetry.api.trace.Span.wrap(mainParent))); + baggage = getAllBaggage(allParents); } else { // No explicit parent Span, but extracted baggage may be available. baggage = Baggage.current(); } - for (io.opentelemetry.api.trace.SpanContext link : parentLinks) { - builder.addLink(link); + // *All* parents are processed as Links, in order to keep the reference type value. + for (SpanParentInfo parentInfo : allParents) { + builder.addLink( + parentInfo.getSpanContext(), + parentInfo.getRefType() == ReferenceType.CHILD_OF ? CHILD_OF_ATTR : FOLLOWS_FROM_ATTR); } if (spanKind != null) { @@ -232,4 +254,69 @@ public Span start() { return new SpanShim(telemetryInfo(), span, baggage); } + + // The first SpanContext with Child Of type in the entire list is used as parent, + // else the the first SpanContext is used as parent. + @Nullable + static io.opentelemetry.api.trace.SpanContext getMainParent(List parents) { + if (parents.size() == 0) { + return null; + } + + SpanParentInfo mainParent = null; + + for (SpanParentInfo parentInfo : parents) { + if (parentInfo.getRefType() == ReferenceType.CHILD_OF) { + mainParent = parentInfo; + break; + } + } + + mainParent = mainParent == null ? parents.get(0) : mainParent; + return mainParent.getSpanContext(); + } + + static Baggage getAllBaggage(List parents) { + if (parents.size() == 0) { + return Baggage.empty(); + } + + if (parents.size() == 1) { + return parents.get(0).getBaggage(); + } + + BaggageBuilder builder = Baggage.builder(); + for (SpanParentInfo parent : parents) { + parent + .getBaggage() + .forEach( + (key, entry) -> { + builder.put(key, entry.getValue()); + }); + } + + return builder.build(); + } + + @AutoValue + @Immutable + abstract static class SpanParentInfo { + public static SpanParentInfo create( + io.opentelemetry.api.trace.SpanContext spanContext, + Baggage baggage, + ReferenceType refType) { + return new AutoValue_SpanBuilderShim_SpanParentInfo(spanContext, baggage, refType); + } + + abstract io.opentelemetry.api.trace.SpanContext getSpanContext(); + + abstract Baggage getBaggage(); + + abstract ReferenceType getRefType(); + } + + enum ReferenceType { + CHILD_OF, + FOLLOWS_FROM + } } diff --git a/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/SpanBuilderShimTest.java b/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/SpanBuilderShimTest.java index 06fc3891d8c..7430c77e6bc 100644 --- a/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/SpanBuilderShimTest.java +++ b/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/SpanBuilderShimTest.java @@ -20,6 +20,8 @@ import io.opentelemetry.sdk.trace.data.SpanData; import io.opentelemetry.sdk.trace.samplers.Sampler; import io.opentelemetry.sdk.trace.samplers.SamplingResult; +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; +import io.opentracing.References; import java.util.List; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -38,6 +40,142 @@ void setUp() { GlobalOpenTelemetry.resetForTest(); } + @Test + void parent_single() { + SpanShim parentSpan = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + try { + SpanShim childSpan = + (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).asChildOf(parentSpan).start(); + + try { + SpanData spanData = ((ReadableSpan) childSpan.getSpan()).toSpanData(); + assertThat(parentSpan.context().toSpanId()).isEqualTo(spanData.getParentSpanId()); + } finally { + childSpan.finish(); + } + } finally { + parentSpan.finish(); + } + } + + @Test + void parent_multipleFollowsFrom() { + SpanShim parentSpan1 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + SpanShim parentSpan2 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + + try { + SpanShim childSpan = + (SpanShim) + new SpanBuilderShim(telemetryInfo, SPAN_NAME) + .addReference(References.FOLLOWS_FROM, parentSpan1.context()) + .addReference(References.FOLLOWS_FROM, parentSpan2.context()) + .start(); + + try { + // If no parent of CHILD_OF type exists, use the first value as main parent. + SpanData spanData = ((ReadableSpan) childSpan.getSpan()).toSpanData(); + assertThat(parentSpan1.context().toSpanId()).isEqualTo(spanData.getParentSpanId()); + } finally { + childSpan.finish(); + } + } finally { + parentSpan1.finish(); + parentSpan2.finish(); + } + } + + @Test + void parent_multipleDifferentRefType() { + SpanShim parentSpan1 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + SpanShim parentSpan2 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + SpanShim parentSpan3 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + + try { + SpanShim childSpan = + (SpanShim) + new SpanBuilderShim(telemetryInfo, SPAN_NAME) + .addReference(References.FOLLOWS_FROM, parentSpan1.context()) + .addReference(References.CHILD_OF, parentSpan2.context()) + .addReference(References.CHILD_OF, parentSpan3.context()) + .start(); + + try { + // The first parent with CHILD_OF becomes the direct parent (if any). + SpanData spanData = ((ReadableSpan) childSpan.getSpan()).toSpanData(); + assertThat(parentSpan2.context().toSpanId()).isEqualTo(spanData.getParentSpanId()); + } finally { + childSpan.finish(); + } + } finally { + parentSpan1.finish(); + parentSpan2.finish(); + parentSpan3.finish(); + } + } + + @Test + void parent_multipleLinks() { + SpanShim parentSpan1 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + SpanShim parentSpan2 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + SpanShim parentSpan3 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + + try { + SpanShim childSpan = + (SpanShim) + new SpanBuilderShim(telemetryInfo, SPAN_NAME) + .addReference(References.FOLLOWS_FROM, parentSpan1.context()) + .addReference(References.CHILD_OF, parentSpan2.context()) + .addReference(References.FOLLOWS_FROM, parentSpan3.context()) + .start(); + + try { + SpanData spanData = ((ReadableSpan) childSpan.getSpan()).toSpanData(); + List links = spanData.getLinks(); + assertThat(links).hasSize(3); + + assertThat(links.get(0).getSpanContext()).isEqualTo(parentSpan1.getSpan().getSpanContext()); + assertThat(links.get(1).getSpanContext()).isEqualTo(parentSpan2.getSpan().getSpanContext()); + assertThat(links.get(2).getSpanContext()).isEqualTo(parentSpan3.getSpan().getSpanContext()); + assertThat(links.get(0).getAttributes().get(SemanticAttributes.OPENTRACING_REF_TYPE)) + .isEqualTo(SemanticAttributes.OpentracingRefTypeValues.FOLLOWS_FROM); + assertThat(links.get(1).getAttributes().get(SemanticAttributes.OPENTRACING_REF_TYPE)) + .isEqualTo(SemanticAttributes.OpentracingRefTypeValues.CHILD_OF); + assertThat(links.get(2).getAttributes().get(SemanticAttributes.OPENTRACING_REF_TYPE)) + .isEqualTo(SemanticAttributes.OpentracingRefTypeValues.FOLLOWS_FROM); + + } finally { + childSpan.finish(); + } + } finally { + parentSpan1.finish(); + parentSpan2.finish(); + parentSpan3.finish(); + } + } + + @Test + void parent_wrongRefType() { + SpanShim parentSpan = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + try { + SpanShim childSpan = + (SpanShim) + new SpanBuilderShim(telemetryInfo, SPAN_NAME) + .addReference("wrongreftype-value", parentSpan.context()) + .start(); + + try { + // Incorrect typeref values get discarded. + SpanData spanData = ((ReadableSpan) childSpan.getSpan()).toSpanData(); + assertThat(spanData.getParentSpanContext()) + .isEqualTo(io.opentelemetry.api.trace.SpanContext.getInvalid()); + } finally { + childSpan.finish(); + } + } finally { + parentSpan.finish(); + } + } + @Test void baggage_parent() { SpanShim parentSpan = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); @@ -79,6 +217,32 @@ void baggage_parentContext() { } } + @Test + void baggage_multipleParents() { + SpanShim parentSpan1 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + SpanShim parentSpan2 = (SpanShim) new SpanBuilderShim(telemetryInfo, SPAN_NAME).start(); + try { + parentSpan1.setBaggageItem("key1", "value1"); + parentSpan2.setBaggageItem("key2", "value2"); + + SpanShim childSpan = + (SpanShim) + new SpanBuilderShim(telemetryInfo, SPAN_NAME) + .addReference(References.FOLLOWS_FROM, parentSpan1.context()) + .addReference(References.CHILD_OF, parentSpan2.context()) + .start(); + try { + assertThat(childSpan.getBaggageItem("key1")).isEqualTo("value1"); + assertThat(childSpan.getBaggageItem("key2")).isEqualTo("value2"); + } finally { + childSpan.finish(); + } + } finally { + parentSpan1.finish(); + parentSpan2.finish(); + } + } + @Test void baggage_spanWithInvalidSpan() { io.opentelemetry.api.baggage.Baggage baggage = From 8e302c917c4cb8c297002bc04c851047297ed5c9 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Mon, 7 Nov 2022 13:59:19 +0100 Subject: [PATCH 2/6] Update opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com> --- .../java/io/opentelemetry/opentracingshim/SpanBuilderShim.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java index 0e89d908543..a2d27175527 100644 --- a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java +++ b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java @@ -103,7 +103,7 @@ public SpanBuilder addReference(@Nullable String referenceType, SpanContext refe contextShim.getSpanContext(), contextShim.getBaggage(), refType)); } else { if (allParents.size() == 1) { - allParents = new ArrayList(allParents); + allParents = new ArrayList<>(allParents); } allParents.add( SpanParentInfo.create(contextShim.getSpanContext(), contextShim.getBaggage(), refType)); From 24a5b26dd77baeaacc4606ad0ab0112fbb6128f9 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Mon, 7 Nov 2022 13:59:26 +0100 Subject: [PATCH 3/6] Update opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com> --- .../java/io/opentelemetry/opentracingshim/SpanBuilderShim.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java index a2d27175527..5250d9773c9 100644 --- a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java +++ b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java @@ -208,7 +208,7 @@ public SpanBuilder withStartTimestamp(long microseconds) { @SuppressWarnings({"rawtypes", "unchecked"}) @Override public Span start() { - Baggage baggage = null; + Baggage baggage; io.opentelemetry.api.trace.SpanBuilder builder = tracer().spanBuilder(spanName); io.opentelemetry.api.trace.SpanContext mainParent = getMainParent(allParents); From 3fa2c31a5dfda68089575439578421aefe8a9dc3 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Mon, 7 Nov 2022 13:59:33 +0100 Subject: [PATCH 4/6] Update opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com> --- .../java/io/opentelemetry/opentracingshim/SpanBuilderShim.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java index 5250d9773c9..2ef1ab8326b 100644 --- a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java +++ b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java @@ -256,7 +256,7 @@ public Span start() { } // The first SpanContext with Child Of type in the entire list is used as parent, - // else the the first SpanContext is used as parent. + // else the first SpanContext is used as parent. @Nullable static io.opentelemetry.api.trace.SpanContext getMainParent(List parents) { if (parents.size() == 0) { From fbcc49e6e228f064f5eaece6c2191e282d59a11b Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Mon, 7 Nov 2022 14:35:30 +0100 Subject: [PATCH 5/6] Apply feedback. --- .../opentracingshim/SpanBuilderShim.java | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java index 2ef1ab8326b..9c3f8db4946 100644 --- a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java +++ b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java @@ -263,8 +263,7 @@ static io.opentelemetry.api.trace.SpanContext getMainParent(List return null; } - SpanParentInfo mainParent = null; - + SpanParentInfo mainParent = parents.get(0); for (SpanParentInfo parentInfo : parents) { if (parentInfo.getRefType() == ReferenceType.CHILD_OF) { mainParent = parentInfo; @@ -272,7 +271,6 @@ static io.opentelemetry.api.trace.SpanContext getMainParent(List } } - mainParent = mainParent == null ? parents.get(0) : mainParent; return mainParent.getSpanContext(); } @@ -287,12 +285,7 @@ static Baggage getAllBaggage(List parents) { BaggageBuilder builder = Baggage.builder(); for (SpanParentInfo parent : parents) { - parent - .getBaggage() - .forEach( - (key, entry) -> { - builder.put(key, entry.getValue()); - }); + parent.getBaggage().forEach((key, entry) -> builder.put(key, entry.getValue())); } return builder.build(); @@ -301,7 +294,7 @@ static Baggage getAllBaggage(List parents) { @AutoValue @Immutable abstract static class SpanParentInfo { - public static SpanParentInfo create( + private static SpanParentInfo create( io.opentelemetry.api.trace.SpanContext spanContext, Baggage baggage, ReferenceType refType) { From e24dcb89f7ed4d8ad860c8203ad32677b0b4d03a Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Wed, 9 Nov 2022 13:54:59 +0100 Subject: [PATCH 6/6] More feedback. --- .../io/opentelemetry/opentracingshim/SpanBuilderShimTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/SpanBuilderShimTest.java b/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/SpanBuilderShimTest.java index 7430c77e6bc..05a412478c3 100644 --- a/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/SpanBuilderShimTest.java +++ b/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/SpanBuilderShimTest.java @@ -168,6 +168,7 @@ void parent_wrongRefType() { SpanData spanData = ((ReadableSpan) childSpan.getSpan()).toSpanData(); assertThat(spanData.getParentSpanContext()) .isEqualTo(io.opentelemetry.api.trace.SpanContext.getInvalid()); + assertThat(spanData.getLinks()).isEmpty(); } finally { childSpan.finish(); }