RUM-3836 Provide the DatadogContextStorage for Opentelemetry by mariusc83 · Pull Request #1970 · DataDog/dd-sdk-android

mariusc83 · 2024-04-05T15:45:54Z

What does this PR do?

In this PR we are providing our own implementation of https://github.com/open-telemetry/opentelemetryjava/blob/main/context/src/main/java/io/opentelemetry/context/ContextStorage.java
interface which wraps inside the default one in order to be able to provide our own implementation of Context -> OtelContext.
The main reason for this update was to be able to tight the Opentelemetry Scopes to the CoreTracer ones.

See more here how you can create a Scope in Opentelemetry:
https://github.com/open-telemetry/opentelemetry-java/blob/main/context/src/main/java/io/opentelemetry/context/ContextStorage.java

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

codecov-commenter · 2024-04-08T08:39:20Z

Codecov Report

Merging #1970 (70acea8) into feature/otel-support (3e814ca) will increase coverage by 0.10%.
The diff coverage is 50.00%.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/otel-support    #1970      +/-   ##
========================================================
+ Coverage                 60.16%   60.26%   +0.10%     
========================================================
  Files                       804      807       +3     
  Lines                     30141    30184      +43     
  Branches                   4933     4945      +12     
========================================================
+ Hits                      18133    18188      +55     
+ Misses                    10801    10776      -25     
- Partials                   1207     1220      +13

Files	Coverage Δ
...java/com/datadog/opentelemetry/trace/OtelSpan.java	`59.38% <100.00%> (+43.75%)`	⬆️
...va/com/datadog/opentelemetry/trace/OtelTracer.java	`100.00% <100.00%> (ø)`
...in/com/datadog/android/trace/OtelTracerProvider.kt	`95.61% <100.00%> (-0.58%)`	⬇️
...m/datadog/android/trace/internal/TracingFeature.kt	`94.12% <100.00%> (+0.18%)`	⬆️
...m/datadog/android/trace/opentelemetry/OtelScope.kt	`100.00% <100.00%> (ø)`
...droid/trace/opentelemetry/DatadogContextStorage.kt	`66.67% <66.67%> (ø)`
...datadog/android/trace/opentelemetry/OtelContext.kt	`90.00% <90.00%> (ø)`
...m/datadog/opentelemetry/trace/OtelSpanBuilder.java	`24.59% <24.59%> (+1.26%)`	⬆️

... and 30 files with indirect coverage changes

mariusc83 · 2024-04-08T12:27:54Z

 import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;

 public class OtelSpanBuilder implements SpanBuilder {


these diffs are mostly from new format applied because this was copied from the Java agent project.

0xnm

overall lgtm, I left some comments/suggestions

0xnm · 2024-04-09T07:32:15Z

+      - "io.opentelemetry.api.trace.Span.getInvalid()"
      - "io.opentelemetry.api.trace.TracerBuilder.build()"
      - "io.opentelemetry.api.trace.SpanBuilder.setAttribute(kotlin.String?, kotlin.String?)"
      - "io.opentelemetry.api.trace.TracerBuilder.setInstrumentationVersion(kotlin.String?)"


very minor: need to fix ordering here

0xnm · 2024-04-09T07:43:23Z

+        private val emailKey: ContextKey<String> = ContextKey.named("email")
+        private val usernameKey: ContextKey<String> = ContextKey.named("username")
+        private val context: Context =
+            Context.current().with(emailKey, email).with(usernameKey, username)
+        val startSpan: Span = tracer
+            .spanBuilder("submitForm")
+            .setParent(context)
+            .startSpan()
+        val scope: Scope = startSpan.makeCurrent()
+
+        @Suppress("CheckInternal")
+        private val logger: Logger by lazy {


minor: why do we explicitly specify types everywhere here? I believe in majority of the lines they can be removed?

actually I need to do that because the Java code under the hood uses generics and it complains if it hasn't a type to inferre from

for some lines - yes, but some lines for sure don't need an explicit type

0xnm · 2024-04-09T12:10:25Z

+        internal const val OTEL_CONTEXT_SPAN_KEY = "opentelemetry-trace-span-key"
+        internal const val OTEL_CONTEXT_ROOT_SPAN_KEY = "opentelemetry-traces-local-root-span"


btw, are these some official keys? Or just made up by us? they have some difference: trace vs traces and also there is a key word in the former, but not in the latter.

official ones, that code in OtelContext is pretty much the one in Java Agent implementation. But it is a good observation and will double check with them also.

mariusc83 self-assigned this Apr 5, 2024

mariusc83 force-pushed the mconstantin/rum-3836/provide-otel-contextstorage-custom-implementation branch from 3083360 to b51edc9 Compare April 8, 2024 08:34

mariusc83 force-pushed the mconstantin/rum-3249/provide-bundle-with-rum-capability branch from fead880 to 141ad20 Compare April 8, 2024 08:38

Base automatically changed from mconstantin/rum-3249/provide-bundle-with-rum-capability to feature/otel-support April 8, 2024 09:56

RUM-3836 Provide the DatadogContextStorage for Opentelemetry

2240ae8

mariusc83 force-pushed the mconstantin/rum-3836/provide-otel-contextstorage-custom-implementation branch 3 times, most recently from ca00753 to a2fcd1b Compare April 8, 2024 12:21

mariusc83 commented Apr 8, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3836/provide-otel-contextstorage-custom-implementation branch from a2fcd1b to 2e65dda Compare April 8, 2024 12:55

mariusc83 marked this pull request as ready for review April 8, 2024 13:46

mariusc83 requested review from a team as code owners April 8, 2024 13:46

mariusc83 force-pushed the mconstantin/rum-3836/provide-otel-contextstorage-custom-implementation branch 2 times, most recently from 8f4c236 to b8617c3 Compare April 9, 2024 08:06

0xnm reviewed Apr 9, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3836/provide-otel-contextstorage-custom-implementation branch from b8617c3 to ca283f6 Compare April 9, 2024 12:02

mariusc83 requested a review from 0xnm April 9, 2024 12:04

0xnm previously approved these changes Apr 9, 2024

View reviewed changes

Add the bundled Otel contexts use case in Sample app

70acea8

mariusc83 dismissed 0xnm’s stale review via 70acea8 April 9, 2024 12:39

mariusc83 force-pushed the mconstantin/rum-3836/provide-otel-contextstorage-custom-implementation branch from ca283f6 to 70acea8 Compare April 9, 2024 12:39

mariusc83 requested a review from 0xnm April 9, 2024 12:49

0xnm approved these changes Apr 9, 2024

View reviewed changes

mariusc83 merged commit 4633820 into feature/otel-support Apr 9, 2024

mariusc83 deleted the mconstantin/rum-3836/provide-otel-contextstorage-custom-implementation branch April 9, 2024 13:04

xgouchet added this to the 2.11.x milestone Jul 31, 2024

		internal const val OTEL_CONTEXT_SPAN_KEY = "opentelemetry-trace-span-key"
		internal const val OTEL_CONTEXT_ROOT_SPAN_KEY = "opentelemetry-traces-local-root-span"

Uh oh!

Conversation

mariusc83 commented Apr 5, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mariusc83 Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

0xnm Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xnm Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

mariusc83 Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

0xnm Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xnm Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

mariusc83 Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Apr 8, 2024 •

edited

Loading