-
Notifications
You must be signed in to change notification settings - Fork 136
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
feat: Support eventMapper on crashes for logs #1741
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 2633 Passed, 0 Skipped, 23m 22.93s Wall Time 🔻 Code Coverage Decreases vs Default Branch (6)
|
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 👍 would be convenient to default the mapper to nil
in the crash receiver, WDYT?
let dateProvider: DateProvider | ||
let logEventMapper: LogEventMapper? |
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.
/siggestion For convenience, we can have an init
that default the mapper to nil
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.
Since this in internal, I thought it would be good to enforce adding it / explicitly setting it to nil. I can add the default if you disagree.
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 also prefer being explicit. This way compiler will guide us on all the places where MessageReceiver
is used and will make us think of injecting mapper where it makes sense. If we seek for testing convenience, it is safer to add it through extension in test module.
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, but please add CHANGELOG
entry 🙂
Added the changelog item for Log mapper being called for crashes.
What and why?
This adds a call to the configured log event mapper when a crash is reported to the Logs feature. It's tested in the integration test app by adding a fingerprint to the reported crash.
refs: RUM-2209
Review checklist
Custom CI job configuration (optional)
tools/