RUM-2925 feat: Add backtrace generation capability to `DatadogCoreProtocol` #1687

ncreated · 2024-02-19T10:37:43Z

What and why?

📦 This PR adds a new capability to the SDK core: generating backtrace. Before, stack traces were only captured for crashes, now we enable the SDK to dump all threads and binary images at any moment at runtime.

This will be used in App Hangs monitoring to associate main thread's stack trace to pending hang. The actual work will be done in next PR, here we only focus on adding "backtrace reporting" capability to core.

How?

An entry point to this functionality is generateLiveReport API in PLCR:

/**
 * Generate a live crash report, without triggering an actual crash condition. This may be used to log
 * current process state without actually crashing. The crash report data will be returned on
 * success.
 * 
 * @return Returns nil if the crash report data could not be loaded.
 */
- (NSData *) generateLiveReport {

It dumps a snapshot of all running threads in the process. Because it uses the exact format of PLCrashReport, we're able to map it into our DDCrashReport representation (that includes threads and binary images).

Because the use case here is different than crash reporting, this PR adds BacktraceReport type that holds essential information for later stack traces symbolication in Datadog app:

public struct BacktraceReport {
    public let stack: String
    public let threads: [DDThread]
    public let binaryImages: [BinaryImage]
    public let wasTruncated: Bool
}

Internal types are shared with DDCrashReport for code reuse.

🏗️ Architecture change

Architecture-wise, the challenge in this PR was to expose backtrace generation (living in PLCR and abstracted by our Crash Reporting) to other feature modules (RUM in particular, but Logs and Trace can follow).

To solve this, when enabled, DatadogCrashReporting registers BacktraceRecordingFeature to the core. This is similar how NetworkInstrumentationFeature operates. Later on, when requesting backtrace, RUM will call core and the call will be delegated to Crash Reporting. Picture:

Following on the action points from our last iOS Dev Chat, I moved DatadogCrashReporting APIs to DatadogInternal in this PR. This way both RUM and Crash Reporting can use the same model definition without requiring type erasing through "baggage" concept:

Moving also the DDCrashReport which unlocks the performance optimisation that we have planned for RUM-2971 as a follow up action from (internal) SDK latency investigation. By sharing the model definition between features, there is no longer a need for coding it through message-bus "baggage".

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/

…eature

datadog-datadog-prod-us1 · 2024-02-19T10:46:42Z

Datadog Report

Branch report: ncreated/RUM-2925/record-the-stack-trace-of-app-hang
Commit report: 1568791
Test service: dd-sdk-ios

✅ 0 Failed, 2758 Passed, 0 Skipped, 10m 39.79s Wall Time

ncreated · 2024-02-19T12:02:09Z

DatadogCore/Sources/Core/DatadogCore.swift

-        let featureDirectories = try directory.getFeatureDirectories(forFeatureNamed: T.name)
+        if let feature = feature as? DatadogRemoteFeature {
+            let featureDirectories = try directory.getFeatureDirectories(forFeatureNamed: T.name)


This was a miss - we need to create storage directories only for DatadogRemoteFeature. Otherwise we will be creating it for NetworkInstrumentationFeature and BacktraceReportingFeature unnecessarily. Added extra unit tests for this change.

ncreated · 2024-02-19T12:04:17Z

DatadogCrashReporting/Sources/CrashReporting.swift

+            if let plcr = PLCrashReporterPlugin.thirdPartyCrashReporter {
+                try core.register(backtraceReporter: BacktraceReporter(reporter: plcr))
+            }


This might be more elegant, but would require revamping the startup sequence for DatadogCrashReporting. It still depends on CrashReportingPlugin, which was V1 dependency-inverter, no longer required in V2. This refactor is medium-size, hence I didn't want to do it for this PR.

ganeshnj

LGTM, just a small question 👌

ganeshnj · 2024-02-19T15:49:52Z

DatadogInternal/Sources/FeatureAPIs/CrashReporting/DDCrashReport.swift

+import Foundation
+
+/// Crash Report format supported by Datadog SDK.
+@objc


why do we need this to be inter-operable with objc?

Fair call, I traced this back to this commit 2f892c2 where we were adding Obj-c compatibility for Crash Reporting. This was back in V1. With V2 aligning module dependencies much better, this can be now internal to Swift domain, so it is not required. Changed here and consequently in few other places. ✅

maxep

Looks great!! I really appreciate the extension pattern applied to the core protocol 👌

DatadogInternal/Sources/FeatureAPIs/CrashReporting/BacktraceReport.swift

…dels`

as in V2 it no longer requires visibility in Obj-c API

maciejburda

LGTM

maciejburda · 2024-02-20T13:29:32Z

Datadog/IntegrationUnitTests/CrashReporting/GeneratingBacktraceTests.swift

+
+/// Tests integration of `DatadogCore` and `DatadogCrashReporting` for backtrace generation.
+class GeneratingBacktraceTests: XCTestCase {
+    private var core: DatadogCoreProxy! // swiftlint:disable:this implicitly_unwrapped_optional


probably worth having this lint rule global to all the tests

maciejburda · 2024-02-20T13:34:04Z

DatadogCrashReporting/Sources/PLCrashReporterIntegration/PLCrashReporterIntegration.swift

+        // This is quite opportunistic - we map PLCR's live report through existing `DDCrashReport` builder to
+        // then extract essential elements for assembling `BacktraceReport`. It works for now, but be careful
+        // with how this evolves. We may need a dedicated `BacktraceReport` builder that only shares some code
+        // with `DDCrashReport` builder.


I guess it's mostly affected by the PLCR update?

Do we expect to have failed unit tests when something like this happen, or we should have some extra measures?

This is PLCR-safe 👍. Basically, both for Crash Reports and Backtrace Reports, the value returned by PLCR is PLCrashReport. We then map it through our DDCrashReportBuilder, whereas implementing distinct BacktraceReportBuilder would be more correct and scalable. We don't need it today, hence I'm leaving this decision context in comment.

ncreated added 5 commits February 19, 2024 09:13

RUM-2925 Move DDCrashReport APIs to DatadogInternal

70858ac

RUM-2925 Add BacktraceReport API to capture thread backtrace

758bc6d

RUM-2925 Add BacktraceReporting capability to DatadogCore

9f62c3e

RUM-2925 Add tests for backtrace generation

17a9b03

RUM-2925 Add test for feature directories creation in Remote vs not F…

4c4fe41

…eature

ncreated changed the title ~~Ncreated/rum 2925/record the stack trace of app hang~~ RUM-2925 feat: Add backtrace generation capability to DatadogCoreProtocol Feb 19, 2024

ncreated commented Feb 19, 2024

View reviewed changes

ncreated marked this pull request as ready for review February 19, 2024 12:04

ncreated requested review from a team as code owners February 19, 2024 12:04

ganeshnj approved these changes Feb 19, 2024

View reviewed changes

maxep approved these changes Feb 19, 2024

View reviewed changes

DatadogInternal/Sources/FeatureAPIs/CrashReporting/BacktraceReport.swift Outdated Show resolved Hide resolved

ncreated added 2 commits February 20, 2024 09:12

RUM-2925 CR feedback - rename DatadogInternal/*/FeatureAPI to `*/Mo…

bcd1428

…dels`

RUM-2925 Make DDCrashReport a struct

1568791

as in V2 it no longer requires visibility in Obj-c API

ncreated mentioned this pull request Feb 20, 2024

RUM-2925 feat: Add stack trace to App Hang errors in RUM #1693

Merged

8 tasks

maciejburda approved these changes Feb 20, 2024

View reviewed changes

ncreated mentioned this pull request Feb 23, 2024

RUM-2816 Send raw webview event on the message bus #1691

Merged

8 tasks

Base automatically changed from ncreated/RUM-2925/report-non-fatal-app-hangs-in-RUM to app-hangs February 23, 2024 14:05

ncreated merged commit 91da074 into app-hangs Feb 23, 2024
8 checks passed

ncreated deleted the ncreated/RUM-2925/record-the-stack-trace-of-app-hang branch February 23, 2024 16:01

ncreated mentioned this pull request Feb 26, 2024

RUM-2925 feat: Non-fatal App Hangs monitoring #1699

Merged

8 tasks

ncreated mentioned this pull request Apr 9, 2024

RUM-3461 feat: Send fatal App Hang after app restart #1759

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-2925 feat: Add backtrace generation capability to `DatadogCoreProtocol` #1687

RUM-2925 feat: Add backtrace generation capability to `DatadogCoreProtocol` #1687

ncreated commented Feb 19, 2024 •

edited

Loading

datadog-datadog-prod-us1 bot commented Feb 19, 2024 •

edited

Loading

ncreated Feb 19, 2024

ncreated Feb 19, 2024

ganeshnj left a comment

ganeshnj Feb 19, 2024

ncreated Feb 20, 2024

maxep left a comment

maciejburda left a comment

maciejburda Feb 20, 2024

maciejburda Feb 20, 2024

ncreated Feb 20, 2024

RUM-2925 feat: Add backtrace generation capability to DatadogCoreProtocol #1687

RUM-2925 feat: Add backtrace generation capability to DatadogCoreProtocol #1687

Conversation

ncreated commented Feb 19, 2024 • edited Loading

What and why?

How?

🏗️ Architecture change

Review checklist

Custom CI job configuration (optional)

datadog-datadog-prod-us1 bot commented Feb 19, 2024 • edited Loading

Datadog Report

ncreated Feb 19, 2024

Choose a reason for hiding this comment

ncreated Feb 19, 2024

Choose a reason for hiding this comment

ganeshnj left a comment

Choose a reason for hiding this comment

ganeshnj Feb 19, 2024

Choose a reason for hiding this comment

ncreated Feb 20, 2024

Choose a reason for hiding this comment

maxep left a comment

Choose a reason for hiding this comment

maciejburda left a comment

Choose a reason for hiding this comment

maciejburda Feb 20, 2024

Choose a reason for hiding this comment

maciejburda Feb 20, 2024

Choose a reason for hiding this comment

ncreated Feb 20, 2024

Choose a reason for hiding this comment

RUM-2925 feat: Add backtrace generation capability to `DatadogCoreProtocol` #1687

RUM-2925 feat: Add backtrace generation capability to `DatadogCoreProtocol` #1687

ncreated commented Feb 19, 2024 •

edited

Loading

datadog-datadog-prod-us1 bot commented Feb 19, 2024 •

edited

Loading