Skip to content

RUM-10489: Make attributes argument optional in the event-related methods of RUM monitor#2760

Merged
0xnm merged 1 commit into
feature/v3from
nogorodnikov/rum-10489/make-attributes-arg-optional-in-rummonitor
Jun 25, 2025
Merged

RUM-10489: Make attributes argument optional in the event-related methods of RUM monitor#2760
0xnm merged 1 commit into
feature/v3from
nogorodnikov/rum-10489/make-attributes-arg-optional-in-rummonitor

Conversation

@0xnm

@0xnm 0xnm commented Jun 24, 2025

Copy link
Copy Markdown
Member

What does this PR do?

If we compare RUM monitor-related methods in iOS SDK with ones in Android SDK, we will see that Android SDK has inconsistency: some event-related methods have attributes as optional argument, some don’t.

We need to align with iOS SDK and make attributes argument for such methods optional.

This PR also removes redundant emptyMap argument value at the call sites, but keeps it at the verify calls.

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 June 24, 2025 16:54
@0xnm
0xnm force-pushed the nogorodnikov/rum-10489/make-attributes-arg-optional-in-rummonitor branch from 0003743 to ee8a6ec Compare June 24, 2025 17:05
@codecov-commenter

codecov-commenter commented Jun 24, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.58%. Comparing base (809abcb) to head (ee8a6ec).
⚠️ Report is 332 commits behind head on feature/v3.

Files with missing lines Patch % Lines
.../main/kotlin/com/datadog/android/rum/RumMonitor.kt 60.00% 2 Missing ⚠️
.../rum/internal/monitor/AdvancedNetworkRumMonitor.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           feature/v3    #2760      +/-   ##
==============================================
+ Coverage       67.53%   67.58%   +0.05%     
==============================================
  Files             818      818              
  Lines           30638    30642       +4     
  Branches         5179     5179              
==============================================
+ Hits            20691    20709      +18     
+ Misses           8551     8543       -8     
+ Partials         1396     1390       -6     
Files with missing lines Coverage Δ
.../instrumentation/gestures/WindowCallbackWrapper.kt 84.34% <100.00%> (ø)
...og/android/rum/internal/utils/CloasableErrorExt.kt 100.00% <100.00%> (ø)
...dog/android/rum/resource/RumResourceInputStream.kt 90.77% <100.00%> (-0.14%) ⬇️
...id/compose/internal/InternalInteractionTracking.kt 91.35% <100.00%> (-0.08%) ⬇️
...droid/glide/DatadogRUMUncaughtThrowableStrategy.kt 46.67% <100.00%> (ø)
...tlin/com/datadog/android/rum/coroutines/FlowExt.kt 90.91% <100.00%> (-0.76%) ⬇️
.../com/datadog/android/rx/DatadogRumErrorConsumer.kt 100.00% <100.00%> (ø)
.../rum/internal/monitor/AdvancedNetworkRumMonitor.kt 0.00% <0.00%> (ø)
.../main/kotlin/com/datadog/android/rum/RumMonitor.kt 54.55% <60.00%> (+21.21%) ⬆️

... and 39 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
ambushwork previously approved these changes Jun 25, 2025

@ambushwork ambushwork 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, just curious why don't target this branch to develop?

@0xnm

0xnm commented Jun 25, 2025

Copy link
Copy Markdown
Member Author

It is possible to target develop, yes, because there is no binary-breaking change. But targeting v3 branch instead just to have this mention in the general v3 overview doc (this can be omitted though).

@ambushwork

Copy link
Copy Markdown
Member

so it should target to feature/v3 instead of nogorodnikov/remove-default-value-for-addAccountExtraInfo-call?

@0xnm

0xnm commented Jun 25, 2025

Copy link
Copy Markdown
Member Author

It will, once underlying PR is merged. It is just a stacked PR.

mariusc83
mariusc83 previously approved these changes Jun 25, 2025
Base automatically changed from nogorodnikov/remove-default-value-for-addAccountExtraInfo-call to feature/v3 June 25, 2025 09:30
@0xnm
0xnm dismissed stale reviews from mariusc83 and ambushwork June 25, 2025 09:30

The base branch was changed.

@0xnm
0xnm requested review from ambushwork and mariusc83 June 25, 2025 09:30
@0xnm
0xnm merged commit 3337e12 into feature/v3 Jun 25, 2025
24 of 25 checks passed
@0xnm
0xnm deleted the nogorodnikov/rum-10489/make-attributes-arg-optional-in-rummonitor branch June 25, 2025 15:00
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.

4 participants