Skip to content

Commit ff1e1e2

Browse files
wedamijawedamija
committed
fix(spring): Pick the cron parser rules and skip fixed delays
Read crons the way CronSequenceGenerator does on Spring before 5.3. Derive the monitor config before the job's try block and catch any Throwable, so a failure there never skips the job. Fixed delays get no config.
1 parent bbdbf02 commit ff1e1e2

6 files changed

Lines changed: 288 additions & 21 deletions

File tree

‎sentry-spring-7/src/main/java/io/sentry/spring7/checkin/SentryCheckInAdvice.java‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.springframework.core.annotation.AnnotatedElementUtils;
3232
import org.springframework.core.annotation.AnnotationUtils;
3333
import org.springframework.scheduling.annotation.Scheduled;
34+
import org.springframework.util.ClassUtils;
3435
import org.springframework.util.ObjectUtils;
3536
import org.springframework.util.StringValueResolver;
3637

@@ -41,6 +42,12 @@
4142
@ApiStatus.Internal
4243
@Open
4344
public class SentryCheckInAdvice implements MethodInterceptor, EmbeddedValueResolverAware {
45+
// Spring before 5.3 parses crons with CronSequenceGenerator
46+
private static final boolean LEGACY_CRON_PARSER =
47+
!ClassUtils.isPresent(
48+
"org.springframework.scheduling.support.CronExpression",
49+
SentryCheckInAdvice.class.getClassLoader());
50+
4451
private final @NotNull IScopes scopes;
4552

4653
private @Nullable StringValueResolver resolver;
@@ -100,6 +107,11 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
100107
return invocation.proceed();
101108
}
102109

