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

ref: Remove breakpad stackwalking code (NATIVE-522) #731

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Mar 25, 2022

This is on top of #723, and completely removes breakpad from the codebase, and the build.

As this switches from breakpad to rust-minidump by default, the effect on a quick wrk shows that rps went from ~270 to ~360 for a very nice win.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 25, 2022

Warnings
⚠️ Snapshot changes likely affect Sentry tests. If the Sentry-Symbolicator Integration Tests in CI are failing for your PR, please check the symbolicator test suite in Sentry and update snapshots as needed.
Instructions for snapshot changes

Sentry runs a symbolicator integration test suite located at tests/symbolicator/. Changes in this PR will likely result in snapshot diffs in Sentry, which will break the master branch and in-progress PRs.

Follow these steps to update snapshots in Sentry:

  1. Check out latest Sentry master and enable the virtualenv.
  2. Enable symbolicator (symbolicator: true) in sentry via ~/.sentry/config.yml.
  3. Make sure your other devservices are running via sentry devservices up --exclude symbolicator. If
    they're already running, stop symbolicator with sentry devservices down symbolicator. You want to use your
    own development symbolicator to update the snapshots.
  4. Run your development symbolicator on port 3021, or whatever port symbolicator is configured to use
    in ~/.sentry/config.yml.
  5. Export SENTRY_SNAPSHOTS_WRITEBACK=1 to automatically update the existing snapshots with your new
    results and run symbolicator tests with pytest (pytest tests/symbolicator).
  6. Review snapshot changes locally, then create a PR to Sentry.
  7. Merge the Symbolicator PR, then merge the Sentry PR.

Generated by 🚫 dangerJS against 290f134

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2022

Codecov Report

Merging #731 (d7d3470) into master (e5bcac3) will increase coverage by 1.28%.
The diff coverage is 18.75%.

❗ Current head d7d3470 differs from pull request most recent head 290f134. Consider uploading reports for the commit 290f134 to get more accurate results

@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
+ Coverage   71.96%   73.25%   +1.28%     
==========================================
  Files          48       48              
  Lines       11093    10838     -255     
==========================================
- Hits         7983     7939      -44     
+ Misses       3110     2899     -211     
Impacted Files Coverage Δ
crates/process-event/src/main.rs 1.12% <ø> (+0.08%) ⬆️
crates/symbolicator/src/services/cficaches.rs 87.14% <ø> (ø)
crates/symbolicator/src/types/mod.rs 55.75% <ø> (+2.36%) ⬆️
...tes/symbolicator/src/services/symbolication/mod.rs 71.56% <18.75%> (+5.96%) ⬆️
...tes/symbolicator/src/endpoints/applecrashreport.rs 97.46% <0.00%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5bcac3...290f134. Read the comment docs.

@Swatinem Swatinem changed the base branch from master to feat/rust-minidump March 25, 2022 12:26
@Swatinem
Copy link
Member Author

The snapshots have these changes, as I now append the crash_address to the type/message.

-    type: EXCEPTION_ACCESS_VIOLATION_WRITE
-    value: 'Fatal Error: EXCEPTION_ACCESS_VIOLATION_WRITE'
+    type: EXCEPTION_ACCESS_VIOLATION_WRITE / 0x45
+    value: 'Fatal Error: EXCEPTION_ACCESS_VIOLATION_WRITE / 0x45'

Base automatically changed from feat/rust-minidump to master March 28, 2022 09:21
@Swatinem
Copy link
Member Author

This is now blocked by getsentry/sentry#32871

Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

can't wait to see this merged :)

@Gankra
Copy link

Gankra commented Mar 31, 2022

😸🎉

@Swatinem Swatinem marked this pull request as ready for review April 1, 2022 09:08
@Swatinem Swatinem requested a review from a team April 1, 2022 09:08
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

🎉

@loewenheim loewenheim merged commit a7ddfde into master Apr 4, 2022
@loewenheim loewenheim deleted the ref/rm-breakpad branch April 4, 2022 10:21
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.

5 participants