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

feat: Capture minidumps for hard crashes [INGEST-566] #1127

Merged
merged 7 commits into from
Nov 24, 2021

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Nov 16, 2021

Uses sentry-native with the breakpad backend to offer an optional crash-handler feature for Linux and macOS. When enabled, it'll capture hard crashes and write minidumps with stack traces of all threads into a database folder on the file system. On next restart, those crashes will be sent to Sentry for debugging. The DSN and release are inherited from the main Sentry SDK.

Breakpad offers advantages over the other backends. First, it works on all major platforms we need to support. Second, it does not need a handler process. Third, it still captures memory dumps with all threads. In the long run, we'd probably opt for the in-house crash reporter without third-party crash reporters in pure rust.

The minidump will have to be persisted in a volume that survives container restarts. There is a new configuration sentry._crash_db for this.

cc @Swatinem @oioki

@jan-auer jan-auer requested a review from a team November 16, 2021 13:41
@jan-auer jan-auer self-assigned this Nov 16, 2021
@jan-auer jan-auer marked this pull request as draft November 16, 2021 13:44
@jan-auer
Copy link
Member Author

Accidentally requested review instead of opening as Draft PR. Please ignore linter and linker errors for the time being. I would like to settle on the approach first and test it out before wrapping this up.

@jan-auer jan-auer changed the title feat: Capture minidumps for hard crashes feat: Capture minidumps for hard crashes [INGEST-566] Nov 16, 2021
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

well, a few more docs would be nice ;-)

relay-crash/src/lib.rs Outdated Show resolved Hide resolved
@jan-auer jan-auer marked this pull request as ready for review November 24, 2021 13:40
@jan-auer jan-auer requested a review from oioki November 24, 2021 13:40
Copy link
Member

@tonyo tonyo left a comment

Choose a reason for hiding this comment

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

LGTM approach-wise, but someone from Owners Ingest should check the Rust bits.
Also, let's make sure that the minidumps are cleaned up after they are submitted (probably already a sentry-native default).

@tonyo tonyo requested a review from a team November 24, 2021 14:07
* master:
  test(outcomes): Fix sort order in flaky test (#1135)
  feat(outcomes): Aggregate more outcomes (#1134)
  ref(outcomes): Fold processing vs non-processing into single actor (#1133)
  build: Update symbolic to support UE5 (#1132)
  feat(metrics): Extract measurement ratings, port from frontend (#1130)
  feat(metrics): Another statsd metric to measure bucket duplication (#1128)
  feat(outcomes): Emit outcomes as client reports (#1119)
  fix: Move changelog line to right version (#1129)
  fix(dangerjs): Do not suggest to add JIRA ticket to changelog (#1125)
  feat(metrics): Tag metrics by transaction name [INGEST-542] (#1126)

#[cfg(not(unix))]
pub fn install(&self) {
unimplemented!("crash handler not supported on this platform");
Copy link
Member

Choose a reason for hiding this comment

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

is it even possible to compile this module on windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's possible, sentry-native with breakpad works on Windows, too. Given this is fully internal, I just did not build this out in the build script (requires linking against a few other libraries).

@jan-auer
Copy link
Member Author

Also, let's make sure that the minidumps are cleaned up after they are submitted (probably already a sentry-native default).

Good point. sentry-native deletes minidumps after handling them.

@jan-auer jan-auer merged commit 36adaab into master Nov 24, 2021
@jan-auer jan-auer deleted the feat/native-crashes branch November 24, 2021 18:58
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

5 participants