-
-
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
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 27d7cf8 | 397.90 ms | 498.65 ms | 100.75 ms |
| 9fbb112 | 361.43 ms | 427.57 ms | 66.14 ms |
| b3d8889 | 371.33 ms | 426.24 ms | 54.92 ms |
| b3d8889 | 420.46 ms | 453.71 ms | 33.26 ms |
| d217708 | 355.34 ms | 381.39 ms | 26.05 ms |
| 27d7cf8 | 314.17 ms | 347.00 ms | 32.83 ms |
| ee747ae | 396.82 ms | 441.67 ms | 44.86 ms |
| 3d205d0 | 352.15 ms | 432.53 ms | 80.38 ms |
| 85d7417 | 347.21 ms | 394.35 ms | 47.15 ms |
| d5a29b6 | 298.62 ms | 391.78 ms | 93.16 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 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 |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 3d205d0 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| 85d7417 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| d5a29b6 | 1.58 MiB | 2.12 MiB | 549.37 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 |
| 2b6b804 | 312.35 ms | 370.80 ms | 58.45 ms |
| c773a3a | 406.73 ms | 491.63 ms | 84.90 ms |
| b59fc8e | 399.24 ms | 444.62 ms | 45.38 ms |
| c4729d6 | 315.52 ms | 362.84 ms | 47.32 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d76029f | 1.58 MiB | 2.12 MiB | 549.52 KiB |
| 2b6b804 | 1.58 MiB | 2.12 MiB | 551.86 KiB |
| c773a3a | 1.58 MiB | 2.12 MiB | 549.51 KiB |
| b59fc8e | 1.58 MiB | 2.11 MiB | 539.90 KiB |
| c4729d6 | 1.58 MiB | 2.12 MiB | 548.31 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 |
| @@ -0,0 +1,77 @@ | |||
| import io.sentry.ILogger | |||
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.
Missing package declaration at the beginning of the file. The Kotlin file should start with a package statement to match the directory structure and avoid namespace conflicts. Expected: package io.sentry.asyncprofiler.init
Severity: HIGH
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
sentry-async-profiler/src/test/java/io/sentry/asyncprofiler/init/AsyncProfilerInitUtilTest.kt#L1
Potential issue: Missing package declaration at the beginning of the file. The Kotlin
file should start with a package statement to match the directory structure and avoid
namespace conflicts. Expected: `package io.sentry.asyncprofiler.init`
Did we get this right? 👍 / 👎 to inform future reviews.
sentry-async-profiler/src/test/java/io/sentry/asyncprofiler/init/AsyncProfilerInitUtilTest.kt
Show resolved
Hide resolved
| final IProfileConverter profileConverter = | ||
| ProfilingServiceLoader.loadProfileConverter(); | ||
| if (profileConverter != null) { | ||
| if (!NoOpProfileConverter.getInstance().equals(profileConverter)) { |
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.
In the fromProfileChunk() method that takes an IProfileConverter parameter (line 307), the code checks if a converter is a NoOp using reference equality: !NoOpProfileConverter.getInstance().equals(profileConverter). This relies on instance equality and the singleton pattern holding throughout the application. A more robust approach would be to use instanceof check: !(profileConverter instanceof NoOpProfileConverter). This makes the code less fragile and independent of instance reuse patterns.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentry/src/main/java/io/sentry/SentryEnvelopeItem.java#L307
Potential issue: In the `fromProfileChunk()` method that takes an `IProfileConverter`
parameter (line 307), the code checks if a converter is a NoOp using reference equality:
`!NoOpProfileConverter.getInstance().equals(profileConverter)`. This relies on instance
equality and the singleton pattern holding throughout the application. A more robust
approach would be to use `instanceof` check: `!(profileConverter instanceof
NoOpProfileConverter)`. This makes the code less fragile and independent of instance
reuse patterns.
Did we get this right? 👍 / 👎 to inform future reviews.
| final @NotNull ProfileChunk profileChunk, final @NotNull ISerializer serializer) | ||
| throws SentryEnvelopeException { | ||
|
|
||
| return fromProfileChunk(profileChunk, serializer, NoOpProfileConverter.getInstance()); |
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 new overload of fromProfileChunk() that takes an IProfileConverter changes the semantics of the original method. The original method at line 282 now delegates to the new overload with NoOpProfileConverter.getInstance(), which means if a real converter was available via ProfilingServiceLoader, it will no longer be used in the delegating method. This is a breaking change in behavior. Consider documenting this change in the javadoc or adding a deprecation notice if the old method is meant to be phased out.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentry/src/main/java/io/sentry/SentryEnvelopeItem.java#L282-L285
Potential issue: The new overload of `fromProfileChunk()` that takes an
`IProfileConverter` changes the semantics of the original method. The original method at
line 282 now delegates to the new overload with `NoOpProfileConverter.getInstance()`,
which means if a real converter was available via `ProfilingServiceLoader`, it will no
longer be used in the delegating method. This is a breaking change in behavior. Consider
documenting this change in the javadoc or adding a deprecation notice if the old method
is meant to be phased out.
Did we get this right? 👍 / 👎 to inform future reviews.
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.
📜 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