Repository navigation
Conversation
When `@SentryCron` is used without a monitor config, read the schedule and time zone from the `@Cron()` decorator of `@nestjs/schedule` on the same method. Check-ins then carry a monitor config, so Sentry creates the monitor on the first run instead of dropping the check-ins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only read the `@Cron()` schedule when `@SentryCron` gets
`{ fromCronDecorator: true }`, so existing usage keeps sending check-ins
without a monitor config. Other monitor settings passed alongside the flag
are kept.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`@SentryCron` now sends the schedule of the method's `@Cron()` decorator unless it gets a monitor config with a schedule. Other monitor settings can still be passed and are merged in, and `fromCronDecorator: false` turns this off. When `@Cron()` has no `timeZone`, the job runs in the server's local time zone, so that zone is sent instead of leaving Sentry to assume UTC. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`@Cron()` presets are now sent: the ones Sentry accepts as is, and `@midnight`, `@minutely`, `@weekdays` and `@weekends` as crontabs. When no schedule can be derived but monitor settings were passed, a debug warning says no monitor config is sent. The settings type now rejects `schedule` and `timezone`, and a full monitor config rejects `fromCronDecorator`, so mixed objects no longer drop fields silently. Adds an E2E case for a disabled `@Cron()` job triggered through an endpoint. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Skip numeric months (cron 2.x counts from 0), day-of-month/day-of-week combinations Sentry would AND, and time zones Sentry rejects. Clarify the warning when fromCronDecorator is false. Co-Authored-By: Claude <noreply@anthropic.com>
Allow month steps like */3 (CronExpression.EVERY_QUARTER), still skipping month numbers. Drop the @midnight and @annually presets, which cron rejects. Skip full day ranges like 1-31 when the other day field is set, and send the canonical time zone name. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
`nestIntegration({ monitorCronJobs: true })` sends check-ins for every
`@Cron()` method, with the schedule from `@Cron()` as monitor config.
The slug is the `@Cron()` name, else `ClassName.methodName`.
`excludeCronJobs` skips jobs; methods with `@SentryCron` keep their slug.
Co-Authored-By: Claude <noreply@anthropic.com>
size-limit report 📦Compared: 6fa050c (PR test merge using develop at 365e761) Against: 365e761 (develop) · base run #64341 ✅ Baseline matches: this diff contains only this PR's changes.
|
wedamija
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
@nestjs/schedule runs @Cron methods as cron CronJobs, so with monitorCronJobs and cronJobIntegration both on each run sent two sets of check-ins. Mark the onTick Nest passes to CronJob when the method already sends check-ins.
…nf/nestjs-cron-auto-monitor # Conflicts: # packages/nestjs/src/integrations/orchestrion-subscriber.ts
Key the monitored check on the @Cron() options metadata, which outer decorators copy, so it doesn't depend on decorator order.
Optional E2E testsSome optional checks were not successful. They do not block the merge. 2 not successful, 53 successful. See the run.
Successful checks
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0c123c6. Configure here.
…s on @nestjs/schedule 4+ A day field that covers every day (like sun-sat or 0,1,2,3,4,5,6) is ignored by cron but ORed by Sentry, so those schedules are now found by counting the expanded values. Numeric months are sent when the @Cron() instrumentation saw @nestjs/schedule 4 or later, where cron counts months from 1.
…-nest-auto # Conflicts: # packages/nestjs/src/decorators.ts # packages/nestjs/src/integrations/helpers.ts # packages/nestjs/src/integrations/orchestrion-subscriber.ts
… jobs monitorCronJobs only covers @Cron() methods whose schedule Sentry can represent. Methods with @SentryCron() keep checking in without a config.
…o-monitor # Conflicts: # CHANGELOG.md # dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts # dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts # dev-packages/e2e-tests/test-applications/nestjs-basic/tests/cron-decorator.test.ts # packages/nestjs/src/decorators.ts # packages/nestjs/src/integrations/helpers.ts # packages/nestjs/src/integrations/orchestrion-subscriber.ts
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Approving to unblock. I think this is fine to land once the trace propagation comment is resolved (assuming that it should come before #25477 and not wait for that one to land first; I haven't looked closely at that PR yet, so it might be obvious, but just saw that's where the mentioned method gets added). The others are pretty minor concerns.
Would also be good to add an "Unreleased" CHANGELOG.md entry for monitorCronJobs to say that it is off by default and that each monitored job creates a monitor.
| if (!monitor) { | ||
| return scheduleHandler.apply(this, args); | ||
| } | ||
| return withMonitor(monitor.slug, () => scheduleHandler.apply(this, args), monitor.config); |
There was a problem hiding this comment.
withMonitor restores the parent's propagation context unless isolateTrace is set (packages/core/src/monitor.ts). wrapScheduleHandler only forks the isolation scope, which copies the trace. A cron tick runs from a timer that the job started at bootstrap. So every run of every auto-monitored job gets the same trace ID as bootstrap.
The sibling scheduler integrations avoid this: wrapScheduledJob in packages/server-utils/src/utils/cronMonitoring.ts wraps each run in startNewTrace. @SentryCron has the same gap today, but that is opt-in per method. This option turns it on for every job at once, and a scheduler run is a natural trace root. (I didn't confirm the shared trace ID at runtime, just took it from the code path, so it is possible I'm misreading this.)
Suggestion: wrap the monitored path in startNewTrace(...), as wrapScheduledJob does. Or set isolateTrace: true in the config that getCronMonitor returns. Add a unit test that two runs get different trace IDs.
| }; | ||
| }); | ||
| // `@nestjs/schedule` passes this function to `CronJob` as `onTick`. Mark it when the method already | ||
| // sends check-ins, so `cronJobIntegration` doesn't send a second set. |
There was a problem hiding this comment.
Oh, is this supposed to be stacked on #25477? It seems like this comment is maybe premature, since the cronJobIntegration doesn't exist in develop yet?
Whether that's the case, or if #25477 is intended to stack on top of this, probably we should make these a PR stack so that we can land them together and not have develop in a halfway state?
(It's low severity, I mean, if we land both, it's fine, and I wouldn't sweat the ordering over a premature comment, as long as the code works at all points along the develop history.)
| end(data) { | ||
| if (typeof data.result === 'function' && sendsCronCheckIns(data.arguments?.[0])) { | ||
| addNonEnumerableProperty(data.result, Symbol.for('sentry.cron.monitored'), true); | ||
| } | ||
| }, |
There was a problem hiding this comment.
sendsCronCheckIns calls Reflect.getMetadata and the monitor resolver, which calls getClient() and stringMatchesSomePattern.
The risk is pretty low, but this code runs once for every scheduled method at bootstrap, inside the user's app. subscribeToJobCreation in packages/server-utils/src/utils/cronMonitoring.ts uses a try/catch for this reason.
| end(data) { | |
| if (typeof data.result === 'function' && sendsCronCheckIns(data.arguments?.[0])) { | |
| addNonEnumerableProperty(data.result, Symbol.for('sentry.cron.monitored'), true); | |
| } | |
| }, | |
| end(data) { | |
| try { | |
| if (typeof data.result === 'function' && sendsCronCheckIns(data.arguments?.[0])) { | |
| addNonEnumerableProperty(data.result, Symbol.for('sentry.cron.monitored'), true); | |
| } | |
| } catch (er) { | |
| DEBUG_BUILD && debug.error('[Nest] Failed to check whether a @Cron() job sends check-ins:', e); | |
| } | |
| }, |
| const slug = | ||
| typeof options.name === 'string' && options.name | ||
| ? options.name | ||
| : typeof className === 'string' && className && propertyKey !== undefined | ||
| ? `${className}.${String(propertyKey)}` | ||
| : undefined; |
There was a problem hiding this comment.
Hm, relay truncates these to 50 chars, so we should probably at least document that users should pass an explicit name for long class/method names? I'm not sure if it's likely enough to be worth a harder gate than that, but it could be a weird gotcha.
| const SCHEDULE_CRON_OPTIONS = 'SCHEDULE_CRON_OPTIONS'; | ||
| const SENTRY_CRON_METADATA = 'sentry:nestjs:sentry_cron'; | ||
|
|
||
| const SENTRY_CRON_METHODS = new WeakSet<object>(); |
There was a problem hiding this comment.
There's three overlapping ways to mark @SentryCron methods in this file, and I think we can streamline it a bit.
markSentryCronMethod (line 58, below) records the method in three places. We've got a WeakSet of functions, a sentry:nestjs:sentry_cron metadata key, and a WeakSet that holds the @Cron() options object. wrap-handlers.ts adds two more maps (CRON_HANDLER_MONITORS, CRON_OPTIONS_MONITORS). Nest needs reflect-metadata, so the metadata key covers every case that the function WeakSet covers. A wrapper that doesn't copy metadata also breaks @Cron() scheduling, so that case isn't a concern. The options-object mark is what handles @SentryCron above @Cron() with a decorator in between, so it is needed.
Suggestion: drop the function WeakSet and the typeof R.defineMetadata === 'function' guards, and keep the metadata key plus the options-object mark.
| return (handler, wrapped) => { | ||
| const client = getClient(); | ||
| const options = client && MONITORING_OPTIONS.get(client); | ||
| if (!options || isSentryCronMethod(handler) || isSentryCronMethod(wrapped)) { |
There was a problem hiding this comment.
Rather than calling the isSentryCronMethod() twice here, we could cache the value like we do for monitor, no? Would save a few Reflect calls.
Part of a series adding automatic Crons monitoring for the most-used job schedulers in each SDK. Check-ins for a monitor that doesn't exist yet are dropped, so today each monitor has to be created by hand with its schedule copied from the code. Sending the job's schedule as the monitor config lets Sentry create the monitor and keep it in sync.
Adds
monitorCronJobstonestIntegration. When it's on, every@Cron()method sends check-ins with the schedule and time zone from@Cron()as the monitor config, so apps don't need@SentryCronon each method. It is off by default. The slug is the@Cron()name, orClassName.methodNamewithout one.Methods that also have
@SentryCronkeep their slug and send one set of check-ins. Methods with schedules Crons can't express, like every 10 seconds, aren't monitored. It needsenableRuntimeChannelInjection, which is on by default.Stacked on #25013.