110+
final @Nullable MonitorConfig monitorConfig =
111+
!isHeartbeatOnly && checkInAnnotation.upsertMonitorConfig()
112+
? monitorConfig(mostSpecificMethod)
113+
: null;
114+
103115
try (final @NotNull ISentryLifecycleToken ignored =
104116
scopes.forkedScopes("SentryCheckInAdvice").makeCurrent()) {
105117
TracingUtils.startNewTrace(scopes);
@@ -112,9 +124,7 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
112124
try {
113125
if (!isHeartbeatOnly) {
114126
final @NotNull CheckIn inProgress = new CheckIn(monitorSlug, CheckInStatus.IN_PROGRESS);
115-
if (checkInAnnotation.upsertMonitorConfig()) {
116-
inProgress.setMonitorConfig(monitorConfig(mostSpecificMethod));
117-
}
127+
inProgress.setMonitorConfig(monitorConfig);
118128
checkInId = scopes.captureCheckIn(inProgress);
119129
}
120130
return invocation.proceed();
@@ -159,8 +169,9 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
159169
cron,
160170
zone,
161171
periodMillis(scheduled.fixedRate(), scheduled.fixedRateString(), timeUnit),
162-
periodMillis(scheduled.fixedDelay(), scheduled.fixedDelayString(), timeUnit));
163-
} catch (RuntimeException e) {
172+
periodMillis(scheduled.fixedDelay(), scheduled.fixedDelayString(), timeUnit),
173+
LEGACY_CRON_PARSER);
174+
} catch (Throwable e) {
164175
scopes
165176
.getOptions()
166177
.getLogger()

‎sentry-spring-7/src/test/kotlin/io/sentry/spring7/SentryCheckInAdviceTest.kt‎

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import org.springframework.context.annotation.EnableAspectJAutoProxy
3838
import org.springframework.context.annotation.Import
3939
import org.springframework.context.support.PropertySourcesPlaceholderConfigurer
4040
import org.springframework.scheduling.annotation.Scheduled
41+
import org.springframework.scheduling.annotation.Schedules
4142
import org.springframework.test.context.TestPropertySource
4243
import org.springframework.test.context.junit.jupiter.SpringJUnitConfig
4344
import org.springframework.test.context.junit4.SpringRunner
@@ -51,6 +52,7 @@ import org.springframework.util.StringValueResolver
5152
"my.cron.slug = mypropertycronslug",
5253
"my.cron.schedule = 0 30 2 * * *",
5354
"my.cron.zone = America/New_York",
55+
"my.cron.empty.zone = ",
5456
]
5557
)
5658
class SentryCheckInAdviceTest {
@@ -284,12 +286,55 @@ class SentryCheckInAdviceTest {
284286
}
285287

286288
@Test
287-
fun `ISO-8601 fixed delay string is sent as interval monitor config`() {
288-
val config = inProgressMonitorConfig { sampleServiceScheduled.fixedDelayIso() }
289+
fun `ISO-8601 fixed rate string is sent as interval monitor config`() {
290+
val config = inProgressMonitorConfig { sampleServiceScheduled.fixedRateIso() }
289291
assertEquals("10", config?.schedule?.value)
290292
assertEquals("minute", config?.schedule?.unit)
291293
}
292294

295+
@Test
296+
fun `simple duration fixed rate string is sent as interval monitor config`() {
297+
val config = inProgressMonitorConfig { sampleServiceScheduled.fixedRateSimpleDuration() }
298+
assertEquals("interval", config?.schedule?.type)
299+
assertEquals("5", config?.schedule?.value)
300+
assertEquals("minute", config?.schedule?.unit)
301+
}
302+
303+
@Test
304+
fun `fixed delay sends no monitor config`() {
305+
assertNull(inProgressMonitorConfig { sampleServiceScheduled.fixedDelay() })
306+
assertNull(inProgressMonitorConfig { sampleServiceScheduled.fixedDelayIso() })
307+
}
308+
309+
@Test
310+
fun `disabled cron sends no monitor config`() {
311+
assertNull(inProgressMonitorConfig { sampleServiceScheduled.disabledCron() })
312+
}
313+
314+
@Test
315+
fun `zone placeholder resolving to empty uses the JVM default zone`() {
316+
val defaultTimeZone = TimeZone.getDefault()
317+
TimeZone.setDefault(TimeZone.getTimeZone("Asia/Tokyo"))
318+
try {
319+
val config = inProgressMonitorConfig { sampleServiceScheduled.cronWithEmptyZone() }
320+
assertEquals("0 3 * * *", config?.schedule?.value)
321+
assertEquals("Asia/Tokyo", config?.timezone)
322+
} finally {
323+
TimeZone.setDefault(defaultTimeZone)
324+
}
325+
}
326+
327+
@Test
328+
fun `cron zone overrides the default timezone from options`() {
329+
val options =
330+
SentryOptions().apply {
331+
cron = SentryOptions.Cron().apply { defaultTimezone = "Europe/Berlin" }
332+
}
333+
whenever(scopes.options).thenReturn(options)
334+
val config = inProgressMonitorConfig { sampleServiceScheduled.cronWithZoneAndDefaults() }
335+
assertEquals("Europe/Vienna", config?.timezone)
336+
}
337+
293338
@Test
294339
fun `unresolvable cron placeholder sends no monitor config`() {
295340
assertNull(inProgressMonitorConfig { sampleServiceScheduled.unresolvableCron() })
@@ -315,6 +360,11 @@ class SentryCheckInAdviceTest {
315360
assertNull(inProgressMonitorConfig { sampleServiceScheduled.multipleSchedules() })
316361
}
317362

363+
@Test
364+
fun `schedules container sends no monitor config`() {
365+
assertNull(inProgressMonitorConfig { sampleServiceScheduled.schedulesContainer() })
366+
}
367+
318368
@Test
319369
fun `upsertMonitorConfig defaults to false and sends no monitor config`() {
320370
assertNull(inProgressMonitorConfig { sampleServiceScheduled.defaultNoConfig() })
@@ -423,10 +473,38 @@ class SentryCheckInAdviceTest {
423473
@Scheduled(fixedRate = 2, timeUnit = TimeUnit.HOURS)
424474
open fun fixedRateHours() {}
425475

476+
@SentryCheckIn("fixed_rate_iso", upsertMonitorConfig = true)
477+
@Scheduled(fixedRateString = "PT10M")
478+
open fun fixedRateIso() {}
479+
480+
@SentryCheckIn("fixed_rate_simple", upsertMonitorConfig = true)
481+
@Scheduled(fixedRateString = "5m")
482+
open fun fixedRateSimpleDuration() {}
483+
484+
@SentryCheckIn("fixed_delay", upsertMonitorConfig = true)
485+
@Scheduled(fixedDelay = 300_000)
486+
open fun fixedDelay() {}
487+
426488
@SentryCheckIn("fixed_delay_iso", upsertMonitorConfig = true)
427489
@Scheduled(fixedDelayString = "PT10M")
428490
open fun fixedDelayIso() {}
429491

492+
@SentryCheckIn("disabled_cron", upsertMonitorConfig = true)
493+
@Scheduled(cron = "-")
494+
open fun disabledCron() {}
495+
496+
@SentryCheckIn("cron_empty_zone", upsertMonitorConfig = true)
497+
@Scheduled(cron = "0 0 3 * * *", zone = "\${my.cron.empty.zone}")
498+
open fun cronWithEmptyZone() {}
499+
500+
@SentryCheckIn("cron_zone_defaults", upsertMonitorConfig = true)
501+
@Scheduled(cron = "0 0 4 * * *", zone = "Europe/Vienna")
502+
open fun cronWithZoneAndDefaults() {}
503+
504+
@SentryCheckIn("schedules_container", upsertMonitorConfig = true)
505+
@Schedules(Scheduled(cron = "0 0 1 * * *"), Scheduled(cron = "0 0 13 * * *"))
506+
open fun schedulesContainer() {}
507+
430508
@SentryCheckIn("unresolvable_cron", upsertMonitorConfig = true)
431509
@Scheduled(cron = "\${my.cron.missing}")
432510
open fun unresolvableCron() {}

‎sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/checkin/SentryCheckInAdvice.java‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.springframework.core.annotation.AnnotatedElementUtils;
3232
import org.springframework.core.annotation.AnnotationUtils;
3333
import org.springframework.scheduling.annotation.Scheduled;
34+
import org.springframework.util.ClassUtils;
3435
import org.springframework.util.ObjectUtils;
3536
import org.springframework.util.StringValueResolver;
3637

@@ -41,6 +42,12 @@
4142
@ApiStatus.Internal
4243
@Open
4344
public class SentryCheckInAdvice implements MethodInterceptor, EmbeddedValueResolverAware {
45+
// Spring before 5.3 parses crons with CronSequenceGenerator
46+
private static final boolean LEGACY_CRON_PARSER =
47+
!ClassUtils.isPresent(
48+
"org.springframework.scheduling.support.CronExpression",
49+
SentryCheckInAdvice.class.getClassLoader());
50+
4451
private final @NotNull IScopes scopes;
4552

4653
private @Nullable StringValueResolver resolver;
@@ -100,6 +107,11 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
100107
return invocation.proceed();
101108
}
102109

110+
final @Nullable MonitorConfig monitorConfig =
111+
!isHeartbeatOnly && checkInAnnotation.upsertMonitorConfig()
112+
? monitorConfig(mostSpecificMethod)
113+
: null;
114+
103115
try (final @NotNull ISentryLifecycleToken ignored =
104116
scopes.forkedScopes("SentryCheckInAdvice").makeCurrent()) {
105117
TracingUtils.startNewTrace(scopes);
@@ -112,9 +124,7 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
112124
try {
113125
if (!isHeartbeatOnly) {
114126
final @NotNull CheckIn inProgress = new CheckIn(monitorSlug, CheckInStatus.IN_PROGRESS);
115-
if (checkInAnnotation.upsertMonitorConfig()) {
116-
inProgress.setMonitorConfig(monitorConfig(mostSpecificMethod));
117-
}
127+
inProgress.setMonitorConfig(monitorConfig);
118128
checkInId = scopes.captureCheckIn(inProgress);
119129
}
120130
return invocation.proceed();
@@ -159,8 +169,9 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl
159169
cron,
160170
zone,
161171
periodMillis(scheduled.fixedRate(), scheduled.fixedRateString(), timeUnit),
162-
periodMillis(scheduled.fixedDelay(), scheduled.fixedDelayString(), timeUnit));
163-
} catch (RuntimeException e) {
172+
periodMillis(scheduled.fixedDelay(), scheduled.fixedDelayString(), timeUnit),
173+
LEGACY_CRON_PARSER);
174+
} catch (Throwable e) {
164175
scopes
165176
.getOptions()
166177
.getLogger()

‎sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/SentryCheckInAdviceTest.kt‎

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import org.springframework.context.annotation.EnableAspectJAutoProxy
3838
import org.springframework.context.annotation.Import
3939
import org.springframework.context.support.PropertySourcesPlaceholderConfigurer
4040
import org.springframework.scheduling.annotation.Scheduled
41+
import org.springframework.scheduling.annotation.Schedules
4142
import org.springframework.test.context.TestPropertySource
4243
import org.springframework.test.context.junit.jupiter.SpringJUnitConfig
4344
import org.springframework.test.context.junit4.SpringRunner
@@ -51,6 +52,7 @@ import org.springframework.util.StringValueResolver
5152
"my.cron.slug = mypropertycronslug",
5253
"my.cron.schedule = 0 30 2 * * *",
5354
"my.cron.zone = America/New_York",
55+
"my.cron.empty.zone = ",
5456
]
5557
)
5658
class SentryCheckInAdviceTest {
@@ -284,12 +286,55 @@ class SentryCheckInAdviceTest {
284286
}
285287

286288
@Test
287-
fun `ISO-8601 fixed delay string is sent as interval monitor config`() {
288-
val config = inProgressMonitorConfig { sampleServiceScheduled.fixedDelayIso() }
289+
fun `ISO-8601 fixed rate string is sent as interval monitor config`() {
290+
val config = inProgressMonitorConfig { sampleServiceScheduled.fixedRateIso() }
289291
assertEquals("10", config?.schedule?.value)
290292
assertEquals("minute", config?.schedule?.unit)
291293
}
292294

295+
@Test
296+
fun `simple duration fixed rate string is sent as interval monitor config`() {
297+
val config = inProgressMonitorConfig { sampleServiceScheduled.fixedRateSimpleDuration() }
298+
assertEquals("interval", config?.schedule?.type)
299+
assertEquals("5", config?.schedule?.value)
300+
assertEquals("minute", config?.schedule?.unit)
301+
}
302+
303+
@Test
304+
fun `fixed delay sends no monitor config`() {
305+
assertNull(inProgressMonitorConfig { sampleServiceScheduled.fixedDelay() })
306+
assertNull(inProgressMonitorConfig { sampleServiceScheduled.fixedDelayIso() })
307+
}
308+
309+
@Test
310+
fun `disabled cron sends no monitor config`() {
311+
assertNull(inProgressMonitorConfig { sampleServiceScheduled.disabledCron() })
312+
}
313+
314+
@Test
315+
fun `zone placeholder resolving to empty uses the JVM default zone`() {
316+
val defaultTimeZone = TimeZone.getDefault()
317+
TimeZone.setDefault(TimeZone.getTimeZone("Asia/Tokyo"))
318+
try {
319+
val config = inProgressMonitorConfig { sampleServiceScheduled.cronWithEmptyZone() }
320+
assertEquals("0 3 * * *", config?.schedule?.value)
321+
assertEquals("Asia/Tokyo", config?.timezone)
322+
} finally {
323+
TimeZone.setDefault(defaultTimeZone)
324+
}
325+
}
326+
327+
@Test
328+
fun `cron zone overrides the default timezone from options`() {
329+
val options =
330+
SentryOptions().apply {
331+
cron = SentryOptions.Cron().apply { defaultTimezone = "Europe/Berlin" }
332+
}
333+
whenever(scopes.options).thenReturn(options)
334+
val config = inProgressMonitorConfig { sampleServiceScheduled.cronWithZoneAndDefaults() }
335+
assertEquals("Europe/Vienna", config?.timezone)
336+
}
337+
293338
@Test
294339
fun `unresolvable cron placeholder sends no monitor config`() {
295340
assertNull(inProgressMonitorConfig { sampleServiceScheduled.unresolvableCron() })
@@ -315,6 +360,11 @@ class SentryCheckInAdviceTest {
315360
assertNull(inProgressMonitorConfig { sampleServiceScheduled.multipleSchedules() })
316361
}
317362

363+
@Test
364+
fun `schedules container sends no monitor config`() {
365+
assertNull(inProgressMonitorConfig { sampleServiceScheduled.schedulesContainer() })
366+
}
367+
318368
@Test
319369
fun `upsertMonitorConfig defaults to false and sends no monitor config`() {
320370
assertNull(inProgressMonitorConfig { sampleServiceScheduled.defaultNoConfig() })
@@ -423,10 +473,38 @@ class SentryCheckInAdviceTest {
423473
@Scheduled(fixedRate = 2, timeUnit = TimeUnit.HOURS)
424474
open fun fixedRateHours() {}
425475

476+
@SentryCheckIn("fixed_rate_iso", upsertMonitorConfig = true)
477+
@Scheduled(fixedRateString = "PT10M")
478+
open fun fixedRateIso() {}
479+
480+
@SentryCheckIn("fixed_rate_simple", upsertMonitorConfig = true)
481+
@Scheduled(fixedRateString = "5m")
482+
open fun fixedRateSimpleDuration() {}
483+
484+
@SentryCheckIn("fixed_delay", upsertMonitorConfig = true)
485+
@Scheduled(fixedDelay = 300_000)
486+
open fun fixedDelay() {}
487+
426488
@SentryCheckIn("fixed_delay_iso", upsertMonitorConfig = true)
427489
@Scheduled(fixedDelayString = "PT10M")
428490
open fun fixedDelayIso() {}
429491

492+
@SentryCheckIn("disabled_cron", upsertMonitorConfig = true)
493+
@Scheduled(cron = "-")
494+
open fun disabledCron() {}
495+
496+
@SentryCheckIn("cron_empty_zone", upsertMonitorConfig = true)
497+
@Scheduled(cron = "0 0 3 * * *", zone = "\${my.cron.empty.zone}")
498+
open fun cronWithEmptyZone() {}
499+
500+
@SentryCheckIn("cron_zone_defaults", upsertMonitorConfig = true)
501+
@Scheduled(cron = "0 0 4 * * *", zone = "Europe/Vienna")
502+
open fun cronWithZoneAndDefaults() {}
503+
504+
@SentryCheckIn("schedules_container", upsertMonitorConfig = true)
505+
@Schedules(Scheduled(cron = "0 0 1 * * *"), Scheduled(cron = "0 0 13 * * *"))
506+
open fun schedulesContainer() {}
507+
430508
@SentryCheckIn("unresolvable_cron", upsertMonitorConfig = true)
431509
@Scheduled(cron = "\${my.cron.missing}")
432510
open fun unresolvableCron() {}

0 commit comments

Comments
 (0)