Add SentryWrapper for Callable and Supplier Interface by lbloder · Pull Request #2720 · getsentry/sentry-java

lbloder · 2023-05-19T07:22:40Z

📜 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

…nd supplier interface

github-actions · 2023-05-19T07:23:42Z

	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 · 2023-05-19T07:23:45Z

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

github-actions · 2023-05-19T07:38:36Z

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

# Conflicts: # CHANGELOG.md

adinauer · 2023-05-23T08:50:09Z

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

adinauer · 2023-05-24T13:30:14Z

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

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

markushi

Looks good to me!

markushi · 2023-05-26T05:59:55Z

+    final IHub oldState = Sentry.getCurrentHub();
+    final IHub newHub = Sentry.getCurrentHub().clone();


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();

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.

stefanosiano

just a couple of quick changes
looks good for the rest

codecov · 2023-05-26T13:54:10Z

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 added 4 commits April 27, 2023 15:46

introduce sentry wrapper class with static methods to wrap callable a…

74cd5d9

…nd supplier interface

Merge branch 'main' into feat/wrap_callable_supplier

26ff70d

Merge branch 'main' into feat/wrap_callable_supplier

527c1f6

Merge branch 'main' into feat/wrap_callable_supplier

e661175

lbloder and others added 2 commits May 19, 2023 09:24

add changelog entry

4d4d867

Format code

d46755f

lbloder added 2 commits May 19, 2023 15:23

Merge branch 'main' into feat/wrap_callable_supplier

3ee3f0f

# Conflicts: # CHANGELOG.md

Merge branch 'main' into feat/wrap_callable_supplier

a6e6090

# Conflicts: # CHANGELOG.md

lbloder added 3 commits May 24, 2023 14:31

add tests

d63b19b

Merge branch 'main' into feat/wrap_callable_supplier

ae85d91

api dump

ef59878

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

lbloder requested review from adinauer, markushi, romtsn and stefanosiano as code owners May 24, 2023 13:31

markushi approved these changes May 26, 2023

View reviewed changes

stefanosiano reviewed May 26, 2023

View reviewed changes

Comment thread sentry/src/main/java/io/sentry/SentryWrapper.java Outdated

stefanosiano reviewed May 26, 2023

View reviewed changes

Comment thread sentry/src/main/java/io/sentry/SentryWrapper.java Outdated

stefanosiano suggested changes May 26, 2023

View reviewed changes

lbloder added 4 commits May 26, 2023 14:49

Merge branch 'main' into feat/wrap_callable_supplier

719603f

merge changelog

75abd40

CR

3402182

format

1078f4a

stefanosiano approved these changes May 26, 2023

View reviewed changes

lbloder merged commit 16cd2b6 into main May 26, 2023

lbloder deleted the feat/wrap_callable_supplier branch May 26, 2023 15:55

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

Uh oh!

Uh oh!

Conversation

lbloder commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions Bot commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions and example for changelog

Uh oh!

lbloder commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/wrap_callable_supplier

Startup times

App size

Uh oh!

adinauer commented May 23, 2023

Uh oh!

adinauer commented May 24, 2023

Uh oh!

markushi left a comment

Choose a reason for hiding this comment

Uh oh!

markushi May 26, 2023

Choose a reason for hiding this comment

Uh oh!

markushi May 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefanosiano left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lbloder commented May 19, 2023 •

edited

Loading

github-actions Bot commented May 19, 2023 •

edited

Loading

lbloder commented May 19, 2023 •

edited

Loading

github-actions Bot commented May 19, 2023 •

edited

Loading

codecov Bot commented May 26, 2023 •

edited

Loading