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

Improve versatility of exception resolver component for Spring with more flexible API for consumers. #2577

Merged
merged 5 commits into from
Mar 6, 2023

Conversation

detouched
Copy link
Contributor

📜 Description

Refactor SentryExceptionResolver to improve its API: extract Sentry event and hint creation from resolveException method into protected methods to allow easier and more flexible override of any of these pieces by the consumers of the Spring integration module.

💡 Motivation and Context

A very common scenario for the apps I work on is to return the Sentry ID in the ModelAndView back to the client who faced a problem so that they could then reach out to me providing that ID which makes it significantly quicker and easier to locate the relevant report and hence trace down the root cause.

The existing implementation of SentryExceptionResolver didn't care about the Sentry ID returned from IHub.captureEvent(event, hint) and didn't provide a simple way to override this behavior. This change allows a subclass to override just the right bit of code and reuse event an hint creation methods in order to implement the aforementioned scenario. Similarly, it allows to override event and/or hint creation logic without requiring to modify the capture call to the IHub.

💚 How did you test it?

The SentryExceptionResolver didn't have any unit tests at all. I added the tests for the existing logic (verifying the shape of the event and the hint passed to the IHub), and for the introduced refactoring (ensuring that an override of event and hint creation methods is applied correctly).

📝 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

None.

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.

Looking good, I just added one comment. 🚀

return event;
}

protected Hint createHint(
Copy link
Member

Choose a reason for hiding this comment

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

This should be annotated with @NotNull (same for createEvent). This way anyone overriding the methods in Kotlin will see the right Nullability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do. I guess this one should actually be @Nullable given that the signature of the hub is:

SentryId captureEvent(@NotNull SentryEvent event, @Nullable Hint hint);

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. As @markushi suggested, can you please use org.jetbrains.annotations.NotNull and org.jetbrains.annotations.Nullable.

And may I also ask you to duplicate your changes and the test into the sentry-spring-jakarta module which supports Spring Boot 3 / Spring 6 🙏 ?

CHANGELOG.md Outdated Show resolved Hide resolved
detouched and others added 3 commits March 3, 2023 10:11
Refactor `SentryExceptionResolver` to improve its API in order to allow
easier behavior tweaking by Spring Boot integration module consumers.
Include pull request id in the changelog record.

Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
Similarly to Spring Boot 2 module, refactor `SentryExceptionResolver`
in Jakarta package to improve its API in order to allow easier
behavior tweaking by Spring Boot integration module consumers.
@adinauer adinauer changed the title feat: Improve API of SentryExceptionResolver Improve versatility of exception resolver component for Spring with more flexible API for consumers. Mar 3, 2023
Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Thank you @detouched! LGTM 🚀

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (7c199d8) 80.42% compared to head (a639db8) 80.43%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2577      +/-   ##
============================================
+ Coverage     80.42%   80.43%   +0.01%     
- Complexity     4024     4028       +4     
============================================
  Files           333      333              
  Lines         15181    15189       +8     
  Branches       1979     1979              
============================================
+ Hits          12210    12218       +8     
  Misses         2192     2192              
  Partials        779      779              
Impacted Files Coverage Δ
...sentry/spring/jakarta/SentryExceptionResolver.java 100.00% <100.00%> (ø)
...java/io/sentry/spring/SentryExceptionResolver.java 100.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.

@@ -17,7 +23,6 @@
- This is still considered experimental. Once we have enough feedback we may turn this on by default.
- Checkout the sample here: https://github.com/getsentry/sentry-java/tree/main/sentry-samples/sentry-samples-spring-boot-webflux-jakarta
- A new hub is now cloned from the main hub for every request
- Improve versatility of exception resolver component for Spring with more flexible API for consumers. ([#2577](https://github.com/getsentry/sentry-java/pull/2577))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, my bad while merging the change from the upstream, sorry.

Thanks for fixing it!

Copy link
Member

Choose a reason for hiding this comment

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

No worries, happens to us as well 😅

@adinauer adinauer merged commit 4eb4794 into getsentry:main Mar 6, 2023
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.

None yet

3 participants