-
-
Notifications
You must be signed in to change notification settings - Fork 461
Fix profiling init for Spring and Spring Boot w Agent auto-init #4815
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
base: main
Are you sure you want to change the base?
Conversation
…options, add configuration class to load the profiler and converter after spring boot starts in agent mode
| import org.springframework.context.annotation.Import; | ||
|
|
||
| @Configuration(proxyBeanMethods = false) | ||
| @ConditionalOnClass(name = {"io.sentry.opentelemetry.agent.AgentMarker"}) |
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.
Make it conditional on the async profiler class too?
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.
Sounds good, I see no reason to run this if profiler isn't there.
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9fbb112 | 401.87 ms | 515.87 ms | 114.00 ms |
| b3d8889 | 371.84 ms | 447.49 ms | 75.65 ms |
| d217708 | 411.22 ms | 430.86 ms | 19.63 ms |
| 27d7cf8 | 397.90 ms | 498.65 ms | 100.75 ms |
| fcec2f2 | 311.35 ms | 384.94 ms | 73.59 ms |
| ee747ae | 358.21 ms | 389.41 ms | 31.20 ms |
| ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
| b77456b | 393.26 ms | 441.10 ms | 47.84 ms |
| 27d7cf8 | 309.43 ms | 364.27 ms | 54.85 ms |
| b3d8889 | 371.33 ms | 426.24 ms | 54.92 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.51 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| b77456b | 1.58 MiB | 2.12 MiB | 548.11 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 KiB |
Previous results on branch: feat/profiling-w-spring-otel-agent
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d76029f | 339.65 ms | 357.82 ms | 18.17 ms |
| c4729d6 | 315.52 ms | 362.84 ms | 47.32 ms |
| 2b6b804 | 312.35 ms | 370.80 ms | 58.45 ms |
| db75a56 | 331.37 ms | 402.82 ms | 71.45 ms |
| c773a3a | 406.73 ms | 491.63 ms | 84.90 ms |
| 3832729 | 297.73 ms | 353.21 ms | 55.48 ms |
| b59fc8e | 399.24 ms | 444.62 ms | 45.38 ms |
| 30b9948 | 314.20 ms | 354.71 ms | 40.51 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d76029f | 1.58 MiB | 2.12 MiB | 549.52 KiB |
| c4729d6 | 1.58 MiB | 2.12 MiB | 548.31 KiB |
| 2b6b804 | 1.58 MiB | 2.12 MiB | 551.86 KiB |
| db75a56 | 1.58 MiB | 2.12 MiB | 551.94 KiB |
| c773a3a | 1.58 MiB | 2.12 MiB | 549.51 KiB |
| 3832729 | 1.58 MiB | 2.12 MiB | 551.94 KiB |
| b59fc8e | 1.58 MiB | 2.11 MiB | 539.90 KiB |
| 30b9948 | 1.58 MiB | 2.12 MiB | 551.90 KiB |
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean(name = "sentryOpenTelemetryProfilerConfiguration") | ||
| public IContinuousProfiler sentryOpenTelemetryProfilerConfiguration() { |
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.
Extract init into util to streamline initialization code
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean(name = "sentryOpenTelemetryProfilerConverterConfiguration") | ||
| public IProfileConverter sentryOpenTelemetryProfilerConverterConfiguration() { |
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.
Extract init into util to streamline initialization code
|
@sentry review |
|
@cursor review |
sentry-async-profiler/src/test/java/io/sentry/asyncprofiler/init/AsyncProfilerInitUtilTest.kt
Show resolved
Hide resolved
sentry-async-profiler/src/test/java/io/sentry/asyncprofiler/init/AsyncProfilerInitUtilTest.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| fun `initialize converter returns no-op converter if converter already initialized`() { |
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.
m should it simply return the existing one instead of noop that potentially causes profiling to break?
sentry-spring-7/src/main/java/io/sentry/spring7/SentryProfilerConfiguration.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean(name = "sentryOpenTelemetryProfilerConfiguration") | ||
| public IContinuousProfiler sentryOpenTelemetryProfilerConfiguration() { |
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.
Is there an expected order to these bean evaluations?
| contextRunner | ||
| .withPropertyValues( | ||
| "sentry.dsn=http://key@localhost/proj", | ||
| "sentry.traces-sample-rate=1.0", |
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.
Should this also have "sentry.profile-session-sample-rate=1.0",?
| "debug=true", | ||
| ) | ||
| .run { | ||
| assertThat(it).hasSingleBean(IContinuousProfiler::class.java) |
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.
Is this currently simply asserting it has noop instances?
| import org.springframework.context.annotation.Import; | ||
|
|
||
| @Configuration(proxyBeanMethods = false) | ||
| @ConditionalOnClass(name = {"io.sentry.opentelemetry.agent.AgentMarker"}) |
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.
Sounds good, I see no reason to run this if profiler isn't there.
| return previousOptions.getInitPriority().ordinal() <= newOptions.getInitPriority().ordinal(); | ||
| } | ||
|
|
||
| public static IContinuousProfiler initializeProfiler(@NotNull SentryOptions options) { |
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.
m should we instead check if options already have a profiler / converter set (that's not noop) and return that?
IMO that makes it easier to reason about SDK init.
Looking at the tests it seems we're potentially replacing a working profiler / converter with noop (if misusing this util).
…Configuration.java Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
…ntry/sentry-java into feat/profiling-w-spring-otel-agent
| assertThat(it).hasSingleBean(IContinuousProfiler::class.java) | ||
| assertThat(it).hasSingleBean(IProfileConverter::class.java) | ||
| } | ||
| } |
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.
Bug: Continuous Profiling Test Lacks Required Sample Rate
The test "when AgentMarker is on the classpath and ContinuousProfiling is enabled..." doesn't actually enable continuous profiling. It's missing the sentry.profile-session-sample-rate=1.0 property, which results in NoOp profiler beans being created instead of functional ones. This makes the test misleading about verifying actual continuous profiling setup, especially since the companion test correctly includes this property.
📜 Description
Add
@Configurationclasses to initialize the profiler when running in OTEL Agent auto-init mode.💡 Motivation and Context
Fixes #4855
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps