From 6d1001bcb0c0c67a504bcf5d65cbeb26528df88a Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sat, 6 Jan 2024 13:38:52 +0100 Subject: [PATCH 1/2] feat(node): Instrumentation for `node-schedule` library --- packages/node/src/cron/cron.ts | 14 +++++ packages/node/src/cron/node-schedule.ts | 59 ++++++++++++++++++ packages/node/src/index.ts | 2 + packages/node/test/cron.test.ts | 80 +++++++++++++++++++++++++ 4 files changed, 155 insertions(+) create mode 100644 packages/node/src/cron/node-schedule.ts diff --git a/packages/node/src/cron/cron.ts b/packages/node/src/cron/cron.ts index a8b42ec0fed7..88a3e9e58eb5 100644 --- a/packages/node/src/cron/cron.ts +++ b/packages/node/src/cron/cron.ts @@ -56,6 +56,8 @@ const ERROR_TEXT = 'Automatic instrumentation of CronJob only supports crontab s * ``` */ export function instrumentCron(lib: T & CronJobConstructor, monitorSlug: string): T { + let jobScheduled = false; + return new Proxy(lib, { construct(target, args: ConstructorParameters) { const [cronTime, onTick, onComplete, start, timeZone, ...rest] = args; @@ -64,6 +66,12 @@ export function instrumentCron(lib: T & CronJobConstructor, monitorSlug: stri throw new Error(ERROR_TEXT); } + if (jobScheduled) { + throw new Error(`A job named '${monitorSlug}' has already been scheduled`); + } + + jobScheduled = true; + const cronString = replaceCronNames(cronTime); function monitoredTick(context: unknown, onComplete?: unknown): void | Promise { @@ -90,6 +98,12 @@ export function instrumentCron(lib: T & CronJobConstructor, monitorSlug: stri throw new Error(ERROR_TEXT); } + if (jobScheduled) { + throw new Error(`A job named '${monitorSlug}' has already been scheduled`); + } + + jobScheduled = true; + const cronString = replaceCronNames(cronTime); param.onTick = (context: unknown, onComplete?: unknown) => { diff --git a/packages/node/src/cron/node-schedule.ts b/packages/node/src/cron/node-schedule.ts new file mode 100644 index 000000000000..6b11542cbe37 --- /dev/null +++ b/packages/node/src/cron/node-schedule.ts @@ -0,0 +1,59 @@ +import { withMonitor } from '@sentry/core'; +import { replaceCronNames } from './common'; + +export interface NodeSchedule { + scheduleJob(expression: string | Date | object, callback: () => void): unknown; +} + +/** + * Instruments the `node-schedule` library to send a check-in event to Sentry for each job execution. + * + * ```ts + * import * as Sentry from '@sentry/node'; + * import * as schedule from 'node-schedule'; + * + * const scheduleWithCheckIn = Sentry.cron.instrumentNodeSchedule(schedule, 'my-cron-job'); + * + * const job = scheduleWithCheckIn.scheduleJob('* * * * *', () => { + * console.log('You will see this message every minute'); + * }); + * ``` + */ +export function instrumentNodeSchedule(lib: T & NodeSchedule, monitorSlug: string): T { + let jobScheduled = false; + + return new Proxy(lib, { + get(target, prop: keyof NodeSchedule) { + if (prop === 'scheduleJob') { + // eslint-disable-next-line @typescript-eslint/unbound-method + return new Proxy(target.scheduleJob, { + apply(target, thisArg, argArray: Parameters) { + const [expression] = argArray; + + if (typeof expression !== 'string') { + throw new Error('Automatic instrumentation of "node-schedule" only supports crontab string'); + } + + if (jobScheduled) { + throw new Error(`A job named '${monitorSlug}' has already been scheduled`); + } + + jobScheduled = true; + + return withMonitor( + monitorSlug, + () => { + return target.apply(thisArg, argArray); + }, + { + schedule: { type: 'crontab', value: replaceCronNames(expression) }, + }, + ); + }, + }); + } + + return target[prop]; + }, + }); +} diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 47206462b937..a157e2bc2028 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -124,9 +124,11 @@ export { hapiErrorPlugin } from './integrations/hapi'; import { instrumentCron } from './cron/cron'; import { instrumentNodeCron } from './cron/node-cron'; +import { instrumentNodeSchedule } from './cron/node-schedule'; /** Methods to instrument cron libraries for Sentry check-ins */ export const cron = { instrumentCron, instrumentNodeCron, + instrumentNodeSchedule, }; diff --git a/packages/node/test/cron.test.ts b/packages/node/test/cron.test.ts index 8f479e7a16d4..ac4a90b58e84 100644 --- a/packages/node/test/cron.test.ts +++ b/packages/node/test/cron.test.ts @@ -78,6 +78,26 @@ describe('cron check-ins', () => { }, }); }); + + test('throws with multiple jobs same name', () => { + const CronJobWithCheckIn = cron.instrumentCron(CronJobMock, 'my-cron-job'); + + CronJobWithCheckIn.from({ + cronTime: '* * * Jan,Sep Sun', + onTick: () => { + // + }, + }); + + expect(() => { + CronJobWithCheckIn.from({ + cronTime: '* * * Jan,Sep Sun', + onTick: () => { + // + }, + }); + }).toThrowError("A job named 'my-cron-job' has already been scheduled"); + }); }); describe('node-cron', () => { @@ -125,4 +145,64 @@ describe('cron check-ins', () => { }).toThrowError('Missing "name" for scheduled job. A name is required for Sentry check-in monitoring.'); }); }); + + describe('node-schedule', () => { + test('calls withMonitor', done => { + expect.assertions(4); + + class NodeScheduleMock { + scheduleJob(expression: string, callback: () => void): unknown { + expect(expression).toBe('* * * Jan,Sep Sun'); + expect(callback).toBeInstanceOf(Function); + return callback(); + } + } + + const scheduleWithCheckIn = cron.instrumentNodeSchedule(new NodeScheduleMock(), 'my-cron-job'); + + scheduleWithCheckIn.scheduleJob('* * * Jan,Sep Sun', () => { + expect(withMonitorSpy).toHaveBeenCalledTimes(1); + expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), { + schedule: { type: 'crontab', value: '* * * 1,9 0' }, + }); + done(); + }); + }); + + test('throws without crontab string', () => { + class NodeScheduleMock { + scheduleJob(_: string | Date, __: () => void): unknown { + return undefined; + } + } + + const scheduleWithCheckIn = cron.instrumentNodeSchedule(new NodeScheduleMock(), 'my-cron-job'); + + expect(() => { + scheduleWithCheckIn.scheduleJob(new Date(), () => { + // + }); + }).toThrowError('Automatic instrumentation of "node-schedule" only supports crontab string'); + }); + + test('throws with multiple jobs same name', () => { + class NodeScheduleMock { + scheduleJob(_: string, __: () => void): unknown { + return undefined; + } + } + + const scheduleWithCheckIn = cron.instrumentNodeSchedule(new NodeScheduleMock(), 'my-cron-job'); + + scheduleWithCheckIn.scheduleJob('* * * * *', () => { + // + }); + + expect(() => { + scheduleWithCheckIn.scheduleJob('* * * * *', () => { + // + }); + }).toThrowError("A job named 'my-cron-job' has already been scheduled"); + }); + }); }); From 898f47dd2378fc0b76bff44bbdc8893d0cc2f3b5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sat, 6 Jan 2024 14:18:14 +0100 Subject: [PATCH 2/2] Require first param name --- packages/node/src/cron/node-schedule.ts | 29 +++++++++---------- packages/node/test/cron.test.ts | 37 ++++++++++++++----------- 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/packages/node/src/cron/node-schedule.ts b/packages/node/src/cron/node-schedule.ts index 6b11542cbe37..79ae44a06e52 100644 --- a/packages/node/src/cron/node-schedule.ts +++ b/packages/node/src/cron/node-schedule.ts @@ -2,7 +2,11 @@ import { withMonitor } from '@sentry/core'; import { replaceCronNames } from './common'; export interface NodeSchedule { - scheduleJob(expression: string | Date | object, callback: () => void): unknown; + scheduleJob( + nameOrExpression: string | Date | object, + expressionOrCallback: string | Date | object | (() => void), + callback?: () => void, + ): unknown; } /** @@ -12,33 +16,30 @@ export interface NodeSchedule { * import * as Sentry from '@sentry/node'; * import * as schedule from 'node-schedule'; * - * const scheduleWithCheckIn = Sentry.cron.instrumentNodeSchedule(schedule, 'my-cron-job'); + * const scheduleWithCheckIn = Sentry.cron.instrumentNodeSchedule(schedule); * - * const job = scheduleWithCheckIn.scheduleJob('* * * * *', () => { + * const job = scheduleWithCheckIn.scheduleJob('my-cron-job', '* * * * *', () => { * console.log('You will see this message every minute'); * }); * ``` */ -export function instrumentNodeSchedule(lib: T & NodeSchedule, monitorSlug: string): T { - let jobScheduled = false; - +export function instrumentNodeSchedule(lib: T & NodeSchedule): T { return new Proxy(lib, { get(target, prop: keyof NodeSchedule) { if (prop === 'scheduleJob') { // eslint-disable-next-line @typescript-eslint/unbound-method return new Proxy(target.scheduleJob, { apply(target, thisArg, argArray: Parameters) { - const [expression] = argArray; - - if (typeof expression !== 'string') { - throw new Error('Automatic instrumentation of "node-schedule" only supports crontab string'); - } + const [nameOrExpression, expressionOrCallback] = argArray; - if (jobScheduled) { - throw new Error(`A job named '${monitorSlug}' has already been scheduled`); + if (typeof nameOrExpression !== 'string' || typeof expressionOrCallback !== 'string') { + throw new Error( + "Automatic instrumentation of 'node-schedule' requires the first parameter of 'scheduleJob' to be a job name string and the second parameter to be a crontab string", + ); } - jobScheduled = true; + const monitorSlug = nameOrExpression; + const expression = expressionOrCallback; return withMonitor( monitorSlug, diff --git a/packages/node/test/cron.test.ts b/packages/node/test/cron.test.ts index ac4a90b58e84..eee6d4a66711 100644 --- a/packages/node/test/cron.test.ts +++ b/packages/node/test/cron.test.ts @@ -148,19 +148,24 @@ describe('cron check-ins', () => { describe('node-schedule', () => { test('calls withMonitor', done => { - expect.assertions(4); + expect.assertions(5); class NodeScheduleMock { - scheduleJob(expression: string, callback: () => void): unknown { - expect(expression).toBe('* * * Jan,Sep Sun'); + scheduleJob( + nameOrExpression: string | Date | object, + expressionOrCallback: string | Date | object | (() => void), + callback: () => void, + ): unknown { + expect(nameOrExpression).toBe('my-cron-job'); + expect(expressionOrCallback).toBe('* * * Jan,Sep Sun'); expect(callback).toBeInstanceOf(Function); return callback(); } } - const scheduleWithCheckIn = cron.instrumentNodeSchedule(new NodeScheduleMock(), 'my-cron-job'); + const scheduleWithCheckIn = cron.instrumentNodeSchedule(new NodeScheduleMock()); - scheduleWithCheckIn.scheduleJob('* * * Jan,Sep Sun', () => { + scheduleWithCheckIn.scheduleJob('my-cron-job', '* * * Jan,Sep Sun', () => { expect(withMonitorSpy).toHaveBeenCalledTimes(1); expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), { schedule: { type: 'crontab', value: '* * * 1,9 0' }, @@ -171,38 +176,38 @@ describe('cron check-ins', () => { test('throws without crontab string', () => { class NodeScheduleMock { - scheduleJob(_: string | Date, __: () => void): unknown { + scheduleJob(_: string, __: string | Date, ___: () => void): unknown { return undefined; } } - const scheduleWithCheckIn = cron.instrumentNodeSchedule(new NodeScheduleMock(), 'my-cron-job'); + const scheduleWithCheckIn = cron.instrumentNodeSchedule(new NodeScheduleMock()); expect(() => { - scheduleWithCheckIn.scheduleJob(new Date(), () => { + scheduleWithCheckIn.scheduleJob('my-cron-job', new Date(), () => { // }); - }).toThrowError('Automatic instrumentation of "node-schedule" only supports crontab string'); + }).toThrowError( + "Automatic instrumentation of 'node-schedule' requires the first parameter of 'scheduleJob' to be a job name string and the second parameter to be a crontab string", + ); }); - test('throws with multiple jobs same name', () => { + test('throws without job name', () => { class NodeScheduleMock { scheduleJob(_: string, __: () => void): unknown { return undefined; } } - const scheduleWithCheckIn = cron.instrumentNodeSchedule(new NodeScheduleMock(), 'my-cron-job'); - - scheduleWithCheckIn.scheduleJob('* * * * *', () => { - // - }); + const scheduleWithCheckIn = cron.instrumentNodeSchedule(new NodeScheduleMock()); expect(() => { scheduleWithCheckIn.scheduleJob('* * * * *', () => { // }); - }).toThrowError("A job named 'my-cron-job' has already been scheduled"); + }).toThrowError( + "Automatic instrumentation of 'node-schedule' requires the first parameter of 'scheduleJob' to be a job name string and the second parameter to be a crontab string", + ); }); }); });