Skip to content

RUM-17009: Extract shared RUM context into internal module - #4019

Merged
kikoveiga merged 1 commit into
developfrom
kikoveiga/RUM-17009/extract-shared-rum-context
Oct 23, 2026
Merged

kikoveiga merged 1 commit into
developfrom
kikoveiga/RUM-17009/extract-shared-rum-context

Conversation

@kikoveiga

Copy link
Copy Markdown
Collaborator

What does this PR do?

This PR moves RumContext and its supporting enums into dd-sdk-android-internal. This is part of the ongoing work in #3970 and after this comment.

The idea is to keep it simple and have no behavior change here.

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)

@datadog-datadog-us1-prod

datadog-datadog-us1-prod Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Pipelines  Tests

⏳ PR checks are still running. This summary will update when all results arrive.

🎉 All green!

🧪 All tests passed

🔄 Datadog retried 1 test - 1 passed on retry View in Datadog

🎯 Code Coverage (details)
• Patch Coverage: 67.66%
• Overall Coverage: 71.72% (-0.10%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 83b1446 | Docs | View more details | Give us feedback!

@kikoveiga
kikoveiga force-pushed the kikoveiga/RUM-17009/extract-shared-rum-context branch 2 times, most recently from 1d1e201 to 049b79c Compare October 21, 2026 14:17
@kikoveiga
kikoveiga marked this pull request as ready for review October 21, 2026 14:17
@kikoveiga
kikoveiga requested review from a team as code owners October 21, 2026 14:17
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-23T10:26:44.728433Z 83b1446 New commits
🔒 Security Review 🔄 Running since 2026-10-23T10:24:15.563542Z 83b1446 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 049b79c39b

ℹ️ 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".

@datadog-datadog-us1-prod datadog-datadog-us1-prod Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bits Code Review: PASS

More details

Existing feature-context maps remain compatible: serialized keys, defaults, and enum encodings are preserved across the module move. The completed static review found no actionable regressions in the updated consumers.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit 049b79c

@kikoveiga
kikoveiga added this pull request to stack #4023 October 21, 2026 14:42
@kikoveiga kikoveiga added the ready-for-human-review All agentic review feedback was addressed and PR is ready for the human review. label Oct 21, 2026
ambushwork
ambushwork previously approved these changes Oct 22, 2026
Comment thread dd-sdk-android-internal/api/apiSurface Outdated
@kikoveiga
kikoveiga force-pushed the kikoveiga/RUM-17009/extract-shared-rum-context branch 2 times, most recently from f0472db to 81501e9 Compare October 22, 2026 09:51

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 81501e9f38

ℹ️ 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".

@kikoveiga
kikoveiga removed this pull request from stack #4023 October 22, 2026 10:52
@kikoveiga
kikoveiga added this pull request to stack #4028 October 22, 2026 10:55
@satween

satween commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Hey @0xnm @kikoveiga! I have some doubts about these changes 🙂

Right now the PR exposes the full internal state of the RUM scopes: 17 fields, including viewTimestampOffset, hasTimeseries, sessionStartReason, isSessionActive and sessionSampleRate. On top of that:

Meanwhile, judging by the diff, the other features only use applicationId, sessionId, viewId, actionId and maybe sessionState.

What I'd suggest:

  • Keep the full RumContext internal to the RUM module.
  • Expose through -internal only a narrow snapshot meant for other features, e.g. CrossFeatureRumContext, with just the fields they actually need. We already have something similar in ProfilingRumContext, which could be replaced by this one.
  • Prefer a regular class, or something else that gives us explicit control over the API, over a data class.

This would keep -internal from turning into a shared bucket without needing a big architectural refactor, and it should make #3970 simpler too.

What do you think?

@0xnm

0xnm commented Oct 22, 2026

Copy link
Copy Markdown
Member

@satween keeping two different classes - one for the shared context, another is internal for RUM, is also an option. Some enums should go to -internal though, to avoid duplication.

@kikoveiga

Copy link
Copy Markdown
Collaborator Author

Hi @satween and cc @0xnm.

Thanks for the review! I'd lean toward keeping one shared RumContext for now. All 17 fields were already published through the flat feature-context map, we're just making that existing snapshot typed. It's true this brings stronger coupling to the class.

The migrated readers currently use 11 fields. Splitting would duplicate those properties and require a mapping. We can always revisit this and separate if needed in the future.

For the sessionId nullability change, I see this as a benefit: consumers need to handle an absent session, and the shared type makes the compiler enforce that. With the previous flat map, the same semantic change could silently break readers.

@kikoveiga
kikoveiga requested a review from 0xnm October 22, 2026 16:54
@kikoveiga
kikoveiga force-pushed the kikoveiga/RUM-17009/extract-shared-rum-context branch from 81501e9 to 0bbc151 Compare October 23, 2026 08:41
@0xnm

0xnm commented Oct 23, 2026

Copy link
Copy Markdown
Member

For the sessionId nullability change, I see this as a benefit: consumers need to handle an absent session, and the shared type makes the compiler enforce that.

Shouldn't session state enum be a decision maker rather that session ID?

0xnm
0xnm previously approved these changes Oct 23, 2026
@kikoveiga
kikoveiga force-pushed the kikoveiga/RUM-17009/extract-shared-rum-context branch from 0bbc151 to 83b1446 Compare October 23, 2026 10:24
@kikoveiga

Copy link
Copy Markdown
Collaborator Author

For the sessionId nullability change, I see this as a benefit: consumers need to handle an absent session, and the shared type makes the compiler enforce that.

Shouldn't session state enum be a decision maker rather that session ID?

@0xnm The enum should decide whether a session is tracked, the nullable ID indicates whether a session exists. Currently, NOT_TRACKED covers both the initial state before session creation and an existing sampled-out session, so it can't distinguish those cases.

@0xnm

0xnm commented Oct 23, 2026

Copy link
Copy Markdown
Member

@kikoveiga we can extend enum values if we want to

@kikoveiga
kikoveiga merged commit 87d2fab into develop Oct 23, 2026
28 checks passed
@kikoveiga
kikoveiga deleted the kikoveiga/RUM-17009/extract-shared-rum-context branch October 23, 2026 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human-review All agentic review feedback was addressed and PR is ready for the human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants