Skip to content

RUMM-2713: Move global RUM context into generic feature context storage#1146

Merged
0xnm merged 2 commits into
feature/sdkv2from
nogorodnikov/rumm-2713/move-away-from-global-rum-context
Nov 17, 2022
Merged

RUMM-2713: Move global RUM context into generic feature context storage#1146
0xnm merged 2 commits into
feature/sdkv2from
nogorodnikov/rumm-2713/move-away-from-global-rum-context

Conversation

@0xnm

@0xnm 0xnm commented Nov 16, 2022

Copy link
Copy Markdown
Member

What does this PR do?

This change removes global RUM context from GlobalRum singleton by moving it to the feature's context (DatadogContext#featuresContext).

Now RUM context will be stored in the generic dictionary Map<String, Any?> and can be accessed either by calling DatadogContext#featuresContext or by calling SdkCore#getFeatureContext.

Also SdkCore#setFeatureContext is removed in the favour of SdkCore#updateFeatureContext method which gives a mutable dictionary for the update, final update is thread-safe.

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 a review from a team as a code owner November 16, 2022 12:53
@0xnm
0xnm force-pushed the nogorodnikov/rumm-2713/move-away-from-global-rum-context branch from 7cb8e6a to 35ccaee Compare November 16, 2022 13:15
@codecov-commenter

codecov-commenter commented Nov 16, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.14085% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.50%. Comparing base (c484bd6) to head (ad1cf8a).

Files with missing lines Patch % Lines
.../kotlin/com/datadog/android/v2/core/DatadogCore.kt 83.33% 0 Missing and 4 partials ⚠️
.../android/rum/internal/domain/scope/RumViewScope.kt 90.32% 1 Missing and 2 partials ⚠️
...otlin/com/datadog/android/tracing/AndroidTracer.kt 71.43% 0 Missing and 2 partials ⚠️
...iew/internal/rum/WebViewRumEventContextProvider.kt 75.00% 0 Missing and 2 partials ⚠️
...c/main/kotlin/com/datadog/android/rum/GlobalRum.kt 0.00% 1 Missing ⚠️
...android/v2/core/internal/DatadogContextProvider.kt 0.00% 0 Missing and 1 partial ⚠️
...og/android/v2/core/internal/NoOpContextProvider.kt 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@                Coverage Diff                @@
##           feature/sdkv2    #1146      +/-   ##
=================================================
+ Coverage          82.43%   82.50%   +0.07%     
=================================================
  Files                350      350              
  Lines              11524    11559      +35     
  Branches            1957     1980      +23     
=================================================
+ Hits                9499     9536      +37     
+ Misses              1424     1419       -5     
- Partials             601      604       +3
Files with missing lines Coverage Δ
...android/log/internal/domain/DatadogLogGenerator.kt 97.32% <100.00%> (+0.93%) ⬆️
...lin/com/datadog/android/rum/internal/RumFeature.kt 93.14% <100.00%> (+0.14%) ⬆️
.../datadog/android/rum/internal/domain/RumContext.kt 97.37% <100.00%> (+7.37%) ⬆️
...droid/rum/internal/domain/scope/RumSessionScope.kt 90.79% <100.00%> (+0.12%) ⬆️
...android/rum/internal/vitals/VitalReaderRunnable.kt 100.00% <100.00%> (ø)
...oid/sessionreplay/internal/SessionReplayFeature.kt 95.92% <100.00%> (ø)
...sionreplay/internal/SessionReplayRecordCallback.kt 100.00% <100.00%> (ø)
...replay/internal/SessionReplayRumContextProvider.kt 100.00% <100.00%> (ø)
...ndroid/telemetry/internal/TelemetryEventHandler.kt 71.67% <100.00%> (+1.08%) ⬆️
...id/webview/internal/log/WebViewLogEventConsumer.kt 86.36% <100.00%> (ø)
... and 9 more

... and 6 files with indirect coverage changes

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

@xgouchet xgouchet added the size-large This PR is large sized label Nov 16, 2022
@0xnm
0xnm force-pushed the nogorodnikov/rumm-2713/move-away-from-global-rum-context branch from 2d356b3 to ad1cf8a Compare November 17, 2022 08:19
mariusc83
mariusc83 reviewed Nov 17, 2022
View reviewed changes
Comment thread dd-sdk-android/src/main/kotlin/com/datadog/android/v2/core/DatadogCore.kt
it.setFeatureContext(featureName, mutableContext)
mutableContext
}
if (featureName == RumFeature.RUM_FEATURE_NAME) {

@mariusc83 mariusc83 Nov 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why are we doing this only for the RUM feature name ?

@0xnm 0xnm Nov 17, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because context which is received by DatadogPlugin contains only RUM-related info, so it makes sense to update it only when RUM context is updated.

Anyway, DatadogPlugin will be removed in v2. This code won't live long.

mariusc83
mariusc83 approved these changes Nov 17, 2022
View reviewed changes

@mariusc83 mariusc83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@0xnm
0xnm merged commit 07c6fe9 into feature/sdkv2 Nov 17, 2022
@0xnm
0xnm deleted the nogorodnikov/rumm-2713/move-away-from-global-rum-context branch November 17, 2022 09:42
@xgouchet xgouchet added this to the 1.16.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mariusc83 mariusc83 mariusc83 approved these changes
+1 more reviewer
@xgouchet xgouchet xgouchet approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size-large This PR is large sized

Projects

None yet

Milestone

1.16.0

Development

Successfully merging this pull request may close these issues.

4 participants

@0xnm @codecov-commenter @xgouchet @mariusc83