feat: Allow crash reporting to use event mappers #1742

fuzzybinary · 2024-03-21T18:27:43Z

What and why?

CrashReportReceiver now supports calling both the view and event mapper before writing the crash to storage.

refs: RUM-2206

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests for Core, RUM, Trace, Logs, CR and WVT
Run unit tests for Session Replay
Run integration tests
Run smoke tests
Run tests for tools/

CrashReportReceiver now supports calling both the view and event mapper before writing the crash to storage. refs: RUM-2206

ncreated

Looks great 👍. I left one suggestion, non-blocking. LMK what we decide on the possibility of dropping crashes from mapper.

Also, please update CHANGELOG.md 🙏

DatadogCore/Tests/Datadog/RUM/Integrations/CrashReportReceiverTests.swift

This is partially to prevent users from accidentally dropping fatal errors in their mappers, and partially to be consistent with Android.

datadog-datadog-prod-us1 · 2024-03-27T13:58:48Z

Datadog Report

Branch report: jward/RUM-2206-rum-crash-event-mappers
Commit report: 7d0ee48
Test service: dd-sdk-ios

✅ 0 Failed, 2699 Passed, 0 Skipped, 22m 51.24s Wall Time
🔻 Test Sessions change in coverage: 12 decreased, 2 increased

🔻 Code Coverage Decreases vs Default Branch (12)

This report shows up to 5 code coverage decreases.

test DatadogInternalTests iOS 79.08% (-0.67%) - Details
test DatadogInternalTests tvOS 79.06% (-0.65%) - Details
test DatadogTraceTests iOS 49.04% (-0.59%) - Details
test DatadogRUMTests iOS 79.22% (-0.49%) - Details
test DatadogTraceTests tvOS 49.26% (-0.48%) - Details

ncreated

🎯 Well integrated 🔗. I left two minor comments.

ncreated · 2024-03-27T19:01:01Z

CHANGELOG.md

+* [FEATURE] Call RUM's `errorEventMapper` for crashes.
 * [FEATURE] Support calling log event mapper for crashes.


nit/ Let's add PR references to both to be consistent with the rest of this file. It helps when generating release notes and later when tracking changes.

ncreated · 2024-03-27T19:06:50Z

DatadogRUM/Sources/Integrations/CrashReportReceiver.swift

-                writer.write(value: rumError)
+                if let mappedError = self.eventsMapper.map(event: rumError) {
+                    writer.write(value: mappedError)
+                }


question/ Why do we allow dropping crash in this mapper call? Seems inconsistent with the next crash write in this file. Is it because in this case it goes without corresponding view event? If it's on purpose, I think it may make sense to add code comment on this decision.

No it's not on purpose. I'll make sure the test covers this and fix.

ncreated

@fuzzybinary I updated this branch with git conflicts resolution after my merge of #1750. It should be all fine now 👍.

The notable change is that from #1750 all RUM components (including CrashReportReceiver) depend on FeatureScope instead of DatadogCoreProtocol. That removes the use of PassthroughCoreMock in all RUM unit tests, replacing them with simpler FeatureScopeMock. This achieves goals outlined in #1744

# Conflicts: # DatadogCore/Tests/Datadog/Mocks/RUMFeatureMocks.swift # DatadogRUM/Sources/Feature/RUMFeature.swift # DatadogRUM/Sources/Integrations/CrashReportReceiver.swift # DatadogRUM/Tests/Mocks/RUMFeatureMocks.swift

ncreated · 2024-03-28T17:18:41Z

DatadogCore/Tests/Datadog/RUM/Integrations/CrashReportReceiverTests.swift

+            receiver.receive(message: .baggage(
+                key: MessageBusSender.MessageKeys.crash,
+                value: MessageBusSender.Crash(report: crashReport, context: crashContext)
+            ), from: core)


This doesn't compile. With my latest changes on develop, we should now test against FeatureScopeMock not the core. See surrounding tests in this file, I updated them this morning.

Ugh, sorry, pulled and rebased and didn't see any conflicts so assumed it was good. Fixed now.

fuzzybinary marked this pull request as ready for review March 21, 2024 18:27

fuzzybinary requested review from a team as code owners March 21, 2024 18:27

feat: Allow crash reporting to use event mappers

0adfeb2

CrashReportReceiver now supports calling both the view and event mapper before writing the crash to storage. refs: RUM-2206

fuzzybinary force-pushed the jward/RUM-2206-rum-crash-event-mappers branch from f2f8e9a to 0adfeb2 Compare March 25, 2024 20:38

ncreated previously approved these changes Mar 26, 2024

View reviewed changes

DatadogCore/Tests/Datadog/RUM/Integrations/CrashReportReceiverTests.swift Outdated Show resolved Hide resolved

Don't allow dropping error events from crash mapper

93f69b9

This is partially to prevent users from accidentally dropping fatal errors in their mappers, and partially to be consistent with Android.

fuzzybinary dismissed ncreated’s stale review via 93f69b9 March 27, 2024 13:19

Merge branch 'develop' into jward/RUM-2206-rum-crash-event-mappers

5b60c64

fuzzybinary force-pushed the jward/RUM-2206-rum-crash-event-mappers branch from d72d7db to 5b60c64 Compare March 27, 2024 13:39

ncreated previously approved these changes Mar 27, 2024

View reviewed changes

ncreated dismissed their stale review via 7a401aa March 28, 2024 08:04

ncreated previously approved these changes Mar 28, 2024

View reviewed changes

ncreated dismissed their stale review via 2d5d2d2 March 28, 2024 08:23

ncreated force-pushed the jward/RUM-2206-rum-crash-event-mappers branch from 7a401aa to 2d5d2d2 Compare March 28, 2024 08:23

ncreated previously approved these changes Mar 28, 2024

View reviewed changes

fuzzybinary dismissed ncreated’s stale review via faf4c70 March 28, 2024 16:01

fuzzybinary requested a review from ncreated March 28, 2024 16:01

ncreated reviewed Mar 28, 2024

View reviewed changes

Fix inconsistency with null returns from mapper

7d0ee48

fuzzybinary force-pushed the jward/RUM-2206-rum-crash-event-mappers branch from faf4c70 to 7d0ee48 Compare March 28, 2024 18:12

ncreated approved these changes Mar 28, 2024

View reviewed changes

fuzzybinary merged commit 478a7a4 into develop Mar 29, 2024
8 checks passed

fuzzybinary deleted the jward/RUM-2206-rum-crash-event-mappers branch March 29, 2024 13:21

ncreated mentioned this pull request Apr 4, 2024

Dogfood recent changes #1761

Merged

8 tasks

maxep mentioned this pull request Apr 10, 2024

Release 2.9.0 #1769

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow crash reporting to use event mappers #1742

feat: Allow crash reporting to use event mappers #1742

fuzzybinary commented Mar 21, 2024

ncreated left a comment

datadog-datadog-prod-us1 bot commented Mar 27, 2024 •

edited

ncreated left a comment

ncreated Mar 27, 2024

ncreated Mar 27, 2024

fuzzybinary Mar 28, 2024

ncreated left a comment

ncreated Mar 28, 2024

fuzzybinary Mar 28, 2024

		* [FEATURE] Call RUM's `errorEventMapper` for crashes.
		* [FEATURE] Support calling log event mapper for crashes.

feat: Allow crash reporting to use event mappers #1742

feat: Allow crash reporting to use event mappers #1742

Conversation

fuzzybinary commented Mar 21, 2024

What and why?

Review checklist

Custom CI job configuration (optional)

ncreated left a comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Mar 27, 2024 • edited

Datadog Report

🔻 Code Coverage Decreases vs Default Branch (12)

ncreated left a comment

Choose a reason for hiding this comment

ncreated Mar 27, 2024

Choose a reason for hiding this comment

ncreated Mar 27, 2024

Choose a reason for hiding this comment

fuzzybinary Mar 28, 2024

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

ncreated Mar 28, 2024

Choose a reason for hiding this comment

fuzzybinary Mar 28, 2024

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Mar 27, 2024 •

edited