Make Crash Reporter Plugin Public

[naftaly]

This PR makes the crash reporter public and adds some hooks to send reports at other times than startup.

---

This pull request includes several changes to the DatadogCrashReporting module, focusing on enhancing crash reporting functionality and improving compatibility with conditional imports. The most important changes include adding a new method to merge RUM attributes in CrashContext, updating the CrashReporting class to handle conditional imports, and making the CrashReportingFeature class and CrashReportingPlugin protocol public.

Enhancements to crash reporting functionality:

• DatadogCrashReporting/Sources/CrashContext/CrashContext.swift: Added a new method merging(rumAttributes:) to the CrashContext struct for merging RUM attributes.
• DatadogCrashReporting/Sources/CrashReporting.swift: Introduced a new method send(_:attributes:in:) to the CrashReporting class for sending crash reports with optional attributes.

Improvements for conditional imports:

• DatadogCrashReporting/Sources/CrashReporting.swift: Updated the enable(in:) method to conditionally compile code based on the availability of the CrashReporter module. [1] [2]
• Various files in DatadogCrashReporting/Sources/PLCrashReporterIntegration/: Added #if canImport(CrashReporter) checks to conditionally compile code depending on the availability of the CrashReporter module. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]

Public API changes:

• DatadogCrashReporting/Sources/CrashReportingFeature.swift: Changed the CrashReportingFeature class and its name and messageReceiver properties to be public. [1] [2] [3]
• DatadogCrashReporting/Sources/CrashReportingPlugin.swift: Made the CrashReportingPlugin protocol public and added a new NoopCrashReportingPlugin class. [1] [2]

[maxep]

Hello @naftaly 👋

Thank you very much for your contribution! Enabling the injection of a different crash reporting library is a great idea.

I have a few questions and comments regarding the proposed changes:

1. Our DatadogCrashReporting module has a direct dependency on PLCrashReporter, so the conditional import statements will always evaluate to true. Could you elaborate on the motivation behind it?

2. I noticed two approaches for reporting crashes: one active, using the send(_ report:) method, and another reactive, through the CrashReportingPlugin protocol and its callback. What specific use cases are these designed to address?
   Typically, crash reporting libraries provide the crash report at app launch. In this case, having the DatadogCrashReporting module retrieve the report from its plugin when ready seems sufficient. Am I missing a scenario where both methods are necessary?

3. Regarding the send(_ report:) method, what type of attributes are you intending to inject? Are they related to the application's current state, or do they refer to information from the previous session?

Thank you again for your effort in improving our crash reporting!

[naftaly]

Hello @naftaly 👋

Thank you very much for your contribution! Enabling the injection of a different crash reporting library is a great idea.

Thank you, I was hoping you'd like it :)

I have a few questions and comments regarding the proposed changes:

1. Our DatadogCrashReporting module has a direct dependency on PLCrashReporter, so the conditional import statements will always evaluate to true. Could you elaborate on the motivation behind it?

The source I'm working with ends up importing through Cocoapod which for some reason leads to "CrashReporter" not being available. I didn't get into it too much, but I thought it would be a good solution until I got around to making a feature request to split out the plugin implementation from the CrashReporting infrastructure since we'd prefer to not include it since we wouldn't be using it, PLCrashReporter that is.

2. I noticed two approaches for reporting crashes: one active, using the send(_ report:) method, and another reactive, through the CrashReportingPlugin protocol and its callback. What specific use cases are these designed to address?
   Typically, crash reporting libraries provide the crash report at app launch. In this case, having the DatadogCrashReporting module retrieve the report from its plugin when ready seems sufficient. Am I missing a scenario where both methods are necessary?

You're totally right, that is a typical scenario. But there are also other scenarios where dependencies might not be ready yet, or simply that the infra responsible to crashes wants to send them at some other point in time, such as during downtime, or not during startup.

In our case, we have crash data coming from some other locations, and we enhance it with multiple different sets of data that for some reason or another are not ready as early as we need to call CrashReporter.enable().

Overall though, we want to start up the reporting system very very early to ensure we have the most complete set of data, and we want to report the crashes when we feel it is appropriate later during the session, or even in some future session (not necessarily the next session).

3. Regarding the send(_ report:) method, what type of attributes are you intending to inject? Are they related to the application's current state, or do they refer to information from the previous session?

The attributes can really be anything, but they are the extended set of data I was talking about earlier that come from other datasets. They don't always refer to the previous session, but they refer to the session of report we are sending.

Thank you again for your effort in improving our crash reporting!

Thank you for considering our PR, we love using Datadog and hope we can continue helping out as much we can.

[maxep]

Hello @naftaly

Thank you for the additional context and clarification, it makes sense.

The source I'm working with ends up importing through Cocoapod which for some reason leads to "CrashReporter" not being available. I didn't get into it too much, but I thought it would be a good solution until I got around to making a feature request to split out the plugin implementation from the CrashReporting infrastructure since we'd prefer to not include it since we wouldn't be using it, PLCrashReporter that is.

Our podspec depends on PLCR, so I'm curious—what specific issue are you trying to address? It seems preferable to resolve the CocoaPods problem directly rather than introducing conditional imports into the source code.

Splitting the plugin interface from its implementation would have a broader impact, as it would require architectural changes, including adding a new module. Submitting a feature request is the way to go 👍

Regarding attributes, I believe it would make sense to add them as a property of DDCrashReport. This way, both active and reactive reporting methods could inject attributes. However, implementing this would require changes to the RUM and Logs modules to ensure proper attribute propagation.
Of course, we don’t expect you to make such changes. Our usual process for external PRs is to merge them into a dedicated branch, where we handle reworking and add the necessary tests ourselves.

One other change I noticed: making the CrashReportingFeature public. Could you explain the reasoning behind this?

Thank you again for your contribution! 🙏

[naftaly]

Hello @naftaly

Thank you for the additional context and clarification, it makes sense.

The source I'm working with ends up importing through Cocoapod which for some reason leads to "CrashReporter" not being available. I didn't get into it too much, but I thought it would be a good solution until I got around to making a feature request to split out the plugin implementation from the CrashReporting infrastructure since we'd prefer to not include it since we wouldn't be using it, PLCrashReporter that is.

Our podspec depends on PLCR, so I'm curious—what specific issue are you trying to address? It seems preferable to resolve the CocoaPods problem directly rather than introducing conditional imports into the source code.

We end up working with a Bazel build system which does special things with Pods, so it's probably an issue on our side that I guess I simply swept away :) I'll figure it out on our side so we can simply forget about those import checks I think.

Splitting the plugin interface from its implementation would have a broader impact, as it would require architectural changes, including adding a new module. Submitting a feature request is the way to go 👍

Great, sounds good. Can we consider this a feature request or is there another route?

Regarding attributes, I believe it would make sense to add them as a property of DDCrashReport. This way, both active and reactive reporting methods could inject attributes. However, implementing this would require changes to the RUM and Logs modules to ensure proper attribute propagation. Of course, we don’t expect you to make such changes. Our usual process for external PRs is to merge them into a dedicated branch, where we handle reworking and add the necessary tests ourselves.

That sounds good to me, thank you.

One other change I noticed: making the CrashReportingFeature public. Could you explain the reasoning behind this?

It could be internal instead. Due to CrashReporting.enabled/send becoming public, it needed to be upgraded as well.

[maxep]

/merge

[dd-devflow]

Devflow running: /merge

View all feedbacks in Devflow UI.

---

2024-11-29 10:44:32 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in RUM-7421/expose-crash-plugin is 0s.