Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix opencensus shim spanBuilderWithRemoteParent behavior #6415

Merged
merged 1 commit into from
May 1, 2024
Merged

Fix opencensus shim spanBuilderWithRemoteParent behavior #6415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ...nsus-shim/src/main/java/io/opentelemetry/opencensusshim/OpenTelemetrySpanBuilderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,11 @@ public Span startSpan() {
otelSpanBuilder.setParent(Context.current().with((OpenTelemetrySpanImpl) ocParent));
}
if (ocRemoteParentSpanContext != null) {
otelSpanBuilder.addLink(SpanConverter.mapSpanContext(ocRemoteParentSpanContext));
io.opentelemetry.api.trace.SpanContext spanContext =
SpanConverter.mapSpanContext(ocRemoteParentSpanContext, /* isRemoteParent= */ true);
otelSpanBuilder.setParent(
Context.current().with(io.opentelemetry.api.trace.Span.wrap(spanContext)));
otelSpanBuilder.addLink(spanContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why adding link also?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure if anyone is dependent on the existing behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we'd remove the addLink before marking the opencensus shim stable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I'd want the spec opencensus compatibility doc to say what to do here before marking stable. That doc is currently lax on details which we see in the opentracing compatibility doc, so I don't see this happening any time soon.

}
if (otelKind != null) {
otelSpanBuilder.setSpanKind(otelKind);
Expand Down
19 changes: 14 additions & 5 deletions opencensus-shim/src/main/java/io/opentelemetry/opencensusshim/SpanConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,22 @@ static SpanContext mapSpanContext(io.opentelemetry.api.trace.SpanContext otelSpa
}

static io.opentelemetry.api.trace.SpanContext mapSpanContext(SpanContext ocSpanContext) {
return io.opentelemetry.api.trace.SpanContext.create(
ocSpanContext.getTraceId().toLowerBase16(),
ocSpanContext.getSpanId().toLowerBase16(),
return mapSpanContext(ocSpanContext, /* isRemoteParent= */ false);
}

static io.opentelemetry.api.trace.SpanContext mapSpanContext(
SpanContext ocSpanContext, boolean isRemoteParent) {
String traceId = ocSpanContext.getTraceId().toLowerBase16();
String spanId = ocSpanContext.getSpanId().toLowerBase16();
TraceFlags traceFlags =
ocSpanContext.getTraceOptions().isSampled()
? TraceFlags.getSampled()
: TraceFlags.getDefault(),
mapTracestate(ocSpanContext.getTracestate()));
: TraceFlags.getDefault();
TraceState traceState = mapTracestate(ocSpanContext.getTracestate());
return isRemoteParent
? io.opentelemetry.api.trace.SpanContext.createFromRemoteParent(
traceId, spanId, traceFlags, traceState)
: io.opentelemetry.api.trace.SpanContext.create(traceId, spanId, traceFlags, traceState);
}

private static TraceState mapTracestate(Tracestate tracestate) {
Expand Down
30 changes: 22 additions & 8 deletions opencensus-shim/src/test/java/io/opentelemetry/opencensusshim/InteroperabilityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import io.opentelemetry.sdk.trace.data.StatusData;
import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor;
import io.opentelemetry.sdk.trace.export.SpanExporter;
import io.opentelemetry.sdk.trace.samplers.Sampler;
import java.util.Collection;
import java.util.concurrent.TimeUnit;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -71,7 +72,11 @@ class InteroperabilityTest {
SpanProcessor spanProcessor = SimpleSpanProcessor.create(spanExporter);
openTelemetry =
OpenTelemetrySdk.builder()
.setTracerProvider(SdkTracerProvider.builder().addSpanProcessor(spanProcessor).build())
.setTracerProvider(
SdkTracerProvider.builder()
.setSampler(Sampler.alwaysOn())
.addSpanProcessor(spanProcessor)
.build())
.buildAndRegisterGlobal();
}

Expand All @@ -84,7 +89,7 @@ void resetMocks() {
}

@Test
void testParentChildRelationshipsAreExportedCorrectly() {
void parentChildRelationshipsAreExportedCorrectly() {
Tracer tracer = openTelemetry.getTracer("io.opentelemetry.test.scoped.span.1");
Span span = tracer.spanBuilder("OpenTelemetrySpan").startSpan();
try (Scope scope = Context.current().with(span).makeCurrent()) {
Expand Down Expand Up @@ -128,7 +133,7 @@ void testParentChildRelationshipsAreExportedCorrectly() {
}

@Test
void testRemoteParent() {
void remoteParent() {
io.opencensus.trace.Tracer tracer = Tracing.getTracer();
io.opencensus.trace.Span remoteParentSpan =
tracer.spanBuilder("remote parent span").startSpan();
Expand All @@ -148,14 +153,23 @@ void testRemoteParent() {

assertThat(export1.size()).isEqualTo(1);
SpanData spanData1 = export1.iterator().next();

// Remote parent should be set to parent span context
assertThat(spanData1.getParentSpanContext().isRemote()).isTrue();
assertThat(spanData1.getParentSpanContext().getTraceId())
.isEqualTo(remoteParentSpan.getContext().getTraceId().toLowerBase16());
assertThat(spanData1.getParentSpanContext().getSpanId())
.isEqualTo(remoteParentSpan.getContext().getSpanId().toLowerBase16());

// Remote parent should be added as link
assertThat(spanData1.getName()).isEqualTo("OpenCensusSpan");
assertThat(spanData1.getLinks().get(0).getSpanContext().getSpanId())
.isEqualTo(remoteParentSpan.getContext().getSpanId().toLowerBase16());
}

@Test
@SuppressLogger(OpenTelemetrySpanImpl.class)
void testParentChildRelationshipsAreExportedCorrectlyForOpenCensusOnly() {
void parentChildRelationshipsAreExportedCorrectlyForOpenCensusOnly() {
io.opencensus.trace.Tracer tracer = Tracing.getTracer();
io.opencensus.trace.Span parentLinkSpan = tracer.spanBuilder("parent link span").startSpan();
try (io.opencensus.common.Scope scope =
Expand Down Expand Up @@ -261,7 +275,7 @@ void testParentChildRelationshipsAreExportedCorrectlyForOpenCensusOnly() {
}

@Test
void testOpenTelemetryMethodsOnOpenCensusSpans() {
void openTelemetryMethodsOnOpenCensusSpans() {
io.opencensus.trace.Tracer tracer = Tracing.getTracer();
try (io.opencensus.common.Scope scope =
tracer
Expand Down Expand Up @@ -308,7 +322,7 @@ void testOpenTelemetryMethodsOnOpenCensusSpans() {
}

@Test
public void testNoSampleDoesNotExport() {
public void noSampleDoesNotExport() {
io.opencensus.trace.Tracer tracer = Tracing.getTracer();
try (io.opencensus.common.Scope scope =
tracer.spanBuilder("OpenCensusSpan").setSampler(Samplers.neverSample()).startScopedSpan()) {
Expand All @@ -324,14 +338,14 @@ public void testNoSampleDoesNotExport() {
}

@Test
public void testOpenCensusSamplerIsAlwaysOn() {
public void openCensusSamplerIsAlwaysOn() {
// OpenTelemetryTraceComponentImpl provides this behavior
assertThat(Tracing.getTraceConfig().getActiveTraceParams().getSampler())
.isEqualTo(Samplers.alwaysSample());
}

@Test
public void testByDefaultDoesExport() {
public void byDefaultDoesExport() {
io.opencensus.trace.Tracer tracer = Tracing.getTracer();
try (io.opencensus.common.Scope scope =
tracer.spanBuilder("OpenCensusSpan").setRecordEvents(false).startScopedSpan()) {
Expand Down
Loading