From bf410ad7eca94402586d9ee4d7823358967246b6 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 4 May 2023 17:44:12 +0200 Subject: [PATCH 1/4] fix(node): Make sure we use same ID for checkIns --- packages/node/src/client.ts | 8 ++++++-- packages/node/src/sdk.ts | 1 + packages/node/test/client.test.ts | 27 +++++++++++++++++++++++---- packages/types/src/checkin.ts | 6 ++++-- 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index fd0e94de595b..052d3475c968 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -153,8 +153,9 @@ export class NodeClient extends BaseClient { * @param checkIn An object that describes a check in. * @param upsertMonitorConfig An optional object that describes a monitor config. Use this if you want * to create a monitor automatically when sending a check in. + * @returns A string representing the id of the check in. */ - public captureCheckIn(checkIn: CheckIn, monitorConfig?: MonitorConfig): void { + public captureCheckIn(checkIn: CheckIn, monitorConfig?: MonitorConfig): string | undefined { if (!this._isEnabled()) { __DEBUG_BUILD__ && logger.warn('SDK not enabled, will not capture checkin.'); return; @@ -163,8 +164,10 @@ export class NodeClient extends BaseClient { const options = this.getOptions(); const { release, environment, tunnel } = options; + const id = checkIn.checkInId || uuid4(); + const serializedCheckIn: SerializedCheckIn = { - check_in_id: uuid4(), + check_in_id: id, monitor_slug: checkIn.monitorSlug, status: checkIn.status, duration: checkIn.duration, @@ -183,6 +186,7 @@ export class NodeClient extends BaseClient { const envelope = createCheckInEnvelope(serializedCheckIn, this.getSdkMetadata(), tunnel, this.getDsn()); void this._sendEnvelope(envelope); + return id; } /** diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index d8bb6c25c989..df3df4018840 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -279,6 +279,7 @@ export function captureCheckIn( } __DEBUG_BUILD__ && logger.warn('Cannot capture check in. No client defined.'); + return; } /** Node.js stack parser */ diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts index c29627accb2e..0226851444f1 100644 --- a/packages/node/test/client.test.ts +++ b/packages/node/test/client.test.ts @@ -294,8 +294,8 @@ describe('NodeClient', () => { // @ts-ignore accessing private method const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope'); - client.captureCheckIn( - { monitorSlug: 'foo', status: 'ok', duration: 1222 }, + const id = client.captureCheckIn( + { monitorSlug: 'foo', status: 'in_progress' }, { schedule: { type: 'crontab', @@ -315,9 +315,8 @@ describe('NodeClient', () => { expect.any(Object), { check_in_id: expect.any(String), - duration: 1222, monitor_slug: 'foo', - status: 'ok', + status: 'in_progress', release: '1.0.0', environment: 'dev', monitor_config: { @@ -333,6 +332,26 @@ describe('NodeClient', () => { ], ], ]); + + client.captureCheckIn({ monitorSlug: 'foo', status: 'ok', duration: 1222, checkInId: id }); + + expect(sendEnvelopeSpy).toHaveBeenCalledTimes(2); + expect(sendEnvelopeSpy).toHaveBeenCalledWith([ + expect.any(Object), + [ + [ + expect.any(Object), + { + check_in_id: id, + monitor_slug: 'foo', + duration: 1222, + status: 'ok', + release: '1.0.0', + environment: 'dev', + }, + ], + ], + ]); }); it('does not send a checkIn envelope if disabled', () => { diff --git a/packages/types/src/checkin.ts b/packages/types/src/checkin.ts index 67537e46d390..67ea0fd6393a 100644 --- a/packages/types/src/checkin.ts +++ b/packages/types/src/checkin.ts @@ -38,14 +38,16 @@ export interface SerializedCheckIn { }; } -export interface CheckIn { +export type CheckIn = { // The distinct slug of the monitor. monitorSlug: SerializedCheckIn['monitor_slug']; + // Check-In ID (unique and client generated). + checkInId?: SerializedCheckIn['check_in_id']; // The status of the check-in. status: SerializedCheckIn['status']; // The duration of the check-in in seconds. Will only take effect if the status is ok or error. duration?: SerializedCheckIn['duration']; -} +}; type SerializedMonitorConfig = NonNullable; From 632a9d06063e1baee2ab695c368da0147ff1fe4c Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 5 May 2023 09:32:16 +0200 Subject: [PATCH 2/4] make types more strict --- packages/node/src/client.ts | 7 +++++-- packages/node/test/client.test.ts | 2 +- packages/types/src/checkin.ts | 17 +++++++++++++---- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 052d3475c968..7ac4eb8b0e6c 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -164,17 +164,20 @@ export class NodeClient extends BaseClient { const options = this.getOptions(); const { release, environment, tunnel } = options; - const id = checkIn.checkInId || uuid4(); + const id = checkIn.status === 'in_progress' ? uuid4() : checkIn.checkInId; const serializedCheckIn: SerializedCheckIn = { check_in_id: id, monitor_slug: checkIn.monitorSlug, status: checkIn.status, - duration: checkIn.duration, release, environment, }; + if (checkIn.status !== 'in_progress') { + serializedCheckIn.duration = checkIn.duration; + } + if (monitorConfig) { serializedCheckIn.monitor_config = { schedule: monitorConfig.schedule, diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts index 0226851444f1..e0d9922e7c43 100644 --- a/packages/node/test/client.test.ts +++ b/packages/node/test/client.test.ts @@ -361,7 +361,7 @@ describe('NodeClient', () => { // @ts-ignore accessing private method const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope'); - client.captureCheckIn({ monitorSlug: 'foo', status: 'ok', duration: 1222 }); + client.captureCheckIn({ monitorSlug: 'foo', status: 'in_progress' }); expect(sendEnvelopeSpy).toHaveBeenCalledTimes(0); }); diff --git a/packages/types/src/checkin.ts b/packages/types/src/checkin.ts index 67ea0fd6393a..779ad047b4a5 100644 --- a/packages/types/src/checkin.ts +++ b/packages/types/src/checkin.ts @@ -38,16 +38,25 @@ export interface SerializedCheckIn { }; } -export type CheckIn = { +interface InProgressCheckIn { // The distinct slug of the monitor. monitorSlug: SerializedCheckIn['monitor_slug']; + // The status of the check-in. + status: 'in_progress'; +} + +interface FinishedCheckIn { + // The distinct slug of the monitor. + monitorSlug: SerializedCheckIn['monitor_slug']; + // The status of the check-in. + status: 'ok' | 'error'; // Check-In ID (unique and client generated). checkInId?: SerializedCheckIn['check_in_id']; - // The status of the check-in. - status: SerializedCheckIn['status']; // The duration of the check-in in seconds. Will only take effect if the status is ok or error. duration?: SerializedCheckIn['duration']; -}; +} + +export type CheckIn = InProgressCheckIn | FinishedCheckIn; type SerializedMonitorConfig = NonNullable; From 91b28a68e0b6624adb6950b110c562cf2f611103 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 5 May 2023 09:41:48 +0200 Subject: [PATCH 3/4] actually get the type correct this time --- packages/node/src/client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 7ac4eb8b0e6c..2f633609deb8 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -164,7 +164,7 @@ export class NodeClient extends BaseClient { const options = this.getOptions(); const { release, environment, tunnel } = options; - const id = checkIn.status === 'in_progress' ? uuid4() : checkIn.checkInId; + const id = checkIn.status !== 'in_progress' && checkIn.checkInId ? checkIn.checkInId : uuid4(); const serializedCheckIn: SerializedCheckIn = { check_in_id: id, From b12db4bc70c078abd59b20f47b3584d27cc3fab3 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 5 May 2023 11:14:08 +0200 Subject: [PATCH 4/4] always return an id from capture methods --- packages/node/src/client.ts | 7 +++---- packages/node/src/sdk.ts | 11 ++++++++--- packages/node/test/client.test.ts | 2 +- packages/node/test/sdk.test.ts | 20 ++++++++++++++++++++ packages/types/src/checkin.ts | 4 ++-- 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 2f633609deb8..af39f786ac3c 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -155,17 +155,16 @@ export class NodeClient extends BaseClient { * to create a monitor automatically when sending a check in. * @returns A string representing the id of the check in. */ - public captureCheckIn(checkIn: CheckIn, monitorConfig?: MonitorConfig): string | undefined { + public captureCheckIn(checkIn: CheckIn, monitorConfig?: MonitorConfig): string { + const id = checkIn.status !== 'in_progress' && checkIn.checkInId ? checkIn.checkInId : uuid4(); if (!this._isEnabled()) { __DEBUG_BUILD__ && logger.warn('SDK not enabled, will not capture checkin.'); - return; + return id; } const options = this.getOptions(); const { release, environment, tunnel } = options; - const id = checkIn.status !== 'in_progress' && checkIn.checkInId ? checkIn.checkInId : uuid4(); - const serializedCheckIn: SerializedCheckIn = { check_in_id: id, monitor_slug: checkIn.monitorSlug, diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index df3df4018840..d4e2df4ac5f9 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -13,6 +13,7 @@ import { logger, nodeStackLineParser, stackParserFromStackParserOptions, + uuid4, } from '@sentry/utils'; import { setNodeAsyncContextStrategy } from './async'; @@ -273,13 +274,17 @@ export function captureCheckIn( checkIn: CheckIn, upsertMonitorConfig?: MonitorConfig, ): ReturnType { + const capturedCheckIn = + checkIn.status !== 'in_progress' && checkIn.checkInId ? checkIn : { ...checkIn, checkInId: uuid4() }; + const client = getCurrentHub().getClient(); if (client) { - return client.captureCheckIn(checkIn, upsertMonitorConfig); + client.captureCheckIn(capturedCheckIn, upsertMonitorConfig); + } else { + __DEBUG_BUILD__ && logger.warn('Cannot capture check in. No client defined.'); } - __DEBUG_BUILD__ && logger.warn('Cannot capture check in. No client defined.'); - return; + return capturedCheckIn.checkInId; } /** Node.js stack parser */ diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts index e0d9922e7c43..ee5fd5bdd957 100644 --- a/packages/node/test/client.test.ts +++ b/packages/node/test/client.test.ts @@ -314,7 +314,7 @@ describe('NodeClient', () => { [ expect.any(Object), { - check_in_id: expect.any(String), + check_in_id: id, monitor_slug: 'foo', status: 'in_progress', release: '1.0.0', diff --git a/packages/node/test/sdk.test.ts b/packages/node/test/sdk.test.ts index abd0265b62c4..f7c2595c66a5 100644 --- a/packages/node/test/sdk.test.ts +++ b/packages/node/test/sdk.test.ts @@ -1,5 +1,7 @@ +import { getCurrentHub } from '@sentry/core'; import type { Integration } from '@sentry/types'; +import type { NodeClient } from '../build/types'; import { init } from '../src/sdk'; import * as sdk from '../src/sdk'; @@ -90,3 +92,21 @@ describe('init()', () => { expect(newIntegration.setupOnce as jest.Mock).toHaveBeenCalledTimes(1); }); }); + +describe('captureCheckIn', () => { + it('always returns an id', () => { + const hub = getCurrentHub(); + const client = hub.getClient(); + expect(client).toBeDefined(); + + const captureCheckInSpy = jest.spyOn(client!, 'captureCheckIn'); + + // test if captureCheckIn returns an id even if client is not defined + hub.bindClient(undefined); + + expect(captureCheckInSpy).toHaveBeenCalledTimes(0); + expect(sdk.captureCheckIn({ monitorSlug: 'gogogo', status: 'in_progress' })).toBeTruthy(); + + hub.bindClient(client); + }); +}); diff --git a/packages/types/src/checkin.ts b/packages/types/src/checkin.ts index 779ad047b4a5..a316c0c7a375 100644 --- a/packages/types/src/checkin.ts +++ b/packages/types/src/checkin.ts @@ -45,13 +45,13 @@ interface InProgressCheckIn { status: 'in_progress'; } -interface FinishedCheckIn { +export interface FinishedCheckIn { // The distinct slug of the monitor. monitorSlug: SerializedCheckIn['monitor_slug']; // The status of the check-in. status: 'ok' | 'error'; // Check-In ID (unique and client generated). - checkInId?: SerializedCheckIn['check_in_id']; + checkInId: SerializedCheckIn['check_in_id']; // The duration of the check-in in seconds. Will only take effect if the status is ok or error. duration?: SerializedCheckIn['duration']; }