RUM-3536 Introduce the TraceContextInjection to handle sampling in distributed traces by mariusc83 · Pull Request #2111 · DataDog/dd-sdk-android

mariusc83 · 2024-06-27T06:57:55Z

What does this PR do?

Following the requirements specified in this RFC we are adding this new API and fixing the traces propagation in our TracingInterceptor.

This also fixes the issue: #2124

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)

mariusc83 · 2024-06-28T08:01:04Z

    private val sampleRate: Float = DEFAULT_SAMPLE_RATE
 ) : AppGlideModule() {

+    private val firstPartyHostsWithHeaderType = firstPartyHosts.associateWith {


the only problem I see with this is that some users will not know what to use for their tracing header types values so we might want to provide a Public Constant: DEFAULT_TYPES here ?

why then we need to provide association if setOf(TracingHeaderType.DATADOG, TracingHeaderType.TRACECONTEXT) is what already set in the SDK by default? Shouldn't we have a public API for both firstPartyHosts and firstPartyHostsWithHeaderType as we have now?

we could but the way I see it firstPartyHosts is mandatory (not optional) so it will have to be in the Builder constructor. If we want to introduce both APIs and if a user wants to provide custom headerTypes for each host it will look like this:

val builder = Builder(tracedHosts).setFirstPartyHostsWithHeaderTypes(firstPartyHostsWithHeaderTypes)

I do not like that.

otherwise we could have:

val builder = Builder().setFirstPartyHostsWithHeaderTypes(firstPartyHostsWithHeaderTypes) val builder = Builder().setFirstPartyHosts(firstPartyHosts)

But this will mean that some users will forget to set this up in the builder and will create support issues.

0xnm · 2024-06-28T08:20:24Z

-        )
+        val interceptorBuilder = DatadogInterceptor.Builder(firstPartyHostsWithHeaderType)
+            .setTraceSampler(RateBasedSampler(sampleRate))
+        sdkInstanceName?.let { interceptorBuilder.setSdkInstanceName(it) }


I would pull sdkInstanceName to the DatadogInterceptor.Builder constructor - SDK core association is a major part of the API, not something optional.

in the same time we have it optional now, I would only do that if we do not allow it as optional anymore otherwise it doesn't make sense to have it in the constructor if it can be NULL

0xnm · 2024-06-28T08:21:43Z

    private val sampleRate: Float = DEFAULT_SAMPLE_RATE
 ) : AppGlideModule() {

+    private val firstPartyHostsWithHeaderType = firstPartyHosts.associateWith {


why then we need to provide association if setOf(TracingHeaderType.DATADOG, TracingHeaderType.TRACECONTEXT) is what already set in the SDK by default? Shouldn't we have a public API for both firstPartyHosts and firstPartyHostsWithHeaderType as we have now?

0xnm · 2024-06-28T08:25:01Z

+    /**
+     * A Builder for the [DatadogInterceptor].
+     */
+    open class Builder(tracedHosts: Map<String, Set<TracingHeaderType>>) :


We probably need to remove open modifier from here and DatadogInterceptor as well. I think it was originally added because we have RumInterceptor : DatadogInterceptor.

But removing it involves a risk - some customers may have already sub-classed DatadogInterceptor.

I don't want to do that, at least not now.

0xnm · 2024-06-28T08:26:21Z

+    /**
+     * A Builder for the [DatadogInterceptor].
+     */
+    open class Builder(tracedHosts: Map<String, Set<TracingHeaderType>>) :


IMO having a map here is unnecessary complication and it should be just tracedHosts: List<String>. Asking to provide tracing header types may be too advanced and increases setup curve. Probably default header types will work for 90%+ installations.

it will but then we will have to provide the setTracedHostWithHeaderTypes method in the Builder and that will create some confusion. It will look like this:

val builder = Builder(tracedHosts).setFirstPartyHostsWithHeaderTypes(firstPartyHostsWithHeaderTypes)

It will look ugly and users will wonder what is the need for the tracedHosts in the Builder.

we will have to provide the setTracedHostWithHeaderTypes method in the Builder

It won't be needed if constructor itself will have another overload. APM setup is already quite complicated, we need to have it simpler with reasonable defaults.

so we provide overloads to this Builder constructor ? I would rather not and just provide a public api DEFAULT_HEADER_TYPES = setOf(TracingHeaderType.DATADOG, TracingHeaderType.TRACECONTEXT)
We are trying to run away from overloads. I am opened to any of this options but I would prefer the one without overloads.
@xgouchet @plousada , what do you think ?

We are trying to run away from overloads
It is just because there were too much of them in the original constructor. Basically we need to put what is mandatory in the constructor and cannot be replaced by some default value.

For now it is only the list of the traced hosts, and the rest of the pieces needed to make interceptor work can have a default value, so they go into the methods of the Builder - this separation was missing in the original design of the DatadogInterceptor. So having an overload just because of the single mandatory parameter looks fine to me.

0xnm · 2024-06-28T08:26:49Z

+
+        private var rumResourceAttributesProvider: RumResourceAttributesProvider = NoOpRumResourceAttributesProvider()
+
+        override fun getThis(): Builder {


getThis? why is it needed?

Generics check the Builder and BaseBuilder class. It is just a patter for having a BaseBuilder using Java Generics. In any case this is internal

Tip
Maybe this method could be protected, otherwise it might be confusing to our customers

@xgouchet it is internal and not visible from outside the package. Check the method definition in the BaseBuilder class.

0xnm · 2024-06-28T08:27:45Z

+
+        /**
+         * Sets the [RumResourceAttributesProvider] to use to provide custom attributes to the RUM.
+         * By default, it is a NoOp.


Suggested change

* By default, it is a NoOp.

* By default, it won't attach any custom attributes.

codecov-commenter · 2024-07-16T16:03:54Z

Codecov Report

Attention: Patch coverage is 92.50000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 69.92%. Comparing base (fc53808) to head (0470b37).
Report is 5 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2111      +/-   ##
===========================================
- Coverage    70.03%   69.92%   -0.11%     
===========================================
  Files          721      722       +1     
  Lines        26773    26859      +86     
  Branches      4499     4506       +7     
===========================================
+ Hits         18749    18780      +31     
- Misses        6771     6819      +48     
- Partials      1253     1260       +7

Files	Coverage Δ
...om/datadog/android/okhttp/TraceContextInjection.kt	`100.00% <100.00%> (ø)`
...in/com/datadog/android/glide/DatadogGlideModule.kt	`95.65% <75.00%> (-4.35%)`	⬇️
...n/com/datadog/android/okhttp/DatadogInterceptor.kt	`60.12% <86.36%> (-13.35%)`	⬇️
...datadog/android/okhttp/trace/TracingInterceptor.kt	`76.80% <94.57%> (-5.19%)`	⬇️

... and 31 files with indirect coverage changes

xgouchet · 2024-07-17T12:27:43Z

+
+        private var rumResourceAttributesProvider: RumResourceAttributesProvider = NoOpRumResourceAttributesProvider()
+
+        override fun getThis(): Builder {


Tip
Maybe this method could be protected, otherwise it might be confusing to our customers

@xgouchet it is internal and not visible from outside the package. Check the method definition in the BaseBuilder class.

0xnm · 2024-07-18T08:27:10Z

+  class Builder : BaseBuilder<DatadogInterceptor, Builder>
+    constructor(Map<String, Set<com.datadog.android.trace.TracingHeaderType>>)
+    constructor(List<String>)
+    override fun getThis(): Builder


blocking: we shouldn't have this as a part of the public API. Is there a way to avoid that?

hmm I really do not know why is added here as it is marked as internal. I need to check again. Thanks for spotting this. Do you have any idea why it still appears here ?

maybe I should mark it as internal also when I override...let me try that...that will be stupid

jonathanmos · 2024-07-18T09:29:30Z

+    }
+
+    fun getExpectedOrigin(): String? {
+        return fakeOrigin


Minor: Why do we need this method and not compare directly to fakeOrigin? Also, can this return a null value? fakeOrigin is defined as an alphabetical string so should be non-null

just legacy code that I did not change

datadog-staging-us1-crawler-test · 2024-07-18T15:40:29Z

Code Vulnerabilities

Code Quality Violations

0xnm

reviewed only prod files (not tests), looks good to me. I left some suggestions.

0xnm · 2024-07-19T08:26:43Z

+	public fun <init> (Ljava/util/Map;)V
+	public fun build ()Lcom/datadog/android/okhttp/DatadogInterceptor;
+	public synthetic fun build ()Lcom/datadog/android/okhttp/trace/TracingInterceptor;
+	public synthetic fun getThis$dd_sdk_android_okhttp_release ()Lcom/datadog/android/okhttp/trace/TracingInterceptor$BaseBuilder;


we are still exposing such things in Java API. Maybe there is some annotation which may hide it?

will try but honestly if there's not any I will not fix this tool in this PR

0xnm · 2024-07-19T08:27:54Z

+    /**
+     * A Builder for the [DatadogInterceptor].
+     * @param tracedHostsWithHeaderType a list of all the hosts and header types that you want to
+     * be automatically tracked by this interceptor. If registering a GlobalTracer, the tracer must be


Suggested change

* be automatically tracked by this interceptor. If registering a GlobalTracer, the tracer must be

* be automatically tracked by this interceptor. If registering a [GlobalTracer], the tracer must be

0xnm · 2024-07-19T08:34:36Z

+    /**
+     * A Builder class for the [TracingInterceptor].
+     * @param tracedHostsWithHeaderType a list of all the hosts and header types that you want to
+     * be automatically tracked by this interceptor. If registering a GlobalTracer, the tracer must be


Suggested change

* be automatically tracked by this interceptor. If registering a GlobalTracer, the tracer must be

* be automatically tracked by this interceptor. If registering a [GlobalTracer], the tracer must be

0xnm · 2024-07-19T08:36:05Z

+            return getThis()
+        }
+
+        @Suppress("UndocumentedPublicFunction")


can be removed, it is not a public function anymore

0xnm · 2024-07-19T08:36:37Z

+        internal abstract fun getThis(): R
+
+        /**
+         * Build the [TracingInterceptor].


Suggested change

* Build the [TracingInterceptor].

* Build the interceptor.

mariusc83 self-assigned this Jun 27, 2024

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch 3 times, most recently from 256c37e to 8b3c6ab Compare June 27, 2024 12:41

mariusc83 force-pushed the mconstantin/rum-3536/correctly-handle-sampling-in-distributed-traces branch from 07e0c12 to bc386dd Compare June 28, 2024 07:44

mariusc83 commented Jun 28, 2024

View reviewed changes

0xnm reviewed Jun 28, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-1829/use-128-bit-trace-ids branch from 8b3c6ab to 446a7b4 Compare June 28, 2024 09:44

Base automatically changed from mconstantin/rum-1829/use-128-bit-trace-ids to develop June 28, 2024 12:33

mariusc83 added 2 commits July 15, 2024 10:06

Introduce the TraceContextInjection API and Interceptor Builders

e3b973f

Handle traced request headers according with TraceContextInjectio option

ca81eb6

mariusc83 force-pushed the mconstantin/rum-3536/correctly-handle-sampling-in-distributed-traces branch 6 times, most recently from 595cd99 to 9e0b008 Compare July 16, 2024 15:36

mariusc83 marked this pull request as ready for review July 16, 2024 16:54

mariusc83 requested review from a team as code owners July 16, 2024 16:54

mariusc83 force-pushed the mconstantin/rum-3536/correctly-handle-sampling-in-distributed-traces branch from 9e0b008 to 1225d92 Compare July 17, 2024 08:37

xgouchet approved these changes Jul 17, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3536/correctly-handle-sampling-in-distributed-traces branch from 1225d92 to 5dfc191 Compare July 17, 2024 12:32

mariusc83 requested a review from 0xnm July 17, 2024 14:41

0xnm requested changes Jul 18, 2024

View reviewed changes

jonathanmos approved these changes Jul 18, 2024

View reviewed changes

ganeshnj self-requested a review July 18, 2024 13:05

mariusc83 force-pushed the mconstantin/rum-3536/correctly-handle-sampling-in-distributed-traces branch 2 times, most recently from e22f2fd to 39f6e13 Compare July 18, 2024 14:28

mariusc83 requested a review from 0xnm July 18, 2024 14:59

0xnm approved these changes Jul 19, 2024

View reviewed changes

Instantiate the DatadogInterceptor through the new Builder

0470b37

mariusc83 force-pushed the mconstantin/rum-3536/correctly-handle-sampling-in-distributed-traces branch from 39f6e13 to 0470b37 Compare July 19, 2024 09:05

mariusc83 merged commit 6286611 into develop Jul 19, 2024

mariusc83 deleted the mconstantin/rum-3536/correctly-handle-sampling-in-distributed-traces branch July 19, 2024 09:33

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


		private var rumResourceAttributesProvider: RumResourceAttributesProvider = NoOpRumResourceAttributesProvider()

		override fun getThis(): Builder {

	* By default, it is a NoOp.
	* By default, it won't attach any custom attributes.

	* be automatically tracked by this interceptor. If registering a GlobalTracer, the tracer must be
	* be automatically tracked by this interceptor. If registering a [GlobalTracer], the tracer must be

Uh oh!

Conversation

mariusc83 commented Jun 27, 2024 • 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!

mariusc83 Jun 28, 2024 • 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!

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!

xgouchet Jul 17, 2024 • edited by mariusc83 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!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

xgouchet Jul 17, 2024 • edited by mariusc83 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!

datadog-staging-us1-crawler-test Bot commented Jul 18, 2024

Code Vulnerabilities

Code Quality Violations

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

mariusc83 commented Jun 27, 2024 •

edited

Loading

mariusc83 Jun 28, 2024 •

edited

Loading

xgouchet Jul 17, 2024 •

edited by mariusc83

Loading

codecov-commenter commented Jul 16, 2024 •

edited

Loading

xgouchet Jul 17, 2024 •

edited by mariusc83

Loading