Skip to content

RUM-12891: Fix RUM resource duration breakdown #3010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

RUM-12891: Fix RUM resource duration breakdown #3010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .../dd-sdk-android-okhttp/src/main/kotlin/com/datadog/android/okhttp/DatadogEventListener.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ package com.datadog.android.okhttp
import com.datadog.android.api.InternalLogger
import com.datadog.android.api.SdkCore
import com.datadog.android.core.SdkReference
import com.datadog.android.okhttp.DatadogEventListener.Factory
import com.datadog.android.okhttp.internal.rum.buildResourceId
import com.datadog.android.rum.GlobalRumMonitor
import com.datadog.android.rum.internal.domain.event.ResourceTiming
Expand Down Expand Up @@ -244,7 +243,7 @@ internal constructor(

/** @inheritdoc */
override fun create(call: Call): EventListener {
val resourceId = call.request().buildResourceId(generateUuid = true)
val resourceId = call.request().buildResourceId(generateUuid = false)
Copy link
Member

@0xnm 0xnm Nov 18, 2025

Choose a reason for hiding this comment

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

I don't see how this fixes the issue given that under the hood it will first try to pull existing UUID from the Request.tag, which should already be there if call to interceptor was made before.

Can you please give more details?

Copy link
Member Author

@ambushwork ambushwork Nov 18, 2025

Choose a reason for hiding this comment

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

that's because the UUID we generate from DatadogIntercptor is not put inside Request.tag, so here if we try to retrieve tag(UUID::class.java) it will always return null. The UUID is only used as a RUM resource key.

Copy link
Member

@0xnm 0xnm Nov 18, 2025

Choose a reason for hiding this comment

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

that's because the UUID we generate from DatadogInterceptor is not put inside Request.tag

This is probably a real cause of the issue, it should be there and it is a miss from our side.

Basically we always should rely on the UUID, because the legacy way to identify the request is not reliable: say there are multiple GET calls in parallel with the same URL - they will be identified as the same call. Same for multiple POST calls with the same body length.

So when we generate UUID we should put it to the request as tag as well.

And we should add the tests to the reliability/single-fit-okhttp that for a call made with instrumentation setup startResource / stopResource/waitForResourceTiming/addResourceTiming all have the same key argument.

Copy link
Member Author

@ambushwork ambushwork Nov 18, 2025

Choose a reason for hiding this comment

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

OK, I see the issue, then this is a bit tricky to handle with our current implementation, because create in DatadogEventListener will be called first, the call is immutable inside, meaning that we are not able to pass our resource id along with the request to DatadogInterceptor

Copy link
Member

@0xnm 0xnm Nov 18, 2025

Choose a reason for hiding this comment

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

At the same time we have an access to the Call object in all the callbacks of EventListener, meaning we can read the actual state of request to get the ID in each callback instead of passing it at the DatadogEventListener constructor, probably it will still get consistent results for the same call.

I'm not sure at which point callStart is called, but if it is after intercept, then we should be good.

Overall, we shouldn't indeed mutate anything here as per the following comment.

Copy link
Member Author

@ambushwork ambushwork Nov 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

callStart also happens before intercept, and other functions are not systematically called after intercept neither, we will need to dig another way out.

val sdkCore = sdkCoreReference.get()
return if (sdkCore != null) {
DatadogEventListener(sdkCore, resourceId)
Expand Down
10 changes: 10 additions & 0 deletions ...roid-okhttp/src/test/kotlin/com/datadog/android/okhttp/DatadogEventListenerFactoryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,16 @@ internal class DatadogEventListenerFactoryTest {
assertThat(result).isSameAs(DatadogEventListener.Factory.NO_OP_EVENT_LISTENER)
}

@Test
fun `M not generate UUID in the resource key W create()`() {
// When
val result = testedFactory.create(mockCall)

// Then
check(result is DatadogEventListener)
assertThat(result.key.uuid).isEqualTo(null)
}

companion object {
val datadogCore = DatadogSingletonTestConfiguration()

Expand Down
Loading