Clone hub from main hub for every WebFlux request #2567

adinauer · 2023-02-24T10:53:46Z

#skip-changelog
(modified changelog entry for this chain of PRs instead of having a separate one)

📜 Description

Clone hub from main hub for every WebFlux request

💡 Motivation and Context

If we just use whatever hub is there on the thread, we could leak between requests.

💚 How did you test it?

manually using debugger

📝 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

… of the thread

github-actions · 2023-02-24T10:54:50Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against c88609d

github-actions · 2023-02-24T11:10:37Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	336.57 ms	451.40 ms	114.83 ms
Size	1.73 MiB	2.34 MiB	626.30 KiB

Previous results on branch: feat/reactor-with-sentry-continue-or-fresh

Startup times

Revision	Plain	With Sentry	Diff
`b19c118`	417.94 ms	478.42 ms	60.48 ms
`a864e2f`	353.66 ms	392.94 ms	39.28 ms
`b300cf4`	371.66 ms	401.00 ms	29.34 ms
`a98c90a`	293.96 ms	333.90 ms	39.94 ms
`b548154`	328.82 ms	405.74 ms	76.92 ms
`098ff45`	273.73 ms	363.61 ms	89.88 ms

App size

Revision	Plain	With Sentry	Diff
`b19c118`	1.73 MiB	2.34 MiB	626.30 KiB
`a864e2f`	1.73 MiB	2.34 MiB	626.31 KiB
`b300cf4`	1.73 MiB	2.34 MiB	626.31 KiB
`a98c90a`	1.73 MiB	2.34 MiB	626.31 KiB
`b548154`	1.73 MiB	2.34 MiB	626.30 KiB
`098ff45`	1.73 MiB	2.34 MiB	626.30 KiB

codecov · 2023-02-24T11:26:15Z

Codecov Report

Patch coverage: 40.00% and project coverage change: +0.06 🎉

Comparison is base (bd5f90e) 80.35% compared to head (1947daf) 80.41%.

❗ Current head 1947daf differs from pull request most recent head c88609d. Consider uploading reports for the commit c88609d to get more accurate results

Additional details and impacted files

@@                                Coverage Diff                                 @@
##             feat/use-thread-local-accessor-for-webclient    #2567      +/-   ##
==================================================================================
+ Coverage                                           80.35%   80.41%   +0.06%     
- Complexity                                           4011     4020       +9     
==================================================================================
  Files                                                 332      332              
  Lines                                               15148    15155       +7     
  Branches                                             1979     1979              
==================================================================================
+ Hits                                                12172    12187      +15     
+ Misses                                               2197     2189       -8     
  Partials                                              779      779

Impacted Files	Coverage Δ
...ebflux/SentryWebFilterWithThreadLocalAccessor.java	`0.00% <0.00%> (ø)`
sentry/src/main/java/io/sentry/Sentry.java	`55.78% <0.00%> (-0.30%)`	⬇️
...io/sentry/spring/jakarta/webflux/ReactorUtils.java	`61.53% <50.00%> (+61.53%)`	⬆️
...arta/webflux/SentryReactorThreadLocalAccessor.java	`100.00% <0.00%> (+100.00%)`	⬆️

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.

…/reactor-with-sentry-continue-or-fresh

romtsn · 2023-02-27T08:44:46Z

sentry/src/main/java/io/sentry/Sentry.java

