Skip to content
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

feat(replay): Add onError callback + other small improvements to debugging #13721

Merged
merged 3 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
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),
billyvg marked this conversation as resolved.
Show resolved Hide resolved
});
} 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
4 changes: 2 additions & 2 deletions packages/replay-internal/src/util/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ function makeReplayLogger(): ReplayLogger {
});

_logger.exception = (error: unknown, ...message: unknown[]) => {
if (_logger.error) {
if (message && _logger.error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think message will never be undefined, but an empty array? so this should probably be:

Suggested change
if (message && _logger.error) {
if (message.length && _logger.error) {

🤔 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch, added some tests :)

_logger.error(...message);
}

Expand All @@ -79,7 +79,7 @@ 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);
}
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to propagate this up so that we're always using the same error capturing logic (e.g. replay.handleException)

}

// 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
Loading