Skip to content

Commit fee86b0

Browse files
authored
fix(browser): Ensure suppressTracing does not leak when async (#16545)
This changes the behavior of `suppressTracing` to be less problematic (or, problematic in a different way). Today, because in the browser we do not have async context isolation, there is only a single shared top scope. Because of this, if you have code like this: ```js const spanPromise = suppressTracing(async () => { await new Promise(resolve => setTimeout(resolve, 100)); return startInactiveSpan({ name: 'span' }); }); const span = startInactiveSpan({ name: 'span2' }); ``` The span2 will also be suppressed, because `suppressTracing` forks the scope and sets data on it to ensure spans are not recorded. This is problematic as that will result in completely unrelated spans, e.g. pageload/navigation spans, being suppressed while e.g. an ongoing fetch call that is suppressed happens. This PR changes this to instead only suppress tracing synchronously in the browser. This obviously is also not really ideal and can lead to things _not_ being suppressed, but it feels like the better tradeoff for now.
1 parent 91ee98d commit fee86b0

File tree

4 files changed

+83
-1
lines changed

4 files changed

+83
-1
lines changed

packages/cloudflare/src/async.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,16 @@ export function setAsyncLocalStorageAsyncContextStrategy(): void {
6464
});
6565
}
6666

67+
// In contrast to the browser, we can rely on async context isolation here
68+
function suppressTracing<T>(callback: () => T): T {
69+
return withScope(scope => {
70+
scope.setSDKProcessingMetadata({ __SENTRY_SUPPRESS_TRACING__: true });
71+
return callback();
72+
});
73+
}
74+
6775
setAsyncContextStrategy({
76+
suppressTracing,
6877
withScope,
6978
withSetScope,
7079
withIsolationScope,

packages/core/src/tracing/trace.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,8 +253,15 @@ export function suppressTracing<T>(callback: () => T): T {
253253
}
254254

255255
return withScope(scope => {
256+
// Note: We do not wait for the callback to finish before we reset the metadata
257+
// the reason for this is that otherwise, in the browser this can lead to very weird behavior
258+
// as there is only a single top scope, if the callback takes longer to finish,
259+
// other, unrelated spans may also be suppressed, which we do not want
260+
// so instead, we only suppress tracing synchronoysly in the browser
256261
scope.setSDKProcessingMetadata({ [SUPPRESS_TRACING_KEY]: true });
257-
return callback();
262+
const res = callback();
263+
scope.setSDKProcessingMetadata({ [SUPPRESS_TRACING_KEY]: undefined });
264+
return res;
258265
});
259266
}
260267

packages/core/test/lib/tracing/trace.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1975,6 +1975,40 @@ describe('suppressTracing', () => {
19751975
expect(spanIsSampled(child)).toBe(false);
19761976
});
19771977
});
1978+
1979+
it('works with parallel processes', async () => {
1980+
const span = suppressTracing(() => {
1981+
return startInactiveSpan({ name: 'span' });
1982+
});
1983+
1984+
// Note: This is unintuitive, but it is the expected behavior
1985+
// because we only suppress tracing synchronously in the browser
1986+
const span2Promise = suppressTracing(async () => {
1987+
await new Promise(resolve => setTimeout(resolve, 100));
1988+
return startInactiveSpan({ name: 'span2' });
1989+
});
1990+
1991+
const span3Promise = suppressTracing(async () => {
1992+
const span = startInactiveSpan({ name: 'span3' });
1993+
await new Promise(resolve => setTimeout(resolve, 100));
1994+
return span;
1995+
});
1996+
1997+
const span4 = suppressTracing(() => {
1998+
return startInactiveSpan({ name: 'span' });
1999+
});
2000+
2001+
const span5 = startInactiveSpan({ name: 'span5' });
2002+
2003+
const span2 = await span2Promise;
2004+
const span3 = await span3Promise;
2005+
2006+
expect(spanIsSampled(span)).toBe(false);
2007+
expect(spanIsSampled(span2)).toBe(true);
2008+
expect(spanIsSampled(span3)).toBe(false);
2009+
expect(spanIsSampled(span4)).toBe(false);
2010+
expect(spanIsSampled(span5)).toBe(true);
2011+
});
19782012
});
19792013

19802014
describe('startNewTrace', () => {

packages/opentelemetry/test/trace.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1921,6 +1921,38 @@ describe('suppressTracing', () => {
19211921
expect(spanIsSampled(child)).toBe(false);
19221922
});
19231923
});
1924+
1925+
it('works with parallel processes', async () => {
1926+
const span = suppressTracing(() => {
1927+
return startInactiveSpan({ name: 'span' });
1928+
});
1929+
1930+
const span2Promise = suppressTracing(async () => {
1931+
await new Promise(resolve => setTimeout(resolve, 100));
1932+
return startInactiveSpan({ name: 'span2' });
1933+
});
1934+
1935+
const span3Promise = suppressTracing(async () => {
1936+
const span = startInactiveSpan({ name: 'span3' });
1937+
await new Promise(resolve => setTimeout(resolve, 100));
1938+
return span;
1939+
});
1940+
1941+
const span4 = suppressTracing(() => {
1942+
return startInactiveSpan({ name: 'span' });
1943+
});
1944+
1945+
const span5 = startInactiveSpan({ name: 'span5' });
1946+
1947+
const span2 = await span2Promise;
1948+
const span3 = await span3Promise;
1949+
1950+
expect(spanIsSampled(span)).toBe(false);
1951+
expect(spanIsSampled(span2)).toBe(false);
1952+
expect(spanIsSampled(span3)).toBe(false);
1953+
expect(spanIsSampled(span4)).toBe(false);
1954+
expect(spanIsSampled(span5)).toBe(true);
1955+
});
19241956
});
19251957

19261958
function getSpanName(span: AbstractSpan): string | undefined {

0 commit comments

Comments
 (0)