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

fix(replay): Start replay in afterAllSetup instead of next tick #12709

Merged
merged 3 commits into from
Jul 2, 2024
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
1 change: 1 addition & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,7 @@ jobs:
- loader_debug
- loader_tracing
- loader_replay
- loader_replay_buffer
- loader_tracing_replay

steps:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ import { expect } from '@playwright/test';
import { sentryTest } from '../../../../utils/fixtures';
import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';

const bundle = process.env.PW_BUNDLE || '';

sentryTest('should capture a replay', async ({ getLocalTestUrl, page }) => {
if (shouldSkipReplayTest()) {
// When in buffer mode, there will not be a replay by default
if (shouldSkipReplayTest() || bundle === 'loader_replay_buffer') {
sentryTest.skip();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
window.doSomethingWrong();
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { envelopeRequestParser, waitForErrorRequestOnUrl } from '../../../../utils/helpers';
import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';

sentryTest('should capture a replay & attach an error', async ({ getLocalTestUrl, page }) => {
if (shouldSkipReplayTest()) {
sentryTest.skip();
}

await page.route('https://dsn.ingest.sentry.io/**/*', route => {
return route.fulfill({
status: 200,
contentType: 'application/json',
body: JSON.stringify({ id: 'test-id' }),
});
});

const req = waitForReplayRequest(page);

const url = await getLocalTestUrl({ testDir: __dirname });
const reqError = await waitForErrorRequestOnUrl(page, url);

const errorEventData = envelopeRequestParser(reqError);
expect(errorEventData.exception?.values?.length).toBe(1);
expect(errorEventData.exception?.values?.[0]?.value).toContain('window.doSomethingWrong is not a function');

const eventData = getReplayEvent(await req);

expect(eventData).toBeDefined();
expect(eventData.segment_id).toBe(0);

expect(errorEventData.tags?.replayId).toEqual(eventData.replay_id);
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,7 @@ Sentry.onLoad(function () {
useCompression: false,
}),
],

replaysSessionSampleRate: 1,
});
});
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
Sentry.onLoad(function () {
Sentry.init({});
Sentry.init({
replaysSessionSampleRate: 1,
});
});
1 change: 1 addition & 0 deletions dev-packages/browser-integration-tests/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"test:loader:eager": "PW_BUNDLE=loader_eager yarn test:loader",
"test:loader:tracing": "PW_BUNDLE=loader_tracing yarn test:loader",
"test:loader:replay": "PW_BUNDLE=loader_replay yarn test:loader",
"test:loader:replay_buffer": "PW_BUNDLE=loader_replay_buffer yarn test:loader",
"test:loader:full": "PW_BUNDLE=loader_tracing_replay yarn test:loader",
"test:loader:debug": "PW_BUNDLE=loader_debug yarn test:loader",
"test:ci": "yarn test:all --reporter='line'",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
window.doSomethingWrong();
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../../utils/fixtures';
import { envelopeRequestParser, waitForErrorRequestOnUrl } from '../../../../utils/helpers';
import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';

sentryTest(
'[error-mode] should capture error that happens immediately after init',
async ({ getLocalTestUrl, page }) => {
if (shouldSkipReplayTest()) {
sentryTest.skip();
}

await page.route('https://dsn.ingest.sentry.io/**/*', route => {
return route.fulfill({
status: 200,
contentType: 'application/json',
body: JSON.stringify({ id: 'test-id' }),
});
});

const req = waitForReplayRequest(page);

const url = await getLocalTestUrl({ testDir: __dirname });
const reqError = await waitForErrorRequestOnUrl(page, url);

const errorEventData = envelopeRequestParser(reqError);
expect(errorEventData.exception?.values?.length).toBe(1);
expect(errorEventData.exception?.values?.[0]?.value).toContain('window.doSomethingWrong is not a function');

const eventData = getReplayEvent(await req);

expect(eventData).toBeDefined();
expect(eventData.segment_id).toBe(0);

expect(errorEventData.tags?.replayId).toEqual(eventData.replay_id);
},
);
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ const BUNDLE_PATHS: Record<string, Record<string, string>> = {
loader_debug: 'build/bundles/bundle.debug.min.js',
loader_tracing: 'build/bundles/bundle.tracing.min.js',
loader_replay: 'build/bundles/bundle.replay.min.js',
loader_replay_buffer: 'build/bundles/bundle.replay.min.js',
loader_tracing_replay: 'build/bundles/bundle.tracing.replay.debug.min.js',
},
integrations: {
Expand Down Expand Up @@ -96,6 +97,10 @@ export const LOADER_CONFIGS: Record<string, { options: Record<string, unknown>;
options: { replaysSessionSampleRate: 1, replaysOnErrorSampleRate: 1 },
lazy: false,
},
loader_replay_buffer: {
options: { replaysSessionSampleRate: 0, replaysOnErrorSampleRate: 1 },
lazy: false,
},
loader_tracing_replay: {
options: { tracesSampleRate: 1, replaysSessionSampleRate: 1, replaysOnErrorSampleRate: 1, debug: true },
lazy: false,
Expand Down
50 changes: 13 additions & 37 deletions packages/replay-internal/src/integration.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getClient, parseSampleRate } from '@sentry/core';
import type { BrowserClientReplayOptions, Integration, IntegrationFn } from '@sentry/types';
import { parseSampleRate } from '@sentry/core';
import type { BrowserClientReplayOptions, Client, Integration, IntegrationFn } from '@sentry/types';
import { consoleSandbox, dropUndefinedKeys, isBrowser } from '@sentry/utils';

import {
Expand Down Expand Up @@ -215,22 +215,13 @@ export class Replay implements Integration {
/**
* Setup and initialize replay container
*/
public setupOnce(): void {
if (!isBrowser()) {
public afterAllSetup(client: Client): void {
if (!isBrowser() || this._replay) {
return;
}

this._setup();

// Once upon a time, we tried to create a transaction in `setupOnce` and it would
// potentially create a transaction before some native SDK integrations have run
// and applied their own global event processor. An example is:
// https://github.com/getsentry/sentry-javascript/blob/b47ceafbdac7f8b99093ce6023726ad4687edc48/packages/browser/src/integrations/useragent.ts
//
// So we call `this._initialize()` in next event loop as a workaround to wait for other
// global event processors to finish. This is no longer needed, but keeping it
// here to avoid any future issues.
setTimeout(() => this._initialize());
this._setup(client);
this._initialize(client);
}

/**
Expand Down Expand Up @@ -301,24 +292,19 @@ export class Replay implements Integration {
/**
* Initializes replay.
*/
protected _initialize(): void {
protected _initialize(client: Client): void {
if (!this._replay) {
return;
}

Comment on lines 296 to 298
Copy link
Member

Choose a reason for hiding this comment

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

Could we remove this now?

// We have to run this in _initialize, because this runs in setTimeout
// So when this runs all integrations have been added
// Before this, we cannot access integrations on the client,
// so we need to mutate the options here
this._maybeLoadFromReplayCanvasIntegration();

this._maybeLoadFromReplayCanvasIntegration(client);
this._replay.initializeSampling();
}

/** Setup the integration. */
private _setup(): void {
private _setup(client: Client): void {
// Client is not available in constructor, so we need to wait until setupOnce
const finalOptions = loadReplayOptionsFromClient(this._initialOptions);
const finalOptions = loadReplayOptionsFromClient(this._initialOptions, client);

this._replay = new ReplayContainer({
options: finalOptions,
Expand All @@ -327,12 +313,11 @@ export class Replay implements Integration {
}

/** Get canvas options from ReplayCanvas integration, if it is also added. */
private _maybeLoadFromReplayCanvasIntegration(): void {
private _maybeLoadFromReplayCanvasIntegration(client: Client): void {
// To save bundle size, we skip checking for stuff here
// and instead just try-catch everything - as generally this should all be defined
/* eslint-disable @typescript-eslint/no-non-null-assertion */
try {
const client = getClient()!;
const canvasIntegration = client.getIntegrationByName('ReplayCanvas') as Integration & {
getOptions(): ReplayCanvasIntegrationOptions;
};
Expand All @@ -349,24 +334,15 @@ export class Replay implements Integration {
}

/** Parse Replay-related options from SDK options */
function loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions): ReplayPluginOptions {
const client = getClient();
const opt = client && (client.getOptions() as BrowserClientReplayOptions);
function loadReplayOptionsFromClient(initialOptions: InitialReplayPluginOptions, client: Client): ReplayPluginOptions {
const opt = client.getOptions() as BrowserClientReplayOptions;

const finalOptions: ReplayPluginOptions = {
sessionSampleRate: 0,
errorSampleRate: 0,
...dropUndefinedKeys(initialOptions),
};

if (!opt) {
consoleSandbox(() => {
// eslint-disable-next-line no-console
console.warn('SDK client is not available.');
});
return finalOptions;
}

const replaysSessionSampleRate = parseSampleRate(opt.replaysSessionSampleRate);
const replaysOnErrorSampleRate = parseSampleRate(opt.replaysOnErrorSampleRate);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { getClient } from '@sentry/core';
import type { Event } from '@sentry/types';

import { REPLAY_EVENT_NAME, SESSION_IDLE_EXPIRE_DURATION } from '../../../src/constants';
Expand Down Expand Up @@ -133,8 +134,8 @@ describe('Integration | coreHandlers | handleGlobalEvent', () => {

it('tags errors and transactions with replay id for session samples', async () => {
const { replay, integration } = await resetSdkMock({});
// @ts-expect-error protected but ok to use for testing
integration._initialize();
integration['_initialize'](getClient()!);
Copy link
Member

@s1gr1d s1gr1d Jul 2, 2024

Choose a reason for hiding this comment

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

omg this syntax 😅 Not wrong at all, just JS/TS 🙄 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, it is definitely weird, but I prefer this over having to do @ts-expect-error 😅


const transaction = Transaction();
const error = Error();
expect(handleGlobalEventListener(replay)(transaction, {})).toEqual(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ describe('Integration | errorSampleRate', () => {
},
autoStart: false,
});
integration['_initialize']();
integration['_initialize'](getClient()!);

expect(replay.recordingMode).toBe('session');
const sessionId = replay.getSessionId();
Expand Down Expand Up @@ -899,7 +899,7 @@ describe('Integration | errorSampleRate', () => {
},
autoStart: false,
});
integration['_initialize']();
integration['_initialize'](getClient()!);

vi.runAllTimers();

Expand Down Expand Up @@ -943,7 +943,7 @@ describe('Integration | errorSampleRate', () => {
},
autoStart: false,
});
integration['_initialize']();
integration['_initialize'](getClient()!);
const optionsEvent = createOptionsEvent(replay);

const TEST_EVENT = getTestEventIncremental({ timestamp: BASE_TIMESTAMP });
Expand Down
4 changes: 2 additions & 2 deletions packages/replay-internal/test/integration/sampling.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { getClient } from '@sentry/core';
import { resetSdkMock } from '../mocks/resetSdkMock';
import { useFakeTimers } from '../utils/use-fake-timers';

Expand Down Expand Up @@ -57,8 +58,7 @@ describe('Integration | sampling', () => {
// @ts-expect-error private API
const spyAddListeners = vi.spyOn(replay, '_addListeners');

// @ts-expect-error protected
integration._initialize();
integration['_initialize'](getClient()!);

vi.runAllTimers();

Expand Down
18 changes: 7 additions & 11 deletions packages/replay-internal/test/mocks/mockSdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,8 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true }
_initialized = value;
}

public setupOnce(): void {
// do nothing
}

public initialize(): void {
return super._initialize();
public afterAllSetup(): void {
// do nothing, we need to manually initialize this
}
}

Expand All @@ -76,7 +72,7 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true }
...replayOptions,
});

init({
const client = init({
...getDefaultClientOptions(),
dsn: 'https://dsn@ingest.f00.f00/1',
autoSessionTracking: false,
Expand All @@ -86,14 +82,14 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true }
replaysOnErrorSampleRate: 0.0,
...sentryOptions,
integrations: [replayIntegration],
});
})!;

// Instead of `setupOnce`, which is tricky to test, we call this manually here
replayIntegration['_setup']();
// Instead of `afterAllSetup`, which is tricky to test, we call this manually here
replayIntegration['_setup'](client);

if (autoStart) {
// Only exists in our mock
replayIntegration.initialize();
replayIntegration['_initialize'](client);
}

const replay = replayIntegration['_replay']!;
Expand Down
5 changes: 3 additions & 2 deletions packages/replay-internal/test/utils/TestClient.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { BaseClient, createTransport, initAndBind } from '@sentry/core';
import type {
BrowserClientReplayOptions,
Client,
ClientOptions,
Event,
ParameterizedString,
Expand Down Expand Up @@ -33,8 +34,8 @@ export class TestClient extends BaseClient<TestClientOptions> {
}
}

export function init(options: TestClientOptions): void {
initAndBind(TestClient, options);
export function init(options: TestClientOptions): Client {
return initAndBind(TestClient, options);
}

export function getDefaultClientOptions(options: Partial<TestClientOptions> = {}): ClientOptions {
Expand Down
Loading