-
Notifications
You must be signed in to change notification settings - Fork 133
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-5248 feat: send memory warning as RUM error #1955
Conversation
eb6a253
to
cd9c78d
Compare
cd9c78d
to
3f527c3
Compare
3f527c3
to
d03a5bf
Compare
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 great 🎯 🏅 with minor comments. Well done!
CHANGELOG.md
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
- [FEATURE] Enable DatadogCore, DatadogLogs and DatadogTrace to compile on watchOS platform. See [#1918][] (Thanks [@jfiser-paylocity][]) [#1946][] | |||
- [IMPROVEMENT] Ability to clear feature data storage using `clearAllData` API. See [#1940][] | |||
- [IMPROVEMENT] Send memory warning as RUM error. |
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.
change-request/ We're missing PR ID here, making it harder to combine release notes when shipping next release.
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.
Fixed.
CHANGELOG.md
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
- [FEATURE] Enable DatadogCore, DatadogLogs and DatadogTrace to compile on watchOS platform. See [#1918][] (Thanks [@jfiser-paylocity][]) [#1946][] | |||
- [IMPROVEMENT] Ability to clear feature data storage using `clearAllData` API. See [#1940][] | |||
- [IMPROVEMENT] Send memory warning as RUM error. |
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.
Fixed.
What and why?
memory warnings are not reported, with WT now in place these warnings can be helpful to get bigger picture and root cause the WT.
How?
Initial plan was to setup a context observer which will listen to memory warnings but the current dependency management around backtrace tracking doesn't allow generating a backtrace in DatadogContextProvider. This would have been a generic solution where any target can listen to memory warnings but had to drop this idea.
Instead, I added
MemoryWarningMonitor
which monitors the memory warnings and reports them when they occur.Reporting part is kept outside the monitor to keep the monitor simple and testable.
Model update DataDog/rum-events-format#215
PS: I had the context observer working property basis which can be retrieved from cd9c78d if needed in future.
Review checklist
Custom CI job configuration (optional)