-
-
Notifications
You must be signed in to change notification settings - Fork 448
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
Refactor ReactorUtils
into its own sentry-reactor
module
#4155
Conversation
The commented out tests in Edit: I was not initializing the SDK :) Also the ThreadLocalAccessor can be set up with SPI instead. |
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c2c78de | 415.28 ms | 505.08 ms | 89.80 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c2c78de | 1.58 MiB | 2.21 MiB | 640.27 KiB |
Previous results on branch: lcian/ref/reactor-as-module
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7276711 | 444.02 ms | 475.80 ms | 31.78 ms |
67c204c | 397.52 ms | 481.22 ms | 83.70 ms |
c320936 | 369.81 ms | 481.23 ms | 111.42 ms |
74b0da5 | 380.19 ms | 454.58 ms | 74.40 ms |
4bbe412 | 423.04 ms | 466.40 ms | 43.36 ms |
10216d9 | 482.39 ms | 514.26 ms | 31.87 ms |
3dd4220 | 373.60 ms | 468.00 ms | 94.40 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7276711 | 1.58 MiB | 2.21 MiB | 640.27 KiB |
67c204c | 1.58 MiB | 2.21 MiB | 640.25 KiB |
c320936 | 1.58 MiB | 2.21 MiB | 640.27 KiB |
74b0da5 | 1.58 MiB | 2.21 MiB | 640.27 KiB |
4bbe412 | 1.58 MiB | 2.21 MiB | 640.27 KiB |
10216d9 | 1.58 MiB | 2.21 MiB | 640.25 KiB |
3dd4220 | 1.58 MiB | 2.21 MiB | 640.27 KiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fully done with reviewing the PR, so I'm just sending the comments I have already.
Also I opened a PR to fix some things: #4160
sentry-reactor/src/main/java/io/sentry/reactor/SentryReactorThreadLocalAccessor.java
Show resolved
Hide resolved
Other things that might be missing from this PR:
We need manually add this new module to Docs would also be great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
We shouldn't forget to update release registry, then update .craft.yml
here.
Also would be great to have an entry in the root README listing this new module.
Thanks @adinauer . I'll add this to the README after it's released, otherwise we'll have broken links. I'll add this to the issue for v8.3.0 so we don't forget about the missing tasks. |
📜 Description
Refactor
ReactorUtils
into its ownsentry-reactor
module💡 Motivation and Context
Part of #3144
💚 How did you test it?
Unit tests and manual tests with the
webflux
samples.📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps
Pending actions:
Comments
No, it will still be included in the Spring and Spring Boot integrations, just as a different module.
We need to add an entry to the
sentry-release-registry
and then update.craft.yml
(post-release)We will add a deprecation warning and make the old classes extend the new ones, so we don't break existing customers.