RUM-12635: RUM support for Cronet #3029

satween · 2025-11-27T10:53:50Z

What does this PR do?

This is a draft PR. work in progress

Adds instrumentation for the Crontet library.
Currently, only RUM Resources are supported (see the RFC in the JIRA ticket).
The common code base aimed to be shared between OkHttp and Cronet has been moved into RumResourceInstrumentation, but neither DatadogInterceptor nor TracingInterceptor are using it.
The instrumentation is added as a wrapper on top of the Cronet API that allows customer to use several instrumentation libraries at the same time.

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 · 2025-11-27T15:47:56Z

Codecov Report

❌ Patch coverage is 56.90608% with 156 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.00%. Comparing base (0c156cc) to head (ec8f689).

Files with missing lines	Patch %	Lines
...oid/rum/internal/net/RumResourceInstrumentation.kt	0.00%	70 Missing ⚠️
...adog/android/okhttp/internal/OkHttpResponseInfo.kt	0.00%	32 Missing ⚠️
...lin/com/datadog/android/rum/resource/RequestExt.kt	0.00%	15 Missing ⚠️
.../com/datadog/android/cronet/DatadogCronetEngine.kt	88.17%	11 Missing ⚠️
.../com/datadog/android/core/internal/net/HttpSpec.kt	0.00%	8 Missing ⚠️
...adog/android/cronet/internal/CronetResponseInfo.kt	25.00%	6 Missing ⚠️
...net/internal/DatadogRequestFinishedInfoListener.kt	89.29%	3 Missing and 3 partials ⚠️
...tadog/android/cronet/internal/CronetRequestInfo.kt	80.00%	1 Missing and 2 partials ⚠️
...tadog/android/okhttp/internal/OkHttpRequestInfo.kt	75.00%	1 Missing and 1 partial ⚠️
...kotlin/com/datadog/android/api/net/ResponseInfo.kt	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@                Coverage Diff                 @@
##           feature/cronet    #3029      +/-   ##
==================================================
- Coverage           71.22%   71.00%   -0.22%     
==================================================
  Files                 859      869      +10     
  Lines               31457    31802     +345     
  Branches             5306     5352      +46     
==================================================
+ Hits                22405    22581     +176     
- Misses               7544     7711     +167     
- Partials             1508     1510       +2

Files with missing lines	Coverage Δ
...tadog/android/cronet/internal/DatadogUrlRequest.kt	`100.00% <100.00%> (ø)`
...com/datadog/android/okhttp/DatadogEventListener.kt	`96.15% <ø> (ø)`
...n/com/datadog/android/okhttp/DatadogInterceptor.kt	`73.86% <ø> (-0.15%)`	⬇️
...kotlin/com/datadog/android/api/net/ResponseInfo.kt	`0.00% <0.00%> (ø)`
...tadog/android/rum/RumResourceAttributesProvider.kt	`0.00% <0.00%> (ø)`
...ndroid/cronet/internal/DatadogUrlRequestBuilder.kt	`97.67% <97.67%> (ø)`
...tadog/android/okhttp/internal/OkHttpRequestInfo.kt	`75.00% <75.00%> (ø)`
...tadog/android/cronet/internal/CronetRequestInfo.kt	`80.00% <80.00%> (ø)`
...adog/android/cronet/internal/CronetResponseInfo.kt	`25.00% <25.00%> (ø)`
...net/internal/DatadogRequestFinishedInfoListener.kt	`89.29% <89.29%> (ø)`
... and 5 more

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:

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

ambushwork

Just finish the review of the half, will continue on next week.

ambushwork · 2025-11-28T15:17:31Z

...-android-core/src/main/kotlin/com/datadog/android/api/instrumentation/network/RequestInfo.kt

+ * Represents information about an HTTP request.
+ * Isolates specific library's details from the Datadog SDK.
+ */
+interface RequestInfo {


Question: should this be put inside internal module instead of core?

ambushwork · 2025-11-28T15:23:15Z

...ions/dd-sdk-android-cronet/src/main/kotlin/com/datadog/android/cronet/DatadogCronetEngine.kt

+
+        /**
+         * Creates a new builder with Datadog instrumentation.
+         * @param context the Android context


nip: should specify as application context

ambushwork · 2025-11-28T15:32:16Z

...ions/dd-sdk-android-cronet/src/main/kotlin/com/datadog/android/cronet/DatadogCronetEngine.kt

+
+        // endregion
+        companion object {
+            // Exception thrown only for wrong arguments, buy those ones is correct


Suggested change

// Exception thrown only for wrong arguments, buy those ones is correct

// Exception thrown only for wrong arguments, but those ones are correct

ambushwork · 2025-11-28T15:43:22Z

...droid-rum/src/main/kotlin/com/datadog/android/rum/internal/net/RumResourceInstrumentation.kt

+    internal val networkLayerName: String,
+    internal val rumResourceAttributesProvider: RumResourceAttributesProvider
+) {
+    private val sdkCoreReference = SdkReference(sdkInstanceName) {


Do you consider to add integration test for cronet <-> rum resoureces?

0xnm

I went only through the production files so far, didn't cover tests

0xnm · 2025-12-01T09:11:38Z

dd-sdk-android-core/api/apiSurface

+  val headers: Map<String, List<String>>
+  val contentType: String?
+  fun contentLength(com.datadog.android.api.InternalLogger): Long?
+  fun headerValue(String): String?


strictly speaking, HTTP standard allows multiple occurrences of the same header, so return type should be List<String>, this can be seen in OkHttp API:

/** Returns an immutable list of the header values for `name`. */ fun values(name: String): List<String> {

0xnm · 2025-12-01T09:12:29Z

...-android-core/src/main/kotlin/com/datadog/android/api/instrumentation/network/RequestInfo.kt

+     * @return the length of the content in bytes, or null if the content length is unavailable.
+     * @throws IOException if an error occurs while calculating the content length.
+     */
+    @Throws(IOException::class)


should we throw/rethrow anything?

0xnm · 2025-12-01T09:14:27Z

...testFixtures/kotlin/com/datadog/android/api/instrumentation/network/RequestInfoAssertions.kt

+    companion object {
+        fun assertThat(info: RequestInfo) = RequestInfoAssertions(info)
+    }
+}


Suggested change

}

}

0xnm · 2025-12-01T09:15:46Z

...testFixtures/kotlin/com/datadog/android/api/instrumentation/network/RequestInfoAssertions.kt

+
+import org.assertj.core.api.Assertions
+
+class RequestInfoAssertions private constructor(private val info: RequestInfo) {


Suggested change

class RequestInfoAssertions private constructor(private val info: RequestInfo) {

class RequestInfoAssert private constructor(private val info: RequestInfo) {

also it is worth to subclass AbstractObjectAssert<RequestInfoAssert, RequestInfo>

0xnm · 2025-12-01T09:47:22Z

.../dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumResourceAttributesProvider.kt

+        request: RequestInfo,
+        response: ResponseInfo?,
+        throwable: Throwable?
+    ): Map<String, Any?> = emptyMap()


nit: default return value is not needed here

0xnm · 2025-12-01T10:47:41Z

...et/src/main/kotlin/com/datadog/android/cronet/internal/DatadogRequestFinishedInfoListener.kt

+) : RequestFinishedInfo.Listener(executor) {
+
+    override fun onRequestFinished(finishedInfo: RequestFinishedInfo) {
+        val requestInfo = finishedInfo.annotations?.firstOrNull { it is RequestInfo } as? RequestInfo


minor

Suggested change

val requestInfo = finishedInfo.annotations?.firstOrNull { it is RequestInfo } as? RequestInfo

val requestInfo = finishedInfo.annotations?.filterIsInstance<RequestInfo>()?.firstOrNull()

0xnm · 2025-12-01T10:50:10Z

...et/src/main/kotlin/com/datadog/android/cronet/internal/DatadogRequestFinishedInfoListener.kt

+        }
+
+        val downloadStartMs = metrics.responseStart - metrics.requestStart
+        val downloadDurationMs = metrics.requestEnd - metrics.responseStart


probably a typo here

Suggested change

val downloadDurationMs = metrics.requestEnd - metrics.responseStart

val downloadDurationMs = metrics. responseEnd - metrics.responseStart

0xnm · 2025-12-01T10:50:42Z

...et/src/main/kotlin/com/datadog/android/cronet/internal/DatadogRequestFinishedInfoListener.kt

+            return if (thisTime == null || otherTime == null) 0L else thisTime - otherTime
+        }
+
+        private fun Long.toNanos(): Long = TimeUnit.MILLISECONDS.toNanos(this)


Suggested change

private fun Long.toNanos(): Long = TimeUnit.MILLISECONDS.toNanos(this)

private fun Long.millisToNanos(): Long = TimeUnit.MILLISECONDS.toNanos(this)

0xnm · 2025-12-01T10:51:10Z

...droid-cronet/src/main/kotlin/com/datadog/android/cronet/internal/DatadogUrlRequestBuilder.kt

+    internal val url: String,
+    internal val delegate: UrlRequest.Builder,
+    internal val rumResourceInstrumentation: RumResourceInstrumentation
+) : UrlRequest.Builder() {


Suggested change

) : UrlRequest.Builder() {

) : UrlRequest.Builder() by delegate {

0xnm · 2025-12-01T10:53:13Z

...droid-cronet/src/main/kotlin/com/datadog/android/cronet/internal/DatadogUrlRequestBuilder.kt

+        )
+
+        addRequestAnnotation(requestInfo)
+        addRequestAnnotation(requestInfo.buildResourceId(generateUuid = true))


should we just add it as a part of CronetRequestInfo?

ambushwork · 2025-12-01T16:08:36Z

...com/datadog/android/okhttp/internal/RumResourceAttributesProviderCompatibilityAdapterTest.kt

