Skip to content

Commit 00a32b4

Browse files
fix(v10/browser): Skip profiling for unsampled active root spans (#25502)
Backport of: #25498 ## Differences to the original PR - `packages/browser/test/profiling/integration.test.ts`: the two new tests use v10's existing `import * as Sentry from '@sentry/browser'` style (`Sentry.init`, `Sentry.addIntegration`, etc.) instead of named imports from `../../src/index`. That import block conflicted because v10's test file still uses the namespace import. - Same file: the unsampled-pageload test uses `Sentry.spanToJSON` instead of `spanToStaticSpanJSON`, because `spanToStaticSpanJSON` doesn't exist on v10. Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com>
1 parent 1243134 commit 00a32b4

2 files changed

Lines changed: 110 additions & 1 deletion

File tree

‎packages/browser/src/profiling/UIProfiler.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,8 @@ export class UIProfiler implements ContinuousProfiler<Client> {
135135

136136
/** Handle an already-active root span at integration setup time (used only in trace mode). */
137137
public notifyRootSpanActive(rootSpan: Span): void {
138-
if (this._lifecycleMode !== 'trace' || !this._sessionSampled) {
138+
// Only count sampled root spans, same as the `spanStart` listener
139+
if (this._lifecycleMode !== 'trace' || !this._sessionSampled || !rootSpan.isRecording()) {
139140
return;
140141
}
141142

‎packages/browser/test/profiling/integration.test.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,114 @@ describe('BrowserProfilingIntegration', () => {
6868
expect(profile.profile.frames[0]).toMatchObject({ function: 'pageload_fn', lineno: 1, colno: 1 });
6969
});
7070

71+
it('does not profile an already active pageload span that is not sampled in trace lifecycle mode', async () => {
72+
const mockProfiler = vi.fn().mockImplementation(function () {
73+
return {
74+
stop: vi.fn().mockResolvedValue({
75+
frames: [{ name: 'pageload_fn', line: 1, column: 1 }],
76+
stacks: [{ frameId: 0 }],
77+
samples: [
78+
{ stackId: 0, timestamp: 0 },
79+
{ stackId: 0, timestamp: 100 },
80+
],
81+
resources: [],
82+
}),
83+
addEventListener: vi.fn(),
84+
};
85+
});
86+
87+
// @ts-expect-error this is a mock constructor
88+
window.Profiler = mockProfiler;
89+
90+
const send = vi.fn().mockResolvedValue(undefined);
91+
const client = Sentry.init({
92+
tracesSampleRate: 0,
93+
traceLifecycle: 'static',
94+
profileSessionSampleRate: 1,
95+
profileLifecycle: 'trace',
96+
dsn: 'https://examplePublicKey@o0.ingest.sentry.io/0',
97+
transport: () => ({
98+
flush: vi.fn().mockResolvedValue(true),
99+
send,
100+
}),
101+
integrations: [Sentry.browserTracingIntegration(), Sentry.browserProfilingIntegration()],
102+
});
103+
104+
const pageloadSpan = Sentry.getActiveSpan();
105+
expect(pageloadSpan).toBeDefined();
106+
expect(Sentry.spanToJSON(pageloadSpan!).op).toBe('pageload');
107+
expect(pageloadSpan!.isRecording()).toBe(false);
108+
109+
// The integration re-checks the active root span in a timeout after setup.
110+
await new Promise(resolve => setTimeout(resolve, 0));
111+
112+
pageloadSpan?.end();
113+
await Promise.resolve();
114+
await client?.flush(1000);
115+
116+
expect(mockProfiler).not.toHaveBeenCalled();
117+
const profileChunkEnvelope = send.mock.calls.find(call => call[0][1][0][0].type === 'profile_chunk');
118+
expect(profileChunkEnvelope).toBeUndefined();
119+
});
120+
121+
it('profiles a sampled root span that is already active when the integration is added', async () => {
122+
const stopProfile = vi.fn().mockResolvedValue({
123+
frames: [{ name: 'pageload_fn', line: 1, column: 1 }],
124+
stacks: [{ frameId: 0 }],
125+
samples: [
126+
{ stackId: 0, timestamp: 0 },
127+
{ stackId: 0, timestamp: 100 },
128+
],
129+
resources: [],
130+
});
131+
132+
const mockProfiler = vi.fn().mockImplementation(function () {
133+
return {
134+
stop: stopProfile,
135+
addEventListener: vi.fn(),
136+
};
137+
});
138+
139+
// @ts-expect-error this is a mock constructor
140+
window.Profiler = mockProfiler;
141+
142+
const send = vi.fn().mockResolvedValue(undefined);
143+
const client = Sentry.init({
144+
tracesSampleRate: 1,
145+
traceLifecycle: 'static',
146+
profileSessionSampleRate: 1,
147+
profileLifecycle: 'trace',
148+
dsn: 'https://examplePublicKey@o0.ingest.sentry.io/0',
149+
transport: () => ({
150+
flush: vi.fn().mockResolvedValue(true),
151+
send,
152+
}),
153+
integrations: [Sentry.browserTracingIntegration()],
154+
});
155+
156+
const pageloadSpan = Sentry.getActiveSpan();
157+
expect(pageloadSpan!.isRecording()).toBe(true);
158+
159+
// The pageload span started before the profiler listens to `spanStart`,
160+
// so only the integration setup can start the profiler for it.
161+
Sentry.addIntegration(Sentry.browserProfilingIntegration());
162+
const profilerCallsAfterSetup = mockProfiler.mock.calls.length;
163+
164+
// The re-check in a timeout notifies the same root span again, which must not start another profiler.
165+
await new Promise(resolve => setTimeout(resolve, 0));
166+
const profilerCallsAfterRecheck = mockProfiler.mock.calls.length;
167+
168+
pageloadSpan?.end();
169+
await Promise.resolve();
170+
await client?.flush(1000);
171+
172+
expect(profilerCallsAfterSetup).toBe(1);
173+
expect(profilerCallsAfterRecheck).toBe(1);
174+
expect(stopProfile).toHaveBeenCalledTimes(1);
175+
const profileChunkEnvelope = send.mock.calls.find(call => call[0][1][0][0].type === 'profile_chunk');
176+
expect(profileChunkEnvelope).toBeDefined();
177+
});
178+
71179
it("warns when profileLifecycle is 'trace' but tracing is disabled", async () => {
72180
const warnSpy = vi.spyOn(debug, 'warn').mockImplementation(() => {});
73181

0 commit comments

Comments
 (0)