Repository navigation
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: b0cd265 | Docs | View more details | Give us feedback! |
7166051 to
83930b2
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83930b2f31
ℹ️ 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".
83930b2 to
c29b60b
Compare
kikoveiga
added
the
ready-for-human-review
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Resources that finish before the first RUM session skip completion bookkeeping, leaving pending counters positive and preventing stopped view/session scopes from completing. Both successful-resource and resource-error paths need the dropped-event notification.
🤖 Bits Code Review · Commit c29b60b
| @Suppress("UnsafeThirdPartyFunctionCall") // currentContext is non-null | ||
| val contextSnapshot = HashMap(currentContext) |
There was a problem hiding this comment.
is it just for an additional guard? onContextUpdate argument for the context is of Map type, so it is not mutable unless receiver tries to cast to the mutable map explicitly (unlikely, I think).
as a price to pay - extra object allocation and iteration to copy.
There was a problem hiding this comment.
Map prevents writes through that reference, but the producer still mutates the underlying object. Session replay retains that reference, for example, so subsequent updates change its previous context too.
There was a problem hiding this comment.
but the producer still mutates the underlying object. Session replay retains that reference, for example, so subsequent updates change its previous context too.
can you point to the code you are referencing? HashMap(currentContext) is also only top-level copy, not the deep one.
There was a problem hiding this comment.
SessionReplayRumContextProvider.onContextUpdate() retains the map with rumContext = context. On the next update, publishRumContext() clears and repopulates that same map before notifying receivers, so previousViewId already contains the new value and the view change is missed.
Yes it's a shallow snapshot. I think it's enough for this issue because the IDs are immutable strings and the producer replaces top-level entries.
| val applicationId: String = NULL_UUID, | ||
| val sessionId: String = NULL_UUID, | ||
| val applicationId: String, | ||
| val sessionId: String? = null, | ||
| val isSessionActive: Boolean = false, | ||
| val viewId: String? = null, | ||
| val viewName: String? = null, | ||
| val viewUrl: String? = null, | ||
| val actionId: String? = null, | ||
| val sessionState: RumSessionScope.State = RumSessionScope.State.NOT_TRACKED, | ||
| val sessionStartReason: RumSessionScope.StartReason = RumSessionScope.StartReason.USER_APP_LAUNCH, | ||
| val sessionState: RumSessionScope.State? = null, | ||
| val sessionStartReason: RumSessionScope.StartReason? = null, |
There was a problem hiding this comment.
what is the benefit of dropping NULL_UUID and RumSessionScope.State.NOT_TRACKED here?
maybe we can extract RumContext into dd-sdk-android-internal, so that all modules can access it directly and use properties/methods specific to this class instead of relying on the generic types (e.g. strings, nulls)?
There was a problem hiding this comment.
Dropping NULL_UUID lets us represent "RUM configured, no session" explicitly: applicationId is still required, while session information is nullable. NOT_TRACKED still describes an existing sampled-out session, while null means no session exists. WDYT?
I agree, we could extract a shared immutable RUM context, I will do it in a follow-up PR.
There was a problem hiding this comment.
I agree, we could extract a shared immutable RUM context, I will do it in a follow-up PR.
Please start it first instead. This change is quote significant, we cannot let it land in develop with a follow-up PRs.
I also don't agree with making most important properties nullable, it feels wrong and brings a lot of changes - this is already surfaced by the number of if you had to introduce in this PR.
| val sessionId = rumContext.sessionId | ||
| if (sessionId == null) { | ||
| sent = true | ||
| return | ||
| } |
There was a problem hiding this comment.
it seems this change is not related and should be (if valid) in a standalone PR?
There was a problem hiding this comment.
This follows from making sessionId nullable, while actions require it. This is just some defensive handling.
| val rulePsr = resourceAttributes.remove(RumAttributes.RULE_PSR) as? Number | ||
|
|
||
| val rumContext = getRumContext() | ||
| val sessionId = rumContext.sessionId |
There was a problem hiding this comment.
Same as my comment above, in the meantime also added the eventDropped call as suggested by Bits.
c29b60b to
476770c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 476770c908
ℹ️ 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".
a86ed21 to
214cf44
Compare
There was a problem hiding this comment.
More details
Application-only context preserves application attribution while session-dependent consumers guard against absent session identifiers. The completed static review identified no reportable regression across context publication, telemetry, logs, tracing, WebView, and replay.
🤖 Bits Code Review · Commit 214cf44
214cf44 to
d15a144
Compare
There was a problem hiding this comment.
More details
Application-only contexts retain application identity while clearing session, view, and action correlation. The static review found no actionable regression in lifecycle handling or affected consumers.
🤖 Bits Code Review · Commit d15a144
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d15a144958
ℹ️ 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".
| val applicationId: String, | ||
| val sessionId: String? = null, |
There was a problem hiding this comment.
Making applicationId mandatory removes the public no-argument constructor (<init>()V) shown as deleted in dd-sdk-android-internal.api; the nullable changes to sessionId, session metadata, and fromFeatureContext also alter the Kotlin source contract. Because dd-sdk-android-internal is a published artifact, a separately versioned consumer compiled against RumContext() will fail with NoSuchMethodError after upgrading this AAR, despite the PR stating that it makes no public API changes. Preserve the existing signatures and introduce the application-only representation through a compatibility-preserving API.
AGENTS.md reference: AGENTS.md:L106-L106
Useful? React with 👍 / 👎.
d15a144 to
50df67a
Compare
50df67a to
4a812cc
Compare
ff79b64 to
8b82ed9
Compare
…sion Preserve the application context and group optional session metadata into a typed snapshot. Migrate readers and fixtures while retaining legacy feature-context fields for cross-platform consumers. Ref: RUM-17009
8b82ed9 to
b0cd265
Compare
What does this PR do?
Preserve application attribution before and between RUM sessions.
RumContextrequires the configuredapplicationId;sessionId,sessionStateandsessionStartReasonare nullable. When no session is available, the monitor publishes an application-only context and clears stale session, view and action correlation.Motivation
RUM-17009: Android PR #3304 cleared the entire RUM context to prevent zero-UUID session correlation in logs and traces. That also removed application attribution when sessions ended. This change preserves application identity while keeping absent session correlation out of events.
The telemetry approach aligns with iOS PR #3095: application identity comes from configuration, independently of session state. Android also updates its shared context model; the iOS PR leaves that model unchanged.
Additional Notes
Targets
develop, which now includes the shared-context extraction (#4019) and reader migration (#4027). This PR contains the lifecycle changes on top of those merged changes.No customer-facing API changes. The SDK-internal
RumContextconstructor and field nullability change. Legacy flat fields remain published for cross-platform compatibility; an application-only snapshot publishes only the application ID alongside the typed context.NULL_UUIDremains for legacy boundaries and required schema fallbacks, but no longer represents an absent session in the internal RUM context.Validation against the latest
develop: Core, Internal, Logs, Profiling, RUM, Session Replay, Trace, WebView and Flags unit tests; Trace and OkHttp JVM integration suites; generated-file checks; and instrumented integration test compilation passed. Formatting and Detekt also passed during rebase validation. Device tests were not run.Review checklist (to be filled by reviewers)