-
Notifications
You must be signed in to change notification settings - Fork 74
RUM-10770: Remove KeepAlive event in RUM #3079
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
base: develop
Are you sure you want to change the base?
Conversation
|
🎯 Code Coverage 🔗 Commit SHA: 0e4edd9 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
b9e9c18 to
1b2198c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3079 +/- ##
===========================================
- Coverage 71.44% 71.37% -0.07%
===========================================
Files 881 881
Lines 32397 32381 -16
Branches 5462 5460 -2
===========================================
- Hits 23144 23111 -33
- Misses 7710 7722 +12
- Partials 1543 1548 +5
🚀 New features to boost your workflow:
|
b7ff1cd to
f3b731b
Compare
f3b731b to
0e4edd9
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| private fun sendViewUpdateToChildren( | ||
| event: RumRawEvent, | ||
| datadogContext: DatadogContext, | ||
| writeScope: EventWriteScope, | ||
| writer: DataWriter<Any> | ||
| ) { | ||
| childrenScopes.forEach { it.sendViewUpdate(event, datadogContext, writeScope, writer) } | ||
| } |
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.
suggestion: let's avoid having this method, because sendViewUpdate was made internal only for tests and we probably don't want to advertise calling this, the main entrypoint is still handleEvent. It is better to inline this at the call site.
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.
this is not only for tests right now, it is also called to update the view event when creating a new foreground view (before it was updated by keep alive).
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.
It is still mainly for tests, because such usage in production code is a small workaround which shouldn't be used in general: there may be some logic between handleEvent and sending view update.
| startForegroundView(event) | ||
| sendViewUpdateToChildren(event, datadogContext, writeScope, writer) |
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 guess this change will trigger zero view duration telemetry
Lines 1361 to 1371 in f7fffa7
| sdkCore.internalLogger.log( | |
| InternalLogger.Level.WARN, | |
| listOf( | |
| InternalLogger.Target.USER, | |
| InternalLogger.Target.TELEMETRY | |
| ), | |
| { ZERO_DURATION_WARNING_MESSAGE.format(Locale.US, key.name) }, | |
| null, | |
| false, | |
| mapOf("view.name" to key.name) | |
| ) |
This is because the time of the scope creation matches event for view update time. And then we will set view duration to 1ns, which may be very puzzling when exploring the telemetry.
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.
is it the case already before the PR? here we try to align with the existing logic
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.
No, here is a minor change of behavior: before it was KeepAlive event which was used to send view update and which is created after the original event used to create view scope, so the duration was always positive. Now it is the same event as it was used to create view scope, so the duration is now zero for such initial view update.
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.
If we remove KeepAlive event and call sendViewUpdate directly, indeed, the view duration becomes 1ns after the creation. But this is somehow aligned to the existing IOS logic after checking with IOS team, because if the view is the last view in the session and it doesn't have any sub events which can update it, we still need to report this view event, and we will use this duration as the time_spent.
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.
If 0 -> 1ns is a legitimate view duration for such case you still have to deal with the logging I mentioned above, because it shouldn't log in this case (especially taking into account that it also logs to telemetry, it will blow up the telemetry volume).
| startForegroundView(event) | ||
| sendViewUpdateToChildren(event, datadogContext, writeScope, writer) |
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.
If 0 -> 1ns is a legitimate view duration for such case you still have to deal with the logging I mentioned above, because it shouldn't log in this case (especially taking into account that it also logs to telemetry, it will blow up the telemetry volume).
What does this PR do?
Main changes for this PR:
KeepAliveinRumRawEventDatadogRumMonitorDue to the removal of
KeepAliveevent, there are some side-effect changes:RumViewScope.sendViewUpdatebecomesinternalsoRumViewManagerScopecan call it directly to update the view.startForegroundViewused to dispatch a keep alive to update the view, now we call sendViewUpdate directly for the same purposeKeepAliveevent to flush the Rum Event into the writer, now we use eitherStopViewto flush them, either we callsendViewUpdatedirectly fromRumViewManagerScopeMotivation
RUM-10770
Review checklist (to be filled by reviewers)