Skip to content

feat(replay): Use new afterSend hook to improve error linking #7390

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 10 commits into from
Mar 16, 2023
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
Original file line number Diff line number Diff line change
@@ -1,37 +1,25 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { getExpectedReplayEvent } from '../../../../utils/replayEventTemplates';
import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
import { envelopeRequestParser } from '../../../../utils/helpers';
import { getReplaySnapshot, isReplayEvent, shouldSkipReplayTest } from '../../../../utils/replayHelpers';

/*
* This scenario currently shows somewhat unexpected behavior from the PoV of a user:
* The error is dropped, but the recording is started and continued anyway.
* If folks only sample error replays, this will lead to a lot of confusion as the resulting replay
* won't contain the error that started it (possibly none or only additional errors that occurred later on).
*
* This is because in error-mode, we start recording as soon as replay's eventProcessor is called with an error.
* If later event processors or beforeSend drop the error, the recording is already started.
*
* We'll need a proper SDK lifecycle hook (WIP) to fix this properly.
* TODO: Once we have lifecycle hooks, we should revisit this test and make sure it behaves as expected.
* This means that the recording should not be started or stopped if the error that triggered it is not sent.
*/
sentryTest(
'[error-mode] should start recording if an error occurred although the error was dropped',
async ({ getLocalTestPath, page }) => {
'[error-mode] should not start recording if an error occurred when the error was dropped',
async ({ getLocalTestPath, page, forceFlushReplay }) => {
if (shouldSkipReplayTest()) {
sentryTest.skip();
}

const reqPromise0 = waitForReplayRequest(page, 0);
const reqPromise1 = waitForReplayRequest(page, 1);
const reqPromise2 = waitForReplayRequest(page, 2);

let callsToSentry = 0;

await page.route('https://dsn.ingest.sentry.io/**/*', route => {
callsToSentry++;
const req = route.request();
const event = envelopeRequestParser(req);

if (isReplayEvent(event)) {
callsToSentry++;
}

return route.fulfill({
status: 200,
Expand All @@ -43,30 +31,17 @@ sentryTest(
const url = await getLocalTestPath({ testDir: __dirname });

await page.goto(url);
await page.click('#go-background');
await forceFlushReplay();
expect(callsToSentry).toEqual(0);

await page.click('#error');
const req0 = await reqPromise0;

await page.click('#go-background');
await reqPromise1;

await page.click('#log');
await page.click('#go-background');
await reqPromise2;
await forceFlushReplay();

// Note: The fact that reqPromise1/reqPromise2 are fulfilled prooves that the recording continues

const event0 = getReplayEvent(req0);
expect(callsToSentry).toEqual(0);

expect(event0).toEqual(
getExpectedReplayEvent({
contexts: { replay: { error_sample_rate: 1, session_sample_rate: 0 } },
// This is by design. A dropped error shouldn't be in this list.
error_ids: [],
replay_type: 'error',
}),
);
const replay = await getReplaySnapshot(page);
expect(replay.recordingMode).toBe('error');
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;
window.Replay = new Sentry.Replay({
flushMinDelay: 500,
flushMaxDelay: 500,
});

Sentry.init({
dsn: 'https://[email protected]/1337',
sampleRate: 1,
replaysSessionSampleRate: 0.0,
replaysOnErrorSampleRate: 1.0,
integrations: [window.Replay],
transport: options => {
const transport = new Sentry.makeXHRTransport(options);

delete transport.send.__sentry__baseTransport__;

return transport;
},
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { getReplaySnapshot, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';

sentryTest(
'[error-mode] should handle errors with custom transport',
async ({ getLocalTestPath, page, forceFlushReplay }) => {
if (shouldSkipReplayTest()) {
sentryTest.skip();
}

const promiseReq0 = waitForReplayRequest(page, 0);
const promiseReq1 = waitForReplayRequest(page, 1);

let callsToSentry = 0;

await page.route('https://dsn.ingest.sentry.io/**/*', route => {
callsToSentry++;

return route.fulfill({
// Only error out for error, then succeed
status: callsToSentry === 1 ? 422 : 200,
});
});

const url = await getLocalTestPath({ testDir: __dirname });

await page.goto(url);
await forceFlushReplay();
expect(callsToSentry).toEqual(0);

await page.click('#error');
await promiseReq0;

await forceFlushReplay();
await promiseReq1;

const replay = await getReplaySnapshot(page);
expect(replay.recordingMode).toBe('session');
},
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;
window.Replay = new Sentry.Replay({
flushMinDelay: 500,
flushMaxDelay: 500,
});

Sentry.init({
dsn: 'https://[email protected]/1337',
sampleRate: 1,
replaysSessionSampleRate: 0.0,
replaysOnErrorSampleRate: 1.0,
integrations: [window.Replay],
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { getReplaySnapshot, shouldSkipReplayTest } from '../../../../utils/replayHelpers';

sentryTest(
'[error-mode] should handle errors that result in API error response',
async ({ getLocalTestPath, page, forceFlushReplay }) => {
if (shouldSkipReplayTest()) {
sentryTest.skip();
}

let callsToSentry = 0;

await page.route('https://dsn.ingest.sentry.io/**/*', route => {
callsToSentry++;

return route.fulfill({
status: 422,
contentType: 'application/json',
});
});

const url = await getLocalTestPath({ testDir: __dirname });

await page.goto(url);
await forceFlushReplay();
expect(callsToSentry).toEqual(0);

await page.click('#error');

await page.click('#log');
await forceFlushReplay();

// Only sent once, but since API failed we do not go into session mode
expect(callsToSentry).toEqual(1);

const replay = await getReplaySnapshot(page);
expect(replay.recordingMode).toBe('error');
},
);
13 changes: 9 additions & 4 deletions packages/browser-integration-tests/utils/replayHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type {
Session,
} from '@sentry/replay/build/npm/types/types';
import type { eventWithTime } from '@sentry/replay/build/npm/types/types/rrweb';
import type { Breadcrumb, Event, ReplayEvent } from '@sentry/types';
import type { Breadcrumb, Event, ReplayEvent, ReplayRecordingMode } from '@sentry/types';
import pako from 'pako';
import type { Page, Request, Response } from 'playwright';

Expand Down Expand Up @@ -104,9 +104,13 @@ function isCustomSnapshot(event: RecordingEvent): event is RecordingEvent & { da
* Note that due to how this works with playwright, this is a POJO copy of replay.
* This means that we cannot access any methods on it, and also not mutate it in any way.
*/
export async function getReplaySnapshot(
page: Page,
): Promise<{ _isPaused: boolean; _isEnabled: boolean; _context: InternalEventContext; session: Session | undefined }> {
export async function getReplaySnapshot(page: Page): Promise<{
_isPaused: boolean;
_isEnabled: boolean;
_context: InternalEventContext;
session: Session | undefined;
recordingMode: ReplayRecordingMode;
}> {
return await page.evaluate(() => {
const replayIntegration = (window as unknown as Window & { Replay: { _replay: ReplayContainer } }).Replay;
const replay = replayIntegration._replay;
Expand All @@ -116,6 +120,7 @@ export async function getReplaySnapshot(
_isEnabled: replay.isEnabled(),
_context: replay.getContext(),
session: replay.session,
recordingMode: replay.recordingMode,
};

return replaySnapshot;
Expand Down
2 changes: 1 addition & 1 deletion packages/browser/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
} else {
// If beacon is not supported or if they are using the tunnel option
// use our regular transport to send client reports to Sentry.
this._sendEnvelope(envelope);
void this._sendEnvelope(envelope);
}
} catch (e) {
__DEBUG_BUILD__ && logger.error(e);
Expand Down
21 changes: 17 additions & 4 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import type {
Transaction,
TransactionEvent,
Transport,
TransportMakeRequestResponse,
} from '@sentry/types';
import {
addItemToEnvelope,
Expand Down Expand Up @@ -320,7 +321,10 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
);
}

this._sendEnvelope(env);
const promise = this._sendEnvelope(env);
if (promise) {
promise.then(sendResponse => this.emit('afterSendEvent', event, sendResponse), null);
}
}
}

Expand All @@ -330,7 +334,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
public sendSession(session: Session | SessionAggregates): void {
if (this._dsn) {
const env = createSessionEnvelope(session, this._dsn, this._options._metadata, this._options.tunnel);
this._sendEnvelope(env);
void this._sendEnvelope(env);
}
}

Expand Down Expand Up @@ -363,6 +367,12 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
/** @inheritdoc */
public on(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): void;

/** @inheritdoc */
public on(
hook: 'afterSendEvent',
callback: (event: Event, sendResponse: TransportMakeRequestResponse | void) => void,
): void;

/** @inheritdoc */
public on(hook: string, callback: unknown): void {
if (!this._hooks[hook]) {
Expand All @@ -379,6 +389,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
/** @inheritdoc */
public emit(hook: 'beforeEnvelope', envelope: Envelope): void;

/** @inheritdoc */
public emit(hook: 'afterSendEvent', event: Event, sendResponse: TransportMakeRequestResponse | void): void;

/** @inheritdoc */
public emit(hook: string, ...rest: unknown[]): void {
if (this._hooks[hook]) {
Expand Down Expand Up @@ -624,9 +637,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
/**
* @inheritdoc
*/
protected _sendEnvelope(envelope: Envelope): void {
protected _sendEnvelope(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> | void {
if (this._transport && this._dsn) {
this._transport.send(envelope).then(null, reason => {
return this._transport.send(envelope).then(null, reason => {
__DEBUG_BUILD__ && logger.error('Error while sending event:', reason);
});
} else {
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/transports/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ export function createTransport(
);
}

// We use this to identifify if the transport is the base transport
// TODO (v8): Remove this again as we'll no longer need it
send.__sentry__baseTransport__ = true;

return {
send,
flush,
Expand Down
Loading