Fix: Sending errors in Spring WebFlux integration.

[maciejwalkowiak]

📜 Description

Fix sending errors in Spring WebFlux integration.

This time, without trying to find a workaround but rather using recommended reactive/reactor way of doing things: instead of copying hub every time executing thread changes, hub is set on Reactor context, which means:

Pros:

• we don't affect the performance as much as we did before - as copying hub happens only once per request
• there is no way that threads interfere with each other

Cons:

• when Sentry static methods are used, scope is not attached to events
• Logging frameworks also have no access to Sentry scope

💡 Motivation and Context

Fixes #1790

💚 How did you test it?

Integration tests, sample project running on the side.

📝 Checklist

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

Only classes marked as experimental are changed.

🔮 Next steps

[codecov-commenter]

Codecov Report

Merging #1819 (8276861) into main (9f87477) will increase coverage by 0.04%.
The diff coverage is 80.85%.

@@             Coverage Diff              @@
##               main    #1819      +/-   ##
============================================
+ Coverage     75.51%   75.56%   +0.04%     
- Complexity     2239     2243       +4     
============================================
  Files           225      225              
  Lines          8001     8016      +15     
  Branches        846      850       +4     
============================================
+ Hits           6042     6057      +15     
+ Misses         1549     1544       -5     
- Partials        410      415       +5     

Impacted Files	Coverage Δ
...o/sentry/spring/webflux/SentryRequestResolver.java	73.68% <33.33%> (-2.51%)	⬇️
...ntry/spring/webflux/SentryWebExceptionHandler.java	86.66% <75.00%> (-6.20%)	⬇️
...n/java/io/sentry/spring/webflux/SentryReactor.java	76.92% <76.92%> (ø)
...java/io/sentry/spring/webflux/SentryWebFilter.java	90.32% <88.00%> (-9.68%)	⬇️
...ry/spring/boot/SentryWebfluxAutoConfiguration.java	100.00% <100.00%> (+33.33%)	⬆️
...n/java/io/sentry/transport/ReusableCountLatch.java	82.35% <0.00%> (-5.89%)	⬇️

---

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 9f87477...8276861. Read the comment docs.

[bruno-garcia]

Cons

• when Sentry static methods are used, scope is not attached to events
• Logging frameworks also have no access to Sentry scope

These are pretty big cons. Wouldn't this break the relationship of breadcrumbs and events from web-requests?
Also, this would break then any static use of Sentry.captureException in terms of having any web-request related information?

Not to say that it's not a better/right thing to do, but it brings to question if it's worth building this integration altogether since it's still fairly broken (though possible less broken if now we have info from the wrong request appended)

[maciejwalkowiak]

Wouldn't this break the relationship of breadcrumbs and events from web-requests?
No, as long as breadcrumbs or events are added through a hub retrieved from Reactor context. So our out-of-the-box integration for error handling works as expected.

Also, this would break then any static use of Sentry.captureException in terms of having any web-request related information?

Yes, I agree this is sub-optimal. I did not find a way to make it work, but will continue investigating.

[maciejwalkowiak]

As far as I can tell - there is no way to do it without a considerable performance hit. Sleuth integration with Reactor, initially worked in a way that scope was copied between threads, now starting from version 3.0 they recommend "manual" mode: https://docs.spring.io/spring-cloud-sleuth/docs/current-SNAPSHOT/reference/html/integrations.html#sleuth-reactor-integration

There are some other solutions to this problem - mainly circulating around Reactor + MDC - but all are home-grown and each faces issues.

I think having out-of-the-box solution for capturing unhandled exceptions in Webflux is useful for users, but at this stage we should not ship integration with static methods or logging statements, as every implementation seems to be risky to me.

[maciejwalkowiak]

Following Reactor docs, the one other way to make it work, including static api and logback as a result is to add following method that will set the thread local and reset it after operator finished work:

public static <T> Consumer<Signal<T>> withSentry(Consumer<T> logStatement) {
  return signal -> {
    if (!signal.isOnNext()) return;
      IHub currentHub = Sentry.getCurrentHub();
      IHub iHub = signal.getContextView().getOrDefault(SentryWebFilter.HUB_REACTOR_CONTEXT_ATTRIBUTE, null);
      if (iHub != null) {
        Sentry.setCurrentHub(iHub);
        logStatement.accept(signal.get());
        Sentry.setCurrentHub(currentHub);
      } else {
        logStatement.accept(signal.get());
      }
  };
}

Then, every call to static method on Sentry class, or logger, must be wrapped into withSentry.

Mono<Person> create(Person person) {
  return Mono.delay(Duration.ofMillis(100))
      .publishOn(Schedulers.boundedElastic())
      .doOnEach(withSentry(s -> Sentry.captureMessage("Creating person")))
      .doOnEach(withSentry(__ -> LOG.error("ERROR LOG")))
      .map(__ -> person);
}

We can either add it to the code base, or add to docs. I would still keep it experimental.

This marks the end of my investigation :-)

[bruno-garcia]

Following Reactor docs, the one other way to make it work, including static api and logback as a result is to add following method that will set the thread local and reset it after operator finished work:

public static <T> Consumer<Signal<T>> withSentry(Consumer<T> logStatement) {
  return signal -> {
    if (!signal.isOnNext()) return;
      IHub currentHub = Sentry.getCurrentHub();
      IHub iHub = signal.getContextView().getOrDefault(SentryWebFilter.HUB_REACTOR_CONTEXT_ATTRIBUTE, null);
      if (iHub != null) {
        Sentry.setCurrentHub(iHub);
        logStatement.accept(signal.get());
        Sentry.setCurrentHub(currentHub);
      } else {
        logStatement.accept(signal.get());
      }
  };
}

Then, every call to static method on Sentry class, or logger, must be wrapped into withSentry.

Mono<Person> create(Person person) {
  return Mono.delay(Duration.ofMillis(100))
      .publishOn(Schedulers.boundedElastic())
      .doOnEach(withSentry(s -> Sentry.captureMessage("Creating person")))
      .doOnEach(withSentry(__ -> LOG.error("ERROR LOG")))
      .map(__ -> person);
}

We can either add it to the code base, or add to docs. I would still keep it experimental.

This marks the end of my investigation :-)

It sounds like this API is the way to go. We can document things to lead users the right path

[bruno-garcia]

Just noticed the snippet u shared is missing a try/finally to undo the hub thing in the finally block if the callback throws

[oliverorav]

Is there any update on this PR and generally about Sentry and WebFlux integration? As from @adinauer comment this is also a blocker to move forward with #1807 .

[adinauer]

@oliverorav I played around with this for a bit during our Hackweek and created this draft PR: #2224

But as you can see it leads to quite a mess in the PersonController and I never finished working on it. It's also net been tested in any real application.

A way to improve on it I can think of is to somehow apply the mess using some sort of bytecode manipulation and make the right hub available without requiring the user to write the mess. This is just an idea though and not very high on our list of priorities at the moment unfortunately.

[adinauer]

Closing this in favor of #2546