Skip to content
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

[RUMM-2997] Send a crash to both RUM and Logging features #1209

Merged
merged 2 commits into from Mar 23, 2023

Conversation

plousada
Copy link
Contributor

What and why?

Sends crash information both to Logs and RUM Features, as opposed to only RUM if both features are enabled.
Adopted a logging strategy similar to Android.

How?

A brief description of implementation details of this PR.

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
  • Run integration tests
  • Run smoke tests

@plousada plousada requested a review from a team as a code owner March 14, 2023 14:04
@plousada plousada force-pushed the plousada/RUMM-2997/send-crash-to-rum-and-logs branch 2 times, most recently from 6d7790c to 1897e65 Compare March 14, 2023 16:42
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Mar 14, 2023

Datadog Report

Branch report: plousada/RUMM-2997/send-crash-to-rum-and-logs
Commit report: f575c72

dd-sdk-ios: 0 Failed, 0 New Flaky, 124 Passed, 0 Skipped, 2m 38.39s Wall Time

Copy link
Collaborator

@ncreated ncreated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 🎯👌 - I left minor feedback, but for me it's good to go, anyway.

@@ -12,7 +12,7 @@ class CrashReporterTests: XCTestCase {
// MARK: - Sending Crash Report

func testWhenPendingCrashReportIsFound_itIsSentAndPurged() throws {
let expectation = self.expectation(description: "`LoggingOrRUMsender` sends the crash report")
let expectation = self.expectation(description: "`LoggingAndRUMsender` sends the crash report")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit/ it seems to be an artifact of V2 refactoring - neither LoggingOrRUMsender nor LoggingAndRUMsender do exist and instead we use CrashReportSender.

Suggested change
let expectation = self.expectation(description: "`LoggingAndRUMsender` sends the crash report")
let expectation = self.expectation(description: "`CrashReportSender` sends the crash report")

@@ -46,6 +46,47 @@ class CrashReporterTests: XCTestCase {
XCTAssertTrue(plugin.hasPurgedCrashReport == true, "It should ask to purge the crash report")
}

func testWhenPendingCrashReportIsFound_itIsSentBothToRumAndLogs() throws {
let expectation = self.expectation(description: "`LoggingAndRUMsender` sends the crash report to both features")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let expectation = self.expectation(description: "`LoggingAndRUMsender` sends the crash report to both features")
let expectation = self.expectation(description: "`CrashReportSender` sends the crash report to both features")

@@ -74,7 +115,7 @@ class CrashReporterTests: XCTestCase {
}

func testWhenPendingCrashReportIsFoundButItHasUnavailableCrashContext_itPurgesTheCrashReportWithNoSending() {
let expectation = self.expectation(description: "`LoggingOrRUMsender` does not send the crash report")
let expectation = self.expectation(description: "`LoggingAndRUMsender` does not send the crash report")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let expectation = self.expectation(description: "`LoggingAndRUMsender` does not send the crash report")
let expectation = self.expectation(description: "`CrashReportSender` does not send the crash report")

Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 👍 another approach would be to send a single "crash" message on the bus, and make the Logs Feature consume that one instead of "crash-log". It would simplify a bit the implementation, but it would have to print a more generic warning if the crash is not consumed at all.

@plousada
Copy link
Contributor Author

Great work 👍 another approach would be to send a single "crash" message on the bus, and make the Logs Feature consume that one instead of "crash-log". It would simplify a bit the implementation, but it would have to print a more generic warning if the crash is not consumed at all.

I can also do that if you prefer. It depends if we want to give importance to the fact that the crash is consumed by a specific feature, or by any feature.
It's a bit easier to debug a crash not appearing in Logs/RUM with the current implementation as the message is feature specific. But it will generate a noisy warning each time a crash is treated if the developer doesn't want to activate one of the features.

@plousada plousada force-pushed the plousada/RUMM-2997/send-crash-to-rum-and-logs branch from 1897e65 to f575c72 Compare March 20, 2023 17:20
@maxep
Copy link
Member

maxep commented Mar 20, 2023

Yes, I am for simplicity :) I think we would benefit from sending the crash only once on the bus, mostly because it will save an encode/decode operation on the message-bus and it could be consume by any Feature. Also, this warning won't have much visibility as it will be printed when the app restart only.

Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

@plousada plousada merged commit fb2d2e1 into develop Mar 23, 2023
6 checks passed
@ncreated ncreated mentioned this pull request Mar 23, 2023
3 tasks
@maxep maxep mentioned this pull request Mar 24, 2023
@ncreated ncreated mentioned this pull request Mar 31, 2023
6 tasks
@maxep maxep mentioned this pull request Apr 3, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants