-
-
Notifications
You must be signed in to change notification settings - Fork 444
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
Auto add OpenTelemetryLinkErrorEventProcessor for Spring Boot #2429
Auto add OpenTelemetryLinkErrorEventProcessor for Spring Boot #2429
Conversation
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f1150bc | 352.62 ms | 453.27 ms | 100.65 ms |
a04f788 | 321.78 ms | 354.12 ms | 32.35 ms |
4a9c176 | 319.77 ms | 363.20 ms | 43.43 ms |
90e9745 | 314.68 ms | 357.28 ms | 42.60 ms |
3695453 | 314.63 ms | 353.10 ms | 38.47 ms |
16371c5 | 314.02 ms | 394.54 ms | 80.52 ms |
507f924 | 342.51 ms | 402.65 ms | 60.14 ms |
703d523 | 330.27 ms | 377.66 ms | 47.39 ms |
4a9c176 | 320.62 ms | 334.68 ms | 14.06 ms |
87598a5 | 310.41 ms | 373.27 ms | 62.86 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f1150bc | 1.73 MiB | 2.33 MiB | 615.66 KiB |
a04f788 | 1.73 MiB | 2.32 MiB | 609.88 KiB |
4a9c176 | 1.73 MiB | 2.33 MiB | 612.69 KiB |
90e9745 | 1.73 MiB | 2.32 MiB | 608.63 KiB |
3695453 | 1.73 MiB | 2.32 MiB | 611.62 KiB |
16371c5 | 1.73 MiB | 2.32 MiB | 611.62 KiB |
507f924 | 1.73 MiB | 2.32 MiB | 609.95 KiB |
703d523 | 1.73 MiB | 2.33 MiB | 613.23 KiB |
4a9c176 | 1.73 MiB | 2.33 MiB | 612.69 KiB |
87598a5 | 1.73 MiB | 2.33 MiB | 614.63 KiB |
Previous results on branch: feat/auto-add-otel-event-processor-for-spring-boot
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
be7a638 | 339.88 ms | 344.96 ms | 5.08 ms |
3993470 | 295.45 ms | 313.83 ms | 18.39 ms |
349e444 | 332.65 ms | 379.73 ms | 47.08 ms |
c76546c | 317.68 ms | 338.29 ms | 20.61 ms |
7aab380 | 342.61 ms | 355.39 ms | 12.78 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
be7a638 | 1.73 MiB | 2.33 MiB | 615.83 KiB |
3993470 | 1.73 MiB | 2.33 MiB | 615.83 KiB |
349e444 | 1.73 MiB | 2.33 MiB | 613.23 KiB |
c76546c | 1.73 MiB | 2.33 MiB | 613.23 KiB |
7aab380 | 1.73 MiB | 2.33 MiB | 613.23 KiB |
Codecov ReportBase: 80.08% // Head: 80.09% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2429 +/- ##
=========================================
Coverage 80.08% 80.09%
Complexity 3807 3807
=========================================
Files 306 306
Lines 14386 14390 +4
Branches 1904 1904
=========================================
+ Hits 11521 11525 +4
Misses 2118 2118
Partials 747 747
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. |
@@ -139,6 +140,19 @@ static class ContextTagsEventProcessorConfiguration { | |||
} | |||
} | |||
|
|||
@Configuration(proxyBeanMethods = false) | |||
@ConditionalOnProperty(name = "sentry.auto-init", havingValue = "false") |
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.
When not using the agent this will not trigger. At the moment it feels like a niche use case to not use the agent JAR and there doesn't seem to be an easy way of detecting whether the agent is in use. We could add some class that serves as a marker (to the bootstrap classloader) or manipulate the options (byte code manipulation) from the agent but decided not to do that for now. If this becomes a more common use case we can revisit.
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.
- 💡 could also look at the classloader of
OpenTelemetryLinkErrorEventProcessor
. If it's the bootstrap classloader the agent is in use. Not sure yet how to turn that into an@Conditional
for spring config.
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.
- 💡 probably even easier to just have
SentryOptions
injected into the@Bean
method and not create the bean if aOpenTelemetryLinkErrorEventProcessor
has already been registered. Also not sure how to add that to the auto config yet.
...starter-jakarta/src/test/kotlin/io/sentry/spring/boot/jakarta/SentryAutoConfigurationTest.kt
Show resolved
Hide resolved
…dd-otel-event-processor-for-spring-boot
...try/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SentrySpanProcessor.java
Show resolved
Hide resolved
@@ -21,7 +21,7 @@ tasks.withType<KotlinCompile>().configureEach { | |||
dependencies { | |||
compileOnly(projects.sentry) | |||
|
|||
compileOnly(Config.Libs.OpenTelemetry.otelSdk) | |||
implementation(Config.Libs.OpenTelemetry.otelSdk) |
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.
hm, why did we change it to implementation
? It's not possible to use this module without otel, right? Or is the assumption that people will just include our sentry-opentelemetry-core
package, but not otel itself?
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.
The intention was to make it easier to use sentry-opentelemetry-agent
with SENTRY_AUTO_INIT=false
. Currently you need to add sentry-opentelemetry-core
to the dependencies so you can enjoy linking of errors to transactions. With compileOnly
you'd also have to include the correct version of the OTEL SDK, with implementation
it just drags it in. For sentry-opentelemetry-agent
we exclude it again, which users also have to do, if they don't want to use the agent but have a different OTEL version than we use or their build system can't figure out versions.
Another approach (for later) would be to have yet another gradle module, that we can then add to the bootstrap classloader in the agent, thus not requiring the explicit dependency on sentry-opentelemetry-core
at all, when setting SENTRY_AUTO_INIT=false
.
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.
alright, just beware that implementation
will override whatever version the users are using (if it's lower than ours). If that's what we want, then I'm good 👍
📜 Description
SentryAutoConfiguration.java
now automatically provides aOpenTelemetryLinkErrorEventProcessor
bean in case it's present and auto init (provided bysentry-opentelemetry-agent
) has been turned off.💡 Motivation and Context
This makes it easier for users who are not using auto init (provided by
sentry-opentelemetry-agent
) as they no longer need to manually provicdeOpenTelemetryLinkErrorEventProcessor
as a bean or add it to the options.💚 How did you test it?
Manually using Spring Boot sample app; automated tests
📝 Checklist
🔮 Next steps