Skip to content

Commit

Permalink
fix(node): Time zone handling for cron (#11225)
Browse files Browse the repository at this point in the history
The `cron` package uses `timeZone`, while Sentry internally uses
`timezone`. This caused Sentry cron jobs to be incorrectly upserted with
no time zone information. Because of the use of the `...(timeZone ? {
timeZone } : undefined)` idiom, TypeScript type checking did not catch
the mistake.

I kept `cron`'s `timeZone` capitalization within Sentry's
instrumentation and changed from the `...(timeZone ? { timeZone } :
undefined)` idiom so TypeScript could catch mistakes of this sort.
(Passing an undefined `timezone` appears to be okay, because Sentry's
`node-cron` instrumentation does it.)
  • Loading branch information
joshkel authored and AbhiPrasad committed Mar 21, 2024
1 parent bf4ea76 commit e3306a6
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 9 deletions.
4 changes: 2 additions & 2 deletions packages/node/src/cron/cron.ts
Expand Up @@ -100,7 +100,7 @@ export function instrumentCron<T>(lib: T & CronJobConstructor, monitorSlug: stri
},
{
schedule: { type: 'crontab', value: cronString },
...(timeZone ? { timeZone } : {}),
timezone: timeZone || undefined,
},
);
}
Expand Down Expand Up @@ -132,7 +132,7 @@ export function instrumentCron<T>(lib: T & CronJobConstructor, monitorSlug: stri
},
{
schedule: { type: 'crontab', value: cronString },
...(timeZone ? { timeZone } : {}),
timezone: timeZone || undefined,
},
);
};
Expand Down
21 changes: 14 additions & 7 deletions packages/node/test/cron.test.ts
Expand Up @@ -53,13 +53,20 @@ describe('cron check-ins', () => {

const CronJobWithCheckIn = cron.instrumentCron(CronJobMock, 'my-cron-job');

new CronJobWithCheckIn('* * * Jan,Sep Sun', () => {
expect(withMonitorSpy).toHaveBeenCalledTimes(1);
expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), {
schedule: { type: 'crontab', value: '* * * 1,9 0' },
});
done();
});
new CronJobWithCheckIn(
'* * * Jan,Sep Sun',
() => {
expect(withMonitorSpy).toHaveBeenCalledTimes(1);
expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), {
schedule: { type: 'crontab', value: '* * * 1,9 0' },
timezone: 'America/Los_Angeles',
});
done();
},
undefined,
true,
'America/Los_Angeles',
);
});

test('CronJob.from()', done => {
Expand Down

0 comments on commit e3306a6

Please sign in to comment.