Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
64 changes: 33 additions & 31 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import type {
Integration,
Outcome,
ParameterizedString,
Scope,
SdkMetadata,
Session,
SessionAggregates,
Expand All @@ -44,6 +43,7 @@ import {
makeDsn,
rejectedSyncPromise,
resolvedSyncPromise,
uuid4,
} from '@sentry/utils';

import { getEnvelopeEndpointWithUrlEncodedAuth } from './api';
Expand All @@ -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';
Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/server-runtime-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/lib/hint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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' },
Expand All @@ -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' },
Expand Down
24 changes: 18 additions & 6 deletions packages/core/test/lib/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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,
);
});
Expand Down Expand Up @@ -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 () => {
Expand Down
4 changes: 1 addition & 3 deletions packages/nextjs/test/clientSdk.test.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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:
Expand Down Expand Up @@ -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.');
});

Expand Down
6 changes: 3 additions & 3 deletions packages/types/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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
Expand Down