-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RUM-3669 avoid feature flag spamming events #1932
RUM-3669 avoid feature flag spamming events #1932
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## release/2.7.0 #1932 +/- ##
=================================================
- Coverage 83.56% 83.36% -0.20%
=================================================
Files 481 481
Lines 17413 17425 +12
Branches 2590 2595 +5
=================================================
- Hits 14550 14526 -24
- Misses 2177 2186 +9
- Partials 686 713 +27
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with the change, we just need to define its shape in terms of the pubilc API and align with iOS, I left an idea regarding that.
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumMonitor.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍. Minor suggestion on API naming, non-blocking.
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumMonitor.kt
Outdated
Show resolved
Hide resolved
...ndroid-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScopeTest.kt
Outdated
Show resolved
Hide resolved
...ndroid-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. regarding the release, I think we will need to release 2.8.0 with buildId
support soon (maybe in 2 weeks?), so maybe this change can be shipped there instead of hotfix.
What does this PR do?
This PR attempts to improve the performance linked to setting FeatureFlag evaluations on a RUM View. Namely:
addFeatureFlagBatchEvaluation()
method allowing to send a batch of feature flags instead of sending them once at a time. This limits the number of event the Rum View Scope has to process, and the number of view updates written on disk.RumViewScope
handling the feature flags updates: instead of writing an event update for every call to theaddFeatureFlagEvaluation()
andaddFeatureFlagBatchEvaluation()
methods, it only writes an event if at least one feature flag value was changed.Motivation
For some customer updating many feature flags at once, that would result in a lot of RawRumEvents being processed, and a lot of RUM View updates being written on disk. In some extreme cases it could lead to a lot of backpressure on the executor queues.