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
Expand Up @@ -29,6 +29,7 @@ sentryTest('should capture replays (@sentry/browser export)', async ({ getLocalT
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
segment_id: 0,
segment_names: [],
replay_type: 'session',
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
Expand Down Expand Up @@ -77,6 +78,7 @@ sentryTest('should capture replays (@sentry/browser export)', async ({ getLocalT
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
segment_id: 1,
segment_names: [],
replay_type: 'session',
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ sentryTest('should capture replays (@sentry/replay export)', async ({ getLocalTe
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
segment_id: 0,
segment_names: [],
replay_type: 'session',
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
Expand Down Expand Up @@ -77,6 +78,7 @@ sentryTest('should capture replays (@sentry/replay export)', async ({ getLocalTe
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
segment_id: 1,
segment_names: [],
replay_type: 'session',
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const DEFAULT_REPLAY_EVENT = {
replay_id: expect.stringMatching(/\w{32}/),
replay_start_timestamp: expect.any(Number),
segment_id: 0,
segment_names: [],
replay_type: 'session',
event_id: expect.stringMatching(/\w{32}/),
environment: 'production',
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/types/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[];
segment_names: string[];
replay_id: string;
segment_id: number;
replay_type: ReplayRecordingMode;
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { ErrorEvent, Event, TransactionEvent, TransportMakeRequestResponse
import { setTimeout } from '@sentry/browser-utils';
import type { ReplayContainer } from '../types';
import { isErrorEvent, isTransactionEvent } from '../util/eventUtils';
import { addTraceIdToContext } from './util/addTraceIdToContext';
import { addSegmentDetailsToContext } from './util/addSegmentDetailsToContext';

type AfterSendEventCallback = (event: Event, sendResponse: TransportMakeRequestResponse) => void;

Expand Down Expand Up @@ -34,8 +34,8 @@ export function handleAfterSendEvent(replay: ReplayContainer): AfterSendEventCal

function handleTransactionEvent(replay: ReplayContainer, event: TransactionEvent): void {
const traceId = event.contexts?.trace?.trace_id;
if (traceId) {
addTraceIdToContext(replay, traceId);
if (traceId && event.transaction) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m/q: This was if (traceId) before, so a transaction event with a trace_id but no transaction name will no longer record its trace_id.

event.transaction is optional, so this is a behavioral change for nameless transactions. I think we can decouple setting a traceId from setting a segmentNames so that both enrich the replay independently where possible.

What do you think?

addSegmentDetailsToContext(replay, traceId, event.transaction);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import type { StreamedSpanJSON } from '@sentry/core';
import type { ReplayContainer } from '../types';
import { addSegmentDetailsToContext } from './util/addSegmentDetailsToContext';

type ProcessSegmentSpanCallback = (spanJSON: StreamedSpanJSON) => void;

export function handleProcessSegmentSpan(replay: ReplayContainer): ProcessSegmentSpanCallback {
return (spanJSON: StreamedSpanJSON) => {
if (!replay.isEnabled()) {
return;
}

const traceId = spanJSON.trace_id;
const segmentName = spanJSON.name;
if (traceId && segmentName) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m/q: This couples the trace_id to the segment name: if a sampled segment span has a valid trace_id but an empty name, we now record neither. I think bugbot flagged this, is this intended?

AFAIK, Since traceIds and segmentNames are independent sets with independent caps and dedup, there's no benefit to gating them together, it just drops trace_ids for nameless segments.

Suggest recording the trace_id whenever present and moving the empty-name guard into addSegmentDetailsToContext.

If the coupling is intentional, a comment explaining why would help.

addSegmentDetailsToContext(replay, traceId, segmentName);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Empty segment name drops trace

Medium Severity

Under span streaming, replay no longer records a trace_id when the segment span’s streamed name is empty. The previous afterSegmentSpanEnd handler still added trace IDs for sampled segment spans regardless of name, so unnamed segments can lose trace_ids on replay events while other linked data still expects them.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8f45b63. Configure here.

};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import type { ReplayContainer } from '../../types';

const MAX_CONTEXT_VALUES = 100;

export function addSegmentDetailsToContext(replay: ReplayContainer, traceId: string, segmentName: string): void {
const replayContext = replay.getContext();
if (replayContext.traceIds.size < MAX_CONTEXT_VALUES) {
replayContext.traceIds.add(traceId);
}
if (replayContext.segmentNames.size < MAX_CONTEXT_VALUES) {
replayContext.segmentNames.add(segmentName);
}
}

This file was deleted.

3 changes: 3 additions & 0 deletions packages/replay-internal/src/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ export class ReplayContainer implements ReplayContainerInterface {
this._context = {
errorIds: new Set(),
traceIds: new Set(),
segmentNames: new Set(),
urls: [],
initialTimestamp: Date.now(),
initialUrl: '',
Expand Down Expand Up @@ -1122,6 +1123,7 @@ export class ReplayContainer implements ReplayContainerInterface {
// XXX: `initialTimestamp` and `initialUrl` do not get cleared
this._context.errorIds.clear();
this._context.traceIds.clear();
this._context.segmentNames.clear();
this._context.urls = [];
}

Expand Down Expand Up @@ -1154,6 +1156,7 @@ export class ReplayContainer implements ReplayContainerInterface {
initialUrl: this._context.initialUrl,
errorIds: Array.from(this._context.errorIds),
traceIds: Array.from(this._context.traceIds),
segmentNames: Array.from(this._context.segmentNames),
urls: this._context.urls,
};

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

/**
* List of Sentry segment names that have occurred during a replay segment
*/
segmentNames: Array<string>;
}

/**
Expand All @@ -375,6 +380,11 @@ export interface InternalEventContext extends CommonEventContext {
* Set of Sentry trace ids that have occurred during a replay segment
*/
traceIds: Set<string>;

/**
* Set of Sentry segment names that have occurred during a replay segment
*/
segmentNames: Set<string>;
}

export type Sampled = false | 'session' | 'buffer';
Expand Down
4 changes: 2 additions & 2 deletions packages/replay-internal/src/util/addGlobalListeners.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { DynamicSamplingContext } from '@sentry/core';
import { addEventProcessor, getClient } from '@sentry/core';
import { addClickKeypressInstrumentationHandler, addHistoryInstrumentationHandler } from '@sentry/browser-utils';
import { handleAfterSegmentSpanEnd } from '../coreHandlers/handleAfterSegmentSpanEnd';
import { handleProcessSegmentSpan } from '../coreHandlers/handleProcessSegmentSpan';
import { handleAfterSendEvent } from '../coreHandlers/handleAfterSendEvent';
import { handleBeforeSendEvent } from '../coreHandlers/handleBeforeSendEvent';
import { handleBreadcrumbs } from '../coreHandlers/handleBreadcrumbs';
Expand Down Expand Up @@ -44,7 +44,7 @@ export function addGlobalListeners(replay: ReplayContainer): void {
}
});

client.on('afterSegmentSpanEnd', handleAfterSegmentSpanEnd(replay));
client.on('processSegmentSpan', handleProcessSegmentSpan(replay));

client.on('spanStart', span => {
replay.lastActiveSpan = span;
Expand Down
3 changes: 2 additions & 1 deletion packages/replay-internal/src/util/sendReplayRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export async function sendReplayRequest({
},
});

const { urls, errorIds, traceIds, initialTimestamp } = eventContext;
const { urls, errorIds, traceIds, segmentNames, initialTimestamp } = eventContext;

const client = getClient();
const scope = getCurrentScope();
Expand All @@ -43,6 +43,7 @@ export async function sendReplayRequest({
timestamp: timestamp / 1000,
error_ids: errorIds,
trace_ids: traceIds,
segment_names: segmentNames,
urls,
replay_id: replayId,
segment_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
* @vitest-environment jsdom
*/

import type { Span } from '@sentry/core';
import type { StreamedSpanJSON } from '@sentry/core';
import { getClient } from '@sentry/core';
import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest';
import type { ReplayContainer } from '../../../src/replay';
import { resetSdkMock } from '../../mocks/resetSdkMock';

let replay: ReplayContainer;

describe('Integration | coreHandlers | handleAfterSegmentSpanEnd', () => {
describe('Integration | coreHandlers | handleProcessSegmentSpan', () => {
beforeAll(() => {
vi.useFakeTimers();
});
Expand All @@ -19,7 +19,7 @@ describe('Integration | coreHandlers | handleAfterSegmentSpanEnd', () => {
replay.stop();
});

it('records traceIds from afterSegmentSpanEnd', async () => {
it('records traceIds and segment names from processSegmentSpan', async () => {
({ replay } = await resetSdkMock({
replayOptions: {
stickySession: false,
Expand All @@ -32,18 +32,31 @@ describe('Integration | coreHandlers | handleAfterSegmentSpanEnd', () => {

const client = getClient()!;

client.emit('afterSegmentSpanEnd', {
spanContext: () => ({ traceId: 'trace-stream-1', spanId: 'span1', traceFlags: 1 }),
} as unknown as Span);

client.emit('afterSegmentSpanEnd', {
spanContext: () => ({ traceId: 'trace-stream-2', spanId: 'span2', traceFlags: 1 }),
} as unknown as Span);
client.emit('processSegmentSpan', {
trace_id: 'trace-stream-1',
span_id: 'span1',
name: 'GET /api/users',
is_segment: true,
start_timestamp: 0,
end_timestamp: 1,
status: 'ok',
} as StreamedSpanJSON);

client.emit('processSegmentSpan', {
trace_id: 'trace-stream-2',
span_id: 'span2',
name: 'POST /api/items',
is_segment: true,
start_timestamp: 0,
end_timestamp: 1,
status: 'ok',
} as StreamedSpanJSON);

expect(Array.from(replay.getContext().traceIds)).toEqual(['trace-stream-1', 'trace-stream-2']);
expect(Array.from(replay.getContext().segmentNames)).toEqual(['GET /api/users', 'POST /api/items']);
});

it('limits traceIds from afterSegmentSpanEnd to max. 100', async () => {
it('limits traceIds from processSegmentSpan to max. 100', async () => {
({ replay } = await resetSdkMock({
replayOptions: {
stickySession: false,
Expand All @@ -57,9 +70,15 @@ describe('Integration | coreHandlers | handleAfterSegmentSpanEnd', () => {
const client = getClient()!;

for (let i = 0; i < 150; i++) {
client.emit('afterSegmentSpanEnd', {
spanContext: () => ({ traceId: `tr-${i}`, spanId: `sp-${i}`, traceFlags: 1 }),
} as unknown as Span);
client.emit('processSegmentSpan', {
trace_id: `tr-${i}`,
span_id: `sp-${i}`,
name: `segment-${i}`,
is_segment: true,
start_timestamp: 0,
end_timestamp: 1,
status: 'ok',
} as StreamedSpanJSON);
}

expect(replay.getContext().traceIds.size).toBe(100);
Expand All @@ -70,7 +89,7 @@ describe('Integration | coreHandlers | handleAfterSegmentSpanEnd', () => {
);
});

it('does not record traceIds from afterSegmentSpanEnd when replay is disabled', async () => {
it('does not record traceIds from processSegmentSpan when replay is disabled', async () => {
({ replay } = await resetSdkMock({
replayOptions: {
stickySession: false,
Expand All @@ -85,14 +104,20 @@ describe('Integration | coreHandlers | handleAfterSegmentSpanEnd', () => {

replay['_isEnabled'] = false;

client.emit('afterSegmentSpanEnd', {
spanContext: () => ({ traceId: 'trace-stream-1', spanId: 'span1', traceFlags: 1 }),
} as unknown as Span);
client.emit('processSegmentSpan', {
trace_id: 'trace-stream-1',
span_id: 'span1',
name: 'GET /api/users',
is_segment: true,
start_timestamp: 0,
end_timestamp: 1,
status: 'ok',
} as StreamedSpanJSON);

expect(Array.from(replay.getContext().traceIds)).toEqual([]);
});

it('does not record traceIds for unsampled spans', async () => {
it('does not record segment spans with empty names', async () => {
({ replay } = await resetSdkMock({
replayOptions: {
stickySession: false,
Expand All @@ -105,10 +130,17 @@ describe('Integration | coreHandlers | handleAfterSegmentSpanEnd', () => {

const client = getClient()!;

client.emit('afterSegmentSpanEnd', {
spanContext: () => ({ traceId: 'trace-unsampled', spanId: 'span1', traceFlags: 0 }),
} as unknown as Span);
client.emit('processSegmentSpan', {
trace_id: 'trace-stream-1',
span_id: 'span1',
name: '',
is_segment: true,
start_timestamp: 0,
end_timestamp: 1,
status: 'ok',
} as StreamedSpanJSON);

expect(Array.from(replay.getContext().traceIds)).toEqual([]);
expect(Array.from(replay.getContext().segmentNames)).toEqual([]);
});
});
2 changes: 2 additions & 0 deletions packages/replay-internal/test/integration/sampling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ describe('Integration | sampling', () => {
expect(replay.getContext()).toEqual({
errorIds: new Set(),
traceIds: new Set(),
segmentNames: new Set(),
urls: [],
initialTimestamp: expect.any(Number),
initialUrl: '',
Expand Down Expand Up @@ -79,6 +80,7 @@ describe('Integration | sampling', () => {
initialTimestamp: expect.any(Number),
initialUrl: 'http://localhost:3000/',
traceIds: new Set(),
segmentNames: new Set(),
urls: ['http://localhost:3000/'],
});
expect(replay.recordingMode).toBe('buffer');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ describe('Integration | session', () => {
urls: [],
errorIds: new Set(),
traceIds: new Set(),
segmentNames: new Set(),
});
});

Expand Down
Loading