Skip to content
Open
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,6 +1,6 @@
import { expect } from '@playwright/test';
import { SDK_VERSION } from '@sentry/browser';
import { sentryTest, TEST_HOST } from '../../../utils/fixtures';
import { sentryTest } from '../../../utils/fixtures';
import { getExpectedReplayEvent } from '../../../utils/replayEventTemplates';
import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers';

sentryTest('should capture replays (@sentry/browser export)', async ({ getLocalTestUrl, page }) => {
Expand All @@ -20,84 +20,17 @@ sentryTest('should capture replays (@sentry/browser export)', async ({ getLocalT
const replayEvent1 = getReplayEvent(await reqPromise1);

expect(replayEvent0).toBeDefined();
expect(replayEvent0).toEqual({
type: 'replay_event',
timestamp: expect.any(Number),
error_ids: [],
trace_ids: [],
urls: [`${TEST_HOST}/index.html`],
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
segment_id: 0,
replay_type: 'session',
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
sdk: {
integrations: expect.arrayContaining([
'InboundFilters',
'FunctionToString',
'BrowserApiErrors',
'Breadcrumbs',
'GlobalHandlers',
'LinkedErrors',
'Dedupe',
'HttpContext',
'BrowserSession',
'Replay',
]),
version: SDK_VERSION,
name: 'sentry.javascript.browser',
settings: {
infer_ip: 'never',
},
},
request: {
url: `${TEST_HOST}/index.html`,
headers: {
'User-Agent': expect.stringContaining(''),
},
},
platform: 'javascript',
});
expect(replayEvent0).toEqual(
getExpectedReplayEvent({
segment_id: 0,
}),
);

expect(replayEvent1).toBeDefined();
expect(replayEvent1).toEqual({
type: 'replay_event',
timestamp: expect.any(Number),
error_ids: [],
trace_ids: [],
urls: [],
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
segment_id: 1,
replay_type: 'session',
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
sdk: {
integrations: expect.arrayContaining([
'InboundFilters',
'FunctionToString',
'BrowserApiErrors',
'Breadcrumbs',
'GlobalHandlers',
'LinkedErrors',
'Dedupe',
'HttpContext',
'BrowserSession',
'Replay',
]),
version: SDK_VERSION,
name: 'sentry.javascript.browser',
settings: {
infer_ip: 'never',
},
},
request: {
url: `${TEST_HOST}/index.html`,
headers: {
'User-Agent': expect.stringContaining(''),
},
},
platform: 'javascript',
});
expect(replayEvent1).toEqual(
getExpectedReplayEvent({
segment_id: 1,
urls: [],
}),
);
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { expect } from '@playwright/test';
import { SDK_VERSION } from '@sentry/browser';
import { sentryTest, TEST_HOST } from '../../../utils/fixtures';
import { sentryTest } from '../../../utils/fixtures';
import { getExpectedReplayEvent } from '../../../utils/replayEventTemplates';
import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../utils/replayHelpers';

sentryTest('should capture replays (@sentry-internal/replay export)', async ({ getLocalTestUrl, page }) => {
Expand All @@ -20,84 +20,17 @@ sentryTest('should capture replays (@sentry-internal/replay export)', async ({ g
const replayEvent1 = getReplayEvent(await reqPromise1);

expect(replayEvent0).toBeDefined();
expect(replayEvent0).toEqual({
type: 'replay_event',
timestamp: expect.any(Number),
error_ids: [],
trace_ids: [],
urls: [`${TEST_HOST}/index.html`],
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
segment_id: 0,
replay_type: 'session',
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
sdk: {
integrations: expect.arrayContaining([
'InboundFilters',
'FunctionToString',
'BrowserApiErrors',
'Breadcrumbs',
'GlobalHandlers',
'LinkedErrors',
'Dedupe',
'HttpContext',
'BrowserSession',
'Replay',
]),
version: SDK_VERSION,
name: 'sentry.javascript.browser',
settings: {
infer_ip: 'never',
},
},
request: {
url: `${TEST_HOST}/index.html`,
headers: {
'User-Agent': expect.stringContaining(''),
},
},
platform: 'javascript',
});
expect(replayEvent0).toEqual(
getExpectedReplayEvent({
segment_id: 0,
}),
);

expect(replayEvent1).toBeDefined();
expect(replayEvent1).toEqual({
type: 'replay_event',
timestamp: expect.any(Number),
error_ids: [],
trace_ids: [],
urls: [],
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
segment_id: 1,
replay_type: 'session',
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
sdk: {
integrations: expect.arrayContaining([
'InboundFilters',
'FunctionToString',
'BrowserApiErrors',
'Breadcrumbs',
'GlobalHandlers',
'LinkedErrors',
'Dedupe',
'HttpContext',
'BrowserSession',
'Replay',
]),
version: SDK_VERSION,
name: 'sentry.javascript.browser',
settings: {
infer_ip: 'never',
},
},
request: {
url: `${TEST_HOST}/index.html`,
headers: {
'User-Agent': expect.stringContaining(''),
},
},
platform: 'javascript',
});
expect(replayEvent1).toEqual(
getExpectedReplayEvent({
urls: [],
segment_id: 1,
}),
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ const DEFAULT_REPLAY_EVENT = {
type: 'replay_event',
timestamp: expect.any(Number),
error_ids: [],
trace_ids: [],
trace_ids: [expect.any(String)],
traces_by_timestamp: [[expect.any(Number), expect.any(String)]],
urls: [expect.stringContaining('/index.html')],
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/types-hoist/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export interface ReplayEvent extends Event {
replay_start_timestamp?: number;
error_ids: string[];
trace_ids: string[];
traces_by_timestamp: [number, string][];
replay_id: string;
segment_id: number;
replay_type: ReplayRecordingMode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ function handleTransactionEvent(replay: ReplayContainer, event: TransactionEvent
// Collect traceIds in _context regardless of `recordingMode`
// In error mode, _context gets cleared on every checkout
// We limit to max. 100 transactions linked
if (event.contexts?.trace?.trace_id && replayContext.traceIds.size < 100) {
replayContext.traceIds.add(event.contexts.trace.trace_id);
if (event.contexts?.trace?.trace_id && event.start_timestamp && replayContext.traceIds.length < 100) {
replayContext.traceIds.push([event.start_timestamp, event.contexts.trace.trace_id]);
Copy link

Choose a reason for hiding this comment

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

Bug: Timestamp Handling Ignores Valid Zero Value

The event.start_timestamp check in handleTransactionEvent uses a truthy evaluation, which causes trace IDs to not be recorded for transactions with a start_timestamp of 0. Although rare, 0 is a valid Unix epoch timestamp.

Fix in Cursor Fix in Web

}
}

Expand Down
25 changes: 21 additions & 4 deletions packages/replay-internal/src/replay.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
/* eslint-disable max-lines */ // TODO: We might want to split this file up
import type { ReplayRecordingMode, Span } from '@sentry/core';
import { getActiveSpan, getClient, getRootSpan, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core';
import {
getActiveSpan,
getClient,
getCurrentScope,
getRootSpan,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
spanToJSON,
} from '@sentry/core';
import { EventType, record } from '@sentry-internal/rrweb';
import {
BUFFER_CHECKOUT_TIME,
Expand Down Expand Up @@ -192,7 +199,7 @@ export class ReplayContainer implements ReplayContainerInterface {
this._hasInitializedCoreListeners = false;
this._context = {
errorIds: new Set(),
traceIds: new Set(),
traceIds: [],
urls: [],
initialTimestamp: Date.now(),
initialUrl: '',
Expand Down Expand Up @@ -1098,7 +1105,11 @@ export class ReplayContainer implements ReplayContainerInterface {
private _clearContext(): void {
// XXX: `initialTimestamp` and `initialUrl` do not get cleared
this._context.errorIds.clear();
this._context.traceIds.clear();
// We want to preserve the most recent trace id for the next replay segment.
// This is so that we can associate replay events w/ the trace.
if (this._context.traceIds.length > 1) {
this._context.traceIds = this._context.traceIds.slice(-1);
}
this._context.urls = [];
}

Expand Down Expand Up @@ -1126,11 +1137,17 @@ export class ReplayContainer implements ReplayContainerInterface {
* Return and clear _context
*/
private _popEventContext(): PopEventContext {
if (this._context.traceIds.length === 0) {
const currentTraceId = getCurrentScope().getPropagationContext().traceId;
if (currentTraceId) {
this._context.traceIds.push([-1, currentTraceId]);
}
}
const _context = {
initialTimestamp: this._context.initialTimestamp,
initialUrl: this._context.initialUrl,
errorIds: Array.from(this._context.errorIds),
traceIds: Array.from(this._context.traceIds),
traceIds: this._context.traceIds,
urls: this._context.urls,
};

Expand Down
6 changes: 3 additions & 3 deletions packages/replay-internal/src/types/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ export interface PopEventContext extends CommonEventContext {
/**
* List of Sentry trace ids that have occurred during a replay segment
*/
traceIds: Array<string>;
traceIds: Array<[number, string]>;
}

/**
Expand All @@ -348,9 +348,9 @@ export interface InternalEventContext extends CommonEventContext {
errorIds: Set<string>;

/**
* Set of Sentry trace ids that have occurred during a replay segment
* List of <timestamp, trace_id> for Sentry traces that have occurred during a replay segment
*/
traceIds: Set<string>;
traceIds: Array<[number, string]>;
}

export type Sampled = false | 'session' | 'buffer';
Expand Down
6 changes: 5 additions & 1 deletion packages/replay-internal/src/util/sendReplayRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,16 @@ export async function sendReplayRequest({
return Promise.resolve({});
}

const uniqueTraceIds = Array.from(new Set(traceIds.map(([_ts, traceId]) => traceId)));
const baseEvent: ReplayEvent = {
type: REPLAY_EVENT_NAME,
replay_start_timestamp: initialTimestamp / 1000,
timestamp: timestamp / 1000,
error_ids: errorIds,
trace_ids: traceIds,
trace_ids: uniqueTraceIds,
traces_by_timestamp: traceIds
.filter(([_ts, traceId]) => uniqueTraceIds.includes(traceId))
.map(([ts, traceId]) => [ts, traceId]),
Copy link

Choose a reason for hiding this comment

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

Bug: Redundant Filter Causes Unnecessary Performance Overhead

The filter for traces_by_timestamp is redundant. Since uniqueTraceIds is derived directly from traceIds, every trace ID in traceIds will always be present in uniqueTraceIds. This makes the filter a no-op, always returning true, and introduces unnecessary O(n²) performance overhead.

Fix in Cursor Fix in Web

urls,
replay_id: replayId,
segment_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
handler(error4, { statusCode: undefined });

expect(Array.from(replay.getContext().errorIds)).toEqual(['err2']);
expect(Array.from(replay.getContext().traceIds)).toEqual([]);
expect(Array.from(replay.getContext().traceIds)).toEqual([[-1, expect.any(String)]]);
});

it('records traceIds from sent transaction events', async () => {
Expand Down Expand Up @@ -84,13 +84,16 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
handler(transaction4, { statusCode: undefined });

expect(Array.from(replay.getContext().errorIds)).toEqual([]);
expect(Array.from(replay.getContext().traceIds)).toEqual(['tr2']);
const traceIds = replay.getContext().traceIds;
expect(traceIds).toEqual([[expect.any(Number), 'tr2']]);

// Does not affect error session
await vi.advanceTimersToNextTimerAsync();

expect(Array.from(replay.getContext().errorIds)).toEqual([]);
expect(Array.from(replay.getContext().traceIds)).toEqual(['tr2']);
// Verify traceIds are still there after advancing timers
const traceIdsAfter = replay.getContext().traceIds;
expect(traceIdsAfter).toEqual([[expect.any(Number), 'tr2']]);
expect(replay.isEnabled()).toBe(true);
expect(replay.isPaused()).toBe(false);
expect(replay.recordingMode).toBe('buffer');
Expand Down Expand Up @@ -119,7 +122,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
.fill(undefined)
.map((_, i) => `err-${i}`),
);
expect(Array.from(replay.getContext().traceIds)).toEqual([]);
expect(replay.getContext().traceIds).toEqual([[-1, expect.any(String)]]);
});

it('limits traceIds to max. 100', async () => {
Expand All @@ -141,11 +144,20 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
}

expect(Array.from(replay.getContext().errorIds)).toEqual([]);
expect(Array.from(replay.getContext().traceIds)).toEqual(
// traceIds is now a Set of [timestamp, trace_id] tuples
const traceIds = Array.from(replay.getContext().traceIds);
expect(traceIds).toHaveLength(100);
// Check that all trace IDs are present
expect(traceIds.map(([_timestamp, traceId]) => traceId)).toEqual(
Array(100)
.fill(undefined)
.map((_, i) => `tr-${i}`),
);
// Check that all tuples have timestamps
traceIds.forEach(([timestamp, _traceId]) => {
expect(typeof timestamp).toBe('number');
expect(timestamp).toBeGreaterThan(0);
});
});

it('flushes when in buffer mode', async () => {
Expand Down
Loading
Loading