RUM-13568: [Cronet]: Supporting RUM <> APM integration for Cronet#3136
RUM-13568: [Cronet]: Supporting RUM <> APM integration for Cronet#3136
Conversation
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ee7daf6e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| override fun addHeader(header: String, value: String?): UrlRequest.Builder = apply { | ||
| headers[header] = listOfNotNull(value) | ||
| delegate.addHeader(header, value) | ||
| val currentValues = headers[header]?.toMutableList() ?: mutableListOf() | ||
| if (value != null) { | ||
| currentValues.add(value) | ||
| } | ||
| headers[header] = currentValues |
There was a problem hiding this comment.
Forward added headers to the Cronet delegate
DatadogUrlRequestBuilder.addHeader() now only updates the local headers map and never forwards to delegate.addHeader. Later, buildCronetRequest() calls addHeaders(headers), which invokes this same override again, so the delegate builder never receives any headers. This means every Cronet request built through DatadogCronetEngine will drop user-supplied headers and tracing headers, breaking authentication and trace propagation. Consider forwarding to delegate.addHeader (or ensuring addHeaders writes to the delegate) so the actual request carries the headers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Interesting catch, but the problem was bit deeper, fixed
21d8777 to
3db3a62
Compare
This comment has been minimized.
This comment has been minimized.
dd-sdk-android-internal/src/main/java/com/datadog/android/internal/utils/LangExt.kt
Outdated
Show resolved
Hide resolved
...droid-rum/src/main/kotlin/com/datadog/android/rum/internal/net/RumResourceInstrumentation.kt
Outdated
Show resolved
Hide resolved
...ndroid-trace/src/main/kotlin/com/datadog/android/trace/internal/ApmNetworkInstrumentation.kt
Show resolved
Hide resolved
| private val tracedHosts = listOf( | ||
| "datadoghq.com", | ||
| "127.0.0.1", | ||
| "storage.googleapis.com" |
There was a problem hiding this comment.
is it added for some validation? we don't own this host, so sending trace context there is useless
There was a problem hiding this comment.
it's added in order to check that cronet is working with QUIC, RUM <> APM integration will not work for this host, but I want to keep this for sample app in order to send spans for it ( and check that APM is working with QUICK)
...e/src/main/kotlin/com/datadog/android/api/instrumentation/network/HttpRequestInfoModifier.kt
Outdated
Show resolved
Hide resolved
dd-sdk-android-internal/src/main/java/com/datadog/android/internal/utils/LangExt.kt
Outdated
Show resolved
Hide resolved
| return builder.setInsightsCollector(insightsCollector) | ||
| } | ||
|
|
||
| fun RumResourceInstrumentationConfiguration?.build(name: String): RumResourceInstrumentation? = |
There was a problem hiding this comment.
why is it a part of _RumInternalProxy? how it is supposed to be called?
There was a problem hiding this comment.
I want to hide build method from users. This made for several reasons:
- build() - basically useless method, it does not add any information to the builder
- Integrations (
CronetandOkHttp) should set their names into the instrumentation.
how it is supposed to be called?
val rumResourceInstrumentation = with(_RumInternalProxy) {
rumInstrumentationConfiguration.build(CRONET_NETWORK_INSTRUMENTATION_NAME)
}There was a problem hiding this comment.
I'm wondering how it looks like in a java bytecode. Are you sure that rumInstrumentationConfiguration.build(CRONET_NETWORK_INSTRUMENTATION_NAME) call is not possible outside of _RumInternalProxy receiver scope?
First time I see such trick given that visibility of extension function is public.
There was a problem hiding this comment.
After several experiments with api calls - I decided to move back to more classic approach, as in current state build method is still visible for customers but they cannot call it which could be confusing
...-android-okhttp/src/main/kotlin/com/datadog/android/okhttp/internal/OkHttpHttpRequestInfo.kt
Outdated
Show resolved
Hide resolved
...id-cronet/src/main/kotlin/com/datadog/android/cronet/internal/DatadogCronetRequestContext.kt
Outdated
Show resolved
Hide resolved
...id-cronet/src/main/kotlin/com/datadog/android/cronet/internal/DatadogCronetRequestContext.kt
Show resolved
Hide resolved
4ef7d94 to
6c726f5
Compare
6c726f5 to
0e81151
Compare
0xnm
left a comment
There was a problem hiding this comment.
I partially covered this PR, but will continue review later.
...re/src/main/kotlin/com/datadog/android/api/instrumentation/network/HttpRequestInfoBuilder.kt
Outdated
Show resolved
Hide resolved
...re/src/main/kotlin/com/datadog/android/api/instrumentation/network/HttpRequestInfoBuilder.kt
Show resolved
Hide resolved
| return builder.setInsightsCollector(insightsCollector) | ||
| } | ||
|
|
||
| fun RumResourceInstrumentationConfiguration?.build(name: String): RumResourceInstrumentation? = |
There was a problem hiding this comment.
I'm wondering how it looks like in a java bytecode. Are you sure that rumInstrumentationConfiguration.build(CRONET_NETWORK_INSTRUMENTATION_NAME) call is not possible outside of _RumInternalProxy receiver scope?
First time I see such trick given that visibility of extension function is public.
...id-trace/src/main/kotlin/com/datadog/android/trace/ApmNetworkInstrumentationConfiguration.kt
Show resolved
Hide resolved
...ndroid-trace/src/main/kotlin/com/datadog/android/trace/internal/ApmNetworkInstrumentation.kt
Outdated
Show resolved
Hide resolved
...ndroid-trace/src/main/kotlin/com/datadog/android/trace/internal/ApmNetworkInstrumentation.kt
Outdated
Show resolved
Hide resolved
...ndroid-trace/src/main/kotlin/com/datadog/android/trace/internal/ApmNetworkInstrumentation.kt
Outdated
Show resolved
Hide resolved
...ndroid-trace/src/main/kotlin/com/datadog/android/trace/internal/ApmNetworkInstrumentation.kt
Outdated
Show resolved
Hide resolved
...id-cronet/src/main/kotlin/com/datadog/android/cronet/internal/DatadogCronetRequestContext.kt
Outdated
Show resolved
Hide resolved
...ndroid-trace/src/main/kotlin/com/datadog/android/trace/internal/ApmNetworkInstrumentation.kt
Outdated
Show resolved
Hide resolved
...ndroid-trace/src/main/kotlin/com/datadog/android/trace/internal/ApmNetworkInstrumentation.kt
Outdated
Show resolved
Hide resolved
...d-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/net/TracerProvider.kt
Show resolved
Hide resolved
| private val localTracerReference: AtomicReference<DatadogTracer> = AtomicReference() | ||
|
|
||
| @Synchronized | ||
| @Suppress("UnsafeThirdPartyFunctionCall") // updateAndGet is safe |
There was a problem hiding this comment.
there is no updateAndGet usage though
...d-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/net/TracerProvider.kt
Outdated
Show resolved
Hide resolved
...d-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/net/TracerProvider.kt
Outdated
Show resolved
Hide resolved
...d-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/net/TracerProvider.kt
Outdated
Show resolved
Hide resolved
0e81151 to
f636bd2
Compare
| val result = with(_RumInternalProxy.Companion) { | ||
| builder.createInstrumentation(fakeInstrumentationName) | ||
| } |
| fun setTraceSampler(com.datadog.android.core.sampling.Sampler<com.datadog.android.trace.api.span.DatadogSpan>) | ||
| fun setTraceContextInjection(TraceContextInjection) | ||
| fun set404ResourcesRedacted(Boolean) | ||
| fun setTracingScope(ApmNetworkTracingScope) |
There was a problem hiding this comment.
very nit: tracing vs trace above, but maybe it is okay if we think of tracing as a process, not as of trace as item
| * Sets the tracing scope for network instrumentation. | ||
| * | ||
| * This controls how detailed the tracing will be: | ||
| * - [ApmNetworkTracingScope.DETAILED]: Traces both application-level requests and internal |
There was a problem hiding this comment.
maybe ALL is a better name? Also this separation applies to OkHttp only, does Cronet also have similar?
|
|
||
| /** | ||
| * Only application level request is gonna be traced. | ||
| * In this mode the Datadog SDK still able to link trace spans to Rum.Resources making possible to navigate |
There was a problem hiding this comment.
| * In this mode the Datadog SDK still able to link trace spans to Rum.Resources making possible to navigate | |
| * In this mode the Datadog SDK still able to link trace spans to RUM Resources making possible to navigate |
| "onRequestIntercepted callback, leading to infinite recursion." | ||
|
|
||
| internal const val ERROR_REQUEST_INFO_IS_NOT_MUTABLE = | ||
| "RequestInfo is not mutable. Your requests won't be traced." |
| val builder = ApmNetworkInstrumentationConfiguration(fakeTracedHosts) | ||
|
|
||
| // When | ||
| val result: ApmNetworkInstrumentation = with(DatadogTracingToolkit) { |
| @Test | ||
| fun `M return false W isExtractedContext() {non-extracted DatadogSpanContextAdapter}`() { | ||
| // Given | ||
| val mockAgentSpanContext: com.datadog.trace.bootstrap.instrumentation.api.AgentSpan.Context = mock() |
There was a problem hiding this comment.
| val mockAgentSpanContext: com.datadog.trace.bootstrap.instrumentation.api.AgentSpan.Context = mock() | |
| val mockAgentSpanContext = mock<AgentSpan.Context>() |
| any< | ||
| ( | ||
| HttpRequestInfo, | ||
| (String, String) -> Boolean | ||
| ) -> Unit | ||
| >() |
There was a problem hiding this comment.
nit: seems like formatting gone wrong
| @OptIn(ExperimentalRumApi::class) | ||
| @Test | ||
| fun `M propagate RumResourceAttributesProvider W setRumResourceAttributesProvider()`() { | ||
| fun `M propagate RumResourceAttributesProvider W setCustomRumInstrumentation()`() { |
| @OptIn(ExperimentalRumApi::class) | ||
| @Test | ||
| fun `M propagate sdkInstanceName W setSdkInstanceName()`(@StringForgery sdkInstanceName: String) { | ||
| fun `M propagate sdkInstanceName W setCustomRumInstrumentation()`(@StringForgery sdkInstanceName: String) { |
What does this PR do?
In this PR we are copying the main logic from
TracingInterceptor/DatadogInterceptorinto theNetworkTracingInstrumentationin order to:CronetMotivation
Continuing to add an integration for
Cronetlibrary meanwhile preparing infrastructure to migrateOkHttponto the same logic.Additional Notes
Note that in this PR we are not supporting deep network instrumentation that is being done by
TracingInterceptorinOkHttp. This logic will be supported in a further PR as this one is already pretty big.NOTE: In this PR as an experiment an AI agent helped to write tests and some docs. I've made few iterations of a fixes for that but if you spot any issues - let me know. AI wasn't involved into production code development.
Review checklist (to be filled by reviewers)