Skip to content

perf: Filter binary images on Sentry Crash #1767

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

Merged
merged 8 commits into from
Apr 25, 2022
Merged

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Apr 22, 2022

📜 Description

Hard crash reports include all binary images, that's a lot of data. This change will filter only binary images that have some reference on any stack frame for any thread in the report, drastically reducing event payload.

💡 Motivation and Context

Part of #1676

💚 How did you test it?

Unit tests

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

Sorry, something went wrong.

@brustolin brustolin changed the title Feat: Filter binary images on Sentry Crash feat: Filter binary images on Sentry Crash Apr 22, 2022
@brustolin
Copy link
Contributor Author

@philipphofmann should we ping someone from native team to review this PR?

@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2022

Codecov Report

Merging #1767 (7405490) into master (758519c) will increase coverage by 0.89%.
The diff coverage is 100.00%.

❗ Current head 7405490 differs from pull request most recent head 8a7a07e. Consider uploading reports for the commit 8a7a07e to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1767      +/-   ##
==========================================
+ Coverage   91.74%   92.64%   +0.89%     
==========================================
  Files         198      198              
  Lines        9004     9012       +8     
==========================================
+ Hits         8261     8349      +88     
+ Misses        743      663      -80     
Impacted Files Coverage Δ
Sources/Sentry/SentryCrashReportConverter.m 96.08% <100.00%> (+0.08%) ⬆️
Sources/Sentry/include/SentryDependencyContainer.h 37.50% <0.00%> (-5.36%) ⬇️
Sources/Sentry/SentryNSDataTracker.m 91.74% <0.00%> (-2.76%) ⬇️
Sources/Sentry/SentryProfiler.mm 91.27% <0.00%> (-2.02%) ⬇️
Sources/Sentry/SentryHybridSKdsOnly.m 100.00% <0.00%> (ø)
Sources/Sentry/SentryDependencyContainer.m 96.61% <0.00%> (+0.31%) ⬆️
Sources/Sentry/SentryBacktrace.cpp 92.63% <0.00%> (+1.05%) ⬆️
Sources/Sentry/SentryThreadInspector.m 100.00% <0.00%> (+3.03%) ⬆️
Sources/Sentry/include/SentryProfilingLogging.hpp 61.90% <0.00%> (+4.76%) ⬆️
Sources/Sentry/include/SentryMachLogging.hpp 77.77% <0.00%> (+22.22%) ⬆️
... and 2 more

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 758519c...8a7a07e. Read the comment docs.

@philipphofmann
Copy link
Member

@philipphofmann should we ping someone from native team to review this PR?

No, as this PR doesn't touch any C code.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time for this big improvement. I added a few comments.

@brustolin brustolin changed the title feat: Filter binary images on Sentry Crash perf: Filter binary images on Sentry Crash Apr 25, 2022
brustolin and others added 2 commits April 25, 2022 09:51
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

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

4 participants