Skip to content

Commit 84d6378

Browse files
authored
fix(profiling-node): Stop the trace lifecycle profiler when spans are unsampled (#25210)
With `profileLifecycle: 'trace'`, the profiler counted every `spanStart` and `spanEnd`. Unsampled child spans emit `spanStart` but never `spanEnd`, so the count never got back to 0. The profiler kept running, and Node never exited. - Trace mode now tracks only sampled spans, by span ID. - A stop just before the next 60s chunk started did not prevent that chunk, so profiling started again (manual and trace mode). A stop now cancels the scheduled chunk. - New debug logs show skipped unsampled traces and cancelled chunks. A fake-timer test stops profiling in the gap between two chunks and checks that no new chunk starts. Fixes #25174
1 parent 31717a4 commit 84d6378

2 files changed

Lines changed: 253 additions & 15 deletions

File tree

‎packages/profiling-node/src/integration.ts‎

Lines changed: 63 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,15 @@
11
/* eslint-disable max-lines */
22
import type { Event, IntegrationFn, ProfileChunk, ProfilingIntegration } from '@sentry/core';
3-
import { consoleSandbox, debug, defineIntegration, getCurrentScope, getGlobalScope, uuid4 } from '@sentry/core';
3+
import {
4+
consoleSandbox,
5+
debug,
6+
defineIntegration,
7+
getCurrentScope,
8+
getGlobalScope,
9+
getRootSpan,
10+
spanIsSampled,
11+
uuid4,
12+
} from '@sentry/core';
413
import type { NodeClient } from '@sentry/node';
514
import { CpuProfilerBindings, ProfileFormat } from '@sentry/node-cpu-profiler';
615
import { isMainThread } from 'worker_threads';
@@ -24,6 +33,7 @@ class ContinuousProfiler {
2433
private _profilerId: string | undefined;
2534
private _client: NodeClient | undefined = undefined;
2635
private _chunkData: ChunkData | undefined = undefined;
36+
private _pendingChunkRestart: NodeJS.Immediate | undefined = undefined;
2737
private _profileLifecycle: 'manual' | 'trace' | undefined = undefined;
2838
private _sampled: boolean | undefined = undefined;
2939
private _sessionSamplingRate: number | undefined = undefined;
@@ -95,7 +105,7 @@ class ContinuousProfiler {
95105
}
96106

97107
private _startProfiler(): void {
98-
if (this._chunkData !== undefined) {
108+
if (this._isSessionRunning()) {
99109
DEBUG_BUILD && debug.log('[Profiling] Profile session already running, no-op.');
100110
return;
101111
}
@@ -125,7 +135,7 @@ class ContinuousProfiler {
125135
return;
126136
}
127137

128-
if (!this._chunkData) {
138+
if (!this._isSessionRunning()) {
129139
DEBUG_BUILD && debug.log('[Profiling] No profile session running, no-op.');
130140
return;
131141
}
@@ -151,19 +161,30 @@ class ContinuousProfiler {
151161
return;
152162
}
153163

154-
let activeSpanCounter = 0;
155-
this._client.on('spanStart', _span => {
156-
if (activeSpanCounter === 0) {
164+
// Unsampled child spans emit `spanStart` but never `spanEnd`, so only sampled spans are tracked.
165+
const activeSpanIds = new Set<string>();
166+
this._client.on('spanStart', span => {
167+
if (!spanIsSampled(span)) {
168+
if (DEBUG_BUILD && span === getRootSpan(span)) {
169+
debug.log('[Profiling] Not profiling trace because its root span was not sampled.');
170+
}
171+
return;
172+
}
173+
if (activeSpanIds.size === 0) {
157174
this._startChunkProfiling();
158175
}
159-
activeSpanCounter++;
176+
activeSpanIds.add(span.spanContext().spanId);
160177
});
161178

162-
this._client.on('spanEnd', _span => {
163-
if (activeSpanCounter === 1) {
179+
this._client.on('spanEnd', span => {
180+
const spanId = span.spanContext().spanId;
181+
if (!activeSpanIds.has(spanId)) {
182+
return;
183+
}
184+
activeSpanIds.delete(spanId);
185+
if (activeSpanIds.size === 0) {
164186
this._stopChunkProfiling();
165187
}
166-
activeSpanCounter--;
167188
});
168189
}
169190

@@ -194,6 +215,10 @@ class ContinuousProfiler {
194215
* Stops profiling of the current chunks and flushes the profile to Sentry
195216
*/
196217
private _stopChunkProfiling(): void {
218+
// The chunk timer stops the current chunk before it schedules the next one, so a stop that lands
219+
// in between must also cancel that restart, or profiling continues with nothing left to profile.
220+
this._cancelPendingChunkRestart();
221+
197222
if (!this._chunkData) {
198223
DEBUG_BUILD && debug.log('[Profiling] No chunk data found, no-op.');
199224
return;
@@ -257,10 +282,6 @@ class ContinuousProfiler {
257282
}
258283

259284
this._flush(chunk);
260-
// Depending on the profile and stack sizes, stopping the profile and converting
261-
// the format may negatively impact the performance of the application. To avoid
262-
// blocking for too long, enqueue the next chunk start inside the next macrotask.
263-
// clear current chunk
264285
this._resetChunkData();
265286
}
266287

@@ -305,13 +326,40 @@ class ContinuousProfiler {
305326
DEBUG_BUILD && debug.log(`[Profiling] Stopping profiling chunk: ${chunk.id}`);
306327
this._stopChunkProfiling();
307328
DEBUG_BUILD && debug.log('[Profiling] Starting new profiling chunk.');
308-
setImmediate(this._restartChunkProfiling.bind(this));
329+
// Depending on the profile and stack sizes, stopping the profile and converting
330+
// the format may negatively impact the performance of the application. To avoid
331+
// blocking for too long, enqueue the next chunk start inside the next macrotask.
332+
this._pendingChunkRestart = setImmediate(() => {
333+
this._pendingChunkRestart = undefined;
334+
this._restartChunkProfiling();
335+
});
309336
}, CHUNK_INTERVAL_MS);
310337

311338
// Unref timeout so it doesn't keep the process alive.
312339
chunk.timer.unref();
313340
}
314341

342+
/**
343+
* A session is also running in the gap after the chunk timer stopped one chunk and before the next one starts.
344+
*/
345+
private _isSessionRunning(): boolean {
346+
return this._chunkData !== undefined || this._pendingChunkRestart !== undefined;
347+
}
348+
349+
/**
350+
* Cancels the next chunk if the chunk timer scheduled one that has not started yet.
351+
*/
352+
private _cancelPendingChunkRestart(): void {
353+
if (!this._pendingChunkRestart) {
354+
return;
355+
}
356+
357+
clearImmediate(this._pendingChunkRestart);
358+
this._pendingChunkRestart = undefined;
359+
DEBUG_BUILD &&
360+
debug.log('[Profiling] Profiling stopped before the next chunk started, cancelled the scheduled chunk.');
361+
}
362+
315363
/**
316364
* Attaches profiling information to spans that were started
317365
* during a profiling session.

‎packages/profiling-node/test/integration.test.ts‎

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { Transport } from '@sentry/core';
2+
import { debug } from '@sentry/core';
23
import * as Sentry from '@sentry/node';
34
import type { NodeClientOptions } from '@sentry/node/build/types/types';
45
import { CpuProfilerBindings } from '@sentry/node-cpu-profiler';
@@ -54,6 +55,21 @@ describe('ProfilingIntegration', () => {
5455
expect(stopProfilingSpy).toHaveBeenCalled();
5556
});
5657

58+
it('logs a no-op when stopping without a running profile session', () => {
59+
const [client] = makeSpanProfilingClient({
60+
profileLifecycle: 'manual',
61+
profileSessionSampleRate: 1,
62+
});
63+
Sentry.setCurrentClient(client);
64+
client.init();
65+
66+
const debugLogSpy = vi.spyOn(debug, 'log');
67+
68+
Sentry.profiler.stopProfiler();
69+
70+
expect(debugLogSpy).toHaveBeenCalledWith('[Profiling] No profile session running, no-op.');
71+
});
72+
5773
it('calling start and stop while profile session is running does nothing', () => {
5874
const [client] = makeSpanProfilingClient({
5975
profileLifecycle: 'manual',
@@ -305,6 +321,90 @@ describe('ProfilingIntegration', () => {
305321
expect(stopProfilingSpy).toHaveBeenCalled();
306322
});
307323

324+
it('logs when it skips a trace because the root span is not sampled', () => {
325+
const [client] = makeSpanProfilingClient({
326+
profileLifecycle: 'trace',
327+
profileSessionSampleRate: 1,
328+
tracesSampleRate: 0,
329+
});
330+
331+
Sentry.setCurrentClient(client);
332+
client.init();
333+
334+
const debugLogSpy = vi.spyOn(debug, 'log');
335+
336+
Sentry.startInactiveSpan({ name: 'GET /users/:id' });
337+
338+
expect(debugLogSpy).toHaveBeenCalledWith(
339+
'[Profiling] Not profiling trace because its root span was not sampled.',
340+
);
341+
});
342+
343+
it('does not log skipped unsampled child spans', () => {
344+
const [client] = makeSpanProfilingClient({
345+
profileLifecycle: 'trace',
346+
profileSessionSampleRate: 1,
347+
tracesSampleRate: 0,
348+
});
349+
350+
Sentry.setCurrentClient(client);
351+
client.init();
352+
353+
const rootSpan = Sentry.startInactiveSpan({ name: 'GET /users/:id' });
354+
const debugLogSpy = vi.spyOn(debug, 'log');
355+
356+
Sentry.startInactiveSpan({ name: 'SELECT * FROM users', parentSpan: rootSpan });
357+
358+
expect(debugLogSpy).not.toHaveBeenCalledWith(
359+
'[Profiling] Not profiling trace because its root span was not sampled.',
360+
);
361+
});
362+
363+
it('does not start the profiler for unsampled spans', () => {
364+
const [client] = makeSpanProfilingClient({
365+
profileLifecycle: 'trace',
366+
profileSessionSampleRate: 1,
367+
tracesSampleRate: 0,
368+
});
369+
370+
Sentry.setCurrentClient(client);
371+
client.init();
372+
373+
const startProfilingSpy = vi.spyOn(CpuProfilerBindings, 'startProfiling');
374+
375+
Sentry.startSpan({ name: 'root' }, () => {
376+
Sentry.startSpan({ name: 'child' }, () => {});
377+
});
378+
379+
expect(startProfilingSpy).not.toHaveBeenCalled();
380+
});
381+
382+
it('stops the profiler after the last sampled span ends, ignoring unsampled spans', () => {
383+
const [client] = makeSpanProfilingClient({
384+
profileLifecycle: 'trace',
385+
profileSessionSampleRate: 1,
386+
tracesSampler: ({ name }) => (name === 'sampled' ? 1 : 0),
387+
});
388+
389+
Sentry.setCurrentClient(client);
390+
client.init();
391+
392+
const startProfilingSpy = vi.spyOn(CpuProfilerBindings, 'startProfiling');
393+
const stopProfilingSpy = vi.spyOn(CpuProfilerBindings, 'stopProfiling');
394+
395+
Sentry.startSpan({ name: 'unsampled' }, () => {
396+
Sentry.startSpan({ name: 'unsampled-child' }, () => {});
397+
});
398+
399+
Sentry.startSpan({ name: 'sampled' }, () => {
400+
Sentry.startSpan({ name: 'sampled-child' }, () => {});
401+
expect(startProfilingSpy).toHaveBeenCalledTimes(1);
402+
expect(stopProfilingSpy).not.toHaveBeenCalled();
403+
});
404+
405+
expect(stopProfilingSpy).toHaveBeenCalledTimes(1);
406+
});
407+
308408
describe('envelope', () => {
309409
beforeEach(() => {
310410
vi.useRealTimers();
@@ -345,6 +445,96 @@ describe('ProfilingIntegration', () => {
345445
});
346446
});
347447
});
448+
449+
describe('chunk restart', () => {
450+
const lifecycles = [
451+
{
452+
profileLifecycle: 'manual',
453+
startProfiling: () => {
454+
Sentry.profiler.startProfiler();
455+
return () => Sentry.profiler.stopProfiler();
456+
},
457+
},
458+
{
459+
profileLifecycle: 'trace',
460+
startProfiling: () => {
461+
const span = Sentry.startInactiveSpan({ name: 'GET /users/:id' });
462+
return () => span.end();
463+
},
464+
},
465+
] as const;
466+
467+
beforeEach(() => {
468+
vi.useFakeTimers();
469+
});
470+
471+
afterEach(() => {
472+
vi.useRealTimers();
473+
});
474+
475+
it.each(lifecycles)(
476+
'does not start a new chunk when $profileLifecycle profiling stops before the scheduled restart',
477+
({ profileLifecycle, startProfiling }) => {
478+
const [client] = makeSpanProfilingClient({ profileLifecycle, profileSessionSampleRate: 1 });
479+
Sentry.setCurrentClient(client);
480+
client.init();
481+
482+
const startProfilingSpy = vi.spyOn(CpuProfilerBindings, 'startProfiling');
483+
const stopProfilingSpy = vi.spyOn(CpuProfilerBindings, 'stopProfiling');
484+
const debugLogSpy = vi.spyOn(debug, 'log');
485+
const stopProfiling = startProfiling();
486+
vi.advanceTimersToNextTimer();
487+
expect(stopProfilingSpy).toHaveBeenCalledTimes(1);
488+
489+
stopProfiling();
490+
vi.runOnlyPendingTimers();
491+
492+
expect(startProfilingSpy).toHaveBeenCalledTimes(1);
493+
expect(debugLogSpy).toHaveBeenCalledWith(
494+
'[Profiling] Profiling stopped before the next chunk started, cancelled the scheduled chunk.',
495+
);
496+
},
497+
);
498+
499+
it('does not start a second chunk when manual profiling starts between two chunks', () => {
500+
const [client] = makeSpanProfilingClient({ profileLifecycle: 'manual', profileSessionSampleRate: 1 });
501+
Sentry.setCurrentClient(client);
502+
client.init();
503+
504+
const startProfilingSpy = vi.spyOn(CpuProfilerBindings, 'startProfiling');
505+
const stopProfilingSpy = vi.spyOn(CpuProfilerBindings, 'stopProfiling');
506+
Sentry.profiler.startProfiler();
507+
vi.advanceTimersToNextTimer();
508+
509+
Sentry.profiler.startProfiler();
510+
vi.runOnlyPendingTimers();
511+
512+
expect(startProfilingSpy).toHaveBeenCalledTimes(2);
513+
expect(stopProfilingSpy).toHaveBeenCalledTimes(1);
514+
});
515+
516+
it.each(lifecycles)(
517+
'does not report a cancelled chunk when $profileLifecycle profiling stops after the restart',
518+
({ profileLifecycle, startProfiling }) => {
519+
const [client] = makeSpanProfilingClient({ profileLifecycle, profileSessionSampleRate: 1 });
520+
Sentry.setCurrentClient(client);
521+
client.init();
522+
523+
const startProfilingSpy = vi.spyOn(CpuProfilerBindings, 'startProfiling');
524+
const debugLogSpy = vi.spyOn(debug, 'log');
525+
const stopProfiling = startProfiling();
526+
vi.advanceTimersToNextTimer();
527+
vi.runOnlyPendingTimers();
528+
expect(startProfilingSpy).toHaveBeenCalledTimes(2);
529+
530+
stopProfiling();
531+
532+
expect(debugLogSpy).not.toHaveBeenCalledWith(
533+
'[Profiling] Profiling stopped before the next chunk started, cancelled the scheduled chunk.',
534+
);
535+
},
536+
);
537+
});
348538
});
349539

350540
describe('NODE_VERSION', () => {

0 commit comments

Comments
 (0)