Skip to content

RUM-9881: Push context changes from public API to the context thread#2635

Merged
0xnm merged 1 commit into
nogorodnikov/rum-8170/feature-context-syncfrom
nogorodnikov/rum-9881/push-context-changes-to-context-thread
May 12, 2025
Merged

RUM-9881: Push context changes from public API to the context thread#2635
0xnm merged 1 commit into
nogorodnikov/rum-8170/feature-context-syncfrom
nogorodnikov/rum-9881/push-context-changes-to-context-thread

Conversation

@0xnm

@0xnm 0xnm commented May 7, 2025

Copy link
Copy Markdown
Member

What does this PR do?

To align the sequence of context updates/reads caused by the SDK API calls, we need to update context on the context thread only, so that something like this is correct:

Datadog.setUserInfo(A)

// RUM View should get User Info = A and not User Info = B (which may happen in case of concurrency).
RumMonitor.startView(...)

Datadog.setUserInfo(B)

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)

@0xnm
0xnm requested review from a team as code owners May 7, 2025 14:25
@codecov-commenter

codecov-commenter commented May 7, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.41935% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.10%. Comparing base (bce2bbf) to head (7b11e61).
⚠️ Report is 188 commits behind head on nogorodnikov/rum-8170/feature-context-sync.

Files with missing lines Patch % Lines
...n/com/datadog/android/core/internal/DatadogCore.kt 76.67% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@                              Coverage Diff                               @@
##           nogorodnikov/rum-8170/feature-context-sync    #2635      +/-   ##
==============================================================================
- Coverage                                       70.12%   70.10%   -0.02%     
==============================================================================
  Files                                             819      819              
  Lines                                           30590    30615      +25     
  Branches                                         5110     5111       +1     
==============================================================================
+ Hits                                            21450    21460      +10     
- Misses                                           7729     7744      +15     
  Partials                                         1411     1411              
Files with missing lines Coverage Δ
...ore/src/main/kotlin/com/datadog/android/Datadog.kt 71.76% <100.00%> (ø)
...adog/android/core/internal/utils/ConcurrencyExt.kt 100.00% <ø> (ø)
...n/com/datadog/android/core/internal/DatadogCore.kt 81.30% <76.67%> (-1.05%) ⬇️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ambushwork

Copy link
Copy Markdown
Member

is it possible to add unit test to protect against the concurrency issue that you mentioned in the description?

@0xnm
0xnm force-pushed the nogorodnikov/rum-9881/push-context-changes-to-context-thread branch 2 times, most recently from 0ac0371 to f51c7e9 Compare May 7, 2025 15:47
@0xnm
0xnm force-pushed the nogorodnikov/rum-9881/push-context-changes-to-context-thread branch from f51c7e9 to 1082f59 Compare May 12, 2025 10:32
@0xnm
0xnm requested a review from ambushwork May 12, 2025 10:47
@0xnm

0xnm commented May 12, 2025

Copy link
Copy Markdown
Member Author

@ambushwork Good point, totally makes sense. I've added the necessary integration test.

@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented May 12, 2025

Copy link
Copy Markdown

Datadog Summary

✅ Code Quality    ✅ Code Security    ✅ Dependencies    ❌ Pipelines

Next Steps


Was this helpful? Give us feedback!

@0xnm
0xnm force-pushed the nogorodnikov/rum-9881/push-context-changes-to-context-thread branch from 1082f59 to 8ec62e1 Compare May 12, 2025 12:05
@0xnm
0xnm force-pushed the nogorodnikov/rum-9881/push-context-changes-to-context-thread branch from 8ec62e1 to 7b11e61 Compare May 12, 2025 12:07
@0xnm
0xnm requested a review from ambushwork May 12, 2025 12:08
@0xnm
0xnm merged commit a3206f3 into nogorodnikov/rum-8170/feature-context-sync May 12, 2025
@0xnm
0xnm deleted the nogorodnikov/rum-9881/push-context-changes-to-context-thread branch May 12, 2025 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants