RUM-7421 Expose Crash Plugin #2126

maxep · 2024-11-29T16:28:47Z

What and why?

Follow-up #2116

Expose CrashReportingPlugin publicly and allow reporting crashes asynchronously with additional attributes.
The plugin can also provide an optional backtrace reporter.

How?

Crash Reporting Plugin

The plugin interface is now public with @escaping reporting closure, allowing reporting crashes at any time.
The interface also define an optional backtraceReporter property for reading backtraces.
This new plugin interface allows integration to fully replace the PLCrashReporter implementation.

Crash Report Additional Attributes

The DDCrashReport now defines additionalAttributes property to propagate atrributes.
These attributes are merged with logs' user attributes and RUM Error's context.

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
Add Objective-C interface for public APIs (see our guidelines [internal]) and run make api-surface)

datadog-datadog-prod-us1 · 2024-11-29T16:38:03Z

Datadog Report

Branch report: RUM-7421/expose-crash-plugin
Commit report: 47e7673
Test service: dd-sdk-ios

✅ 0 Failed, 3583 Passed, 0 Skipped, 2m 32.38s Total Time
🔻 Test Sessions change in coverage: 7 decreased, 3 increased, 4 no change

🔻 Code Coverage Decreases vs Default Branch (7)

This report shows up to 5 code coverage decreases.

test DatadogInternalTests iOS 80.46% (-0.06%) - Details
test DatadogLogsTests tvOS 47.69% (-0.04%) - Details
test DatadogLogsTests iOS 47.63% (-0.04%) - Details
test DatadogRUMTests iOS 81.11% (-0.02%) - Details
test DatadogSessionReplayTests iOS 34.25% (-0.02%) - Details

ncreated

👌 Everything looks great except the removal of queue that will cause performance regression (see my other comment).

DatadogCrashReporting/Sources/CrashReporting.swift

DatadogCrashReporting/Sources/CrashReportingFeature.swift

ncreated · 2024-12-03T08:36:32Z

Datadog/IntegrationUnitTests/CrashReporting/SendingCrashReportTests.swift

-        DDAssertJSONEqual(rumEvent.context!.contextInfo, crashContext.lastRUMAttributes!)
+        let contextAttributes = try XCTUnwrap(rumEvent.context?.contextInfo)
+        let lastRUMAttributes = try XCTUnwrap(crashContext.lastRUMAttributes?.attributes)
+        DDAssertJSONEqual(contextAttributes, lastRUMAttributes.merging(crashReportAttributes) { $1 })


ncreated · 2024-12-03T08:39:41Z

DatadogInternal/Sources/Models/CrashReporting/DDCrashReport.swift

@@ -97,5 +100,6 @@ public struct DDCrashReport: Codable, PassthroughAnyCodable {
        self.meta = meta
        self.wasTruncated = wasTruncated
        self.context = context
+        self.additionalAttributes = AnyCodable(additionalAttributes)


question/ Am I correct by saying that Codable requirement for DDCrashReport only comes from the message bus interface? We're using PassthroughAnyCodable so there's no actual coding and perf hit, but things like passing attribtues ☝️ are far from easy.

Exactly, it is only for conformance.
The DDCrashReport and CrashContext are now both in DatadogInternal, so it's no longer required to pass them as FeatureBaggage. We could pass them directly as a .crash(report: DDCrashReport, context: CrashContext) message on the bus, but such change has wider impact and should be treated separately IMO.

ncreated

Looks great 💪

maxep · 2024-12-10T13:28:38Z

Hello @naftaly 👋

Here is the follow up of your contribution. The 2 main changes are:

The added attributes are now part of the DDCrashReport object. They are merged with either the RUM Error context-info or with the error log's user attributes.
We have removed the public interface to send a report. Instead, we have made the CrashReportingPlugin.readPendingCrashReport closure escaping so you can report the crash at any time. In practice, you will store the closure in your plugin implementation to report the crash later.

I hope this will answer your needs!

naftaly · 2024-12-10T15:52:00Z

Hello @naftaly 👋

Here is the follow up of your contribution. The 2 main changes are:

The added attributes are now part of the DDCrashReport object. They are merged with either the RUM Error context-info or with the error log's user attributes.

We have removed the public interface to send a report. Instead, we have made the CrashReportingPlugin.readPendingCrashReport closure escaping so you can report the crash at any time. In practice, you will store the closure in your plugin implementation to report the crash later.

I hope this will answer your needs!

Thank you for your response. I'm curious why you chose to have the user store a block. Approaches like this can easily lead to retain cycles and unexpected behavior. In contrast, a simple send call is more explicit and easier to understand.

maxep · 2024-12-10T16:36:59Z

Thank you for your response. I'm curious why you chose to have the user store a block. Approaches like this can easily lead to retain cycles and unexpected behavior. In contrast, a simple send call is more explicit and easier to understand.

Internally, we make sure that the closure does not retain anything, if the Datadog SDK instance is deallocated, the closure will have no effect. Additionally, the crash-reporting feature is immutable and the closure is only about transmitting the crash report by jumping on a dedicated queue (we've called the message-bus), so the closure is also thread-safe.

The reason we went with this approach is that we want a single entry-point for interacting with the crash reporter lib, and that entry-point is the CrashReportingPlugin. By solely relying on the closure, we also control the lifecycle: The SDK will provide the closure when it's ready to make a report.
Providing a custom plugin is an advanced usage that is far from standard integration, one that require data injection and report formatting. Having a send method is indeed simple, but could also lead to mis-use.

I hope this make sense!

This reverts commit cb6d250.

maxep · 2024-12-10T17:19:30Z

/merge

maxep · 2024-12-11T09:16:00Z

/merge

dd-devflow · 2024-12-11T09:16:16Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-12-11 09:16:15 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2024-12-11 09:46:52 UTC ℹ️ MergeQueue: merge request added to the queue

The median merge time in develop is 27m.

2024-12-11 10:03:37 UTC ❌ MergeQueue: The build pipeline failed for this merge request

Build pipeline has failing jobs for d0acd15:

UI Tests: [NetworkInstrumentation]

⚠️ Do NOT retry failed jobs directly (why?).

What to do next?

Investigate the failures and when ready, re-add your pull request to the queue!
Any question, go check the FAQ.

maxep · 2024-12-11T10:55:02Z

/merge

dd-devflow · 2024-12-11T10:55:13Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-12-11 10:55:13 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in develop is 27m.

2024-12-11 11:23:02 UTC ℹ️ MergeQueue: This merge request was merged

naftaly · 2024-12-11T21:22:55Z

Thank you for your response. I'm curious why you chose to have the user store a block. Approaches like this can easily lead to retain cycles and unexpected behavior. In contrast, a simple send call is more explicit and easier to understand.

Internally, we make sure that the closure does not retain anything, if the Datadog SDK instance is deallocated, the closure will have no effect. Additionally, the crash-reporting feature is immutable and the closure is only about transmitting the crash report by jumping on a dedicated queue (we've called the message-bus), so the closure is also thread-safe.

The reason we went with this approach is that we want a single entry-point for interacting with the crash reporter lib, and that entry-point is the CrashReportingPlugin. By solely relying on the closure, we also control the lifecycle: The SDK will provide the closure when it's ready to make a report. Providing a custom plugin is an advanced usage that is far from standard integration, one that require data injection and report formatting. Having a send method is indeed simple, but could also lead to mis-use.

I hope this make sense!

Hi @maxep ,

I just wanted to validate that you don't expect the completion block to be called on any specific if called in the future by a 3rd party. For example, if I hold onto the block and call it at a later point, it won't be on the internal queue as if it was called in a non-escaping manner in the crash plugin.

thanks.

maxep · 2024-12-12T14:31:08Z

Hey @naftaly 👋

I just wanted to validate that you don't expect the completion block to be called on any specific if called in the future by a 3rd party. For example, if I hold onto the block and call it at a later point, it won't be on the internal queue as if it was called in a non-escaping manner in the crash plugin.

Exactly, you can hold the completion block and call it later from any thread 👍

mobile-app-ci force-pushed the RUM-7421/expose-crash-plugin branch from 8e89ba5 to cb6d250 Compare November 29, 2024 16:30

maxep marked this pull request as ready for review December 2, 2024 10:40

maxep requested review from a team as code owners December 2, 2024 10:40

ncreated requested changes Dec 3, 2024

View reviewed changes

DatadogCrashReporting/Sources/CrashReporting.swift Outdated Show resolved Hide resolved

DatadogCrashReporting/Sources/CrashReportingFeature.swift Outdated Show resolved Hide resolved

ncreated reviewed Dec 3, 2024

View reviewed changes

maxep requested a review from ncreated December 3, 2024 13:10

mariedm previously approved these changes Dec 4, 2024

View reviewed changes

ncreated previously approved these changes Dec 10, 2024

View reviewed changes

naftaly and others added 14 commits December 10, 2024 18:18

Extend crash reporter

5f6d841

Update CrashReportingPlugin.swift

92f6aa6

Remove conditional import

99c31d2

Keep CrashReportingFeature internal

c064c6f

Escape pending cash report

56dd9e8

Add backtraceReporter to plugin

5893d2c

Add addtional attributes to crash report

bcea798

Additional attributes in crash log

f099b4f

Additional attributes in RUM crash

c029c5b

Remove CrashReportingFeature GDC queue

09d0b13

Update CHANGELOG.md

735a810

Revert "Remove CrashReportingFeature GDC queue"

8e6562e

This reverts commit cb6d250.

Update public interface comment

2b3e132

Update CrashReporting.swift

5005800

mobile-app-ci force-pushed the RUM-7421/expose-crash-plugin branch from 2fd49df to 5005800 Compare December 10, 2024 17:18

dd-devflow bot added the mergequeue-status: removed label Dec 10, 2024

Fix crash reporting test flakiness

47e7673

mobile-app-ci dismissed stale reviews from ncreated and mariedm via 47e7673 December 11, 2024 09:11

mariedm approved these changes Dec 11, 2024

View reviewed changes

dd-devflow bot added mergequeue-status: waiting and removed mergequeue-status: removed labels Dec 11, 2024

ncreated approved these changes Dec 11, 2024

View reviewed changes

maciejburda approved these changes Dec 11, 2024

View reviewed changes

dd-devflow bot added mergequeue-status: queued mergequeue-status: in_progress mergequeue-status: rejected and removed mergequeue-status: waiting mergequeue-status: queued mergequeue-status: in_progress labels Dec 11, 2024

dd-devflow bot added mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: rejected mergequeue-status: queued labels Dec 11, 2024

dd-mergequeue bot merged commit 7f6df2b into develop Dec 11, 2024
17 checks passed

dd-devflow bot added mergequeue-status: done and removed mergequeue-status: in_progress labels Dec 11, 2024

maxep mentioned this pull request Dec 11, 2024

Release 2.21.0 #2141

Merged

4 tasks

maxep deleted the RUM-7421/expose-crash-plugin branch December 12, 2024 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-7421 Expose Crash Plugin #2126

RUM-7421 Expose Crash Plugin #2126

maxep commented Nov 29, 2024 •

edited

Loading

datadog-datadog-prod-us1 bot commented Nov 29, 2024 •

edited

Loading

ncreated left a comment

ncreated Dec 3, 2024

ncreated Dec 3, 2024

maxep Dec 3, 2024

ncreated left a comment

maxep commented Dec 10, 2024

naftaly commented Dec 10, 2024

maxep commented Dec 10, 2024

maxep commented Dec 10, 2024

maxep commented Dec 11, 2024

dd-devflow bot commented Dec 11, 2024 •

edited

Loading

maxep commented Dec 11, 2024

dd-devflow bot commented Dec 11, 2024 •

edited

Loading

naftaly commented Dec 11, 2024

maxep commented Dec 12, 2024

RUM-7421 Expose Crash Plugin #2126

RUM-7421 Expose Crash Plugin #2126

Conversation

maxep commented Nov 29, 2024 • edited Loading

What and why?

How?

Crash Reporting Plugin

Crash Report Additional Attributes

Review checklist

datadog-datadog-prod-us1 bot commented Nov 29, 2024 • edited Loading

Datadog Report

🔻 Code Coverage Decreases vs Default Branch (7)

ncreated left a comment

Choose a reason for hiding this comment

ncreated Dec 3, 2024

Choose a reason for hiding this comment

ncreated Dec 3, 2024

Choose a reason for hiding this comment

maxep Dec 3, 2024

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

maxep commented Dec 10, 2024

naftaly commented Dec 10, 2024

maxep commented Dec 10, 2024

maxep commented Dec 10, 2024

maxep commented Dec 11, 2024

dd-devflow bot commented Dec 11, 2024 • edited Loading

Devflow running: /merge

What to do next?

maxep commented Dec 11, 2024

dd-devflow bot commented Dec 11, 2024 • edited Loading

Devflow running: /merge

naftaly commented Dec 11, 2024

maxep commented Dec 12, 2024

maxep commented Nov 29, 2024 •

edited

Loading

datadog-datadog-prod-us1 bot commented Nov 29, 2024 •

edited

Loading

dd-devflow bot commented Dec 11, 2024 •

edited

Loading

Devflow running: `/merge`

dd-devflow bot commented Dec 11, 2024 •

edited

Loading

Devflow running: `/merge`