RUM-3249 Provide the bundleWithRum capability for OtelTracer by mariusc83 · Pull Request #1960 · DataDog/dd-sdk-android

mariusc83 · 2024-04-02T12:20:11Z

What does this PR do?

A brief description of the change being made with this pull request.

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-02T12:44:15Z

Codecov Report

Merging #1960 (141ad20) into feature/otel-support (0e43b0d) will increase coverage by 0.15%.
The diff coverage is 95.65%.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/otel-support    #1960      +/-   ##
========================================================
+ Coverage                 60.03%   60.18%   +0.15%     
========================================================
  Files                       804      804              
  Lines                     30121    30152      +31     
  Branches                   4930     4937       +7     
========================================================
+ Hits                      18081    18145      +64     
+ Misses                    10840    10799      -41     
- Partials                   1200     1208       +8

Files	Coverage Δ
...va/com/datadog/opentelemetry/trace/OtelTracer.java	`100.00% <100.00%> (ø)`
...datadog/opentelemetry/trace/OtelTracerBuilder.java	`87.50% <100.00%> (+1.79%)`	⬆️
.../kotlin/com/datadog/android/trace/AndroidTracer.kt	`96.23% <ø> (ø)`
...in/com/datadog/android/trace/OtelTracerProvider.kt	`95.61% <94.44%> (+1.50%)`	⬆️

... and 26 files with indirect coverage changes

0xnm · 2024-04-03T08:03:54Z

      # region Opentelemetry
      - "io.opentelemetry.api.trace.TracerBuilder.build()"
      - "io.opentelemetry.api.trace.TracerBuilder.setInstrumentationVersion(kotlin.String?)"
+      - "io.opentelemetry.api.trace.SpanBuilder.setAttribute(kotlin.String?, kotlin.String?)"


minor: just ordering

Suggested change

# region Opentelemetry

- "io.opentelemetry.api.trace.TracerBuilder.build()"

- "io.opentelemetry.api.trace.TracerBuilder.setInstrumentationVersion(kotlin.String?)"

- "io.opentelemetry.api.trace.SpanBuilder.setAttribute(kotlin.String?, kotlin.String?)"

# region OpenTelemetry

- "io.opentelemetry.api.trace.SpanBuilder.setAttribute(kotlin.String?, kotlin.String?)"

- "io.opentelemetry.api.trace.TracerBuilder.build()"

- "io.opentelemetry.api.trace.TracerBuilder.setInstrumentationVersion(kotlin.String?)"

0xnm · 2024-04-03T08:08:39Z

+            val rumFeature = sdkCore.getFeature(Feature.RUM_FEATURE_NAME)
+            if (bundleWithRumEnabled && rumFeature == null) {
+                sdkCore.internalLogger.log(
+                    InternalLogger.Level.WARN,
+                    InternalLogger.Target.USER,
+                    { RUM_NOT_ENABLED_ERROR_MESSAGE }
+                )
+                bundleWithRumEnabled = false
+            }


IMO we need to revisit this logic (I know it is already exists for AndroidTracer): in theory with v2 we can have things enabled independently, so nothing in the setup prevents enabling RUM after Tracer is enabled. IMO this shouldn't prevent us to do bundling with RUM. Is such logic exist on iOS as well?

Hmmm...good point. I just wanted to follow what was already in the AndroidTracer but it makes sense maybe to remove it. Waiting for @xgouchet opinion here also.

I agree we need to evaluat the bundling at span creation as the RUM feature can be enabled/disabled independently

yes agree, we are already checking and warning the user at the spanbuilder decoration level so there is no need for other log for now. I will just remove this from here and add a ticket to remove it also for the AndroidTracer.

0xnm · 2024-04-03T08:09:05Z

+        /**
+         * Enables the trace bundling with the current active View. If this feature is enabled all
+         * the spans from this moment on will be bundled with the current view information and you
+         * will be able to see all the traces sent during a specific view in the Rum Explorer.


Suggested change

* will be able to see all the traces sent during a specific view in the Rum Explorer.

* will be able to see all the traces sent during a specific view in the RUM Explorer.

0xnm · 2024-04-03T08:10:54Z

+        } else {
+            internalLogger.log(
+                InternalLogger.Level.WARN,
+                listOf(InternalLogger.Target.USER, InternalLogger.Target.TELEMETRY),


is it useful in the telemetry?

Good question. I was thinking that maybe it is useful for us to see how many times we are missing the link with RUM because RUM context is corrupted or session is not there. But I am opened to discussions here.

My concern is just to avoid spamming telemetry / avoid having many telemetry events to process in the SDK itself, given that it is tracing, so we may have a lot of spans.

I'll remove it in that case and we will see if we need it in case someone reports missing links.

0xnm · 2024-04-03T08:11:53Z

+    // region bundle with RUM
+
+    @Test
+    fun `M build a Span with RUM context W buildSpan`() {


same applies to the tests below

Suggested change

fun `M build a Span with RUM context W buildSpan`() {

fun `M build a Span with RUM context W startSpan`() {

0xnm

Is change to remove RUM feature check in AndroidTracer will be in another PR?

0xnm · 2024-04-05T07:54:08Z

                " OtelTracerProvider creation, did you initialize SDK?"
+        internal const val RUM_NOT_ENABLED_ERROR_MESSAGE =
+            "You're trying to bundle the traces with a RUM context, " +
+                "but the RUM feature was disabled in your Configuration. " +


we need to update this message (and similar message in AndroidTracer) - RUM is not enabled in core Configuration class anymore in v2.

0xnm · 2024-04-08T09:44:07Z

+        }
+    }
+
+    private fun resolveSpanBuilderDecoratorFromContext(): (SpanBuilder) -> SpanBuilder = { spanBuilder ->


minor: I think the name can be improved here (can be done in another PR) - nothing in the name points that it will be RUM content applied. Maybe something like rumAwareSpanBuilderDecorator for this method will be better (also this can be just a property, not a method).

yes but that is the idea, this time is for RUM but it should be feature agnostic that's the reason why it is not having any RUM mentioned.

then still the name doesn't resolve anything from context, it decorates with context.

mariusc83 self-assigned this Apr 2, 2024

mariusc83 force-pushed the mconstantin/rum-3249/provide-bundle-with-rum-capability branch from cb0c3cb to 4a3e8dd Compare April 2, 2024 12:43

mariusc83 force-pushed the mconstantin/rum-3249/provide-bundle-with-rum-capability branch 2 times, most recently from ccaaf3e to 4e1dcee Compare April 2, 2024 12:45

mariusc83 force-pushed the mconstantin/rum-3405/provide-core-tracer-logger-implementation branch 3 times, most recently from 92bd5f3 to ef74896 Compare April 2, 2024 14:42

mariusc83 force-pushed the mconstantin/rum-3249/provide-bundle-with-rum-capability branch from 4e1dcee to 3b4e692 Compare April 2, 2024 15:05

mariusc83 marked this pull request as ready for review April 3, 2024 07:54

mariusc83 requested review from a team as code owners April 3, 2024 07:54

0xnm reviewed Apr 3, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3405/provide-core-tracer-logger-implementation branch from ef74896 to 41370c3 Compare April 3, 2024 08:34

Base automatically changed from mconstantin/rum-3405/provide-core-tracer-logger-implementation to feature/otel-support April 3, 2024 09:30

mariusc83 force-pushed the mconstantin/rum-3249/provide-bundle-with-rum-capability branch 2 times, most recently from 5fa2926 to fead880 Compare April 3, 2024 13:32

mariusc83 requested review from 0xnm and xgouchet April 3, 2024 14:13

xgouchet previously approved these changes Apr 5, 2024

View reviewed changes

0xnm previously approved these changes Apr 5, 2024

View reviewed changes

RUM-3249 Provide the bundleWithRum capability for OtelTracer

141ad20

mariusc83 dismissed stale reviews from 0xnm and xgouchet via 141ad20 April 8, 2024 08:38

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

mariusc83 requested a review from 0xnm April 8, 2024 09:29

0xnm approved these changes Apr 8, 2024

View reviewed changes

mariusc83 merged commit 3e814ca into feature/otel-support Apr 8, 2024

mariusc83 deleted the mconstantin/rum-3249/provide-bundle-with-rum-capability branch April 8, 2024 09:56

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

	* will be able to see all the traces sent during a specific view in the Rum Explorer.
	* will be able to see all the traces sent during a specific view in the RUM Explorer.

	fun `M build a Span with RUM context W buildSpan`() {
	fun `M build a Span with RUM context W startSpan`() {

Uh oh!

Conversation

mariusc83 commented Apr 2, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Apr 2, 2024 •

edited

Loading