RUM-14843: Set active span ID as RUM resource parent span ID.#2726
Conversation
8f0465a to
233f846
Compare
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: a0a81e2 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
gonzalezreal
left a comment
There was a problem hiding this comment.
👌🏽Great work! I think the approach is clean and fits well with the existing architecture.
As a suggestion, consider adding an integration test (in this PR or a follow-up) that enables both RUM and Trace, starts an active span, fires a request within that span's scope, and asserts the RUM resource carries the active span's trace/span IDs and sampling priority. The unit tests cover the logic, but an integration test would validate the full wiring end-to-end, from Trace.enable() registering the ActiveSpanProviderAdditionalContext, through ActiveSpanProviderReceiver picking it up, to DistributedTracing using it. The existing infrastructure (HTTPServerMock, RUMSessionMatcher, SpanMatcher) should make this possible.
|
I just pushed a new commit with Integration Tests. |
This commit adds the implementation of this functionality: - A new mechanism was created for RUM to access the current active span from the Trace feature, if any. - If an active span exists, RUM will use its trace ID, and the span ID as the parent span ID for the resource augmented info. This allows the backend to attach the APM span synthesized from the RUM resource to the trace on the APM side, instead of creating a root span. It also ensures RUM propagates the correct trace ID headers so the trace is fully assembled on the backend.
…e previous commit. For improved testability, `ActiveSpanProvider` and `ActiveSpanProviderReceiver` were abstracted by protocols so mocks can be used.
The priority is used exclusively for the conversion from RUM resource event to span on the backend (the RUM event itself is always sampled as long as the session itself is sampled). This commit makes the active span override the RUM resource sampler priority. This way, all the trace spans (including the one generated from RUM) will have the same priority, so either will all be sampled, or none will.
…ass. This worked somewhat accidentally because of how the ReadWriteLock is implemented, but it was conceptually wrong, since we need to refer to the same instance from different places.
Since more message receivers need to listen to context changes, `ActiveSpanProviderReceiver` must return false from the receive(…) method.
8e54fc0 to
9634607
Compare
simaoseica-dd
left a comment
There was a problem hiding this comment.
Left some comments since I see some room for improvement on the architecture.
Also, please add this description point to the RUM.Configuration since it changes behavior.
This is ignored if there is an active span. It's still used otherwise.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dfc295d97
ℹ️ 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".
- Remove ActiveSpanProviderReceiver, and use NetworkContextProvider instead; - Move ActiveSpanProvider to TraceCoreContext; - Fix wrong sampling rate source.
What and why?
Currently, when
URLSessionrequests are instrumented by the RUM feature (through the use of theRUM.Configuration.urlSessionTrackingconfiguration option) to generate a RUM Resource, that resource is augmented with a trace and span ID, both generated on the fly, and later used by the backend to synthesize an APM trace span from the RUM resource using those IDs. This is not ideal since that span will be detached from any other trace, breaking the continuity of the generated distributed trace.This PR links both features: if there is a currently active span for the relevant activity, RUM will use it for augmenting the RUM resource. Specifically:
The use of the active span's priority and decision mechanism implies the
sampleRatepart of theFirstPartyHostsTracingargument ofurlSessionTrackingwill be ignored if there is an active span. As an example:As a reminder, even if the active span is not sampled, the RUM resource itself will be sampled if the RUM session is sampled. The only thing the active span sampling controls is if Datadog backend generates the APM trace span out of the RUM resource, or not.
How?
Trace feature initialization adds an instance of
ActiveSpanProvideras additional context to the core. This entity contains a provider function that is used to dynamically obtain the active span.The RUM feature creates an
ActiveSpanProviderReceiverand registers it as a feature message receiver. When the context is updated, this receiver obtains theActiveSpanProviderfrom the context and stores it. The instance ofActiveSpanProviderReceiveris also placed on theDistributedTracinginstance, allowingURLSessionRUMResourcesHandlerto finally obtain the active span and properly augment the RUM resource with it.Notes
parent-idkey. However, this key expects the RUM resource span ID, not its parent, the active span ID. This is confusing since the other header formats (Datadog and B3) both have explicit fields for the span and parent span IDs, implying the meaning of "parent" ID is different in the W3C case. This, however, makes sense, since the RUM resource span ID will be used as the parent span for any span created in the customers' backends.Review checklist
make api-surfacewhen adding new APIs