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

Update OpenTelemetry Bridge to support OTel Java API 0.11.0 #2054

Closed
wants to merge 2 commits into from
Closed

Update OpenTelemetry Bridge to support OTel Java API 0.11.0 #2054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1fef18f
Move Otel classes
tylerbenson Nov 5, 2020
10aee28
Update OpenTelemetry Bridge to latest version
tylerbenson Nov 2, 2020
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
33 changes: 18 additions & 15 deletions dd-java-agent/instrumentation/opentelemetry/opentelemetry.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,38 @@
ext {
minJavaVersionForTests = JavaVersion.VERSION_1_8
}

// Made this a variable so we can easily update to latest releases.
def otelVersion = '0.3.0'
def otelVersion = '0.11.0'

muzzle {
pass {
module = 'opentelemetry-api'
group = 'io.opentelemetry'
versions = "[$otelVersion,0.8.0)"
versions = "[$otelVersion,]"
assertInverse = true
// I have no idea why gradle doesn't respect the version range above
// but I also have no time to figure it out for an instrumentation
// that is experimental
skipVersions = ['0.2.2', '0.2.3', '0.9.0']
}
}

apply from: "$rootDir/gradle/java.gradle"

apply plugin: 'org.unbroken-dome.test-sets'

testSets {
latestDepTest {
dirName = 'test'
}
}
// Can't have a latest test until a new version is released.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why. Care to elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our latestDepTest has an assertion to verify that the version it is testing isn't the same as what is verified in test to ensure we declare the wildcard properly. There is a new version out now (0.10.1), so I can fix this.

Copy link
Member

Choose a reason for hiding this comment

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

I think that assert is counterproductive. I would have updated the gRPC instrumentation when I had time if it weren't for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I added that, we had cases where we thought we were testing something new but were not. The assertion was an attempt to prevent a regression. Seems silly to run the same tests twice.

//apply plugin: 'org.unbroken-dome.test-sets'
//
//testSets {
// latestDepTest {
// dirName = 'test'
// }
//}

dependencies {
compileOnly group: 'io.opentelemetry', name: 'opentelemetry-api', version: otelVersion
main_java8CompileOnly group: 'io.opentelemetry', name: 'opentelemetry-api', version: otelVersion

compileOnly group: 'com.google.code.findbugs', name: 'jsr305', version: '3.0.2'
compileOnly group: 'com.google.auto.value', name: 'auto-value-annotations', version: '1.6.6'
main_java8CompileOnly group: 'com.google.code.findbugs', name: 'jsr305', version: '3.0.2'
main_java8CompileOnly group: 'com.google.auto.value', name: 'auto-value-annotations', version: '1.6.6'

testCompile group: 'io.opentelemetry', name: 'opentelemetry-api', version: otelVersion
latestDepTestCompile group: 'io.opentelemetry', name: 'opentelemetry-api', version: '0.7+'
// latestDepTestCompile group: 'io.opentelemetry', name: 'opentelemetry-api', version: '0.7+'
}
59 changes: 26 additions & 33 deletions ...c/main/java/datadog/trace/instrumentation/opentelemetry/OpenTelemetryInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
package datadog.trace.instrumentation.opentelemetry;

import static datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers.implementsInterface;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.returns;
import static net.bytebuddy.matcher.ElementMatchers.takesNoArguments;

import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Instrumenter;
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.trace.TracerProvider;
import java.util.HashMap;
import datadog.trace.bootstrap.ContextStore;
import datadog.trace.bootstrap.InstrumentationContext;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import io.opentelemetry.api.OpenTelemetryBuilder;
import io.opentelemetry.api.trace.SpanContext;
import java.util.Map;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
Expand All @@ -30,58 +34,47 @@ protected boolean defaultEnabled() {

@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return named("io.opentelemetry.OpenTelemetry");
return implementsInterface(named("io.opentelemetry.api.OpenTelemetryBuilder"));
}

@Override
public String[] helperClassNames() {
return new String[] {
packageName + ".OtelScope",
packageName + ".OtelSpan",
packageName + ".OtelSpan$1", // switch statement
packageName + ".OtelSpanContext",
packageName + ".OtelTracer",
packageName + ".OtelTracer$1", // switch statement
packageName + ".OtelTracerProvider",
packageName + ".OtelTracer$SpanBuilder",
packageName + ".OtelTracer$OtelSpanBuilder",
packageName + ".OtelContextPropagators",
packageName + ".OtelContextPropagators$1", // switch statement
packageName + ".OtelContextPropagators$OtelHttpTextFormat",
packageName + ".OtelContextPropagators$OtelTextMapPropagator",
packageName + ".OtelContextPropagators$OtelSetter",
packageName + ".OtelContextPropagators$OtelGetter",
packageName + ".TypeConverter",
packageName + ".TypeConverter$OtelSpanContext"
};
}

@Override
public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
final Map<ElementMatcher<? super MethodDescription>, String> transformers = new HashMap<>();
transformers.put(
named("getTracerProvider").and(returns(named("io.opentelemetry.trace.TracerProvider"))),
OpenTelemetryInstrumentation.class.getName() + "$TracerProviderAdvice");
transformers.put(
named("getPropagators")
.and(returns(named("io.opentelemetry.context.propagation.ContextPropagators"))),
OpenTelemetryInstrumentation.class.getName() + "$ContextPropagatorsAdvice");
return transformers;
return singletonMap(
named("build").and(takesNoArguments()),
OpenTelemetryInstrumentation.class.getName() + "$BuilderAdvice");
}

public static class TracerProviderAdvice {
@Advice.OnMethodExit(suppress = Throwable.class)
public static void returnProvider(@Advice.Return(readOnly = false) TracerProvider result) {
result = OtelTracerProvider.INSTANCE;
}
@Override
public Map<String, String> contextStore() {
return singletonMap(
"io.opentelemetry.api.trace.SpanContext", AgentSpan.Context.class.getName());
}

public static class ContextPropagatorsAdvice {
@Advice.OnMethodExit(suppress = Throwable.class)
public static void returnProvider(@Advice.Return(readOnly = false) ContextPropagators result) {
result = OtelContextPropagators.INSTANCE;
}

// Muzzle doesn't detect the advice method's argument type, so we have to help it a bit.
public static void muzzleCheck(final ContextPropagators propagators) {
propagators.getHttpTextFormat();
public static class BuilderAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void beforeBuild(@Advice.This OpenTelemetryBuilder builder) {
ContextStore<SpanContext, AgentSpan.Context> spanContextStore =
InstrumentationContext.get(SpanContext.class, AgentSpan.Context.class);
builder.setTracerProvider(new OtelTracerProvider(spanContextStore));
Copy link
Contributor

Choose a reason for hiding this comment

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

How deeply does OTelTracerProvider & OTelContextPropagators get checked by muzzle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see both classes enumerated in the printReferences which suggests they are covered sufficiently. Is there something specific you're looking for?

Copy link
Contributor

@dougqh dougqh Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I want to know how deeply we check that OTelTracerProvider and OtelContextPropagators fit the latest OTel API. For instance, do we check that OTelTracerProvider implements all the necessary methods.

Otherwise, I think this has holes where we'll activate when we shouldn't. I think that is what happened recently with servlet-3.1. Mostly, I'm just curious, and I'm fine with addressing that issue in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Muzzle is validating those classes right now, but we have known issues with the depth that muzzle validates. I don't think that is specific to this PR. I suggest we schedule a separate effort for improving muzzle.

builder.setPropagators(new OtelContextPropagators(spanContextStore));
}
}
}
110 changes: 0 additions & 110 deletions ...try/src/main/java/datadog/trace/instrumentation/opentelemetry/OtelContextPropagators.java
View file
Open in desktop

This file was deleted.

42 changes: 0 additions & 42 deletions ...on/opentelemetry/src/main/java/datadog/trace/instrumentation/opentelemetry/OtelScope.java
View file
Open in desktop

This file was deleted.

Loading