Skip to content

fix(java): Ensure potential callback exceptions are caught #2123#2291

Merged
markushi merged 5 commits into
mainfrom
fix/capture-callback-exceptions
Oct 14, 2022
Merged

fix(java): Ensure potential callback exceptions are caught #2123#2291
markushi merged 5 commits into
mainfrom
fix/capture-callback-exceptions

Conversation

@markushi

@markushi markushi commented Oct 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

📜 Description

Try/Catches callback executions and add logging in case of errors.

💡 Motivation and Context

If certain callbacks provided via the Sentry, Hub or SentryOptions crash during execution, it could crash the whole application. So let's catch 'em all 😄

Related issues:

Callbacks which were already handled:

  • BeforeSendCallback
  • BeforeBreadcrumbCallback

Callbacks:

  • SentryOptions.TracesSamplerCallback
  • SentryOptions.ProfilesSamplerCallback
  • OptionsConfiguration.configure
  • Hub.captureMessage
  • Hub.captureException
  • Hub.captureEvent
  • Hub.withScope
  • Hub.configureScope

💚 How did you test it?

Added tests where required

📝 Checklist

  • [ x ] I reviewed the submitted code
  • [ x ] I added tests to verify the changes
  • [ x ] I updated the docs if needed
  • [ x ] No breaking changes

🔮 Next steps

@github-actions

github-actions Bot commented Oct 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 310.88 ms 339.26 ms 28.38 ms
Size 1.73 MiB 2.29 MiB 580.10 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7300956 337.57 ms 384.21 ms 46.64 ms
4ca1d7b 328.46 ms 368.22 ms 39.76 ms
54cebc8 300.86 ms 341.43 ms 40.57 ms
c5ccd8a 329.98 ms 365.52 ms 35.54 ms
1e4690d 354.69 ms 387.88 ms 33.19 ms
4dd88fe 306.88 ms 391.58 ms 84.70 ms
4dd88fe 302.12 ms 331.17 ms 29.04 ms
7300956 324.20 ms 353.79 ms 29.58 ms
3d89dea 345.59 ms 364.06 ms 18.47 ms
54cebc8 331.12 ms 385.14 ms 54.02 ms

App size

Revision Plain With Sentry Diff
7300956 1.73 MiB 2.29 MiB 578.69 KiB
4ca1d7b 1.73 MiB 2.29 MiB 579.88 KiB
54cebc8 1.73 MiB 2.29 MiB 579.43 KiB
c5ccd8a 1.74 MiB 2.33 MiB 607.44 KiB
1e4690d 1.74 MiB 2.33 MiB 604.92 KiB
4dd88fe 1.73 MiB 2.29 MiB 579.50 KiB
4dd88fe 1.73 MiB 2.29 MiB 579.50 KiB
7300956 1.73 MiB 2.29 MiB 578.69 KiB
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB
54cebc8 1.73 MiB 2.29 MiB 579.43 KiB

Previous results on branch: fix/capture-callback-exceptions

Startup times

Revision Plain With Sentry Diff
a6facbc 305.74 ms 333.83 ms 28.09 ms
179e40b 308.38 ms 313.78 ms 5.40 ms
558a3bd 277.29 ms 350.02 ms 72.73 ms
2e5138d 276.08 ms 343.17 ms 67.09 ms

App size

Revision Plain With Sentry Diff
a6facbc 1.73 MiB 2.29 MiB 579.94 KiB
179e40b 1.73 MiB 2.29 MiB 579.96 KiB
558a3bd 1.73 MiB 2.29 MiB 579.94 KiB
2e5138d 1.73 MiB 2.29 MiB 579.94 KiB

@codecov-commenter

codecov-commenter commented Oct 12, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Base: 79.96% // Head: 80.10% // Increases project coverage by +0.13% 🎉

Coverage data is based on head (1171870) compared to base (7d87f22).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #2291      +/-   ##
============================================
+ Coverage     79.96%   80.10%   +0.13%     
- Complexity     3425     3430       +5     
============================================
  Files           242      242              
  Lines         12734    12754      +20     
  Branches       1702     1702              
============================================
+ Hits          10183    10216      +33     
+ Misses         1901     1889      -12     
+ Partials        650      649       -1
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/Hub.java 75.53% <100.00%> (+1.08%) ⬆️
sentry/src/main/java/io/sentry/Sentry.java 53.17% <100.00%> (+6.79%) ⬆️
sentry/src/main/java/io/sentry/TracesSampler.java 100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/IHub.java 91.66% <0.00%> (+4.16%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@markushi markushi linked an issue Oct 12, 2022 that may be closed by this pull request
Some callbacks do not have their Exceptions caught yet #2123
Closed
marandaneto
marandaneto reviewed Oct 12, 2022
View reviewed changes
Comment thread sentry/src/test/java/io/sentry/HubTest.kt Outdated
romtsn
romtsn approved these changes Oct 13, 2022
View reviewed changes

@romtsn romtsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

@markushi
markushi merged commit adee765 into main Oct 14, 2022
@markushi
markushi deleted the fix/capture-callback-exceptions branch October 14, 2022 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romtsn romtsn romtsn approved these changes
@adinauer adinauer Awaiting requested review from adinauer adinauer is a code owner
@stefanosiano stefanosiano Awaiting requested review from stefanosiano
+1 more reviewer
@marandaneto marandaneto marandaneto left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Some callbacks do not have their Exceptions caught yet

4 participants

@markushi @codecov-commenter @romtsn @marandaneto