Skip to content

Commit

Permalink
feat(replay): Add onError callback + other small improvements to de…
Browse files Browse the repository at this point in the history
…bugging (#13721)

* Adds an `onError` callback for replay SDK exceptions
* Do not log empty messages when calling `logger.exception`
* Send `ratelimit_backoff` client report when necessary (instead of
generic `send_error`)
  • Loading branch information
billyvg committed Sep 19, 2024
1 parent 336a236 commit cf0152a
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 15 deletions.
9 changes: 7 additions & 2 deletions packages/replay-internal/src/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import { getHandleRecordingEmit } from './util/handleRecordingEmit';
import { isExpired } from './util/isExpired';
import { isSessionExpired } from './util/isSessionExpired';
import { sendReplay } from './util/sendReplay';
import { RateLimitError } from './util/sendReplayRequest';
import type { SKIPPED } from './util/throttle';
import { THROTTLED, throttle } from './util/throttle';

Expand Down Expand Up @@ -245,6 +246,9 @@ export class ReplayContainer implements ReplayContainerInterface {
/** A wrapper to conditionally capture exceptions. */
public handleException(error: unknown): void {
DEBUG_BUILD && logger.exception(error);
if (this._options.onError) {
this._options.onError(error);
}
}

/**
Expand Down Expand Up @@ -1157,8 +1161,8 @@ export class ReplayContainer implements ReplayContainerInterface {
segmentId,
eventContext,
session: this.session,
options: this.getOptions(),
timestamp,
onError: err => this.handleException(err),
});
} catch (err) {
this.handleException(err);
Expand All @@ -1173,7 +1177,8 @@ export class ReplayContainer implements ReplayContainerInterface {
const client = getClient();

if (client) {
client.recordDroppedEvent('send_error', 'replay');
const dropReason = err instanceof RateLimitError ? 'ratelimit_backoff' : 'send_error';
client.recordDroppedEvent(dropReason, 'replay');
}
}
}
Expand Down
8 changes: 7 additions & 1 deletion packages/replay-internal/src/types/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export interface SendReplayData {
eventContext: PopEventContext;
timestamp: number;
session: Session;
options: ReplayPluginOptions;
onError?: (err: unknown) => void;
}

export interface Timeouts {
Expand Down Expand Up @@ -222,6 +222,12 @@ export interface ReplayPluginOptions extends ReplayNetworkOptions {
*/
beforeErrorSampling?: (event: ErrorEvent) => boolean;

/**
* Callback when an internal SDK error occurs. This can be used to debug SDK
* issues.
*/
onError?: (err: unknown) => void;

/**
* _experiments allows users to enable experimental or internal features.
* We don't consider such features as part of the public API and hence we don't guarantee semver for them.
Expand Down
10 changes: 5 additions & 5 deletions packages/replay-internal/src/util/logger.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { addBreadcrumb, captureException } from '@sentry/core';
import type { ConsoleLevel, SeverityLevel } from '@sentry/types';
import { logger as coreLogger } from '@sentry/utils';
import { logger as coreLogger, severityLevelFromString } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';

Expand Down Expand Up @@ -64,13 +64,13 @@ function makeReplayLogger(): ReplayLogger {
_logger[name] = (...args: unknown[]) => {
coreLogger[name](PREFIX, ...args);
if (_trace) {
_addBreadcrumb(args[0]);
_addBreadcrumb(args.join(''), severityLevelFromString(name));
}
};
});

_logger.exception = (error: unknown, ...message: unknown[]) => {
if (_logger.error) {
if (message.length && _logger.error) {
_logger.error(...message);
}

Expand All @@ -79,9 +79,9 @@ function makeReplayLogger(): ReplayLogger {
if (_capture) {
captureException(error);
} else if (_trace) {
// No need for a breadcrumb is `_capture` is enabled since it should be
// No need for a breadcrumb if `_capture` is enabled since it should be
// captured as an exception
_addBreadcrumb(error);
_addBreadcrumb(error, 'error');
}
};

Expand Down
9 changes: 4 additions & 5 deletions packages/replay-internal/src/util/sendReplay.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { setTimeout } from '@sentry-internal/browser-utils';
import { captureException, setContext } from '@sentry/core';
import { setContext } from '@sentry/core';

import { RETRY_BASE_INTERVAL, RETRY_MAX_COUNT, UNABLE_TO_SEND_REPLAY } from '../constants';
import { DEBUG_BUILD } from '../debug-build';
import type { SendReplayData } from '../types';
import { RateLimitError, TransportStatusCodeError, sendReplayRequest } from './sendReplayRequest';

Expand All @@ -16,7 +15,7 @@ export async function sendReplay(
interval: RETRY_BASE_INTERVAL,
},
): Promise<unknown> {
const { recordingData, options } = replayData;
const { recordingData, onError } = replayData;

// short circuit if there's no events to upload (this shouldn't happen as _runFlush makes this check)
if (!recordingData.length) {
Expand All @@ -36,8 +35,8 @@ export async function sendReplay(
_retryCount: retryConfig.count,
});

if (DEBUG_BUILD && options._experiments && options._experiments.captureExceptions) {
captureException(err);
if (onError) {
onError(err);
}

// If an error happened here, it's likely that uploading the attachment
Expand Down
4 changes: 2 additions & 2 deletions packages/replay-internal/test/integration/flush.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,8 @@ describe('Integration | flush', () => {
segmentId: 0,
eventContext: expect.anything(),
session: expect.any(Object),
options: expect.any(Object),
timestamp: expect.any(Number),
onError: expect.any(Function),
});

// Add this to test that segment ID increases
Expand Down Expand Up @@ -238,7 +238,7 @@ describe('Integration | flush', () => {
segmentId: 1,
eventContext: expect.anything(),
session: expect.any(Object),
options: expect.any(Object),
onError: expect.any(Function),
timestamp: expect.any(Number),
});

Expand Down
88 changes: 88 additions & 0 deletions packages/replay-internal/test/unit/util/logger.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import { beforeEach, describe, expect, it } from 'vitest';

import * as SentryCore from '@sentry/core';
import { logger as coreLogger } from '@sentry/utils';
import { logger } from '../../../src/util/logger';

const mockCaptureException = vi.spyOn(SentryCore, 'captureException');
const mockAddBreadcrumb = vi.spyOn(SentryCore, 'addBreadcrumb');
const mockLogError = vi.spyOn(coreLogger, 'error');
vi.spyOn(coreLogger, 'info');
vi.spyOn(coreLogger, 'log');
vi.spyOn(coreLogger, 'warn');

describe('logger', () => {
beforeEach(() => {
vi.clearAllMocks();
});

describe.each([
[false, false],
[false, true],
[true, false],
[true, true],
])('with options: captureExceptions:%s, traceInternals:%s', (captureExceptions, traceInternals) => {
beforeEach(() => {
logger.setConfig({
captureExceptions,
traceInternals,
});
});

it.each([
['info', 'info', 'info message'],
['log', 'log', 'log message'],
['warn', 'warning', 'warn message'],
['error', 'error', 'error message'],
])('%s', (fn, level, message) => {
logger[fn](message);
expect(coreLogger[fn]).toHaveBeenCalledWith('[Replay] ', message);

if (traceInternals) {
expect(mockAddBreadcrumb).toHaveBeenLastCalledWith(
{
category: 'console',
data: { logger: 'replay' },
level,
message: `[Replay] ${message}`,
},
{ level },
);
}
});

it('logs exceptions with a message', () => {
const err = new Error('An error');
logger.exception(err, 'a message');
if (captureExceptions) {
expect(mockCaptureException).toHaveBeenCalledWith(err);
}
expect(mockLogError).toHaveBeenCalledWith('[Replay] ', 'a message');
expect(mockLogError).toHaveBeenLastCalledWith('[Replay] ', err);
expect(mockLogError).toHaveBeenCalledTimes(2);

if (traceInternals) {
expect(mockAddBreadcrumb).toHaveBeenCalledWith(
{
category: 'console',
data: { logger: 'replay' },
level: 'error',
message: '[Replay] a message',
},
{ level: 'error' },
);
}
});

it('logs exceptions without a message', () => {
const err = new Error('An error');
logger.exception(err);
if (captureExceptions) {
expect(mockCaptureException).toHaveBeenCalledWith(err);
expect(mockAddBreadcrumb).not.toHaveBeenCalled();
}
expect(mockLogError).toHaveBeenCalledTimes(1);
expect(mockLogError).toHaveBeenLastCalledWith('[Replay] ', err);
});
});
});

0 comments on commit cf0152a

Please sign in to comment.