Skip to content

Commit 603d188

Browse files
mydeamydea
authored
Merge 1d97cde into 3161576 
2 parents 3161576 + 1d97cde commit 603d188

File tree

22 files changed

+899
-194
lines changed

22 files changed

+899
-194
lines changed

‎packages/browser-integration-tests/suites/replay/errors/droppedError/test.ts‎

Lines changed: 15 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,25 @@
11
import { expect } from '@playwright/test';
22

33
import { sentryTest } from '../../../../utils/fixtures';
4-
import { getExpectedReplayEvent } from '../../../../utils/replayEventTemplates';
5-
import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
4+
import { envelopeRequestParser } from '../../../../utils/helpers';
5+
import { getReplaySnapshot, isReplayEvent, shouldSkipReplayTest } from '../../../../utils/replayHelpers';
66

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

27-
const reqPromise0 = waitForReplayRequest(page, 0);
28-
const reqPromise1 = waitForReplayRequest(page, 1);
29-
const reqPromise2 = waitForReplayRequest(page, 2);
30-
3114
let callsToSentry = 0;
3215

3316
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
34-
callsToSentry++;
17+
const req = route.request();
18+
const event = envelopeRequestParser(req);
19+
20+
if (isReplayEvent(event)) {
21+
callsToSentry++;
22+
}
3523

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

4533
await page.goto(url);
46-
await page.click('#go-background');
34+
await forceFlushReplay();
4735
expect(callsToSentry).toEqual(0);
4836

4937
await page.click('#error');
50-
const req0 = await reqPromise0;
51-
52-
await page.click('#go-background');
53-
await reqPromise1;
5438

5539
await page.click('#log');
56-
await page.click('#go-background');
57-
await reqPromise2;
40+
await forceFlushReplay();
5841

59-
// Note: The fact that reqPromise1/reqPromise2 are fulfilled prooves that the recording continues
60-
61-
const event0 = getReplayEvent(req0);
42+
expect(callsToSentry).toEqual(0);
6243

63-
expect(event0).toEqual(
64-
getExpectedReplayEvent({
65-
contexts: { replay: { error_sample_rate: 1, session_sample_rate: 0 } },
66-
// This is by design. A dropped error shouldn't be in this list.
67-
error_ids: [],
68-
replay_type: 'error',
69-
}),
70-
);
44+
const replay = await getReplaySnapshot(page);
45+
expect(replay.recordingMode).toBe('error');
7146
},
7247
);

‎packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
window.Replay = new Sentry.Replay({
5+
flushMinDelay: 500,
6+
flushMaxDelay: 500,
7+
});
8+
9+
Sentry.init({
10+
dsn: 'https://[email protected]/1337',
11+
sampleRate: 1,
12+
replaysSessionSampleRate: 0.0,
13+
replaysOnErrorSampleRate: 1.0,
14+
integrations: [window.Replay],
15+
transport: options => {
16+
const transport = new Sentry.makeXHRTransport(options);
17+
18+
delete transport.send.__sentry__baseTransport__;
19+
20+
return transport;
21+
},
22+
});

‎packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import { expect } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../../utils/fixtures';
4+
import { getReplaySnapshot, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
5+
6+
sentryTest(
7+
'[error-mode] should handle errors with custom transport',
8+
async ({ getLocalTestPath, page, forceFlushReplay }) => {
9+
if (shouldSkipReplayTest()) {
10+
sentryTest.skip();
11+
}
12+
13+
const promiseReq0 = waitForReplayRequest(page, 0);
14+
const promiseReq1 = waitForReplayRequest(page, 1);
15+
16+
let callsToSentry = 0;
17+
18+
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
19+
callsToSentry++;
20+
21+
return route.fulfill({
22+
// Only error out for error, then succeed
23+
status: callsToSentry === 1 ? 422 : 200,
24+
});
25+
});
26+
27+
const url = await getLocalTestPath({ testDir: __dirname });
28+
29+
await page.goto(url);
30+
await forceFlushReplay();
31+
expect(callsToSentry).toEqual(0);
32+
33+
await page.click('#error');
34+
await promiseReq0;
35+
36+
await forceFlushReplay();
37+
await promiseReq1;
38+
39+
const replay = await getReplaySnapshot(page);
40+
expect(replay.recordingMode).toBe('session');
41+
},
42+
);

‎packages/browser-integration-tests/suites/replay/errors/errorNotSent/init.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
window.Replay = new Sentry.Replay({
5+
flushMinDelay: 500,
6+
flushMaxDelay: 500,
7+
});
8+
9+
Sentry.init({
10+
dsn: 'https://[email protected]/1337',
11+
sampleRate: 1,
12+
replaysSessionSampleRate: 0.0,
13+
replaysOnErrorSampleRate: 1.0,
14+
integrations: [window.Replay],
15+
});

