-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(cloudflare): Add opt-in cron monitoring for Cron Triggers #25014
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
Open
wedamija
wants to merge
7
commits into
develop
Choose a base branch
from
danf/cloudflare-cron-monitors
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4c9812f
feat(cloudflare): Add opt-in cron monitoring for Cron Triggers
wedamija 804e1a9
fix(cloudflare): Convert Cron Trigger weekdays and harden check-ins
wedamija 73f550f
ref(cloudflare): Move Cron Trigger check-ins into cronTriggersIntegra…
wedamija 58f7a7e
ref(cloudflare): Shorten cronTriggersIntegration JSDoc
wedamija eb22bd1
fix(cloudflare): Keep */n weekdays and skip W/? days of month
wedamija ef0d768
fix(cloudflare): Keep cron check-in failures out of the scheduled han…
wedamija 370d891
docs(cloudflare): Use MON-FRI in the weekday cron examples
wedamija File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| import type { IntegrationFn, MonitorConfig } from '@sentry/core'; | ||
| import { captureCheckIn, debug, defineIntegration, timestampInSeconds } from '@sentry/core'; | ||
| import { DEBUG_BUILD } from '../debug-build'; | ||
|
|
||
| const INTEGRATION_NAME = 'CronTriggers' as const; | ||
|
|
||
| /** | ||
| * The monitor slug and settings for a Cron Trigger, see `cronTriggersIntegration`. | ||
| */ | ||
| export type CronTriggerMonitorSettings = { slug: string } & Omit<MonitorConfig, 'schedule' | 'timezone'>; | ||
|
|
||
| export interface CronTriggersOptions { | ||
| /** | ||
| * Chooses the monitor slug for a cron expression. It can also return an object with the `slug` | ||
| * and other monitor settings, such as `checkinMargin` or `maxRuntime`. Returning `undefined` | ||
| * sends no check-ins for that trigger, and so does a function that throws. | ||
| */ | ||
| slug?: (cron: string) => string | CronTriggerMonitorSettings | undefined; | ||
| } | ||
|
|
||
| /** @internal Used by the scheduled handler instrumentation. */ | ||
| export interface CronTriggersIntegration { | ||
| name: string; | ||
| startCheckIn(cron: string): ((status: 'ok' | 'error') => void) | undefined; | ||
| } | ||
|
|
||
| const SLUG_TOKENS: Record<string, string> = { ' ': '-', '*': 'x', ',': '_', '-': 'to', '/': 'by' }; | ||
|
|
||
| /** | ||
| * Derives a monitor slug from a cron expression, e.g. `30 9 * * MON-FRI` -> `cron-30-9-x-x-montofri`. | ||
| * | ||
| * A hash of the expression is appended when it has any other characters or the slug would be | ||
| * longer than 50 characters, so different expressions don't share a slug. | ||
| */ | ||
| function cronToMonitorSlug(cron: string): string { | ||
| const expression = cron.trim().toLowerCase().split(/\s+/).join(' '); | ||
| const slug = `cron-${expression.replace(/[ *,\-/]/g, char => SLUG_TOKENS[char] as string)}`; | ||
| if (/^[a-z0-9_-]{1,50}$/.test(slug)) { | ||
| return slug; | ||
| } | ||
|
|
||
| // A polynomial string hash modulo 2^31 - 1, at most 6 characters in base 36. | ||
| let hash = 0; | ||
| for (let i = 0; i < expression.length; i++) { | ||
| hash = (hash * 31 + expression.charCodeAt(i)) % 2147483647; | ||
| } | ||
| return `${slug.replace(/[^a-z0-9_-]+/g, '-').slice(0, 43)}-${hash.toString(36)}`; | ||
| } | ||
|
|
||
| const WEEKDAYS = ['SUN', 'MON', 'TUE', 'WED', 'THU', 'FRI', 'SAT']; | ||
|
|
||
| // Cloudflare numbers weekdays from 1 = Sunday to 7 = Saturday. Returns -1 for anything else. | ||
| function weekdayIndex(value: string): number { | ||
| return /^[1-7]$/.test(value) ? Number(value) - 1 : WEEKDAYS.indexOf(value.toUpperCase()); | ||
| } | ||
|
|
||
| function convertWeekdayItem(item: string): string | undefined { | ||
| const match = item.match(/^(\*|\w+)(?:-(\w+))?(?:\/(\d+))?$/); | ||
| if (!match) { | ||
| return undefined; | ||
| } | ||
| const [, from, to, step] = match; | ||
| if (step && !(Number(step) > 0)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // `*` and `*/n` give the same days in both numberings, and keep their `*` meaning for Sentry. | ||
| if (from === '*') { | ||
| return to ? undefined : item; | ||
| } | ||
|
|
||
| const start = weekdayIndex(from as string); | ||
| // A step without an end runs to Saturday, so it is written as a range. | ||
| const end = to ? weekdayIndex(to) : step ? 6 : start; | ||
| if (start < 0 || end < start) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // A step over a single day would run on to Sunday in Sentry's cron parser, so it is left out. | ||
| if (start === end) { | ||
| return WEEKDAYS[start]; | ||
| } | ||
| const days = `${WEEKDAYS[start]}-${WEEKDAYS[end]}`; | ||
| return step ? `${days}/${step}` : days; | ||
| } | ||
|
|
||
| /** | ||
| * Converts a Cloudflare cron expression into a crontab Sentry accepts, which numbers weekdays from | ||
| * 0 = Sunday. Returns `undefined` if the weekday field can't be converted. | ||
| */ | ||
| function cloudflareCronToCrontab(cron: string): string | undefined { | ||
| const fields = cron.trim().split(/\s+/); | ||
| // Sentry rejects `W` and `?` in the day of month. | ||
| if (fields.length !== 5 || /[w?]/i.test(fields[2] as string)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const weekdays = (fields[4] as string).split(',').map(convertWeekdayItem); | ||
| return weekdays.includes(undefined) ? undefined : [...fields.slice(0, 4), weekdays.join(',')].join(' '); | ||
| } | ||
|
|
||
| const _cronTriggersIntegration = ((options: CronTriggersOptions = {}): CronTriggersIntegration => { | ||
| return { | ||
| name: INTEGRATION_NAME, | ||
| startCheckIn(cron) { | ||
| // Manual runs, e.g. through `wrangler dev --test-scheduled`, can have no cron expression. | ||
| if (!cron) { | ||
| return undefined; | ||
| } | ||
|
|
||
| let monitor: string | CronTriggerMonitorSettings | undefined; | ||
| try { | ||
| monitor = options.slug ? options.slug(cron) : cronToMonitorSlug(cron); | ||
| } catch (e) { | ||
| DEBUG_BUILD && debug.warn(`[Cron Triggers] \`slug\` threw for "${cron}", sending no check-ins:`, e); | ||
| return undefined; | ||
| } | ||
|
|
||
| if (!monitor) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const { slug: monitorSlug, ...monitorSettings } = typeof monitor === 'string' ? { slug: monitor } : monitor; | ||
| if (!monitorSlug) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const crontab = cloudflareCronToCrontab(cron); | ||
| if (!crontab) { | ||
| DEBUG_BUILD && | ||
| debug.warn(`[Cron Triggers] Can't convert "${cron}" to a Sentry schedule, sending check-ins without one.`); | ||
| } | ||
|
|
||
| // Check-ins are captured directly rather than through `withMonitor`, which would fork the | ||
| // isolation scope and lose the invocation state attached to it. | ||
| const checkInId = captureCheckIn( | ||
| { monitorSlug, status: 'in_progress' }, | ||
| crontab ? { ...monitorSettings, schedule: { type: 'crontab', value: crontab } } : undefined, | ||
| ); | ||
| const startTime = timestampInSeconds(); | ||
|
|
||
| return status => { | ||
| captureCheckIn({ monitorSlug, status, checkInId, duration: timestampInSeconds() - startTime }); | ||
| }; | ||
| }, | ||
| }; | ||
| }) satisfies IntegrationFn; | ||
|
|
||
| /** | ||
| * Sends cron check-ins for every Cron Trigger run of the `scheduled` handler, with the trigger's | ||
| * schedule, so Sentry creates the monitor on the first run. | ||
| * | ||
| * Cron Triggers have no names, so map each cron expression to a slug. Without `slug`, the slug is | ||
| * derived from the expression (`30 9 * * MON-FRI` becomes `cron-30-9-x-x-montofri`) and changes with it. | ||
| * | ||
| * @example | ||
| * ```ts | ||
| * const jobs = { | ||
| * '30 9 * * MON-FRI': { slug: 'daily-report', run: dailyReport }, | ||
| * }; | ||
| * | ||
| * export default Sentry.withSentry( | ||
| * (env) => ({ | ||
| * dsn: env.SENTRY_DSN, | ||
| * integrations: [Sentry.cronTriggersIntegration({ slug: (cron) => jobs[cron]?.slug })], | ||
| * }), | ||
| * { | ||
| * async scheduled(controller, env) { | ||
| * await jobs[controller.cron]?.run(env); | ||
| * }, | ||
| * }, | ||
| * ); | ||
| * ``` | ||
| */ | ||
| export const cronTriggersIntegration = defineIntegration(_cronTriggersIntegration); | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Bug: When a cron expression is unconvertible, all monitor settings like
checkinMarginare silently discarded, not just the schedule, because the argument tocaptureCheckInbecomesundefined.Severity: MEDIUM
Suggested Fix
Update the ternary operator to ensure
monitorSettingsare passed tocaptureCheckIneven whencrontabis undefined. The expression should return themonitorSettingsobject if it has keys, otherwiseundefined.Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.