+    }
+
+    @Test
+    fun `M call delegate first THEN deprecated W onProvideAttributes { RequestInfo }`(


what does THEN deprecated mean here?

ambushwork · 2025-12-01T16:09:39Z

...sdk-android-okhttp/src/main/kotlin/com/datadog/android/okhttp/internal/OkHttpResponseInfo.kt

+    internal companion object {
+
+        // We need to limit this value as the body will be loaded in memory
+        private const val MAX_BODY_PEEK: Long = 32 * 1024L * 1024L


it will be nice to add unit (bytes) in the name

ambushwork · 2025-12-01T16:14:58Z

...-android-cronet/src/test/kotlin/com/datadog/android/cronet/internal/CronetRequestInfoTest.kt

+    }
+
+    @Test
+    fun `M return null W contentLength() { upload data provider length is unknown }`() {


Not sure the name of this function corresponds the body, upload data provider is not unknown, it is -1, and the return value is not null, it is -1.

ambushwork · 2025-12-01T16:15:53Z

...-sdk-android-okhttp/src/main/kotlin/com/datadog/android/okhttp/internal/OkHttpRequestInfo.kt

+    override val method: String get() = request.method
+
+    override fun <T> tag(type: Class<out T>): T? = request.tag(type)
+    override fun contentLength(): Long = request.body?.contentLength() ?: 0L


Question, CronetRequestInfo has Long? as return value here, but OkHttpRequestInfo return 0 as default, is that expected?

jonathanmos · 2025-12-02T09:31:15Z

...sdk-android-okhttp/src/main/kotlin/com/datadog/android/okhttp/internal/OkHttpResponseInfo.kt

@@ -0,0 +1,69 @@
+package com.datadog.android.okhttp.internal


minor: Missing copyright

jonathanmos · 2025-12-02T09:31:41Z

...et/src/main/kotlin/com/datadog/android/cronet/internal/DatadogRequestFinishedInfoListener.kt

@@ -0,0 +1,127 @@
+package com.datadog.android.cronet.internal


minor: Missing copyright

jonathanmos · 2025-12-02T09:45:52Z

...android-okhttp/src/test/kotlin/com/datadog/android/okhttp/DatadogEventListenerFactoryTest.kt

-import com.datadog.android.okhttp.internal.rum.buildResourceId
+import com.datadog.android.okhttp.internal.buildResourceId
 import com.datadog.android.okhttp.utils.config.DatadogSingletonTestConfiguration
+import com.datadog.android.rum.resource.buildResourceId


how does this import work with the other buildResourceId import?

satween changed the title ~~RUM-12635: RUM support for Cronet~~ [WIP] RUM-12635: RUM support for Cronet Nov 27, 2025

satween force-pushed the tvaleev/feature/RUM-12635-wrapper-support branch from 0a842f7 to ec8f689 Compare November 27, 2025 13:51

This comment has been minimized.

Sign in to view

RUM-12635: Adding wrappers

a111d25

satween force-pushed the tvaleev/feature/RUM-12635-wrapper-support branch from ec8f689 to a111d25 Compare November 28, 2025 10:09

satween changed the title ~~[WIP] RUM-12635: RUM support for Cronet~~ RUM-12635: RUM support for Cronet Nov 28, 2025

satween marked this pull request as ready for review November 28, 2025 10:10

satween requested review from a team as code owners November 28, 2025 10:10

ambushwork requested changes Nov 28, 2025

View reviewed changes

0xnm reviewed Dec 1, 2025

View reviewed changes

ambushwork reviewed Dec 1, 2025

View reviewed changes

jonathanmos reviewed Dec 2, 2025

View reviewed changes

	// Exception thrown only for wrong arguments, buy those ones is correct
	// Exception thrown only for wrong arguments, but those ones are correct


		import org.assertj.core.api.Assertions

		class RequestInfoAssertions private constructor(private val info: RequestInfo) {

	class RequestInfoAssertions private constructor(private val info: RequestInfo) {
	class RequestInfoAssert private constructor(private val info: RequestInfo) {

	val requestInfo = finishedInfo.annotations?.firstOrNull { it is RequestInfo } as? RequestInfo
	val requestInfo = finishedInfo.annotations?.filterIsInstance<RequestInfo>()?.firstOrNull()

	val downloadDurationMs = metrics.requestEnd - metrics.responseStart
	val downloadDurationMs = metrics. responseEnd - metrics.responseStart

	private fun Long.toNanos(): Long = TimeUnit.MILLISECONDS.toNanos(this)
	private fun Long.millisToNanos(): Long = TimeUnit.MILLISECONDS.toNanos(this)

	) : UrlRequest.Builder() {
	) : UrlRequest.Builder() by delegate {

		@@ -0,0 +1,127 @@
		package com.datadog.android.cronet.internal

RUM-12635: RUM support for Cronet #3029

Are you sure you want to change the base?

RUM-12635: RUM support for Cronet #3029

Uh oh!

Conversation

satween commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Review checklist (to be filled by reviewers)

Uh oh!

This comment has been minimized.

codecov-commenter commented Nov 27, 2025

Codecov Report

Uh oh!

ambushwork 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

satween commented Nov 27, 2025 •

edited

Loading