‎packages/browser-integration-tests/suites/replay/errors/errorNotSent/test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import { expect } from '@playwright/test';
2+
3+
import { sentryTest } from '../../../../utils/fixtures';
4+
import { getReplaySnapshot, shouldSkipReplayTest } from '../../../../utils/replayHelpers';
5+
6+
sentryTest(
7+
'[error-mode] should handle errors that result in API error response',
8+
async ({ getLocalTestPath, page, forceFlushReplay }) => {
9+
if (shouldSkipReplayTest()) {
10+
sentryTest.skip();
11+
}
12+
13+
let callsToSentry = 0;
14+
15+
await page.route('https://dsn.ingest.sentry.io/**/*', route => {
16+
callsToSentry++;
17+
18+
return route.fulfill({
19+
status: 422,
20+
contentType: 'application/json',
21+
});
22+
});
23+
24+
const url = await getLocalTestPath({ testDir: __dirname });
25+
26+
await page.goto(url);
27+
await forceFlushReplay();
28+
expect(callsToSentry).toEqual(0);
29+
30+
await page.click('#error');
31+
32+
await page.click('#log');
33+
await forceFlushReplay();
34+
35+
// Only sent once, but since API failed we do not go into session mode
36+
expect(callsToSentry).toEqual(1);
37+
38+
const replay = await getReplaySnapshot(page);
39+
expect(replay.recordingMode).toBe('error');
40+
},
41+
);

‎packages/browser-integration-tests/utils/replayHelpers.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import type {
77
Session,
88
} from '@sentry/replay/build/npm/types/types';
99
import type { eventWithTime } from '@sentry/replay/build/npm/types/types/rrweb';
10-
import type { Breadcrumb, Event, ReplayEvent } from '@sentry/types';
10+
import type { Breadcrumb, Event, ReplayEvent, ReplayRecordingMode } from '@sentry/types';
1111
import pako from 'pako';
1212
import type { Page, Request, Response } from 'playwright';
1313

@@ -104,9 +104,13 @@ function isCustomSnapshot(event: RecordingEvent): event is RecordingEvent & { da
104104
* Note that due to how this works with playwright, this is a POJO copy of replay.
105105
* This means that we cannot access any methods on it, and also not mutate it in any way.
106106
*/
107-
export async function getReplaySnapshot(
108-
page: Page,
109-
): Promise<{ _isPaused: boolean; _isEnabled: boolean; _context: InternalEventContext; session: Session | undefined }> {
107+
export async function getReplaySnapshot(page: Page): Promise<{
108+
_isPaused: boolean;
109+
_isEnabled: boolean;
110+
_context: InternalEventContext;
111+
session: Session | undefined;
112+
recordingMode: ReplayRecordingMode;
113+
}> {
110114
return await page.evaluate(() => {
111115
const replayIntegration = (window as unknown as Window & { Replay: { _replay: ReplayContainer } }).Replay;
112116
const replay = replayIntegration._replay;
@@ -116,6 +120,7 @@ export async function getReplaySnapshot(
116120
_isEnabled: replay.isEnabled(),
117121
_context: replay.getContext(),
118122
session: replay.session,
123+
recordingMode: replay.recordingMode,
119124
};
120125

121126
return replaySnapshot;

‎packages/browser/src/client.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
146146
} else {
147147
// If beacon is not supported or if they are using the tunnel option
148148
// use our regular transport to send client reports to Sentry.
149-
this._sendEnvelope(envelope);
149+
void this._sendEnvelope(envelope);
150150
}
151151
} catch (e) {
152152
__DEBUG_BUILD__ && logger.error(e);

‎packages/core/src/baseclient.ts‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import type {
2020
Transaction,
2121
TransactionEvent,
2222
Transport,
23+
TransportMakeRequestResponse,
2324
} from '@sentry/types';
2425
import {
2526
addItemToEnvelope,
@@ -320,7 +321,10 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
320321
);
321322
}
322323

323-
this._sendEnvelope(env);
324+
const promise = this._sendEnvelope(env);
325+
if (promise) {
326+
promise.then(sendResponse => this.emit('afterSendEvent', event, sendResponse), null);
327+
}
324328
}
325329
}
326330

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

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

370+
/** @inheritdoc */
371+
public on(
372+
hook: 'afterSendEvent',
373+
callback: (event: Event, sendResponse: TransportMakeRequestResponse | void) => void,
374+
): void;
375+
366376
/** @inheritdoc */
367377
public on(hook: string, callback: unknown): void {
368378
if (!this._hooks[hook]) {
@@ -379,6 +389,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
379389
/** @inheritdoc */
380390
public emit(hook: 'beforeEnvelope', envelope: Envelope): void;
381391

392+
/** @inheritdoc */
393+
public emit(hook: 'afterSendEvent', event: Event, sendResponse: TransportMakeRequestResponse | void): void;
394+
382395
/** @inheritdoc */
383396
public emit(hook: string, ...rest: unknown[]): void {
384397
if (this._hooks[hook]) {
@@ -624,9 +637,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
624637
/**
625638
* @inheritdoc
626639
*/
627-
protected _sendEnvelope(envelope: Envelope): void {
640+
protected _sendEnvelope(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> | void {
628641
if (this._transport && this._dsn) {
629-
this._transport.send(envelope).then(null, reason => {
642+
return this._transport.send(envelope).then(null, reason => {
630643
__DEBUG_BUILD__ && logger.error('Error while sending event:', reason);
631644
});
632645
} else {

‎packages/core/src/transports/base.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,10 @@ export function createTransport(
109109
);
110110
}
111111

112+
// We use this to identifify if the transport is the base transport
113+
// TODO (v8): Remove this again as we'll no longer need it
114+
send.__sentry__baseTransport__ = true;
115+
112116
return {
113117
send,
114118
flush,

0 commit comments

Comments
 (0)