Skip to content

[RUM-9902] Isolating implementation layer from integration modules#2773

Merged
satween merged 90 commits into
tvaleev/feature/RUM-9899_2from
tvaleev/feature/RUM-9902-removing-unused-code
Jul 18, 2025
Merged

[RUM-9902] Isolating implementation layer from integration modules#2773
satween merged 90 commits into
tvaleev/feature/RUM-9899_2from
tvaleev/feature/RUM-9902-removing-unused-code

Conversation

@satween

@satween satween commented Jul 1, 2025

Copy link
Copy Markdown
Contributor

What does this PR do?

The goal of this PR is to isolate the implementation from the API specification. The Datadog tracing integration is provided by the dd-trace-java library, which we moved into the dd-android-trace-internal module in a previous change.

We are now trying to finally eliminate opentracing dependencies and are facing the following issues:

  1. The opentracing API specification is used as an internal layer for module communication. For example, the dd-android-sdk-trace-http module uses the following objects from opentracing: Span, Scope, etc. The diagram below displays the existing architecture:
graph BT
J(otel) --> H(io.opentelemetry)
B(trace) --> L(trace-internal)
M[ ]:::empty --> B
J --> L
F(trace-coroutines) --- M
B -- api --> A(io.opentracing)
E(sqldelight) --- M
D(okhttp) --- M
D -- api--> A
classDef empty width:0px,height:0px;
Loading

As you might have noticed, there are a lot of modules that are aware of the specification classes and are bound to a specific implementation of the opentracing specification, such as AndroidTracing.

  1. The second problem is that most modules know not only the specifications, but also the implementation.
    A lot of downcasting logic can be found inside the integration module:
if (activeSpan is DDSpan) {
        activeSpan.traceId.toString()
    }

To solve the described issues and decouple the implementation from the internal and external specifications, we provide the following architecture:

graph BT
C --impl--> B(trace-internal)
C(trace) --api--> A(trace-api)
C --(ready to remove)--> O(io.opentracing)
M[ ]:::empty --> C
D(okhttp)  --- M
F(trace-coroutines) --- M
E(sqldelight) --- M
J(otel) --- M
J --> H(io.opentelemetry) 
classDef empty width:0px,height:0px;
Loading

This PR mainly includes:

  1. The dd-sdk-android-trace-api module was added to provide the internal API specification for trace functionality.
  2. The dd-android-sdk-trace module is now the only point that knows both the specification and the implementation.
  3. The opentracing API specification was replaced with the dd-sdk-android-trace-api for all modules that used the opentracing specification previously.
    4.All api dependencies were replaced with implementation dependencies for all modules except for dd-sdk-android-trace-api.

TL;DR:
In this pull request, we isolated the specific implementation from the specification and removed all of the downcasting logic from the integration modules. Now, the only place where the implementation meets the specification is the dd-sdk-android-trace module.

Motivation

Why just not to use Otel's specification instead of opentraicing?

Otel requires API 26 or desugaring support

Why just not to use dd-sdk-android-trace-internal's interfaces as a specification?

Most of the files in the dd-sdk-android-trace-internal directory are written in Java because it adopts the dd-trace-java directory. Therefore, some Kotlin features, such as the internal modifier, will not work if they are made public. Additionally, dd-trace-java was initially designed for a server-side agent and contains specific methods that are not applicable to Android, which could confuse customers.

Additional Notes

Note that there is still dependency on opentracing and internal module mostly for tests. I decided to remove them in a separate PR to reduce amount of changes.

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)

satween added 30 commits July 1, 2025 14:59

@jonathanmos jonathanmos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice pr! I've left a few comments

Comment thread instrumented/integration/proguard-rules.pro Outdated
@satween
satween requested review from 0xnm and mariusc83 July 14, 2025 17:10

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some questions/comments, but overall lgtm.

Comment thread detekt_custom.yml Outdated
satween and others added 12 commits July 16, 2025 14:50
This reverts commit 4cfff41

# Conflicts:
#	integrations/dd-sdk-android-okhttp-otel/src/main/kotlin/com/datadog/android/okhttp/otel/OkHttpExt.kt
#	integrations/dd-sdk-android-okhttp-otel/src/test/kotlin/com/datadog/android/okhttp/otel/OkHttpExtTest.kt
Co-authored-by: Nikita Ogorodnikov <[email protected]>
Co-authored-by: Nikita Ogorodnikov <[email protected]>
…g-unused-code' into tvaleev/feature/RUM-9902-removing-unused-code
…/android/okhttp/trace/TracingInterceptor.kt

Co-authored-by: Nikita Ogorodnikov <[email protected]>
…g-unused-code' into tvaleev/feature/RUM-9902-removing-unused-code

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I left two comments, one of them, regarding traceRateLimit, should be addressed before merging this PR.

Also I have a concern about SAMPLE_KEEP -> USER_KEEP change for the sampling priority. We need to check with APM spec if it may have any impact on the intake processing (e.g. metrics computation) or trace propagation.

Maybe it has impact only for the tracer -> agent -> intake setup, and since we are in the agent-less mode and setup is tracer -> intake, this doesn't change anything for us, but still it is better to double-check.

This can be done after merging PR.

@satween
satween merged commit 99a684b into tvaleev/feature/RUM-9899_2 Jul 18, 2025
24 checks passed
@satween
satween deleted the tvaleev/feature/RUM-9902-removing-unused-code branch July 18, 2025 08:58
@0xnm
0xnm restored the tvaleev/feature/RUM-9902-removing-unused-code branch March 26, 2026 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants