RUM-4055 Report Opentelemetry data in the configuration Telemetry #2006

mariusc83 · 2024-04-23T14:10:48Z

What does this PR do?

Following the iOS Pr we are going to report the usage of the OpenTelemetry API in our configuration telemetry as follows:
hasTrace:true | false, tracerApi: OpenTracing | OpenTelemetry | null, traceApiVersion: [openTelemetryApiVersion] | null

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-23T15:17:03Z

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 63.92%. Comparing base (478ba84) to head (a0e5b5f).
Report is 1 commits behind head on feature/otel-support.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/otel-support    #2006      +/-   ##
========================================================
+ Coverage                 63.88%   63.92%   +0.05%     
========================================================
  Files                       752      752              
  Lines                     28368    28382      +14     
  Branches                   4680     4686       +6     
========================================================
+ Hits                      18121    18143      +22     
+ Misses                     9043     9036       -7     
+ Partials                   1204     1203       -1

Files	Coverage Δ
...in/com/datadog/android/trace/OtelTracerProvider.kt	`95.97% <100.00%> (+0.10%)`	⬆️
...m/datadog/android/trace/internal/TracingFeature.kt	`94.29% <100.00%> (+0.17%)`	⬆️
...ndroid/telemetry/internal/TelemetryEventHandler.kt	`81.32% <88.24%> (+6.62%)`	⬆️

... and 34 files with indirect coverage changes

xgouchet

Important

It feels like we need to review the way we send the configuration telemetry information for other features than RUM.
Right now we detect the OT by reflection, and this PR tries to add the OpenTelemetry version in the RUM build config. This doesn't make any sense and adds noise to a feature that should not have those kind of information.
Maybe we should consider a different way for features to communicate this information and RUM would be agnostic of the keys and values.

mariusc83 · 2024-04-24T11:17:16Z

Important

It feels like we need to review the way we send the configuration telemetry information for other features than RUM. Right now we detect the OT by reflection, and this PR tries to add the OpenTelemetry version in the RUM build config. This doesn't make any sense and adds noise to a feature that should not have those kind of information. Maybe we should consider a different way for features to communicate this information and RUM would be agnostic of the keys and values.

@xgouchet I understand your concern, we could send it in the Trace feature context as we do for the is_opentelemetry_enabled property. But we still need to keep the build config entry as the other option is to send it as a hardcoded value that we will definitely forget to update. What do you think ?

P.S. I think opening a RFC only for this topic only would be too much IMO. @plousada what do you think ?

xgouchet · 2024-04-24T11:46:14Z

@xgouchet I understand your concern, we could send it in the Trace feature context as we do for the is_opentelemetry_enabled property. But we still need to keep the build config entry as the other option is to send it as a hardcoded value that we will definitely forget to update. What do you think ?

P.S. I think opening a RFC only for this topic only would be too much IMO. @plousada what do you think ?

I don't have an issue having the info set by build config, it just would make more sense to do it on the otel/tracing module, instead of the RUM one

mariusc83 · 2024-04-24T11:57:13Z

@xgouchet I understand your concern, we could send it in the Trace feature context as we do for the is_opentelemetry_enabled property. But we still need to keep the build config entry as the other option is to send it as a hardcoded value that we will definitely forget to update. What do you think ?
P.S. I think opening a RFC only for this topic only would be too much IMO. @plousada what do you think ?

I don't have an issue having the info set by build config, it just would make more sense to do it on the otel/tracing module, instead of the RUM one

Yes this is what I am changing now, makes more sense to have it there. I am pushing a commit in few seconds.

0xnm · 2024-04-25T09:41:50Z

...roid-rum/src/test/kotlin/com/datadog/android/telemetry/internal/TelemetryEventHandlerTest.kt

+    @ParameterizedTest
+    @MethodSource("tracingConfigurationParameters")
+    fun `𝕄 create config event 𝕎 handleEvent(SendTelemetry) { tracing configuration with tracing settings }`(
+        configuration: Triple<Boolean, TelemetryEventHandler.TracerApi?, String?>,
+        @Forgery fakeConfiguration: TelemetryCoreConfiguration
    ) {


instead of using Triple without clear names it is possible to be more explicit:

Suggested change

@ParameterizedTest

@MethodSource("tracingConfigurationParameters")

fun `𝕄 create config event 𝕎 handleEvent(SendTelemetry) { tracing configuration with tracing settings }`(

configuration: Triple<Boolean, TelemetryEventHandler.TracerApi?, String?>,

@Forgery fakeConfiguration: TelemetryCoreConfiguration

) {

@ParameterizedTest

@MethodSource("tracingConfigurationParameters")

fun `𝕄 create config event 𝕎 handleEvent(SendTelemetry) { tracing configuration with tracing settings }`(

useTracer: Boolean,

tracerApi: TelemetryEventHandler.TracerApi?,

tracerApiVersion: String?,

@Forgery fakeConfiguration: TelemetryCoreConfiguration

) {

and then in the method provider instead of Triple you can use Arguments.of

0xnm · 2024-04-25T09:48:33Z

features/dd-sdk-android-trace/build.gradle.kts

+            "OPENTELEMETRY_API_VERSION_NAME",
+            "\"${libs.versions.openTelemetry.get()}\""


maybe there is something from OpenTelemetry in the META-INF folder of the resulting package? this would help to avoid BuildConfig usage.

Also the value here may be wrong, because it is written for the version which was used to compile our library, but once final app is compiled, there is a chance that there is a newer OpenTelemetry version in the classpath, so it means that our telemetry will report the wrong version.

Additionally, in theory since this property is written at the our library compile time, we can know the version used just by looking on the SDK version. This means that we can set up logs processing pipeline, which will transform SDK version to the OpenTelemetry version used (but that would require to maintain this rule).

In that case it will mean problems on the customer side because it is our package that should dictate the OpenTelemetry API version. Otherwise it might mean errors at Runtime/Compile due to API implementation incompatibility. For me it is more reliable to read this as a static BuildConfig value which reflects the exact API version we implement with each package release.

Not necessarily. Customer may just have some 3rd party dependency which references newer OpenTelemetry API artifact (or maybe they have their own OpenTelemetry tooling as well). In this case overall dependency graph will be resolved to the newer version, it will be used in the application, but our telemetry will still report the older version (because it will be the one which was used to compiler our SDK, not the one which will be used to compile the application).

I am aware of that and in that case it's on customer's end. What matters for us is to report the OpenTelemetry API that we support in the tracer and not the one the customer gets in his code through the dependencies resolver. We will specifically mark it in our docs which version of OpenTelemetry API we support with each version of our artifact release.

0xnm · 2024-04-25T09:49:19Z

...sdk-android-trace/src/test/kotlin/com/datadog/android/trace/OtelTracerBuilderProviderTest.kt

+    // region Tracing context
+
+    @Test
+    fun `M set opentelemetry as enabled in Context W build { TracingFeature enabled }`() {


Suggested change

fun `M set opentelemetry as enabled in Context W build { TracingFeature enabled }`() {

fun `M set OpenTelemetry as enabled in Context W build { TracingFeature enabled }`() {

0xnm · 2024-04-25T09:49:30Z

...sdk-android-trace/src/test/kotlin/com/datadog/android/trace/OtelTracerBuilderProviderTest.kt

+    }
+
+    @Test
+    fun `M set opentelemetry as enabled in Context W build { TracingFeature not enabled }`() {


Suggested change

fun `M set opentelemetry as enabled in Context W build { TracingFeature not enabled }`() {

fun `M set OpenTelemetry as enabled in Context W build { TracingFeature not enabled }`() {

0xnm · 2024-04-25T09:52:02Z

...sdk-android-trace/src/test/kotlin/com/datadog/android/trace/OtelTracerBuilderProviderTest.kt

+        argumentCaptor<(MutableMap<String, Any?>) -> Unit> {
+            val traceContext: MutableMap<String, Any?> = mutableMapOf()
+            verify(mockSdkCore, times(1)).updateFeatureContext(eq(Feature.TRACING_FEATURE_NAME), capture())
+            lastValue.invoke(traceContext)
+            assertThat(traceContext[TracingFeature.IS_OPENTELEMETRY_ENABLED_CONFIG_KEY]).isEqualTo(true)
+            assertThat(traceContext[TracingFeature.OPENTELEMETRY_API_VERSION_CONFIG_KEY])
+                .isEqualTo(BuildConfig.OPENTELEMETRY_API_VERSION_NAME)
+        }


the behavior shouldn't be like that, no context should be updated if feature is disabled, check the implementation

dd-sdk-android/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/DatadogCore.kt

Line 193 in cff8b54

override fun updateFeatureContext(

Yes I am aware of that but in that case all the other places where we actually call that method should be under if(featureExists) flag to be consistent. The only reason I am calling this method even if the feature is not registered is to be consistent with:

dd-sdk-android/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/AndroidTracer.kt

Line 57 in cff8b54

sdkCore.updateFeatureContext(Feature.TRACING_FEATURE_NAME) {

and

dd-sdk-android/features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/AndroidTracer.kt

Line 68 in cff8b54

sdkCore.updateFeatureContext(Feature.TRACING_FEATURE_NAME) {

and same
thing for OtelTracerProvider.
The real question here is : in case the Trace feature is not registered should we return a workable AndroidTracer or OtelTracerProvider in the build method or just a NoOp ? Let's discuss this in the daily.

I don't mean the need to check if feature registered or not before calling updateFeatureContext, I mean that this test name is wrong. The name is M set opentelemetry as enabled in Context W build { TracingFeature not enabled }, but no, if tracing feature is not registered, then nothing should be set, because the real implementation of updateFeatureContext won't do anything. We may assert only up to the point checking that updateFeatureContext was called, but not that callback to update values was executed (and test name should be updated).

But probably, actually, it makes no sense to even call updateFeatureContext if tracingFeature = null. By adding this check (and it makes sense, because tracingFeature is already in the method anyway) this test can be updated to the proper behavior.

IMO it makes no sense to call it at all but in the same time I still argue that we should probably talk about returning NoOps in case trace feature is not initialized.

mariusc83 self-assigned this Apr 23, 2024

mariusc83 force-pushed the mconstantin/rum-4055/add-otel-configuration-telemetry branch from 9a2feed to e0c7b05 Compare April 23, 2024 14:44

RUM-4055 Report Opentelemetry data in the configuration Telemetry

f50ccd4

mariusc83 force-pushed the mconstantin/rum-4055/add-otel-configuration-telemetry branch from e0c7b05 to f50ccd4 Compare April 24, 2024 08:58

mariusc83 marked this pull request as ready for review April 24, 2024 08:58

mariusc83 requested review from a team as code owners April 24, 2024 08:58

xgouchet requested changes Apr 24, 2024

View reviewed changes

mariusc83 requested a review from xgouchet April 24, 2024 12:00

0xnm reviewed Apr 25, 2024

View reviewed changes

Send the Otel API version through Trace feature context

a0e5b5f

mariusc83 force-pushed the mconstantin/rum-4055/add-otel-configuration-telemetry branch from 3ebdc53 to a0e5b5f Compare April 25, 2024 14:07

mariusc83 requested a review from 0xnm April 26, 2024 09:13

0xnm approved these changes Apr 26, 2024

View reviewed changes

xgouchet approved these changes Apr 26, 2024

View reviewed changes

mariusc83 merged commit ee9af33 into feature/otel-support Apr 29, 2024
25 checks passed

mariusc83 deleted the mconstantin/rum-4055/add-otel-configuration-telemetry branch April 29, 2024 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-4055 Report Opentelemetry data in the configuration Telemetry #2006

RUM-4055 Report Opentelemetry data in the configuration Telemetry #2006

mariusc83 commented Apr 23, 2024 •

edited

Loading

codecov-commenter commented Apr 23, 2024 •

edited

Loading

xgouchet left a comment

mariusc83 commented Apr 24, 2024

xgouchet commented Apr 24, 2024

mariusc83 commented Apr 24, 2024

0xnm Apr 25, 2024

0xnm Apr 25, 2024

mariusc83 Apr 25, 2024

0xnm Apr 25, 2024

mariusc83 Apr 25, 2024

0xnm Apr 25, 2024

0xnm Apr 25, 2024

0xnm Apr 25, 2024

mariusc83 Apr 25, 2024

0xnm Apr 25, 2024 •

edited

Loading

mariusc83 Apr 25, 2024

		"OPENTELEMETRY_API_VERSION_NAME",
		"\"${libs.versions.openTelemetry.get()}\""

	fun `M set opentelemetry as enabled in Context W build { TracingFeature enabled }`() {
	fun `M set OpenTelemetry as enabled in Context W build { TracingFeature enabled }`() {

RUM-4055 Report Opentelemetry data in the configuration Telemetry #2006

RUM-4055 Report Opentelemetry data in the configuration Telemetry #2006

Conversation

mariusc83 commented Apr 23, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov-commenter commented Apr 23, 2024 • edited Loading

Codecov Report

xgouchet left a comment

Choose a reason for hiding this comment

mariusc83 commented Apr 24, 2024

xgouchet commented Apr 24, 2024

mariusc83 commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnm Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusc83 commented Apr 23, 2024 •

edited

Loading

codecov-commenter commented Apr 23, 2024 •

edited

Loading

0xnm Apr 25, 2024 •

edited

Loading