-
-
Notifications
You must be signed in to change notification settings - Fork 465
Refactor ReactorUtils into its own sentry-reactor module
#4155
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
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 |
adinauer
left a comment
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. |
adinauer
left a comment
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
ReactorUtilsinto its ownsentry-reactormodule💡 Motivation and Context
Part of #3144
💚 How did you test it?
Unit tests and manual tests with the
webfluxsamples.📝 Checklist
sendDefaultPIIis 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-registryand 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.