From 6c826b26c8d00b719f57a655aaeaac51c769a12b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 21 Nov 2022 13:13:40 +0100 Subject: [PATCH 1/4] fix(core): Only generate eventIds in client --- packages/core/src/baseclient.ts | 7 ++- packages/core/src/hub.ts | 76 ++++++++++++++++----------------- 2 files changed, 39 insertions(+), 44 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 683420217613..23f70bb9a8b9 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -127,8 +127,7 @@ export abstract class BaseClient implements Client { return; } - let eventId: string | undefined = hint && hint.event_id; - + let eventId: string | undefined; this._process( this.eventFromException(exception, hint) .then(event => this._captureEvent(event, hint, scope)) @@ -150,7 +149,7 @@ export abstract class BaseClient implements Client { hint?: EventHint, scope?: Scope, ): string | undefined { - let eventId: string | undefined = hint && hint.event_id; + let eventId: string | undefined; const promisedEvent = isPrimitive(message) ? this.eventFromMessage(String(message), level, hint) @@ -177,7 +176,7 @@ export abstract class BaseClient implements Client { return; } - let eventId: string | undefined = hint && hint.event_id; + let eventId: string | undefined; this._process( this._captureEvent(event, hint, scope).then(result => { diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 5036158e08eb..c85bd1c7ebd8 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -27,12 +27,13 @@ import { GLOBAL_OBJ, isNodeEnv, logger, - uuid4, } from '@sentry/utils'; import { Scope } from './scope'; import { closeSession, makeSession, updateSession } from './session'; +const NIL_EVENT_ID = '00000000000000000000000000000000'; + /** * API compatibility version of this hub. * @@ -184,21 +185,19 @@ export class Hub implements HubInterface { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types public captureException(exception: any, hint?: EventHint): string { - const eventId = (this._lastEventId = hint && hint.event_id ? hint.event_id : uuid4()); const syntheticException = new Error('Sentry syntheticException'); - this._withClient((client, scope) => { - client.captureException( - exception, - { - originalException: exception, - syntheticException, - ...hint, - event_id: eventId, - }, - scope, - ); - }); - return eventId; + return (this._lastEventId = + this._withClient((client, scope) => { + return client.captureException( + exception, + { + originalException: exception, + syntheticException, + ...hint, + }, + scope, + ); + }) || NIL_EVENT_ID); } /** @@ -210,37 +209,36 @@ export class Hub implements HubInterface { level?: Severity | SeverityLevel, hint?: EventHint, ): string { - const eventId = (this._lastEventId = hint && hint.event_id ? hint.event_id : uuid4()); const syntheticException = new Error(message); - this._withClient((client, scope) => { - client.captureMessage( - message, - level, - { - originalException: message, - syntheticException, - ...hint, - event_id: eventId, - }, - scope, - ); - }); - return eventId; + return (this._lastEventId = + this._withClient((client, scope) => { + return client.captureMessage( + message, + level, + { + originalException: message, + syntheticException, + ...hint, + }, + scope, + ); + }) || NIL_EVENT_ID); } /** * @inheritDoc */ public captureEvent(event: Event, hint?: EventHint): string { - const eventId = hint && hint.event_id ? hint.event_id : uuid4(); + const clientId = + this._withClient((client, scope) => { + return client.captureEvent(event, { ...hint }, scope); + }) || NIL_EVENT_ID; + if (event.type !== 'transaction') { - this._lastEventId = eventId; + this._lastEventId = clientId; } - this._withClient((client, scope) => { - client.captureEvent(event, { ...hint, event_id: eventId }, scope); - }); - return eventId; + return clientId; } /** @@ -469,11 +467,9 @@ export class Hub implements HubInterface { * @param method The method to call on the client. * @param args Arguments to pass to the client function. */ - private _withClient(callback: (client: Client, scope: Scope | undefined) => void): void { + private _withClient(callback: (client: Client, scope: Scope | undefined) => T): T | undefined { const { scope, client } = this.getStackTop(); - if (client) { - callback(client, scope); - } + return client && callback(client, scope); } /** From 83518f46788aacd48f0a29d48adda524ccc9e22c Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 21 Nov 2022 14:05:58 +0100 Subject: [PATCH 2/4] update test --- packages/hub/test/hub.test.ts | 42 +++++++++++------------------------ 1 file changed, 13 insertions(+), 29 deletions(-) diff --git a/packages/hub/test/hub.test.ts b/packages/hub/test/hub.test.ts index 5ce521ead4c4..058f46c1bcbf 100644 --- a/packages/hub/test/hub.test.ts +++ b/packages/hub/test/hub.test.ts @@ -7,11 +7,13 @@ import { getCurrentHub, Hub, Scope } from '../src'; const clientFn: any = jest.fn(); +const MOCK_EVENT_ID = '7bab5137428b4de29891fb8bd34a31cb'; + function makeClient() { return { getOptions: jest.fn(), captureEvent: jest.fn(), - captureException: jest.fn(), + captureException: jest.fn().mockReturnValue(MOCK_EVENT_ID), close: jest.fn(), flush: jest.fn(), getDsn: jest.fn(), @@ -224,16 +226,6 @@ describe('Hub', () => { expect(args[0]).toBe('a'); }); - test('should set event_id in hint', () => { - const testClient = makeClient(); - const hub = new Hub(testClient); - - hub.captureException('a'); - const args = getPassedArgs(testClient.captureException); - - expect(args[1].event_id).toBeTruthy(); - }); - test('should keep event_id from hint', () => { const testClient = makeClient(); const hub = new Hub(testClient); @@ -270,14 +262,12 @@ describe('Hub', () => { expect(args[0]).toBe('a'); }); - test('should set event_id in hint', () => { + test('should get event_id from client', () => { const testClient = makeClient(); const hub = new Hub(testClient); - hub.captureMessage('a'); - const args = getPassedArgs(testClient.captureMessage); - - expect(args[2].event_id).toBeTruthy(); + const id = hub.captureMessage('a'); + expect(id).toBeTruthy(); }); test('should keep event_id from hint', () => { @@ -318,17 +308,15 @@ describe('Hub', () => { expect(args[0]).toBe(event); }); - test('should set event_id in hint', () => { + test('should get event_id from client', () => { const event: Event = { extra: { b: 3 }, }; const testClient = makeClient(); const hub = new Hub(testClient); - hub.captureEvent(event); - const args = getPassedArgs(testClient.captureEvent); - - expect(args[1].event_id).toBeTruthy(); + const id = hub.captureEvent(event); + expect(id).toBeTruthy(); }); test('should keep event_id from hint', () => { @@ -352,10 +340,8 @@ describe('Hub', () => { const testClient = makeClient(); const hub = new Hub(testClient); - hub.captureEvent(event); - const args = getPassedArgs(testClient.captureEvent); - - expect(args[1].event_id).toEqual(hub.lastEventId()); + const id = hub.captureEvent(event); + expect(id).toEqual(hub.lastEventId()); }); test('transactions do not set lastEventId', () => { @@ -366,10 +352,8 @@ describe('Hub', () => { const testClient = makeClient(); const hub = new Hub(testClient); - hub.captureEvent(event); - const args = getPassedArgs(testClient.captureEvent); - - expect(args[1].event_id).not.toEqual(hub.lastEventId()); + const id = hub.captureEvent(event); + expect(id).not.toEqual(hub.lastEventId()); }); }); From 8f810c2a58e416425e91114a2a952ba8c93f8e2f Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 22 Nov 2022 15:18:41 +0100 Subject: [PATCH 3/4] explicit returns --- packages/core/src/hub.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index c85bd1c7ebd8..1966269a9b47 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -186,7 +186,7 @@ export class Hub implements HubInterface { // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types public captureException(exception: any, hint?: EventHint): string { const syntheticException = new Error('Sentry syntheticException'); - return (this._lastEventId = + this._lastEventId = this._withClient((client, scope) => { return client.captureException( exception, @@ -197,7 +197,8 @@ export class Hub implements HubInterface { }, scope, ); - }) || NIL_EVENT_ID); + }) || NIL_EVENT_ID; + return this._lastEventId; } /** @@ -210,7 +211,7 @@ export class Hub implements HubInterface { hint?: EventHint, ): string { const syntheticException = new Error(message); - return (this._lastEventId = + this._lastEventId = this._withClient((client, scope) => { return client.captureMessage( message, @@ -222,7 +223,8 @@ export class Hub implements HubInterface { }, scope, ); - }) || NIL_EVENT_ID); + }) || NIL_EVENT_ID; + return this._lastEventId; } /** From 49bf93077112c12de480aac569ff35d2bc7aaa2d Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 22 Nov 2022 15:26:40 +0100 Subject: [PATCH 4/4] clean up tests --- packages/hub/test/hub.test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/hub/test/hub.test.ts b/packages/hub/test/hub.test.ts index 058f46c1bcbf..2a1c9d4754a4 100644 --- a/packages/hub/test/hub.test.ts +++ b/packages/hub/test/hub.test.ts @@ -226,6 +226,14 @@ describe('Hub', () => { expect(args[0]).toBe('a'); }); + test('should get event_id from client', () => { + const testClient = makeClient(); + const hub = new Hub(testClient); + + const id = hub.captureException('a'); + expect(id).toBeDefined(); + }); + test('should keep event_id from hint', () => { const testClient = makeClient(); const hub = new Hub(testClient); @@ -267,7 +275,7 @@ describe('Hub', () => { const hub = new Hub(testClient); const id = hub.captureMessage('a'); - expect(id).toBeTruthy(); + expect(id).toBeDefined(); }); test('should keep event_id from hint', () => { @@ -316,7 +324,7 @@ describe('Hub', () => { const hub = new Hub(testClient); const id = hub.captureEvent(event); - expect(id).toBeTruthy(); + expect(id).toBeDefined(); }); test('should keep event_id from hint', () => {