Configurable SentryExceptionResolver Order for getsentry/sentry-java#681

[timtebeek]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

Make SentryExceptionResolver Order configurable as per #681

💡 Motivation and Context

This allows the spring-boot-starter to be used without reporting all handled web exceptions to Sentry, which up to now blew through our quota.

💚 How did you test it?

No.

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• All tests passing
• No breaking changes

🔮 Next steps

Critique; Refine; Test; Merge

[timtebeek]

Now this is a very rough first attempt, slightly hindered by SentryProperties being in the starter, which makes it unavailable to SentryWebConfiguration in sentry-spring. I've added it to SentryProperties anyway to have the property meta data generated.

Also, since SentryAutoConfiguration does a plain @Import of SentryWebConfiguration I saw no easy way to push the order parameter that way. If there is such a way please let me know and I will update accordingly.

[maciejwalkowiak]

Thanks @timtebeek for the PR! This behaviour of catching all exceptions, even those that were handled with @ExceptionHandler is consistent with Sentry SDK 1.7.x but I believe SDK should catch only unhandled exceptions by default - what do you think @bruno-garcia?

If we go this way, to make it easier for users to configure the SDK, instead of integer property I think a boolean sentry.catch-unhandled-exceptions with a default value false would be sufficient. It would be false in sentry-spring module and in Spring Boot auto-configuration instead of importing SentryWebConfiguration we would duplicate these 3 beans making sentryExceptionResolver use SentryProperties in the bean factory method.

[bruno-garcia]

I agree the int value of the order isn't an ideal API, but I think we need to work on the this option, what it does and its name.

It makes sense to me for a middleware error handler to be the last thing in the pipeline tbqh. The user should be able to handle and error and avoid it being captured completely. There are apps out there that use exceptions as control flow a lot, and throw from controllers and then "have a single place" to deal with them by type to return some known error codes.

It's a fair default IMO to only capture exceptions that are not caught by such middlewares, either because they've rethrown, or they errored while processing an exception. And handled exceptions would not be reported to Sentry.

In such cases users can also add their own captureException calls in there, on their catch block.

Since this would be a breaking change we don't want to take right now, it's probably fine to go with your suggestion of having an "opt-out" only. An option called unhandled=false is confusing because it sounds like the user isn't getting unhandled exceptions at all.

I don't have a better suggestion right now though. @marandaneto, @maciejwalkowiak ?

[marandaneto]

I agree the int value of the order isn't an ideal API, but I think we need to work on the this option, what it does and its name.

It makes sense to me for a middleware error handler to be the last thing in the pipeline tbqh. The user should be able to handle and error and avoid it being captured completely. There are apps out there that use exceptions as control flow a lot, and throw from controllers and then "have a single place" to deal with them by type to return some known error codes.

It's a fair default IMO to only capture exceptions that are not caught by such middlewares, either because they've rethrown, or they errored while processing an exception. And handled exceptions would not be reported to Sentry.

In such cases users can also add their own captureException calls in there, on their catch block.

Since this would be a breaking change we don't want to take right now, it's probably fine to go with your suggestion of having an "opt-out" only. An option called unhandled=false is confusing because it sounds like the user isn't getting unhandled exceptions at all.

I don't have a better suggestion right now though. @marandaneto, @maciejwalkowiak ?

yeah I agree with @bruno-garcia here, but not sure about the boolean.

a boolean would capture either all the exception (if highest priority) or only unhandled exception if min. priority.

looks like an int would give the flexibility to configure as one wishes, that means, something in between depending on the exception control flow.

if we decide to go for a boolean, remember that we already have a enableUncaughtExceptionHandler boolean on the options to enable/disable the UncaughtException handler, so if the idea is just to not get any unhandled exception, could we use this one instead or Spring triggers on its own way?

[codecov-io]

Codecov Report

Merging #1008 (57de115) into main (233d562) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1008      +/-   ##
============================================
+ Coverage     71.98%   72.06%   +0.07%     
- Complexity     1322     1324       +2     
============================================
  Files           135      135              
  Lines          4816     4829      +13     
  Branches        492      492              
============================================
+ Hits           3467     3480      +13     
  Misses         1091     1091              
  Partials        258      258              

Impacted Files	Coverage Δ	Complexity Δ
.../java/io/sentry/spring/SentryWebConfiguration.java	100.00% <ø> (ø)	3.00 <0.00> (-1.00)
...io/sentry/spring/boot/SentryAutoConfiguration.java	90.90% <100.00%> (+0.28%)	1.00 <0.00> (ø)
...n/java/io/sentry/spring/boot/SentryProperties.java	79.16% <100.00%> (+4.16%)	6.00 <2.00> (+2.00)
...java/io/sentry/spring/SentryExceptionResolver.java	100.00% <100.00%> (ø)	3.00 <2.00> (ø)
...main/java/io/sentry/spring/SentryHubRegistrar.java	90.47% <100.00%> (+2.24%)	7.00 <1.00> (+1.00)

---

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 233d562...b4b11e5. Read the comment docs.

[timtebeek]

yeah I agree with @bruno-garcia here, but not sure about the boolean.

a boolean would capture either all the exception (if highest priority) or only unhandled exception if min. priority.

looks like an int would give the flexibility to configure as one wishes, that means, something in between depending on the exception control flow.

That's indeed why I thought to use an int over a boolean first/last; That way people can choose to make the exception resolver second-to-last for instance.

if we decide to go for a boolean, remember that we already have a enableUncaughtExceptionHandler boolean on the options to enable/disable the UncaughtException handler, so if the idea is just to not get any unhandled exception, could we use this one instead or Spring triggers on its own way?

I'd missed that io.sentry.SentryOptions.enableUncaughtExceptionHandler, but it seems that one is any uncaught exceptions (as you probably already know). The SentryExceptionResolver is only for servlet web exceptions; maybe we should fit that into the configuration option name in a more obvious way, as the nuance might be lost otherwise.

[timtebeek]

Anything I can do to move this issue along? There seems to be some discussion around the preferable configuration option type & name, and whether we want to import SentryWebConfiguration or duplicate the three beans it defines in the starter. Any definitive answer there that'll allow for the necessary changes to be made?

[marandaneto]

Anything I can do to move this issue along? There seems to be some discussion around the preferable configuration option type & name, and whether we want to import SentryWebConfiguration or duplicate the three beans it defines in the starter. Any definitive answer there that'll allow for the necessary changes to be made?

@maciejwalkowiak mind taking this one? as you've written the other web integrations, your opinion will be the most valuable here, thanks.

[maciejwalkowiak]

@timtebeek thanks for updates! I will get back to you next week once I am done with some of the performance related changes.

[maciejwalkowiak]

@timtebeek thank you for your patience.

One issue with this implementation I see is that with Spring (not Spring Boot) integration we have now two ways to configure sentry - some properties using @EnableSentry(...) annotation and some with Spring environment.

Since we already went with @EnableSentry, I think this property exceptionResolverOrder should be added there and the SentryExceptionResolver as a result would be registered in the SentryHubRegistrar.

In Spring Boot auto-configuration we would have to create this bean again, but instead of taking taking the value of exceptionResolverOrder with @Value we could take it directly from SentryProperties.

Add also tests please.

[timtebeek]

@maciejwalkowiak I've updated my pull request with what I think your suggestions were; Hadn't used a BeanDefinitionBuilder before, and not too familiar with Kotlin for tests yet, so you might want to scrutinize those areas a bit more. Welcome to push any changes if it's faster than a back-and-forth in comments. :)

[maciejwalkowiak]

@timtebeek I moved new property back to Spring Boot specific SentryProperties as it should not pollute SentryOptions class. Other than that I believe it's good! Thanks a lot for this PR!

@marandaneto & @bruno-garcia could you please do another review?

[marandaneto]

thanks for doing this @timtebeek and @maciejwalkowiak :) really appreciate it.

2 small suggestions but rather than that LGTM

@maciejwalkowiak lets not squash-merge this one so you can get the rewards too :)

[marandaneto]

thanks a lot, @timtebeek and @maciejwalkowiak appreciate it.

[timtebeek]

Glad to help, and glad to see this merged; Thanks for the finishing touches!