+   */
+  @ApiStatus.Internal
+  @ApiStatus.Experimental
+  public static @NotNull IHub getNewHub() {


maybe naming it something like cloneMainHub or mainHubClone is better? Because getNewHub is not self-descriptive (e.g. from the first look it's not clear what's the diff between getCurrentHub and getNewHub)

Yeah, didn't like the name either. cloneMainHub sounds good to me.

stefanosiano · 2023-02-27T13:04:46Z

I think it’s fine. My only thought would be if we should skip cloning on global hub mode?

cleptric

LGTM, maybe call it withSentryMain/GlobalHub

adinauer · 2023-02-24T11:19:08Z

sentry/src/main/java/io/sentry/Sentry.java

+  @ApiStatus.Internal
+  @ApiStatus.Experimental
+  public static @NotNull IHub getNewHub() {
+    return mainHub.clone();


The idea was to get a fresh hub cloned from the main hub to not risk leaking data from one request into another. Any static interaction with Sentry on a thread that isn't being wrapped by the whole ThreadLocalAccessor thing could possibly write to another requests hub.

In case you need more context on where this is used, here's another PR that might help understand: [sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/SentryWebFilterWithThreadLocalAccessor.java](https://github.com/getsentry/sentry-java/pull/2570/files#diff-d21df3f03432d4f76a5539cb4bf199385dc8c956116371c4144153cde2e73de3)

adinauer · 2023-02-27T09:44:08Z

sentry/src/main/java/io/sentry/Sentry.java

+   */
+  @ApiStatus.Internal
+  @ApiStatus.Experimental
+  public static @NotNull IHub getNewHub() {


Yeah, didn't like the name either. cloneMainHub sounds good to me.

adinauer · 2023-02-28T10:13:35Z

sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/webflux/ReactorUtils.java

+   *  - having `io.projectreactor:reactor-core:3.5.3+` (provided by Spring Boot 3.0.3+)
+   */
+  @ApiStatus.Experimental
+  public static <T> Mono<T> withFreshSentry(final @NotNull Mono<T> mono) {


TODO find better name

…2566)

Clone hub from main hub for requests instead of using the current hub…

198825b

… of the thread

adinauer requested review from romtsn, stefanosiano and markushi as code owners February 24, 2023 10:53

adinauer marked this pull request as draft February 24, 2023 10:53

adinauer changed the base branch from main to feat/use-thread-local-accessor-for-webclient February 24, 2023 10:54

adinauer mentioned this pull request Feb 24, 2023

Support ThreadLocalAccessor for reactor based libraries (WebFlux, WebClient) #2546

Closed

adinauer added 5 commits February 27, 2023 07:25

Merge branch 'feat/use-thread-local-accessor-for-webclient' into feat…

c6e6853

…/reactor-with-sentry-continue-or-fresh

Add test for ReactorUtils

1360c09

more tests

86ab136

Merge branch 'feat/use-thread-local-accessor-for-webclient' into feat…

f5aa0f9

…/reactor-with-sentry-continue-or-fresh

Mention spring boot 3.0.3 in ReactorUtils

1947daf

romtsn reviewed Feb 27, 2023

View reviewed changes

romtsn approved these changes Feb 27, 2023

View reviewed changes

adinauer added 2 commits February 28, 2023 08:10

Rename getNewHub to cloneMainHub and check globalHubMode

0f58307

changelog

7fd96a4

adinauer marked this pull request as ready for review February 28, 2023 07:21

cleptric approved these changes Feb 28, 2023

View reviewed changes

adinauer commented Feb 28, 2023

View reviewed changes

adinauer and others added 2 commits March 1, 2023 05:57

Rename method

ba5120d

Use the same hub in WebFlux exception handler as we do in WebFilter (#…

c88609d

…2566)

adinauer merged commit d3f277b into feat/use-thread-local-accessor-for-webclient Mar 1, 2023

adinauer deleted the feat/reactor-with-sentry-continue-or-fresh branch March 1, 2023 05:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clone hub from main hub for every WebFlux request #2567

Clone hub from main hub for every WebFlux request #2567

adinauer commented Feb 24, 2023 •

edited

Loading

github-actions bot commented Feb 24, 2023 •

edited

Loading

github-actions bot commented Feb 24, 2023 •

edited

Loading

Previous results on branch: feat/reactor-with-sentry-continue-or-fresh

Startup times

App size

codecov bot commented Feb 24, 2023 •

edited

Loading

romtsn Feb 27, 2023

adinauer Feb 27, 2023

stefanosiano commented Feb 27, 2023

cleptric left a comment

adinauer Feb 24, 2023

adinauer Feb 24, 2023

adinauer Feb 27, 2023

adinauer Feb 28, 2023

Clone hub from main hub for every WebFlux request #2567

Clone hub from main hub for every WebFlux request #2567

Conversation

adinauer commented Feb 24, 2023 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Feb 24, 2023 • edited Loading

github-actions bot commented Feb 24, 2023 • edited Loading

Performance metrics 🚀

Previous results on branch: feat/reactor-with-sentry-continue-or-fresh

Startup times

App size

codecov bot commented Feb 24, 2023 • edited Loading

Codecov Report

romtsn Feb 27, 2023

Choose a reason for hiding this comment

adinauer Feb 27, 2023

Choose a reason for hiding this comment

stefanosiano commented Feb 27, 2023

cleptric left a comment

Choose a reason for hiding this comment

adinauer Feb 24, 2023

Choose a reason for hiding this comment

adinauer Feb 24, 2023

Choose a reason for hiding this comment

adinauer Feb 27, 2023

Choose a reason for hiding this comment

adinauer Feb 28, 2023

Choose a reason for hiding this comment

adinauer commented Feb 24, 2023 •

edited

Loading

github-actions bot commented Feb 24, 2023 •

edited

Loading

github-actions bot commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading