From cf0152a9e25e1c2911e30c154c54d56fe0b79117 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 19 Sep 2024 12:30:30 -0400 Subject: [PATCH] feat(replay): Add `onError` callback + other small improvements to debugging (#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`) --- packages/replay-internal/src/replay.ts | 9 +- packages/replay-internal/src/types/replay.ts | 8 +- packages/replay-internal/src/util/logger.ts | 10 +-- .../replay-internal/src/util/sendReplay.ts | 9 +- .../test/integration/flush.test.ts | 4 +- .../test/unit/util/logger.test.ts | 88 +++++++++++++++++++ 6 files changed, 113 insertions(+), 15 deletions(-) create mode 100644 packages/replay-internal/test/unit/util/logger.test.ts diff --git a/packages/replay-internal/src/replay.ts b/packages/replay-internal/src/replay.ts index 06f81b6982c6..ea7df8b7afa7 100644 --- a/packages/replay-internal/src/replay.ts +++ b/packages/replay-internal/src/replay.ts @@ -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'; @@ -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); + } } /** @@ -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); @@ -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'); } } } diff --git a/packages/replay-internal/src/types/replay.ts b/packages/replay-internal/src/types/replay.ts index 0605ba97449a..6892c05ee179 100644 --- a/packages/replay-internal/src/types/replay.ts +++ b/packages/replay-internal/src/types/replay.ts @@ -26,7 +26,7 @@ export interface SendReplayData { eventContext: PopEventContext; timestamp: number; session: Session; - options: ReplayPluginOptions; + onError?: (err: unknown) => void; } export interface Timeouts { @@ -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. diff --git a/packages/replay-internal/src/util/logger.ts b/packages/replay-internal/src/util/logger.ts index 80445409164b..1b505f41703a 100644 --- a/packages/replay-internal/src/util/logger.ts +++ b/packages/replay-internal/src/util/logger.ts @@ -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'; @@ -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); } @@ -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'); } }; diff --git a/packages/replay-internal/src/util/sendReplay.ts b/packages/replay-internal/src/util/sendReplay.ts index 973c3fb9a556..c0c6483502b9 100644 --- a/packages/replay-internal/src/util/sendReplay.ts +++ b/packages/replay-internal/src/util/sendReplay.ts @@ -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'; @@ -16,7 +15,7 @@ export async function sendReplay( interval: RETRY_BASE_INTERVAL, }, ): Promise { - 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) { @@ -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 diff --git a/packages/replay-internal/test/integration/flush.test.ts b/packages/replay-internal/test/integration/flush.test.ts index 52654fa909d3..72ef104d0633 100644 --- a/packages/replay-internal/test/integration/flush.test.ts +++ b/packages/replay-internal/test/integration/flush.test.ts @@ -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 @@ -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), }); diff --git a/packages/replay-internal/test/unit/util/logger.test.ts b/packages/replay-internal/test/unit/util/logger.test.ts new file mode 100644 index 000000000000..075a6f27a841 --- /dev/null +++ b/packages/replay-internal/test/unit/util/logger.test.ts @@ -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); + }); + }); +});