Resolve spring properties in @SentryCheckIn annotation (#3194)

* resolve spring properties in @SentryCheckIn annotation * format * add changelog * guard against exceptions when resolving property, add test * Format code * Update sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/checkin/SentryCheckInAdvice.java Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com> * Format code * cr changes, fix tests --------- Co-authored-by: Sentry Github Bot <bot+github-bot@sentry.io> Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
getsentry · Feb 19, 2024 · d007225 · d007225
1 parent 95a98b5
commit d007225
Show file tree

Hide file tree

Showing 7 changed files with 278 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 - Add new threshold parameters to monitor config ([#3181](https://github.com/getsentry/sentry-java/pull/3181))
 - Report process init time as a span for app start performance ([#3159](https://github.com/getsentry/sentry-java/pull/3159))
 - (perf-v2): Calculate frame delay on a span level ([#3197](https://github.com/getsentry/sentry-java/pull/3197))
+- Resolve spring properties in @SentryCheckIn annotation ([#3194](https://github.com/getsentry/sentry-java/pull/3194))
 
 ### Fixes
 

diff --git a/sentry-spring-jakarta/api/sentry-spring-jakarta.api b/sentry-spring-jakarta/api/sentry-spring-jakarta.api
@@ -94,10 +94,11 @@ public abstract interface annotation class io/sentry/spring/jakarta/checkin/Sent
  public abstract fun value ()Ljava/lang/String;
 }
 
-public class io/sentry/spring/jakarta/checkin/SentryCheckInAdvice : org/aopalliance/intercept/MethodInterceptor {
+public class io/sentry/spring/jakarta/checkin/SentryCheckInAdvice : org/aopalliance/intercept/MethodInterceptor, org/springframework/context/EmbeddedValueResolverAware {
  public fun <init> ()V
  public fun <init> (Lio/sentry/IHub;)V
  public fun invoke (Lorg/aopalliance/intercept/MethodInvocation;)Ljava/lang/Object;
+ public fun setEmbeddedValueResolver (Lorg/springframework/util/StringValueResolver;)V
 }
 
 public class io/sentry/spring/jakarta/checkin/SentryCheckInAdviceConfiguration {

diff --git a/...ry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/checkin/SentryCheckInAdvice.java b/...ry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/checkin/SentryCheckInAdvice.java
@@ -17,8 +17,10 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.springframework.aop.support.AopUtils;
+import org.springframework.context.EmbeddedValueResolverAware;
 import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.util.ObjectUtils;
+import org.springframework.util.StringValueResolver;
 
 /**
  * Reports execution of every bean method annotated with {@link SentryCheckIn} as a monitor
@@ -27,9 +29,11 @@
 @ApiStatus.Internal
 @ApiStatus.Experimental
 @Open
-public class SentryCheckInAdvice implements MethodInterceptor {
+public class SentryCheckInAdvice implements MethodInterceptor, EmbeddedValueResolverAware {
  private final @NotNull IHub hub;
 
+ private @Nullable StringValueResolver resolver;
+
  public SentryCheckInAdvice() {
  this(HubAdapter.getInstance());
  }
@@ -51,7 +55,25 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
  }
 
  final boolean isHeartbeatOnly = checkInAnnotation.heartbeat();
- final @Nullable String monitorSlug = checkInAnnotation.value();
+
+ @Nullable String monitorSlug = checkInAnnotation.value();
+
+ if (resolver != null) {
+ try {
+ monitorSlug = resolver.resolveStringValue(checkInAnnotation.value());
+ } catch (Throwable e) {
+ // When resolving fails, we fall back to the original string which may contain unresolved
+ // expressions. Testing shows this can also happen if properties cannot be resolved (without
+ // an exception being thrown). Sentry should alert the user about missed checkins in this
+ // case since the monitor slug won't match what is configured in Sentry.
+ hub.getOptions()
+ .getLogger()
+ .log(
+ SentryLevel.WARNING,
+ "Slug for method annotated with @SentryCheckIn could not be resolved from properties.",
+ e);
+ }
+ }
 
  if (ObjectUtils.isEmpty(monitorSlug)) {
  hub.getOptions()
@@ -85,4 +107,9 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
  hub.popScope();
  }
  }
+
+ @Override
+ public void setEmbeddedValueResolver(StringValueResolver resolver) {
+ this.resolver = resolver;
+ }
 }
diff --git a/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryCheckInAdviceTest.kt b/sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryCheckInAdviceTest.kt
@@ -21,20 +21,24 @@ import org.mockito.kotlin.times
 import org.mockito.kotlin.verify
 import org.mockito.kotlin.whenever
 import org.springframework.beans.factory.annotation.Autowired
+import org.springframework.beans.factory.config.ConfigurableListableBeanFactory
 import org.springframework.context.annotation.Bean
 import org.springframework.context.annotation.Configuration
 import org.springframework.context.annotation.EnableAspectJAutoProxy
 import org.springframework.context.annotation.Import
+import org.springframework.context.support.PropertySourcesPlaceholderConfigurer
+import org.springframework.test.context.TestPropertySource
 import org.springframework.test.context.junit.jupiter.SpringJUnitConfig
 import org.springframework.test.context.junit4.SpringRunner
-import kotlin.RuntimeException
+import org.springframework.util.StringValueResolver
 import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertNotNull
 
 @RunWith(SpringRunner::class)
 @SpringJUnitConfig(SentryCheckInAdviceTest.Config::class)
+@TestPropertySource(properties = ["my.cron.slug = mypropertycronslug"])
 class SentryCheckInAdviceTest {
 
  @Autowired
@@ -46,6 +50,9 @@ class SentryCheckInAdviceTest {
  @Autowired
  lateinit var sampleServiceHeartbeat: SampleServiceHeartbeat
 
+ @Autowired
+ lateinit var sampleServiceSpringProperties: SampleServiceSpringProperties
+
  @Autowired
  lateinit var hub: IHub
 
@@ -157,6 +164,66 @@ class SentryCheckInAdviceTest {
  verify(hub, never()).popScope()
  }
 
+ @Test
+ fun `when @SentryCheckIn is passed a spring property it is resolved correctly`() {
+ val checkInId = SentryId()
+ val checkInCaptor = argumentCaptor<CheckIn>()
+ whenever(hub.captureCheckIn(checkInCaptor.capture())).thenReturn(checkInId)
+ val result = sampleServiceSpringProperties.hello()
+ assertEquals(1, result)
+ assertEquals(1, checkInCaptor.allValues.size)
+
+ val doneCheckIn = checkInCaptor.lastValue
+ assertEquals("mypropertycronslug", doneCheckIn.monitorSlug)
+ assertEquals(CheckInStatus.OK.apiName(), doneCheckIn.status)
+ assertNotNull(doneCheckIn.duration)
+
+ val order = inOrder(hub)
+ order.verify(hub).pushScope()
+ order.verify(hub).captureCheckIn(any())
+ order.verify(hub).popScope()
+ }
+
+ @Test
+ fun `when @SentryCheckIn is passed a spring property that does not exist, raw value is used`() {
+ val checkInId = SentryId()
+ val checkInCaptor = argumentCaptor<CheckIn>()
+ whenever(hub.captureCheckIn(checkInCaptor.capture())).thenReturn(checkInId)
+ val result = sampleServiceSpringProperties.helloUnresolvedProperty()
+ assertEquals(1, result)
+ assertEquals(1, checkInCaptor.allValues.size)
+
+ val doneCheckIn = checkInCaptor.lastValue
+ assertEquals("\${my.cron.unresolved.property}", doneCheckIn.monitorSlug)
+ assertEquals(CheckInStatus.OK.apiName(), doneCheckIn.status)
+ assertNotNull(doneCheckIn.duration)
+
+ val order = inOrder(hub)
+ order.verify(hub).pushScope()
+ order.verify(hub).captureCheckIn(any())
+ order.verify(hub).popScope()
+ }
+
+ @Test
+ fun `when @SentryCheckIn is passed a spring property that causes an exception, raw value is used`() {
+ val checkInId = SentryId()
+ val checkInCaptor = argumentCaptor<CheckIn>()
+ whenever(hub.captureCheckIn(checkInCaptor.capture())).thenReturn(checkInId)
+ val result = sampleServiceSpringProperties.helloExceptionProperty()
+ assertEquals(1, result)
+ assertEquals(1, checkInCaptor.allValues.size)
+
+ val doneCheckIn = checkInCaptor.lastValue
+ assertEquals("\${my.cron.exception.property}", doneCheckIn.monitorSlug)
+ assertEquals(CheckInStatus.OK.apiName(), doneCheckIn.status)
+ assertNotNull(doneCheckIn.duration)
+
+ val order = inOrder(hub)
+ order.verify(hub).pushScope()
+ order.verify(hub).captureCheckIn(any())
+ order.verify(hub).popScope()
+ }
+
  @Configuration
  @EnableAspectJAutoProxy(proxyTargetClass = true)
  @Import(SentryCheckInAdviceConfiguration::class, SentryCheckInPointcutConfiguration::class)
@@ -171,12 +238,21 @@ class SentryCheckInAdviceTest {
  @Bean
  open fun sampleServiceHeartbeat() = SampleServiceHeartbeat()
 
+ @Bean
+ open fun sampleServiceSpringProperties() = SampleServiceSpringProperties()
+
  @Bean
  open fun hub(): IHub {
  val hub = mock<IHub>()
  Sentry.setCurrentHub(hub)
  return hub
  }
+
+ companion object {
+ @Bean
+ @JvmStatic
+ fun propertySourcesPlaceholderConfigurer() = MyPropertyPlaceholderConfigurer()
+ }
  }
 
  open class SampleService {
@@ -206,4 +282,33 @@ class SentryCheckInAdviceTest {
  throw RuntimeException("thrown on purpose")
  }
  }
+
+ open class SampleServiceSpringProperties {
+
+ @SentryCheckIn("\${my.cron.slug}", heartbeat = true)
+ open fun hello() = 1
+
+ @SentryCheckIn("\${my.cron.unresolved.property}", heartbeat = true)
+ open fun helloUnresolvedProperty() = 1
+
+ @SentryCheckIn("\${my.cron.exception.property}", heartbeat = true)
+ open fun helloExceptionProperty() = 1
+ }
+
+ class MyPropertyPlaceholderConfigurer : PropertySourcesPlaceholderConfigurer() {
+
+ override fun doProcessProperties(
+ beanFactoryToProcess: ConfigurableListableBeanFactory,
+ valueResolver: StringValueResolver
+ ) {
+ val wrappedResolver = StringValueResolver { strVal: String ->
+ if ("\${my.cron.exception.property}".equals(strVal)) {
+ throw IllegalArgumentException("Cannot resolve property: $strVal")
+ } else {
+ valueResolver.resolveStringValue(strVal)
+ }
+ }
+ super.doProcessProperties(beanFactoryToProcess, wrappedResolver)
+ }
+ }
 }
diff --git a/sentry-spring/api/sentry-spring.api b/sentry-spring/api/sentry-spring.api
@@ -94,10 +94,11 @@ public abstract interface annotation class io/sentry/spring/checkin/SentryCheckI
  public abstract fun value ()Ljava/lang/String;
 }
 
-public class io/sentry/spring/checkin/SentryCheckInAdvice : org/aopalliance/intercept/MethodInterceptor {
+public class io/sentry/spring/checkin/SentryCheckInAdvice : org/aopalliance/intercept/MethodInterceptor, org/springframework/context/EmbeddedValueResolverAware {
  public fun <init> ()V
  public fun <init> (Lio/sentry/IHub;)V
  public fun invoke (Lorg/aopalliance/intercept/MethodInvocation;)Ljava/lang/Object;
+ public fun setEmbeddedValueResolver (Lorg/springframework/util/StringValueResolver;)V
 }
 
 public class io/sentry/spring/checkin/SentryCheckInAdviceConfiguration {

diff --git a/sentry-spring/src/main/java/io/sentry/spring/checkin/SentryCheckInAdvice.java b/sentry-spring/src/main/java/io/sentry/spring/checkin/SentryCheckInAdvice.java
@@ -17,8 +17,10 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.springframework.aop.support.AopUtils;
+import org.springframework.context.EmbeddedValueResolverAware;
 import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.util.ObjectUtils;
+import org.springframework.util.StringValueResolver;
 
 /**
  * Reports execution of every bean method annotated with {@link SentryCheckIn} as a monitor
@@ -27,9 +29,11 @@
 @ApiStatus.Internal
 @ApiStatus.Experimental
 @Open
-public class SentryCheckInAdvice implements MethodInterceptor {
+public class SentryCheckInAdvice implements MethodInterceptor, EmbeddedValueResolverAware {
  private final @NotNull IHub hub;
 
+ private @Nullable StringValueResolver resolver;
+
  public SentryCheckInAdvice() {
  this(HubAdapter.getInstance());
  }
@@ -51,7 +55,28 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
  }
 
  final boolean isHeartbeatOnly = checkInAnnotation.heartbeat();
- final @Nullable String monitorSlug = checkInAnnotation.value();
+
+ @Nullable String monitorSlug = checkInAnnotation.value();
+
+ if (resolver != null) {
+ try {
+ monitorSlug = resolver.resolveStringValue(checkInAnnotation.value());
+ } catch (Throwable e) {
+ // When resolving fails, we fall back to the original string which may contain unresolved
+ // expressions.
+ // Testing shows this can also happen if properties cannot be resolved (without an exception
+ // being thrown).
+ // Sentry should alert the user about missed checkins in this case since the monitor slug
+ // won't match
+ // what is configured in Sentry.
+ hub.getOptions()
+ .getLogger()
+ .log(
+ SentryLevel.WARNING,
+ "Slug for method annotated with @SentryCheckIn could not be resolved from properties.",
+ e);
+ }
+ }
 
  if (ObjectUtils.isEmpty(monitorSlug)) {
  hub.getOptions()
@@ -85,4 +110,9 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
  hub.popScope();
  }
  }
+
+ @Override
+ public void setEmbeddedValueResolver(StringValueResolver resolver) {
+ this.resolver = resolver;
+ }
 }