RUM-562: Report configured distributed tracing headers as part of Configuration telemetry by aleksandr-gringauz · Pull Request #2572 · DataDog/dd-sdk-android

aleksandr-gringauz · 2025-03-28T16:42:12Z

What does this PR do?

We provide distributed tracing header types to the selectedTracingPropagators field of the TelemetryConfigurationEvent in a similar way we did with the traceSampleRate here #2563. The disadvantages described in that pr also apply here.

demo [internal]:
link logs from the sample app. Select at least 1 week window so that you can see events from Friday March 28th.

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)

0xnm · 2025-03-31T10:07:00Z

        testedCore.features += Feature.TRACING_FEATURE_NAME to mockTracingFeature

        val traceSampleRate = forge.aFloat(min = 0f, max = 100f)
+        val hostsWithHeaderTypes: HostsWithHeaderTypes? = forge.aNullable<HostsWithHeaderTypes>()


Suggested change

val hostsWithHeaderTypes: HostsWithHeaderTypes? = forge.aNullable<HostsWithHeaderTypes>()

val hostsWithHeaderTypes = forge.aNullable<HostsWithHeaderTypes>()

0xnm · 2025-03-31T10:12:29Z

+    override fun getForgery(forge: Forge): HostsWithHeaderTypes {
+        return HostsWithHeaderTypes(
+            hosts = forge.aMap {
+                aString() to aList {


minor: maybe generate something like a host look-like? by using say aStringMatching("https://[a-z0-9]+\\.com")

0xnm · 2025-03-31T10:15:39Z

+        return headerTypes.hosts
+            .values


so why do we collect (or in other words, have it in the model) hosts information if in the end we only send header types to the configuration telemetry?

I felt that if we are making some information (header types) available to the outside it is better to expose "the original" information, not the result of some function which removes most of it.

Suppose we need something else connected to these hosts and headers (maybe a set of hosts?), now we can just use the thing I did here.

However I actually don't have a strong opinion here, one can argue for example that it is better to expose as little as possible to the outside.

I have no objections to refactor the code to expose only a set of header types.

yes, let's change this PR to have only tracing header types passed around, it will greatly simply the things. We can add hosts information later if needed.

0xnm · 2025-03-31T10:18:12Z

+import com.datadog.android.telemetry.model.TelemetryConfigurationEvent
+
+internal object HostsWithHeaderTypesMapper {
+    fun mapToSelectedTracingPropagators(


suggestion: maybe have it as an extension method is simpler?

0xnm · 2025-03-31T10:19:35Z

+        val host1 = forge.aString()
+        val host2 = forge.aString()
+        val host3 = forge.aString()
+
+        val headerType1 = forge.aValueFrom(HostsWithHeaderTypes.HeaderType::class.java)
+        val headerType2 = forge.aValueFrom(HostsWithHeaderTypes.HeaderType::class.java, exclude = listOf(headerType1))
+
+        val mappedHeaderType1 = headerType1.toSelectedTracingPropagator()
+        val mappedHeaderType2 = headerType2.toSelectedTracingPropagator()
+
+        val hostsWithHeaderTypes = HostsWithHeaderTypes(
+            hosts = mapOf(
+                host1 to setOf(headerType1),
+                host2 to setOf(headerType2, headerType1),
+                host3 to emptySet()
+            )
+        )
+
+        val result = HostsWithHeaderTypesMapper.mapToSelectedTracingPropagators(hostsWithHeaderTypes)
+
+        assertThat(result)
+            .containsExactlyInAnyOrder(mappedHeaderType1, mappedHeaderType2)
+            .doesNotHaveDuplicates()
+    }


Suggested change

val host1 = forge.aString()

val host2 = forge.aString()

val host3 = forge.aString()

val headerType1 = forge.aValueFrom(HostsWithHeaderTypes.HeaderType::class.java)

val headerType2 = forge.aValueFrom(HostsWithHeaderTypes.HeaderType::class.java, exclude = listOf(headerType1))

val mappedHeaderType1 = headerType1.toSelectedTracingPropagator()

val mappedHeaderType2 = headerType2.toSelectedTracingPropagator()

val hostsWithHeaderTypes = HostsWithHeaderTypes(

hosts = mapOf(

host1 to setOf(headerType1),

host2 to setOf(headerType2, headerType1),

host3 to emptySet()

)

)

val result = HostsWithHeaderTypesMapper.mapToSelectedTracingPropagators(hostsWithHeaderTypes)

assertThat(result)

.containsExactlyInAnyOrder(mappedHeaderType1, mappedHeaderType2)

.doesNotHaveDuplicates()

}

// Given

val host1 = forge.aString()

val host2 = forge.aString()

val host3 = forge.aString()

val headerType1 = forge.aValueFrom(HostsWithHeaderTypes.HeaderType::class.java)

val headerType2 = forge.aValueFrom(HostsWithHeaderTypes.HeaderType::class.java, exclude = listOf(headerType1))

val mappedHeaderType1 = headerType1.toSelectedTracingPropagator()

val mappedHeaderType2 = headerType2.toSelectedTracingPropagator()

val hostsWithHeaderTypes = HostsWithHeaderTypes(

hosts = mapOf(

host1 to setOf(headerType1),

host2 to setOf(headerType2, headerType1),

host3 to emptySet()

)

)

// When

val result = HostsWithHeaderTypesMapper.mapToSelectedTracingPropagators(hostsWithHeaderTypes)

// Then

assertThat(result)

.containsExactlyInAnyOrder(mappedHeaderType1, mappedHeaderType2)

.doesNotHaveDuplicates()

}

0xnm · 2025-03-31T10:21:04Z

+            .hasSelectedTracingPropagators(
+                internalConfigurationEvent.hostsWithHeaderTypes
+                    ?.let(HostsWithHeaderTypesMapper::mapToSelectedTracingPropagators)
+            )


if we drop hosts from HostsWithHeaderTypes, then we don't need mapToSelectedTracingPropagators call and can probably have simply .hasSelectedTracingPropagators(internalConfigurationEvent.hostsWithHeaderTypes) call

No, I don't think so.

selectedTracingPropagators field of TelemetryConfigurationEvent has the type List<SelectedTracingPropagator>.

SelectedTracingPropagator is located in dd-sdk-android-rum and is not available in dd-sdk-android-core where InternalTelemetryEvent.Configuration is constructed.

We still have to pass a set of enums in internalConfigurationEvent.hostsWithHeaderTypes and then map it to List<SelectedTracingPropagator> in the TelemetryEventHandler.

And the same mapping has to be done in tests.

We could make internalConfigurationEvent.hostsWithHeaderTypes a Set<String> and then construct SelectedTracingPropagator from String. But I think it is a bad approach.

codecov-commenter · 2025-03-31T10:27:51Z

Codecov Report

Attention: Patch coverage is 59.25926% with 11 lines in your changes missing coverage. Please review.

Project coverage is 70.20%. Comparing base (bfceeac) to head (067ef5f).
Report is 220 commits behind head on develop.

❗ Current head 067ef5f differs from pull request most recent head 68b4789

Please upload reports for the commit 68b4789 to get more accurate results.

Files with missing lines	Patch %	Lines
...id/internal/telemetry/InternalTracingHeaderType.kt	0.00%	4 Missing ⚠️
...droid/internal/telemetry/InternalTelemetryEvent.kt	0.00%	2 Missing ⚠️
...nternal/telemetry/InternalTracingHeaderTypesSet.kt	0.00%	2 Missing ⚠️
...d/okhttp/internal/trace/TracingHeaderTypeMapper.kt	75.00%	1 Missing and 1 partial ⚠️
...n/com/datadog/android/core/internal/DatadogCore.kt	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2572   +/-   ##
========================================
  Coverage    70.20%   70.20%           
========================================
  Files          804      808    +4     
  Lines        30218    30224    +6     
  Branches      5052     5056    +4     
========================================
+ Hits         21213    21218    +5     
+ Misses        7602     7599    -3     
- Partials      1403     1407    +4

Files with missing lines	Coverage Δ
...d/telemetry/internal/HostsWithHeaderTypesMapper.kt	`100.00% <100.00%> (ø)`
...ndroid/telemetry/internal/TelemetryEventHandler.kt	`88.57% <ø> (-0.27%)`	⬇️
...datadog/android/okhttp/trace/TracingInterceptor.kt	`75.48% <100.00%> (+0.07%)`	⬆️
...n/com/datadog/android/core/internal/DatadogCore.kt	`81.86% <75.00%> (-0.20%)`	⬇️
...droid/internal/telemetry/InternalTelemetryEvent.kt	`28.12% <0.00%> (-0.91%)`	⬇️
...nternal/telemetry/InternalTracingHeaderTypesSet.kt	`0.00% <0.00%> (ø)`
...d/okhttp/internal/trace/TracingHeaderTypeMapper.kt	`75.00% <75.00%> (ø)`
...id/internal/telemetry/InternalTracingHeaderType.kt	`0.00% <0.00%> (ø)`

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

0xnm · 2025-03-31T14:57:53Z

+package com.datadog.android.internal.telemetry
+
+@Suppress("UndocumentedPublicClass", "UndocumentedPublicProperty")
+data class InternalTracingHeaderTypesSet(


do you want to keep this class with only one property just for the possible extension in the future? Otherwise we can drop this class and just use Set<InternalTracingHeaderType> directly

do you want to keep this class with only one property just for the possible extension in the future?

No, it's not about extending. The only purpose of this class is to be able to do an easy cast here. Otherwise I would have to first do as? Set<*> and then check every element of the set that it is of the needed enum type. Or do an unchecked cast.

0xnm · 2025-03-31T14:58:33Z

+package com.datadog.android.internal.telemetry
+
+@Suppress("UndocumentedPublicClass")
+enum class InternalTracingHeaderType {


idea: maybe drop Internal prefix from the name? Is it useful to have it?

Same for the InternalTracingHeaderTypesSet.

Also dd-sdk-android-okhttp already depends on the dd-sdk-android-rum module, so we can reference SelectedTracingPropagator from there directly, without having a dedicated intermediate enum with basically same values. Is it useful to have it?

good point, I'll try to do this

ok, I thought about it

maybe drop Internal prefix from the name? Is it useful to have it?

My idea was to distinguish it from an already existing TracingHeaderType. link which is public and can be used by clients of the sdk. Thus the prefix - internal.

Also dd-sdk-android-okhttp already depends on the dd-sdk-android-rum module, so we can reference SelectedTracingPropagator from there directly, without having a dedicated intermediate enum with basically same values. Is it useful to have it?

Now these header types are passed to InternalTelemetryEvent.Configuration here. It is in the dd-sdk-android-internal and can't access neighter TracingHeaderType nor SelectedTracingPropagator.

There are 2 choices:

Continue using InternalTelemetryEvent.Configuration, but pass the Set<SelectedTracingPropagator> as Any. Not the best because we have Any.

Extract the set of headers right in the TelemetryEventHandler similar to this. Not the best because we in TelemetryEventHandler (rum feature) we rely on the fact that there is tracing feature somewhere (features start knowing about one another).

I like approach with InternalTelemetryEvent.Configuration (the way I did it in pr, not with Any) because:

It uses static typing (at least in the TelemetryConfigurationHandler).

The code in TelemetryConfigurationHandler.createConfigurationEvent is "cleaner" - just a function that accepts InternalTelemetryEvent.Configuration as parameter and creates an event (if we don't take this into account).

I suggest 2 options:

Either I leave it the way it is now (InternalTracingHeaderType remains).

Stop putting things in InternalTelemetryEvent.Configuration and just extract everything from different features' contexts in TelemetryEventHandler. I can even change my implementation for okhttpInterceptorSampleRate from the previous pr.

I like option 1 more. The fact that we have a duplicate for TracingHeaderType isn't a big problem imho. Duplication - yes. But we have kind of bigger separation of public api TracingHeaderType and internal api InternalTracingHeaderType. There is close to no risk that we forget to change TracingHeaderType if InternalTracingHeaderType changes because it won't compile. And we can change InternalTracingHeaderType more freely because it is not public api. Maybe dd-sdk-android-internal will become a place where we store shared models that can be accessed by all features instead of using HashMaps to communicate between modules.

But I have no problem refactoring to option 2.

My idea was to distinguish it from an already existing TracingHeaderType. link which is public and can be used by clients of the sdk. Thus the prefix - internal.

What is being added already have a different package name (with internal in the path) and moreover it is a part of dd-sdk-android-internal, which is added as implementation everywhere, so it is not re-exported to the public API.

I suggest 2 options:

Let's do option 1 then, I'm fine with that.

What is being added already have a different package name (with internal in the path) and moreover it is a part of dd-sdk-android-internal, which is added as implementation everywhere, so it is not re-exported to the public API.

I will clarify a bit. I mean that when we (sdk devs) try to search for the class in Android Studio, we don't see 2 classes with exactly the same name. I understand that dd-sdk-android-internal is added as implementation everywhere and its classes are not visible to the clients of the sdk.

Let's do option 1 then, I'm fine with that.

done

idea: maybe drop Internal prefix from the name? Is it useful to have it?

done, droped internal prefix from both TracingHeaderType and TracingHeaderTypesSet

0xnm · 2025-03-31T14:59:02Z

+import fr.xgouchet.elmyr.Forge
+import fr.xgouchet.elmyr.ForgeryFactory
+
+class HostsWithHeaderTypesForgeryFactory : ForgeryFactory<InternalTracingHeaderTypesSet> {


this class name should be InternalTracingHeaderTypesSetForgeryFactory then

0xnm · 2025-03-31T15:00:40Z

+    fun InternalTracingHeaderTypesSet.toSelectedTracingPropagators() = types.map { it.toSelectedTracingPropagator() }
+
+    private fun InternalTracingHeaderType.toSelectedTracingPropagator():


suggestion: maybe make them 1st class citizens? without HostsWithHeaderTypesMapper object wrapper (which should also be renamed). Given that they have custom receiver, it seems there is no benefit.

removed the objects that I created, changed to top-level functions

0xnm · 2025-03-31T15:06:10Z

-    fun `M add trace sample rate to tracing feature context W constructor called`(
+    fun `M add trace sample rate and header types to tracing feature context W constructor called`(


maybe split in 2 different tests?

I don't see why I should. The test does verifyNoMoreInteractions in the end. If I split it in two, the separate tests would not be able to do that.

If you have some specific idea in mind, please tell me.

ambushwork · 2025-03-31T15:19:08Z

+import com.datadog.android.internal.telemetry.InternalTracingHeaderTypesSet
+import com.datadog.android.trace.TracingHeaderType
+
+internal object TracingHeaderTypeMapper {


In my opinion, We try to avoid to use Kotlin object if it doesn't have a strong argument in our SDK, what's the necessity of this one?

No problem, I will remove the objects. To me it is just a question of style and preference.

For our SDK it's just not just a style, the main problem of using object is that it's hard to mock it since we don't use mockk for static mocking. And it also stays globally in the memory even if it is never used.

0xnm

lgtm, just some minor naming update suggestions

0xnm · 2025-04-01T14:48:52Z

        internal val startupTimeNs: Long = System.nanoTime()

        internal const val OKHTTP_INTERCEPTOR_SAMPLE_RATE = "okhttp_interceptor_sample_rate"
+        internal const val OKHTTP_INTERCEPTOR_HOSTS_WITH_HEADER_TYPES = "okhttp_interceptor_hosts_with_header_types"


minor: there is no hosts anymore

Suggested change

internal const val OKHTTP_INTERCEPTOR_HOSTS_WITH_HEADER_TYPES = "okhttp_interceptor_hosts_with_header_types"

internal const val OKHTTP_INTERCEPTOR_HEADER_TYPES = "okhttp_interceptor_header_types"

0xnm · 2025-04-01T14:50:01Z

 class com.datadog.android.sqlite.DatadogDatabaseErrorHandler : android.database.DatabaseErrorHandler
  constructor(String? = null, android.database.DatabaseErrorHandler = DefaultDatabaseErrorHandler())
  override fun onCorruption(android.database.sqlite.SQLiteDatabase)
+fun com.datadog.android.internal.telemetry.TracingHeaderTypesSet.toSelectedTracingPropagators()


this function should be internal

0xnm · 2025-04-01T14:53:29Z

        internal const val W3C_PARENT_ID_LENGTH = 16

        internal const val OKHTTP_INTERCEPTOR_SAMPLE_RATE = "okhttp_interceptor_sample_rate"
+        internal const val OKHTTP_INTERCEPTOR_HOSTS_WITH_HEADER_TYPES = "okhttp_interceptor_hosts_with_header_types"


Suggested change

internal const val OKHTTP_INTERCEPTOR_HOSTS_WITH_HEADER_TYPES = "okhttp_interceptor_hosts_with_header_types"

internal const val OKHTTP_INTERCEPTOR_HEADER_TYPES = "okhttp_interceptor_header_types"

thanks for spotting, fixed

…figuration telemetry

aleksandr-gringauz force-pushed the aleksandr-gringauz/rum-562/report-tracing-headers branch 3 times, most recently from 38c23db to 90b56fd Compare March 31, 2025 09:52

aleksandr-gringauz marked this pull request as ready for review March 31, 2025 09:52

aleksandr-gringauz requested review from a team as code owners March 31, 2025 09:52

0xnm reviewed Mar 31, 2025

View reviewed changes

aleksandr-gringauz force-pushed the aleksandr-gringauz/rum-562/report-tracing-headers branch 2 times, most recently from fc2ef36 to 067ef5f Compare March 31, 2025 14:55

0xnm reviewed Mar 31, 2025

View reviewed changes

ambushwork previously approved these changes Mar 31, 2025

View reviewed changes

aleksandr-gringauz dismissed ambushwork’s stale review via e8a2407 April 1, 2025 14:36

aleksandr-gringauz force-pushed the aleksandr-gringauz/rum-562/report-tracing-headers branch 2 times, most recently from e8a2407 to 82d8e97 Compare April 1, 2025 14:51

0xnm previously approved these changes Apr 1, 2025

View reviewed changes

RUM-562: Report configured distributed tracing headers as part of Con…

68b4789

…figuration telemetry

aleksandr-gringauz dismissed 0xnm’s stale review via 68b4789 April 1, 2025 15:09

aleksandr-gringauz force-pushed the aleksandr-gringauz/rum-562/report-tracing-headers branch from 82d8e97 to 68b4789 Compare April 1, 2025 15:09

0xnm approved these changes Apr 1, 2025

View reviewed changes

aleksandr-gringauz merged commit 05e490a into develop Apr 1, 2025

aleksandr-gringauz deleted the aleksandr-gringauz/rum-562/report-tracing-headers branch April 1, 2025 19:27

	val hostsWithHeaderTypes: HostsWithHeaderTypes? = forge.aNullable<HostsWithHeaderTypes>()
	val hostsWithHeaderTypes = forge.aNullable<HostsWithHeaderTypes>()

		fun InternalTracingHeaderTypesSet.toSelectedTracingPropagators() = types.map { it.toSelectedTracingPropagator() }

		private fun InternalTracingHeaderType.toSelectedTracingPropagator():

		fun `M add trace sample rate to tracing feature context W constructor called`(
		fun `M add trace sample rate and header types to tracing feature context W constructor called`(

	internal const val OKHTTP_INTERCEPTOR_HOSTS_WITH_HEADER_TYPES = "okhttp_interceptor_hosts_with_header_types"
	internal const val OKHTTP_INTERCEPTOR_HEADER_TYPES = "okhttp_interceptor_header_types"

Uh oh!

Conversation

aleksandr-gringauz commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

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!

aleksandr-gringauz Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

codecov-commenter commented Mar 31, 2025 • 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!

aleksandr-gringauz Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aleksandr-gringauz Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

aleksandr-gringauz commented Mar 28, 2025 •

edited

Loading

aleksandr-gringauz Mar 31, 2025 •

edited

Loading

codecov-commenter commented Mar 31, 2025 •

edited

Loading

aleksandr-gringauz Apr 1, 2025 •

edited

Loading

0xnm Apr 1, 2025 •

edited

Loading

aleksandr-gringauz Apr 1, 2025 •

edited

Loading