Update OpenTelemetry Bridge to support OTel Java API 0.11.0 #2054

tylerbenson · 2020-11-05T19:37:37Z

See also #1605.

To enable:

System Property: -Ddd.integration.opentelemetry-beta.enabled=true
Environment Variable: DD_INTEGRATION_OPENTELEMETRY_BETA_ENABLED=true

Remove some of the interfaces from the wrapped objects to stop encouraging casting to our interfaces as a way to integrate. (We will need to expose better APIs in the future, depending on what we want to allow.)

Add instrumentation to sync our context with the new otel context. Due to implementation differences, there are still some edge cases where they can get out of sync. (See the test for a demonstration of this.)

(If you skip over the first commit where the classes get moved, it gives a nicer diff.)

...datadog/trace/instrumentation/opentelemetry/context/OpenTelemetryContextInstrumentation.java

bantonsson · 2020-11-11T16:06:18Z

dd-java-agent/instrumentation/opentelemetry/opentelemetry.gradle

-    dirName = 'test'
-  }
-}
+// Can't have a latest test until a new version is released.


I don't understand why. Care to elaborate?

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.

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

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.

bantonsson · 2020-11-11T16:09:26Z

internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java

@@ -73,6 +73,8 @@

    DDId getSpanId();

+    boolean isRemote();


What does isRemote mean?

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#isremote

if the SpanContext was propagated from a remote parent

ie, if the context was the result of a distributed trace propagation. (extract call)

randomanderson

Some minor nits but looks correct from a "upgrade to latest version" perspective. I assumed the previous version was correct

.../opentelemetry/src/main/java8/datadog/trace/instrumentation/opentelemetry/TypeConverter.java

dd-java-agent/instrumentation/opentelemetry/src/test/groovy/OpenTelemetryTest.groovy

internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java

dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java

dd-java-agent/instrumentation/opentelemetry/src/test/groovy/OpenTelemetryTest.groovy

dougqh · 2020-11-11T20:45:33Z

...src/main/java8/datadog/trace/instrumentation/opentelemetry/context/ContextStorageAdvice.java

