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

Add SentryWrapper for Callable and Supplier Interface #2720

Merged
merged 15 commits into from
May 26, 2023

Conversation

lbloder
Copy link
Collaborator

@lbloder lbloder commented May 19, 2023

📜 Description

Allow Injection of SentryHub into Callable and Supplier Contexts.

💡 Motivation and Context

Fixes #2426

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2023

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Add SentryWrapper for Callable and Supplier Interface ([#2720](https://github.com/getsentry/sentry-java/pull/2720))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 1078f4a

@lbloder
Copy link
Collaborator Author

lbloder commented May 19, 2023

Not yet ready for review. Created the draft PR to have the checks run

@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 361.24 ms 406.20 ms 44.96 ms
Size 1.72 MiB 2.28 MiB 570.95 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
46b1782 387.72 ms 458.74 ms 71.02 ms
1707044 338.80 ms 384.79 ms 46.00 ms

App size

Revision Plain With Sentry Diff
46b1782 1.72 MiB 2.28 MiB 570.44 KiB
1707044 1.72 MiB 2.28 MiB 570.44 KiB

Previous results on branch: feat/wrap_callable_supplier

Startup times

Revision Plain With Sentry Diff
05983b3 348.50 ms 391.60 ms 43.10 ms
207c2b4 311.66 ms 354.66 ms 43.00 ms
b934ebe 295.77 ms 379.27 ms 83.50 ms
998fee2 311.46 ms 383.30 ms 71.84 ms

App size

Revision Plain With Sentry Diff
05983b3 1.72 MiB 2.28 MiB 570.44 KiB
207c2b4 1.72 MiB 2.28 MiB 570.45 KiB
b934ebe 1.72 MiB 2.28 MiB 570.44 KiB
998fee2 1.72 MiB 2.28 MiB 570.44 KiB

@adinauer
Copy link
Member

Should we automatically capture exceptions in there? Maybe configurable to turn on/off?

@adinauer
Copy link
Member

Should we automatically capture exceptions in there? Maybe configurable to turn on/off?

To not block this PR, let's decide via #2729

@lbloder lbloder marked this pull request as ready for review May 24, 2023 13:31
Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Comment on lines 8 to 9
final IHub oldState = Sentry.getCurrentHub();
final IHub newHub = Sentry.getCurrentHub().clone();
Copy link
Member

Choose a reason for hiding this comment

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

Nit-picking here, but I think this could be simplified to the following. In theory it's also technically "more correct" in case some other thread is executing Sentry.setCurrentHub in between those statements.

Suggested change
final IHub oldState = Sentry.getCurrentHub();
final IHub newHub = Sentry.getCurrentHub().clone();
final IHub oldHub = Sentry.getCurrentHub();
final IHub newHub = oldHub.clone();

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see that Sentry.currentHub is thread-local anyway for non global hub mode setups (ThreadLocal<IHub> currentHub), so my suggestion only improves if globalHubMode=true.

Copy link
Member

@stefanosiano stefanosiano left a comment

Choose a reason for hiding this comment

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

just a couple of quick changes
looks good for the rest

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Patch coverage: 92.30% and no project coverage change.

Comparison is base (e5c250b) 81.11% compared to head (1078f4a) 81.12%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2720   +/-   ##
=========================================
  Coverage     81.11%   81.12%           
- Complexity     4449     4453    +4     
=========================================
  Files           345      346    +1     
  Lines         16407    16420   +13     
  Branches       2226     2226           
=========================================
+ Hits          13308    13320   +12     
- Misses         2172     2173    +1     
  Partials        927      927           
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/SentryWrapper.java 92.30% <92.30%> (ø)

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

@lbloder lbloder merged commit 16cd2b6 into main May 26, 2023
26 of 27 checks passed
@lbloder lbloder deleted the feat/wrap_callable_supplier branch May 26, 2023 15:55
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.

Add support for injecting hub from the main thread to Callable interface
5 participants