RUM-3438 Add the OkHttp Otel extensions module by mariusc83 · Pull Request #2073 · DataDog/dd-sdk-android

mariusc83 · 2024-06-04T15:10:00Z

What does this PR do?

In this PR:

we are adding the dd-sdk-android-okhttp-otel module in which we will provide the Request.Builder.addParentSpan extension method
we are adding the use case for this new Otel extension in the Sample application

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-04T15:11:27Z

+import io.opentelemetry.api.trace.Span
+import okhttp3.Request
+
+fun Request.Builder.addParenSpan(span: Span) {


this one NO !!

0xnm

approach lgtm, I left some suggestions.

0xnm · 2024-06-05T07:12:27Z

    api(libs.openTelemetryApi)
    implementation(libs.kotlin)
    implementation(libs.androidXAnnotation)
+    implementation(libs.okHttp)


this one shouldn't be here, there is no code change in dd-sdk-android-trace-otel module.

yes was a left over from previous try outs :)

0xnm · 2024-06-05T07:14:51Z

+    id("com.github.ben-manes.versions")
+
+    // Tests
+    id("de.mobilej.unmock")


this one is probably useless for this module

0xnm · 2024-06-05T07:19:00Z

  override fun onRequestIntercepted(com.datadog.android.api.feature.FeatureSdkCore, okhttp3.Request, io.opentracing.Span?, okhttp3.Response?, Throwable?)
  override fun canSendSpan(): Boolean
  override fun onSdkInstanceReady(com.datadog.android.core.InternalSdkCore)
+data class com.datadog.android.okhttp.DdOtelContext


you probably can name this something like TraceContext (even without Dd prefix), because there is nothing from Otel here actually, this class only contains trace ID, span ID and sampling as int, all of these are applicable to the OpenTracing as well.

yeah was not sure about the naming

0xnm · 2024-06-05T07:19:43Z

+    val traceId: String,
+    val spanId: String,
+    val samplingPriority: Int


Detekt Public API check will require adding javadoc

0xnm · 2024-06-05T14:56:13Z

+    } catch (e: NumberFormatException) {
+        BigInteger.ZERO
+    } catch (e: ArithmeticException) {
+        BigInteger.ZERO
+    }


we can maybe log that using InternalLogger

was thinking about that, need to check to see if I have access to it but in the same time this problem will never arrive as I mentioned there we are the ones providing this values from the CoreTracer so there's no outside input. I don't think it is worth it to bring the internal logger here.

buraizu

Thanks, I've suggested some very minor updates to the README.md, but mainly want to flag what appears to be a broken link

buraizu · 2024-06-05T18:55:02Z

+See the dedicated [Datadog SDK Android for OpenTelemetry documentation][2] to learn how to add a parent span to network requests made by `OkHttp` library.
+
+[1]: https://docs.datadoghq.com/real_user_monitoring/android/advanced_configuration/?tab=kotlin#automatically-track-network-requests
+[2]: https://docs.datadoghq.com/tracing/trace_collection/custom_instrumentation/android?tab=kotlin


This link doesn't seem to be working. Is there another page it should open, or is the linked page expected to be merged soon?

it will be available soon, the PR is in review on the documentation repo. We will merge that PR once we release this feature. This is the PR: DataDog/documentation#23389

Co-authored-by: Bryce Eadie <[email protected]>

…/android/okhttp/TraceContext.kt Co-authored-by: Nikita Ogorodnikov <[email protected]>

codecov-commenter · 2024-06-06T11:33:22Z

Codecov Report

Attention: Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 67.20%. Comparing base (ee23810) to head (eb5ffe8).

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/otel-support    #2073      +/-   ##
========================================================
- Coverage                 67.39%   67.20%   -0.19%     
========================================================
  Files                       733      736       +3     
  Lines                     27028    27046      +18     
  Branches                   4574     4576       +2     
========================================================
- Hits                      18213    18174      -39     
- Misses                     7587     7654      +67     
+ Partials                   1228     1218      -10

Files	Coverage Δ
.../kotlin/com/datadog/android/okhttp/TraceContext.kt	`100.00% <100.00%> (ø)`
...datadog/android/okhttp/trace/TracingInterceptor.kt	`81.92% <100.00%> (-0.32%)`	⬇️
...otlin/com/datadog/android/okhttp/otel/OkHttpExt.kt	`75.00% <75.00%> (ø)`
...og/android/okhttp/internal/otel/TraceContextExt.kt	`86.67% <86.67%> (ø)`

... and 35 files with indirect coverage changes

xgouchet · 2024-06-06T13:54:45Z

+    val context = span.spanContext
+    val prioritySampling = resolveSamplingPriority(context)
+    // the context will always be a TraceContext
+    @Suppress("UnsafeThirdPartyFunctionCall")


Note
If I am not mistaken, the tag method can throw an NPE. It would be better to let the comment address the exact issue we're suppressing. It's also a best practice to have the comment on the same line as the suppress as it will appear when searching for @Suppress in the codebase, e.g.:

@Suppress("UnsafeThirdPartyFunctionCall") // NPE cannot happen here

It throws a ClassCastException if your values is not of the given type but in this case this cannot happen.

xgouchet · 2024-06-06T13:56:31Z

+
+        // Then
+        val taggedContext = request.tag(TraceContext::class.java)
+        assertThat(taggedContext).isNotNull()


Note
You can both check this and mark the result as non null for the compiler by doing checkNotNull(taggedContext). That way you can remove the ?.` in following lines

xgouchet · 2024-06-06T14:10:18Z

+import com.datadog.tools.unit.forge.BaseConfigurator
+import fr.xgouchet.elmyr.Forge
+
+internal class Configurator : BaseConfigurator() {


Note
As we have a Configurator in every module, it'd be better to rename this to reflect which module it's in, otherwise it's sometimes difficult to navigate to the one you want. E.g.: OkHttpConfigurator

buraizu

Thanks for confirming about the link. I've left an optional suggestion to remove the tab from the new link, but feel free to ignore this suggestion if it's expected that the users should be on the Kotlin tab when opening the link.

buraizu · 2024-06-06T16:19:59Z

+See the dedicated [Datadog SDK Android for OpenTelemetry documentation][2] to learn how to add a parent span to network requests made by the `OkHttp` library.
+
+[1]: https://docs.datadoghq.com/real_user_monitoring/android/advanced_configuration/#automatically-track-network-requests
+[2]: https://docs.datadoghq.com/tracing/trace_collection/custom_instrumentation/android?tab=kotlin


Suggested change

[2]: https://docs.datadoghq.com/tracing/trace_collection/custom_instrumentation/android?tab=kotlin

[2]: https://docs.datadoghq.com/tracing/trace_collection/custom_instrumentation/android

mariusc83 self-assigned this Jun 4, 2024

mariusc83 commented Jun 4, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3438/support-parent-otel-span-in-interceptor branch 3 times, most recently from 716fa50 to 9e598bb Compare June 4, 2024 15:22

0xnm reviewed Jun 5, 2024

View reviewed changes

RUM-3438 Introduce the dd-sdk-android-okhttp-otel module

90e7b17

mariusc83 force-pushed the mconstantin/rum-3438/support-parent-otel-span-in-interceptor branch from 9e598bb to 178a467 Compare June 5, 2024 13:37

mariusc83 marked this pull request as ready for review June 5, 2024 13:46

mariusc83 requested review from a team as code owners June 5, 2024 13:46

mariusc83 force-pushed the mconstantin/rum-3438/support-parent-otel-span-in-interceptor branch from 178a467 to a760ab7 Compare June 5, 2024 14:39

0xnm previously approved these changes Jun 5, 2024

View reviewed changes

buraizu requested changes Jun 5, 2024

View reviewed changes

mariusc83 dismissed 0xnm’s stale review via b369d6e June 6, 2024 08:03

mariusc83 requested a review from buraizu June 6, 2024 09:20

mariusc83 and others added 5 commits June 6, 2024 12:00

Provide the addParentSpan extension for the OkHttp Request.Builder

6873d3f

Add the Otel parent span for OkHttp request use case in sample app

78a4d1b

Update integrations/dd-sdk-android-okhttp-otel/README.md

d096d57

Co-authored-by: Bryce Eadie <[email protected]>

Update integrations/dd-sdk-android-okhttp-otel/README.md

b0c0c48

Co-authored-by: Bryce Eadie <[email protected]>

Update integrations/dd-sdk-android-okhttp/src/main/kotlin/com/datadog…

dc9c63f

…/android/okhttp/TraceContext.kt Co-authored-by: Nikita Ogorodnikov <[email protected]>

mariusc83 force-pushed the mconstantin/rum-3438/support-parent-otel-span-in-interceptor branch from 87fa0b3 to dc9c63f Compare June 6, 2024 10:00

mariusc83 requested a review from 0xnm June 6, 2024 11:06

0xnm previously approved these changes Jun 6, 2024

View reviewed changes

xgouchet reviewed Jun 6, 2024

View reviewed changes

Apply review changes

eb5ffe8

mariusc83 dismissed 0xnm’s stale review via eb5ffe8 June 6, 2024 14:26

buraizu approved these changes Jun 6, 2024

View reviewed changes

mariusc83 requested review from 0xnm and xgouchet June 7, 2024 07:18

0xnm approved these changes Jun 7, 2024

View reviewed changes

jonathanmos approved these changes Jun 7, 2024

View reviewed changes

mariusc83 merged commit 8805d24 into feature/otel-support Jun 7, 2024

mariusc83 deleted the mconstantin/rum-3438/support-parent-otel-span-in-interceptor branch June 7, 2024 09:43

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

	[2]: https://docs.datadoghq.com/tracing/trace_collection/custom_instrumentation/android?tab=kotlin
	[2]: https://docs.datadoghq.com/tracing/trace_collection/custom_instrumentation/android

Uh oh!

Conversation

mariusc83 commented Jun 4, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buraizu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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!

buraizu left a comment

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!

6 participants

mariusc83 commented Jun 4, 2024 •

edited

Loading

codecov-commenter commented Jun 6, 2024 •

edited

Loading