-
-
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?
Changes from all commits
a4ce01a
39225ff
40395fb
f8f253f
c02c3dd
d180c19
5e75584
dde02b9
0514903
2cb26e1
737c147
41d2c6a
87071dd
8fca29a
40bb0a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| package io.sentry.asyncprofiler.init | ||
|
|
||
| import io.sentry.ILogger | ||
| import io.sentry.ISentryExecutorService | ||
| import io.sentry.NoOpContinuousProfiler | ||
| import io.sentry.NoOpProfileConverter | ||
| import io.sentry.SentryOptions | ||
| import io.sentry.asyncprofiler.profiling.JavaContinuousProfiler | ||
| import io.sentry.asyncprofiler.provider.AsyncProfilerProfileConverterProvider | ||
| import io.sentry.util.InitUtil | ||
| import kotlin.test.Test | ||
| import kotlin.test.assertSame | ||
| import org.mockito.kotlin.mock | ||
|
|
||
| class AsyncProfilerInitUtilTest { | ||
|
|
||
| @Test | ||
| fun `initialize Profiler returns no-op profiler if profiling disabled`() { | ||
| val options = SentryOptions() | ||
| val profiler = InitUtil.initializeProfiler(options) | ||
| assert(profiler is NoOpContinuousProfiler) | ||
| } | ||
|
|
||
| @Test | ||
| fun `initialize Converter returns no-op converter if profiling disabled`() { | ||
| val options = SentryOptions() | ||
| val converter = InitUtil.initializeProfileConverter(options) | ||
| assert(converter is NoOpProfileConverter) | ||
| } | ||
|
|
||
| @Test | ||
| fun `initialize profiler returns the existing profiler from options if already initialized`() { | ||
| val initialProfiler = | ||
| JavaContinuousProfiler(mock<ILogger>(), "", 10, mock<ISentryExecutorService>()) | ||
| val options = | ||
| SentryOptions().also { | ||
| it.setProfileSessionSampleRate(1.0) | ||
| it.setContinuousProfiler(initialProfiler) | ||
| } | ||
|
|
||
| val profiler = InitUtil.initializeProfiler(options) | ||
| assertSame(initialProfiler, profiler) | ||
| } | ||
|
|
||
| @Test | ||
| fun `initialize converter returns the existing converter from options if already initialized`() { | ||
| val initialConverter = AsyncProfilerProfileConverterProvider.AsyncProfilerProfileConverter() | ||
| val options = | ||
| SentryOptions().also { | ||
| it.setProfileSessionSampleRate(1.0) | ||
| it.profilerConverter = initialConverter | ||
| } | ||
|
|
||
| val converter = InitUtil.initializeProfileConverter(options) | ||
| assertSame(initialConverter, converter) | ||
| } | ||
|
|
||
| @Test | ||
| fun `initialize Profiler returns JavaContinuousProfiler if profiling enabled but profiler not yet initialized`() { | ||
| val options = SentryOptions().also { it.setProfileSessionSampleRate(1.0) } | ||
| val profiler = InitUtil.initializeProfiler(options) | ||
| assertSame(profiler, options.continuousProfiler) | ||
| assert(profiler is JavaContinuousProfiler) | ||
| } | ||
|
|
||
| @Test | ||
| fun `initialize Converter returns AsyncProfilerProfileConverterProvider if profiling enabled but profiler not yet initialized`() { | ||
| val options = SentryOptions().also { it.setProfileSessionSampleRate(1.0) } | ||
| val converter = InitUtil.initializeProfileConverter(options) | ||
| assertSame(converter, options.profilerConverter) | ||
| assert(converter is AsyncProfilerProfileConverterProvider.AsyncProfilerProfileConverter) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| package io.sentry.spring7; | ||
|
|
||
| import com.jakewharton.nopen.annotation.Open; | ||
| import io.sentry.IContinuousProfiler; | ||
| import io.sentry.IProfileConverter; | ||
| import io.sentry.NoOpContinuousProfiler; | ||
| import io.sentry.NoOpProfileConverter; | ||
| import io.sentry.Sentry; | ||
| import io.sentry.SentryOptions; | ||
| import io.sentry.util.InitUtil; | ||
| import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; | ||
| import org.springframework.context.annotation.Bean; | ||
| import org.springframework.context.annotation.Configuration; | ||
|
|
||
| /** | ||
| * Handles late initialization of the profiler if the application is run with the Opentelemetry | ||
| * Agent in auto-init mode. In that case the agent cannot initialize the profiler yet and falls back | ||
| * to No-Op implementations. This Configuration sets the profiler and converter on the options if | ||
| * that was the case. | ||
| */ | ||
| @Configuration(proxyBeanMethods = false) | ||
| @Open | ||
| public class SentryProfilerConfiguration { | ||
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean(name = "sentryOpenTelemetryProfilerConfiguration") | ||
| public IContinuousProfiler sentryOpenTelemetryProfilerConfiguration() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there an expected order to these bean evaluations? |
||
| SentryOptions options = Sentry.getGlobalScope().getOptions(); | ||
| IContinuousProfiler profiler = NoOpContinuousProfiler.getInstance(); | ||
|
|
||
| if (Sentry.isEnabled()) { | ||
| return InitUtil.initializeProfiler(options); | ||
| } else { | ||
| return profiler; | ||
| } | ||
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean(name = "sentryOpenTelemetryProfilerConverterConfiguration") | ||
| public IProfileConverter sentryOpenTelemetryProfilerConverterConfiguration() { | ||
| SentryOptions options = Sentry.getGlobalScope().getOptions(); | ||
| IProfileConverter converter = NoOpProfileConverter.getInstance(); | ||
|
|
||
| if (Sentry.isEnabled()) { | ||
| return InitUtil.initializeProfileConverter(options); | ||
| } else { | ||
| return converter; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package io.sentry.spring.boot4; | ||
|
|
||
| import com.jakewharton.nopen.annotation.Open; | ||
| import io.sentry.spring7.SentryProfilerConfiguration; | ||
| import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; | ||
| import org.springframework.context.annotation.Configuration; | ||
| import org.springframework.context.annotation.Import; | ||
|
|
||
| @Configuration(proxyBeanMethods = false) | ||
| @ConditionalOnClass(name = {"io.sentry.opentelemetry.agent.AgentMarker"}) | ||
| @Open | ||
| @Import(SentryProfilerConfiguration.class) | ||
| public class SentryProfilerAutoConfiguration {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| io.sentry.spring.boot4.SentryAutoConfiguration | ||
| io.sentry.spring.boot4.SentryProfilerAutoConfiguration | ||
| io.sentry.spring.boot4.SentryLogbackAppenderAutoConfiguration | ||
| io.sentry.spring.boot4.SentryWebfluxAutoConfiguration |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ import io.sentry.Breadcrumb | |
| import io.sentry.EventProcessor | ||
| import io.sentry.FilterString | ||
| import io.sentry.Hint | ||
| import io.sentry.IContinuousProfiler | ||
| import io.sentry.IProfileConverter | ||
| import io.sentry.IScopes | ||
| import io.sentry.ITransportFactory | ||
| import io.sentry.Integration | ||
|
|
@@ -87,6 +89,7 @@ class SentryAutoConfigurationTest { | |
| AutoConfigurations.of( | ||
| SentryAutoConfiguration::class.java, | ||
| WebMvcAutoConfiguration::class.java, | ||
| SentryProfilerAutoConfiguration::class.java, | ||
| ) | ||
| ) | ||
|
|
||
|
|
@@ -1037,6 +1040,39 @@ class SentryAutoConfigurationTest { | |
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `when AgentMarker is on the classpath and ContinuousProfiling is enabled IContinuousProfiler and IProfileConverter beans are created and set on options`() { | ||
| SentryIntegrationPackageStorage.getInstance().clearStorage() | ||
| 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 commentThe reason will be displayed to describe this comment to others. Learn more. Should this also have |
||
| "sentry.auto-init=false", | ||
| "debug=true", | ||
| ) | ||
| .run { | ||
| assertThat(it).hasSingleBean(IContinuousProfiler::class.java) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this currently simply asserting it has noop instances? |
||
| assertThat(it).hasSingleBean(IProfileConverter::class.java) | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Continuous Profiling Test Lacks Required Sample RateThe test "when AgentMarker is on the classpath and ContinuousProfiling is enabled..." doesn't actually enable continuous profiling. It's missing the Additional Locations (2) |
||
|
|
||
| @Test | ||
| fun `when AgentMarker is not on the classpath and ContinuousProfiling is enabled IContinuousProfiler and IProfileConverter beans are not created`() { | ||
| SentryIntegrationPackageStorage.getInstance().clearStorage() | ||
| contextRunner | ||
| .withPropertyValues( | ||
| "sentry.dsn=http://key@localhost/proj", | ||
| "sentry.traces-sample-rate=1.0", | ||
| "sentry.profile-session-sample-rate=1.0", | ||
| "debug=true", | ||
| ) | ||
| .withClassLoader(FilteredClassLoader(AgentMarker::class.java, OpenTelemetry::class.java)) | ||
| .run { | ||
| assertThat(it).doesNotHaveBean(IContinuousProfiler::class.java) | ||
| assertThat(it).doesNotHaveBean(IProfileConverter::class.java) | ||
| } | ||
| } | ||
|
|
||
| @Configuration(proxyBeanMethods = false) | ||
| open class CustomSchedulerFactoryBeanCustomizerConfiguration { | ||
| class MyJobListener : JobListener { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package io.sentry.spring.boot.jakarta; | ||
|
|
||
| import com.jakewharton.nopen.annotation.Open; | ||
| import io.sentry.spring.jakarta.SentryProfilerConfiguration; | ||
| import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; | ||
| import org.springframework.context.annotation.Configuration; | ||
| 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 commentThe 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 commentThe 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. |
||
| @Open | ||
| @Import(SentryProfilerConfiguration.class) | ||
| public class SentryProfilerAutoConfiguration {} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| io.sentry.spring.boot.jakarta.SentryAutoConfiguration | ||
| io.sentry.spring.boot.jakarta.SentryProfilerAutoConfiguration | ||
| io.sentry.spring.boot.jakarta.SentryLogbackAppenderAutoConfiguration | ||
| io.sentry.spring.boot.jakarta.SentryWebfluxAutoConfiguration |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ import io.sentry.DataCategory | |
| import io.sentry.EventProcessor | ||
| import io.sentry.FilterString | ||
| import io.sentry.Hint | ||
| import io.sentry.IContinuousProfiler | ||
| import io.sentry.IProfileConverter | ||
| import io.sentry.IScopes | ||
| import io.sentry.ITransportFactory | ||
| import io.sentry.Integration | ||
|
|
@@ -91,6 +93,7 @@ class SentryAutoConfigurationTest { | |
| AutoConfigurations.of( | ||
| SentryAutoConfiguration::class.java, | ||
| WebMvcAutoConfiguration::class.java, | ||
| SentryProfilerAutoConfiguration::class.java, | ||
| ) | ||
| ) | ||
|
|
||
|
|
@@ -1059,6 +1062,39 @@ class SentryAutoConfigurationTest { | |
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `when AgentMarker is on the classpath and ContinuousProfiling is enabled IContinuousProfiler and IProfileConverter beans are created and set on options`() { | ||
| SentryIntegrationPackageStorage.getInstance().clearStorage() | ||
| contextRunner | ||
| .withPropertyValues( | ||
| "sentry.dsn=http://key@localhost/proj", | ||
| "sentry.traces-sample-rate=1.0", | ||
| "sentry.auto-init=false", | ||
| "debug=true", | ||
| ) | ||
| .run { | ||
| 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 commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Missing profile-session sample rate in testsThe tests asserting continuous profiling is enabled are missing the Additional Locations (2) |
||
|
|
||
| @Test | ||
| fun `when AgentMarker is not on the classpath and ContinuousProfiling is enabled IContinuousProfiler and IProfileConverter beans are not created`() { | ||
| SentryIntegrationPackageStorage.getInstance().clearStorage() | ||
| contextRunner | ||
| .withPropertyValues( | ||
| "sentry.dsn=http://key@localhost/proj", | ||
| "sentry.traces-sample-rate=1.0", | ||
| "sentry.profile-session-sample-rate=1.0", | ||
| "debug=true", | ||
| ) | ||
| .withClassLoader(FilteredClassLoader(AgentMarker::class.java, OpenTelemetry::class.java)) | ||
| .run { | ||
| assertThat(it).doesNotHaveBean(IContinuousProfiler::class.java) | ||
| assertThat(it).doesNotHaveBean(IProfileConverter::class.java) | ||
| } | ||
| } | ||
|
|
||
| @Configuration(proxyBeanMethods = false) | ||
| open class CustomSchedulerFactoryBeanCustomizerConfiguration { | ||
| class MyJobListener : JobListener { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package io.sentry.spring.boot; | ||
|
|
||
| import com.jakewharton.nopen.annotation.Open; | ||
| import io.sentry.spring.SentryProfilerConfiguration; | ||
| import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; | ||
| import org.springframework.context.annotation.Configuration; | ||
| import org.springframework.context.annotation.Import; | ||
|
|
||
| @Configuration(proxyBeanMethods = false) | ||
| @ConditionalOnClass(name = {"io.sentry.opentelemetry.agent.AgentMarker"}) | ||
| @Open | ||
| @Import(SentryProfilerConfiguration.class) | ||
| public class SentryProfilerAutoConfiguration {} |
Uh oh!
There was an error while loading. Please reload this page.