+  @Advice.OnMethodExit(suppress = Throwable.class)
+  public static void updateScope(
+      @Advice.Argument(0) Context context, @Advice.Return(readOnly = false) Scope scope) {
+    Span span = Span.fromContext(context);


I'd like some more explanation as to what is happening here.

I don't really understand the need for WrappedScope.
I'd expect the OTelScope to be a wrapper around an AgentScope -- and the fact that they are closed together suggests they have the same lifecycle.

.../opentelemetry/src/main/java8/datadog/trace/instrumentation/opentelemetry/TypeConverter.java

dougqh · 2020-11-11T20:51:48Z

.../opentelemetry/src/main/java8/datadog/trace/instrumentation/opentelemetry/TypeConverter.java

+  }
+
+  public AgentSpan.Context toAgentSpanContext(final SpanContext spanContext) {
+    // Currently assuming the span context was created above so it should contain an existing


Given that this is public, this is worrying assumption. Should this method just be private/package visible instead?

I will make it package visible, but I don't think that will avoid the problem. Someone can still call SpanContext.create and pass it into SpanBuilder.setParent and it will result in this problem. Fixing this would require significant changes to the core.

Can you please describe the ramifications of calling SpanContext.create and SpanBuilder.setParent?
In general, I don't want to assume that significant changes to the core aren't necessary without fully understanding the problem first.

I have updated to remove this assumption.

.../opentelemetry/src/main/java8/datadog/trace/instrumentation/opentelemetry/TypeConverter.java

dougqh · 2020-11-11T20:56:54Z

...ation/opentelemetry/src/main/java8/datadog/trace/instrumentation/opentelemetry/OtelSpan.java

+
+  @Override
+  public Span recordException(Throwable throwable, Attributes attributes) {
+    delegate.addThrowable(throwable);


Shouldn't this attach the Attributes as well? Or are those Attributes of the Throwable?
If the later, a comment would help here.

dd-java-agent/instrumentation/opentelemetry/src/test/groovy/OpenTelemetryTest.groovy

dougqh

I have few concerns mostly around WrappedScope & performance.

dougqh · 2020-11-11T21:04:36Z

.../src/main/java/datadog/trace/instrumentation/opentelemetry/OpenTelemetryInstrumentation.java

+    public static void beforeBuild(@Advice.This OpenTelemetry.Builder builder) {
+      ContextStore<SpanContext, AgentSpan.Context> spanContextStore =
+          InstrumentationContext.get(SpanContext.class, AgentSpan.Context.class);
+      builder.setTracerProvider(new OtelTracerProvider(spanContextStore));


How deeply does OTelTracerProvider & OTelContextPropagators get checked by muzzle?

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

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.

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.

dougqh · 2020-11-12T19:12:32Z

.../opentelemetry/src/main/java8/datadog/trace/instrumentation/opentelemetry/TypeConverter.java

+  }
+
+  public AgentSpan.Context toAgentSpanContext(final SpanContext spanContext) {
+    // Currently assuming the span context was created above so it should contain an existing


Can you please describe the ramifications of calling SpanContext.create and SpanBuilder.setParent?
In general, I don't want to assume that significant changes to the core aren't necessary without fully understanding the problem first.

dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java

internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java

dougqh · 2020-11-13T13:07:37Z

The more I think about this -- the more I think this isn't quite the right approach.

I think if we're going to have a bridge for OTel. We should have a package similar to ddtrace-ot.
Such a ddtrace-otel should have minimal dependencies and shouldn't require any instrumentation.

I think the instrumentation based solution is good for those using the full agent, but using the full agent should be optional.
Since I think we want both, I think large parts of this PR are fine, but I think we need to rethink the adapters.

In my opinion, the adapters should not and cannot rely on instrumentation.
I think that's for the better anyway. As is, the instrumentation tries to keep two data structures in sync, I think that approach is fraught with peril and best avoided.

I think that means we'll have to push OpenTel's more generic Context concept into our core.
While it would be nice if this weren't necessary, I think it is largely inevitable that our core will need to support a union of the capabilities of the APIs that we support externally.

Added an optional context field onto our Scope seems fairly straight-forwarded. Although, I'd prefer that we don't actually have our Scope depend on OTel directly. The tricky part appears to be deciding how Context is propagated.

I think for the bridge alone we can wait on deciding, since OTel users are expected to propagate Context explicitly. However when integrated with our full agent, we'll need to think about how we lazily materialize a Context when the Scope is created by our instrumentation or through our OpenTracing bridge.

tylerbenson · 2020-11-19T17:59:26Z

I'm going to put this back in draft until we make progress on the context issue.

Remove some of the interfaces from the wrapped objects to stop encouraging casting to our interfaces as a way to integrate. Add instrumentation to sync our context with the new otel context. Due to implementation differences, there are still some edge cases where they can get out of sync.

tylerbenson · 2021-05-25T14:36:50Z

Going to close this until we get around to the prerequisite scope manager changes.

dajulia3 · 2021-05-31T07:28:41Z

I'm considering using datadog vs splunk as an opentelemetry endpoint. I'm curious to know-- is this actively being worked on? I'd love to be able to use my existing opentelemetry instrumentation with DataDog (without needing to run a separate opentelemetry collector).

tylerbenson · 2021-06-01T14:31:37Z

@dajulia3 Would you mind filing a support ticket (support@datadoghq.com) so we can get more details on your use case and figure out a good path forward?

Thanks!

dajulia3 · 2021-06-03T04:07:35Z

@tylerbenson sure thing!

reify-marcio-faria · 2022-06-08T22:58:49Z

Hey @tylerbenson, as the Opentracing library is deprecated, what are the plans to support, so that we can use custom instrumentation with all other goodness of datadog continuous profiling and the like?

There is also this related issue

bantonsson · 2022-06-09T07:59:49Z

@reify-marcio-faria Adding support for OpenTelemetry is definitely on our roadmap, but I can't give you any solid dates. In the meantime it is still possible to use the OpenTracing API for custom instrumentation, just because it's deprecated doesn't mean that the artifacts will disappear from maven central, and if you switch to OpenTelemetry later, there is a compatibility layer available.

tylerbenson force-pushed the tyler/otel-update branch 2 times, most recently from 4b3b774 to fa5d213 Compare November 6, 2020 16:46

tylerbenson marked this pull request as ready for review November 6, 2020 18:10

tylerbenson requested a review from a team as a code owner November 6, 2020 18:10

tylerbenson changed the title ~~Update OpenTelemetry Bridge to latest version~~ Update OpenTelemetry Bridge to support OTel Java API 0.10.0 Nov 6, 2020

tylerbenson commented Nov 6, 2020

View reviewed changes

...datadog/trace/instrumentation/opentelemetry/context/OpenTelemetryContextInstrumentation.java Show resolved Hide resolved

bantonsson reviewed Nov 11, 2020

View reviewed changes

randomanderson approved these changes Nov 11, 2020

View reviewed changes

.../opentelemetry/src/main/java8/datadog/trace/instrumentation/opentelemetry/TypeConverter.java Outdated Show resolved Hide resolved

dd-java-agent/instrumentation/opentelemetry/src/test/groovy/OpenTelemetryTest.groovy Outdated Show resolved Hide resolved