ref: Always return an immediately generated event ID from captureException(), captureMessage(), and captureEvent() (#11805)

Luca Forstner · web-flow · commit 48995b183e71 · 2024-04-26T14:18:14.000+02:00
diff --git a/MIGRATION.md b/MIGRATION.md
@@ -1119,6 +1119,7 @@ Sentry.init({
 - [Updated behaviour of `transactionContext` passed to `tracesSampler`](./MIGRATION.md#transactioncontext-no-longer-passed-to-tracessampler)
 - [Updated behaviour of `getClient()`](./MIGRATION.md#getclient-always-returns-a-client)
 - [Updated behaviour of the SDK in combination with `onUncaughtException` handlers in Node.js](./MIGRATION.md#behaviour-in-combination-with-onuncaughtexception-handlers-in-node.js)
+- [Updated expected return value for `captureException()`, `captureMessage()` and `captureEvent` methods on Clients](./MIGRATION.md#updated-expected-return-value-for-captureexception-capturemessage-and-captureevent-methods-on-clients)
 - [Removal of Client-Side health check transaction filters](./MIGRATION.md#removal-of-client-side-health-check-transaction-filters)
 - [Change of Replay default options (`unblock` and `unmask`)](./MIGRATION.md#change-of-replay-default-options-unblock-and-unmask)
 - [Angular Tracing Decorator renaming](./MIGRATION.md#angular-tracing-decorator-renaming)
@@ -1179,6 +1180,11 @@ for this option defaulted to `true`.
 Going forward, the default value for `exitEvenIfOtherHandlersAreRegistered` will be `false`, meaning that the SDK will
 not exit your process when you have registered other `onUncaughtException` handlers.
 
+#### Updated expected return value for `captureException()`, `captureMessage()` and `captureEvent` methods on Clients
+
+The `Client` interface now expects implementations to always return a string representing the generated event ID for the
+`captureException()`, `captureMessage()`, `captureEvent()` methods. Previously `undefined` was a valid return value.
+
 #### Removal of Client-Side health check transaction filters
 
 The SDK no longer filters out health check transactions by default. Instead, they are sent to Sentry but still dropped
diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts
@@ -17,7 +17,6 @@ import type {
   Integration,
   Outcome,
   ParameterizedString,
-  Scope,
   SdkMetadata,
   Session,
   SessionAggregates,
@@ -44,6 +43,7 @@ import {
   makeDsn,
   rejectedSyncPromise,
   resolvedSyncPromise,
+  uuid4,
 } from '@sentry/utils';
 
 import { getEnvelopeEndpointWithUrlEncodedAuth } from './api';
@@ -53,6 +53,7 @@ import { createEventEnvelope, createSessionEnvelope } from './envelope';
 import type { IntegrationIndex } from './integration';
 import { afterSetupIntegrations } from './integration';
 import { setupIntegration, setupIntegrations } from './integration';
+import type { Scope } from './scope';
 import { updateSession } from './session';
 import { getDynamicSamplingContextFromClient } from './tracing/dynamicSamplingContext';
 import { parseSampleRate } from './utils/parseSampleRate';
@@ -151,24 +152,27 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
    * @inheritDoc
    */
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  public captureException(exception: any, hint?: EventHint, currentScope?: Scope): string | undefined {
+  public captureException(exception: any, hint?: EventHint, scope?: Scope): string {
+    const eventId = uuid4();
+
     // ensure we haven't captured this very object before
     if (checkOrSetAlreadyCaught(exception)) {
       DEBUG_BUILD && logger.log(ALREADY_SEEN_ERROR);
-      return;
+      return eventId;
     }
 
-    let eventId: string | undefined = hint && hint.event_id;
+    const hintWithEventId = {
+      event_id: eventId,
+      ...hint,
+    };
 
     this._process(
-      this.eventFromException(exception, hint)
-        .then(event => this._captureEvent(event, hint, currentScope))
-        .then(result => {
-          eventId = result;
-        }),
+      this.eventFromException(exception, hintWithEventId).then(event =>
+        this._captureEvent(event, hintWithEventId, scope),
+      ),
     );
 
-    return eventId;
+    return hintWithEventId.event_id;
   }
 
   /**
@@ -179,48 +183,46 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     level?: SeverityLevel,
     hint?: EventHint,
     currentScope?: Scope,
-  ): string | undefined {
-    let eventId: string | undefined = hint && hint.event_id;
+  ): string {
+    const hintWithEventId = {
+      event_id: uuid4(),
+      ...hint,
+    };
 
     const eventMessage = isParameterizedString(message) ? message : String(message);
 
     const promisedEvent = isPrimitive(message)
-      ? this.eventFromMessage(eventMessage, level, hint)
-      : this.eventFromException(message, hint);
+      ? this.eventFromMessage(eventMessage, level, hintWithEventId)
+      : this.eventFromException(message, hintWithEventId);
 
-    this._process(
-      promisedEvent
-        .then(event => this._captureEvent(event, hint, currentScope))
-        .then(result => {
-          eventId = result;
-        }),
-    );
+    this._process(promisedEvent.then(event => this._captureEvent(event, hintWithEventId, currentScope)));
 
-    return eventId;
+    return hintWithEventId.event_id;
   }
 
   /**
    * @inheritDoc
    */
-  public captureEvent(event: Event, hint?: EventHint, currentScope?: Scope): string | undefined {
+  public captureEvent(event: Event, hint?: EventHint, currentScope?: Scope): string {
+    const eventId = uuid4();
+
     // ensure we haven't captured this very object before
     if (hint && hint.originalException && checkOrSetAlreadyCaught(hint.originalException)) {
       DEBUG_BUILD && logger.log(ALREADY_SEEN_ERROR);
-      return;
+      return eventId;
     }
 
-    let eventId: string | undefined = hint && hint.event_id;
+    const hintWithEventId = {
+      event_id: eventId,
+      ...hint,
+    };
 
     const sdkProcessingMetadata = event.sdkProcessingMetadata || {};
     const capturedSpanScope: Scope | undefined = sdkProcessingMetadata.capturedSpanScope;
 
-    this._process(
-      this._captureEvent(event, hint, capturedSpanScope || currentScope).then(result => {
-        eventId = result;
-      }),
-    );
+    this._process(this._captureEvent(event, hintWithEventId, capturedSpanScope || currentScope));
 
-    return eventId;
+    return hintWithEventId.event_id;
   }
 
   /**
diff --git a/packages/core/src/server-runtime-client.ts b/packages/core/src/server-runtime-client.ts
@@ -76,7 +76,7 @@ export class ServerRuntimeClient<
    * @inheritDoc
    */
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  public captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined {
+  public captureException(exception: any, hint?: EventHint, scope?: Scope): string {
     // Check if the flag `autoSessionTracking` is enabled, and if `_sessionFlusher` exists because it is initialised only
     // when the `requestHandler` middleware is used, and hence the expectation is to have SessionAggregates payload
     // sent to the Server only when the `requestHandler` middleware is used
@@ -96,7 +96,7 @@ export class ServerRuntimeClient<
   /**
    * @inheritDoc
    */
-  public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string | undefined {
+  public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string {
     // Check if the flag `autoSessionTracking` is enabled, and if `_sessionFlusher` exists because it is initialised only
     // when the `requestHandler` middleware is used, and hence the expectation is to have SessionAggregates payload
     // sent to the Server only when the `requestHandler` middleware is used
diff --git a/packages/core/test/lib/hint.test.ts b/packages/core/test/lib/hint.test.ts
@@ -36,7 +36,7 @@ describe('Hint', () => {
       client.captureEvent({});
 
       const [, hint] = sendEvent.mock.calls[0];
-      expect(hint).toEqual({ attachments: [{ filename: 'another.file', data: 'more text' }] });
+      expect(hint).toMatchObject({ attachments: [{ filename: 'another.file', data: 'more text' }] });
     });
 
     test('gets passed through to `beforeSend` and can be further mutated', () => {
@@ -54,7 +54,7 @@ describe('Hint', () => {
       client.captureEvent({}, { attachments: [{ filename: 'some-file.txt', data: 'Hello' }] });
 
       const [, hint] = sendEvent.mock.calls[0];
-      expect(hint).toEqual({
+      expect(hint).toMatchObject({
         attachments: [
           { filename: 'some-file.txt', data: 'Hello' },
           { filename: 'another.file', data: 'more text' },
@@ -80,7 +80,7 @@ describe('Hint', () => {
       );
 
       const [, hint] = sendEvent.mock.calls[0];
-      expect(hint).toEqual({
+      expect(hint).toMatchObject({
         attachments: [
           { filename: 'some-file.txt', data: 'Hello' },
           { filename: 'another.file', data: 'more text' },
diff --git a/packages/core/test/lib/integration.test.ts b/packages/core/test/lib/integration.test.ts
@@ -457,8 +457,16 @@ describe('setupIntegration', () => {
     expect(integration3.preprocessEvent).toHaveBeenCalledTimes(3);
     expect(integration4.preprocessEvent).toHaveBeenCalledTimes(0);
 
-    expect(integration1.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '1b' }, {}, client1);
-    expect(integration3.preprocessEvent).toHaveBeenLastCalledWith({ event_id: '2c' }, {}, client2);
+    expect(integration1.preprocessEvent).toHaveBeenLastCalledWith(
+      { event_id: '1b' },
+      { event_id: expect.any(String) },
+      client1,
+    );
+    expect(integration3.preprocessEvent).toHaveBeenLastCalledWith(
+      { event_id: '2c' },
+      { event_id: expect.any(String) },
+      client2,
+    );
   });
 
   it('allows to mutate events in preprocessEvent', async () => {
@@ -484,7 +492,9 @@ describe('setupIntegration', () => {
     await client.flush();
 
     expect(sendEvent).toHaveBeenCalledTimes(1);
-    expect(sendEvent).toHaveBeenCalledWith(expect.objectContaining({ event_id: 'mutated' }), {});
+    expect(sendEvent).toHaveBeenCalledWith(expect.objectContaining({ event_id: 'mutated' }), {
+      event_id: expect.any(String),
+    });
   });
 
   it('binds processEvent for each client', () => {
@@ -531,12 +541,12 @@ describe('setupIntegration', () => {
 
     expect(integration1.processEvent).toHaveBeenLastCalledWith(
       expect.objectContaining({ event_id: '1b' }),
-      {},
+      { event_id: expect.any(String) },
       client1,
     );
     expect(integration3.processEvent).toHaveBeenLastCalledWith(
       expect.objectContaining({ event_id: '2c' }),
-      {},
+      { event_id: expect.any(String) },
       client2,
     );
   });
@@ -564,7 +574,9 @@ describe('setupIntegration', () => {
     await client.flush();
 
     expect(sendEvent).toHaveBeenCalledTimes(1);
-    expect(sendEvent).toHaveBeenCalledWith(expect.objectContaining({ event_id: 'mutated' }), {});
+    expect(sendEvent).toHaveBeenCalledWith(expect.objectContaining({ event_id: 'mutated' }), {
+      event_id: expect.any(String),
+    });
   });
 
   it('allows to drop events in processEvent', async () => {
diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts
@@ -1,4 +1,4 @@
-import { BaseClient, getGlobalScope, getIsolationScope } from '@sentry/core';
+import { getGlobalScope, getIsolationScope } from '@sentry/core';
 import * as SentryReact from '@sentry/react';
 import type { BrowserClient } from '@sentry/react';
 import { WINDOW, getClient, getCurrentScope } from '@sentry/react';
@@ -9,7 +9,6 @@ import { JSDOM } from 'jsdom';
 import { breadcrumbsIntegration, browserTracingIntegration, init } from '../src/client';
 
 const reactInit = jest.spyOn(SentryReact, 'init');
-const captureEvent = jest.spyOn(BaseClient.prototype, 'captureEvent');
 const loggerLogSpy = jest.spyOn(logger, 'log');
 
 // We're setting up JSDom here because the Next.js routing instrumentations requires a few things to be present on pageload:
@@ -96,7 +95,6 @@ describe('Client init()', () => {
     });
 
     expect(transportSend).not.toHaveBeenCalled();
-    expect(captureEvent.mock.results[0].value).toBeUndefined();
     expect(loggerLogSpy).toHaveBeenCalledWith('An event processor returned `null`, will not send event.');
   });
 
diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts
@@ -38,7 +38,7 @@ export interface Client<O extends ClientOptions = ClientOptions> {
    * @param currentScope An optional scope containing event metadata.
    * @returns The event id
    */
-  captureException(exception: any, hint?: EventHint, currentScope?: Scope): string | undefined;
+  captureException(exception: any, hint?: EventHint, currentScope?: Scope): string;
 
   /**
    * Captures a message event and sends it to Sentry.
@@ -51,7 +51,7 @@ export interface Client<O extends ClientOptions = ClientOptions> {
    * @param currentScope An optional scope containing event metadata.
    * @returns The event id
    */
-  captureMessage(message: string, level?: SeverityLevel, hint?: EventHint, currentScope?: Scope): string | undefined;
+  captureMessage(message: string, level?: SeverityLevel, hint?: EventHint, currentScope?: Scope): string;
 
   /**
    * Captures a manually created event and sends it to Sentry.
@@ -63,7 +63,7 @@ export interface Client<O extends ClientOptions = ClientOptions> {
    * @param currentScope An optional scope containing event metadata.
    * @returns The event id
    */
-  captureEvent(event: Event, hint?: EventHint, currentScope?: Scope): string | undefined;
+  captureEvent(event: Event, hint?: EventHint, currentScope?: Scope): string;
 
   /**
    * Captures a session
