From d31850ccddc437fd3c3a4b5e41769ad58a5e0d22 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 8 Apr 2024 15:21:26 +0200 Subject: [PATCH] feat(core): Allow to pass scope to `withIsolationScope()` Also adds tests for this for core, vercel-edge and opentelemetry. NOTE: In core, this does not do anything, as we cannot actually update the isolation scope in browser. Co-authored-by: Luca Forstner --- packages/core/src/currentScopes.ts | 39 ++++- packages/core/test/lib/scope.test.ts | 144 ++++++++++++++--- .../test/asyncContextStrategy.test.ts | 129 +++++++++++++++ packages/vercel-edge/src/async.ts | 6 +- packages/vercel-edge/test/async.test.ts | 150 ++++++++++++++++++ 5 files changed, 436 insertions(+), 32 deletions(-) create mode 100644 packages/vercel-edge/test/async.test.ts diff --git a/packages/core/src/currentScopes.ts b/packages/core/src/currentScopes.ts index 1e68c9583372..e0b598c1ad39 100644 --- a/packages/core/src/currentScopes.ts +++ b/packages/core/src/currentScopes.ts @@ -74,15 +74,44 @@ export function withScope( * * This function is intended for Sentry SDK and SDK integration development. It is not recommended to be used in "normal" * applications directly because it comes with pitfalls. Use at your own risk! + */ +export function withIsolationScope(callback: (isolationScope: Scope) => T): T; +/** + * Set the provided isolation scope as active in the given callback. If no + * async context strategy is set, the isolation scope and the current scope will not be forked (this is currently the + * case, for example, in the browser). + * + * Usage of this function in environments without async context strategy is discouraged and may lead to unexpected behaviour. * - * @param callback The callback in which the passed isolation scope is active. (Note: In environments without async - * context strategy, the currently active isolation scope may change within execution of the callback.) - * @returns The same value that `callback` returns. + * This function is intended for Sentry SDK and SDK integration development. It is not recommended to be used in "normal" + * applications directly because it comes with pitfalls. Use at your own risk! + * + * If you pass in `undefined` as a scope, it will fork a new isolation scope, the same as if no scope is passed. + */ +export function withIsolationScope(isolationScope: Scope | undefined, callback: (isolationScope: Scope) => T): T; +/** + * Either creates a new active isolation scope, or sets the given isolation scope as active scope in the given callback. */ -export function withIsolationScope(callback: (isolationScope: Scope) => T): T { +export function withIsolationScope( + ...rest: + | [callback: (isolationScope: Scope) => T] + | [isolationScope: Scope | undefined, callback: (isolationScope: Scope) => T] +): T { const carrier = getMainCarrier(); const acs = getAsyncContextStrategy(carrier); - return acs.withIsolationScope(callback); + + // If a scope is defined, we want to make this the active scope instead of the default one + if (rest.length === 2) { + const [isolationScope, callback] = rest; + + if (!isolationScope) { + return acs.withIsolationScope(callback); + } + + return acs.withSetIsolationScope(isolationScope, callback); + } + + return acs.withIsolationScope(rest[0]); } /** diff --git a/packages/core/test/lib/scope.test.ts b/packages/core/test/lib/scope.test.ts index bbd40af742d1..aadc26856c6e 100644 --- a/packages/core/test/lib/scope.test.ts +++ b/packages/core/test/lib/scope.test.ts @@ -5,6 +5,7 @@ import { getGlobalScope, getIsolationScope, withIsolationScope, + withScope, } from '../../src'; import { Scope } from '../../src/scope'; @@ -853,40 +854,135 @@ describe('Scope', () => { }); }); -describe('isolation scope', () => { - describe('withIsolationScope()', () => { - it('will pass an isolation scope without Sentry.init()', done => { - expect.assertions(1); - withIsolationScope(scope => { - expect(scope).toBeDefined(); - done(); - }); +describe('withScope()', () => { + beforeEach(() => { + getIsolationScope().clear(); + getCurrentScope().clear(); + getGlobalScope().clear(); + }); + + it('will make the passed scope the active scope within the callback', done => { + withScope(scope => { + expect(getCurrentScope()).toBe(scope); + done(); + }); + }); + + it('will pass a scope that is different from the current active isolation scope', done => { + withScope(scope => { + expect(getIsolationScope()).not.toBe(scope); + done(); }); + }); - it('will make the passed isolation scope the active isolation scope within the callback', done => { - expect.assertions(1); - withIsolationScope(scope => { - expect(getIsolationScope()).toBe(scope); + it('will always make the inner most passed scope the current isolation scope when nesting calls', done => { + withIsolationScope(_scope1 => { + withIsolationScope(scope2 => { + expect(getIsolationScope()).toBe(scope2); done(); }); }); + }); + + it('forks the scope when not passing any scope', done => { + const initialScope = getCurrentScope(); + initialScope.setTag('aa', 'aa'); - it('will pass an isolation scope that is different from the current active scope', done => { - expect.assertions(1); - withIsolationScope(scope => { - expect(getCurrentScope()).not.toBe(scope); + withScope(scope => { + expect(getCurrentScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('forks the scope when passing undefined', done => { + const initialScope = getCurrentScope(); + initialScope.setTag('aa', 'aa'); + + withScope(undefined, scope => { + expect(getCurrentScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('sets the passed in scope as active scope', done => { + const initialScope = getCurrentScope(); + initialScope.setTag('aa', 'aa'); + + const customScope = new Scope(); + + withScope(customScope, scope => { + expect(getCurrentScope()).toBe(customScope); + expect(scope).toBe(customScope); + done(); + }); + }); +}); + +describe('withIsolationScope()', () => { + beforeEach(() => { + getIsolationScope().clear(); + getCurrentScope().clear(); + getGlobalScope().clear(); + }); + + it('will make the passed isolation scope the active isolation scope within the callback', done => { + withIsolationScope(scope => { + expect(getIsolationScope()).toBe(scope); + done(); + }); + }); + + it('will pass an isolation scope that is different from the current active scope', done => { + withIsolationScope(scope => { + expect(getCurrentScope()).not.toBe(scope); + done(); + }); + }); + + it('will always make the inner most passed scope the current scope when nesting calls', done => { + withIsolationScope(_scope1 => { + withIsolationScope(scope2 => { + expect(getIsolationScope()).toBe(scope2); done(); }); }); + }); + + // Note: This is expected! In browser, we do not actually fork this + it('does not fork isolation scope when not passing any isolation scope', done => { + const isolationScope = getIsolationScope(); + + withIsolationScope(scope => { + expect(getIsolationScope()).toBe(scope); + expect(scope).toBe(isolationScope); + done(); + }); + }); - it('will always make the inner most passed scope the current scope when nesting calls', done => { - expect.assertions(1); - withIsolationScope(_scope1 => { - withIsolationScope(scope2 => { - expect(getIsolationScope()).toBe(scope2); - done(); - }); - }); + it('does not fork isolation scope when passing undefined', done => { + const isolationScope = getIsolationScope(); + + withIsolationScope(undefined, scope => { + expect(getIsolationScope()).toBe(scope); + expect(scope).toBe(isolationScope); + done(); + }); + }); + + it('ignores passed in isolation scope', done => { + const isolationScope = getIsolationScope(); + const customIsolationScope = new Scope(); + + withIsolationScope(customIsolationScope, scope => { + expect(getIsolationScope()).toBe(isolationScope); + expect(scope).toBe(isolationScope); + done(); }); }); }); diff --git a/packages/opentelemetry/test/asyncContextStrategy.test.ts b/packages/opentelemetry/test/asyncContextStrategy.test.ts index 16eea942655e..f3b664fcaf44 100644 --- a/packages/opentelemetry/test/asyncContextStrategy.test.ts +++ b/packages/opentelemetry/test/asyncContextStrategy.test.ts @@ -1,5 +1,6 @@ import type { BasicTracerProvider } from '@opentelemetry/sdk-trace-base'; import { + Scope as ScopeClass, getCurrentScope, getIsolationScope, setAsyncContextStrategy, @@ -298,4 +299,132 @@ describe('asyncContextStrategy', () => { }); }); }); + + describe('withScope()', () => { + it('will make the passed scope the active scope within the callback', done => { + withScope(scope => { + expect(getCurrentScope()).toBe(scope); + done(); + }); + }); + + it('will pass a scope that is different from the current active isolation scope', done => { + withScope(scope => { + expect(getIsolationScope()).not.toBe(scope); + done(); + }); + }); + + it('will always make the inner most passed scope the current scope when nesting calls', done => { + withIsolationScope(_scope1 => { + withIsolationScope(scope2 => { + expect(getIsolationScope()).toBe(scope2); + done(); + }); + }); + }); + + it('forks the scope when not passing any scope', done => { + const initialScope = getCurrentScope(); + initialScope.setTag('aa', 'aa'); + + withScope(scope => { + expect(getCurrentScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('forks the scope when passing undefined', done => { + const initialScope = getCurrentScope(); + initialScope.setTag('aa', 'aa'); + + withScope(undefined, scope => { + expect(getCurrentScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('sets the passed in scope as active scope', done => { + const initialScope = getCurrentScope(); + initialScope.setTag('aa', 'aa'); + + const customScope = new ScopeClass(); + + withScope(customScope, scope => { + expect(getCurrentScope()).toBe(customScope); + expect(scope).toBe(customScope); + done(); + }); + }); + }); + + describe('withIsolationScope()', () => { + it('will make the passed isolation scope the active isolation scope within the callback', done => { + withIsolationScope(scope => { + expect(getIsolationScope()).toBe(scope); + done(); + }); + }); + + it('will pass an isolation scope that is different from the current active scope', done => { + withIsolationScope(scope => { + expect(getCurrentScope()).not.toBe(scope); + done(); + }); + }); + + it('will always make the inner most passed scope the current scope when nesting calls', done => { + withIsolationScope(_scope1 => { + withIsolationScope(scope2 => { + expect(getIsolationScope()).toBe(scope2); + done(); + }); + }); + }); + + it('forks the isolation scope when not passing any isolation scope', done => { + const initialScope = getIsolationScope(); + initialScope.setTag('aa', 'aa'); + + withIsolationScope(scope => { + expect(getIsolationScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('forks the isolation scope when passing undefined', done => { + const initialScope = getIsolationScope(); + initialScope.setTag('aa', 'aa'); + + withIsolationScope(undefined, scope => { + expect(getIsolationScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('sets the passed in isolation scope as active isolation scope', done => { + const initialScope = getIsolationScope(); + initialScope.setTag('aa', 'aa'); + + const customScope = new ScopeClass(); + + withIsolationScope(customScope, scope => { + expect(getIsolationScope()).toBe(customScope); + expect(scope).toBe(customScope); + done(); + }); + }); + }); }); diff --git a/packages/vercel-edge/src/async.ts b/packages/vercel-edge/src/async.ts index 11b6b790df1a..50e2d80ad652 100644 --- a/packages/vercel-edge/src/async.ts +++ b/packages/vercel-edge/src/async.ts @@ -11,15 +11,15 @@ interface AsyncLocalStorage { run(store: T, callback: (...args: TArgs) => R, ...args: TArgs): R; } -// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any -const MaybeGlobalAsyncLocalStorage = (GLOBAL_OBJ as any).AsyncLocalStorage; - let asyncStorage: AsyncLocalStorage; /** * Sets the async context strategy to use AsyncLocalStorage which should be available in the edge runtime. */ export function setAsyncLocalStorageAsyncContextStrategy(): void { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any + const MaybeGlobalAsyncLocalStorage = (GLOBAL_OBJ as any).AsyncLocalStorage; + if (!MaybeGlobalAsyncLocalStorage) { DEBUG_BUILD && logger.warn( diff --git a/packages/vercel-edge/test/async.test.ts b/packages/vercel-edge/test/async.test.ts new file mode 100644 index 000000000000..37b357ab4910 --- /dev/null +++ b/packages/vercel-edge/test/async.test.ts @@ -0,0 +1,150 @@ +import { Scope, getCurrentScope, getGlobalScope, getIsolationScope, withIsolationScope, withScope } from '@sentry/core'; +import { GLOBAL_OBJ } from '@sentry/utils'; +import { AsyncLocalStorage } from 'async_hooks'; +import { setAsyncLocalStorageAsyncContextStrategy } from '../src/async'; + +describe('withScope()', () => { + beforeEach(() => { + getIsolationScope().clear(); + getCurrentScope().clear(); + getGlobalScope().clear(); + + (GLOBAL_OBJ as any).AsyncLocalStorage = AsyncLocalStorage; + setAsyncLocalStorageAsyncContextStrategy(); + }); + + it('will make the passed scope the active scope within the callback', done => { + withScope(scope => { + expect(getCurrentScope()).toBe(scope); + done(); + }); + }); + + it('will pass a scope that is different from the current active isolation scope', done => { + withScope(scope => { + expect(getIsolationScope()).not.toBe(scope); + done(); + }); + }); + + it('will always make the inner most passed scope the current scope when nesting calls', done => { + withIsolationScope(_scope1 => { + withIsolationScope(scope2 => { + expect(getIsolationScope()).toBe(scope2); + done(); + }); + }); + }); + + it('forks the scope when not passing any scope', done => { + const initialScope = getCurrentScope(); + initialScope.setTag('aa', 'aa'); + + withScope(scope => { + expect(getCurrentScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('forks the scope when passing undefined', done => { + const initialScope = getCurrentScope(); + initialScope.setTag('aa', 'aa'); + + withScope(undefined, scope => { + expect(getCurrentScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('sets the passed in scope as active scope', done => { + const initialScope = getCurrentScope(); + initialScope.setTag('aa', 'aa'); + + const customScope = new Scope(); + + withScope(customScope, scope => { + expect(getCurrentScope()).toBe(customScope); + expect(scope).toBe(customScope); + done(); + }); + }); +}); + +describe('withIsolationScope()', () => { + beforeEach(() => { + getIsolationScope().clear(); + getCurrentScope().clear(); + getGlobalScope().clear(); + (GLOBAL_OBJ as any).AsyncLocalStorage = AsyncLocalStorage; + + setAsyncLocalStorageAsyncContextStrategy(); + }); + + it('will make the passed isolation scope the active isolation scope within the callback', done => { + withIsolationScope(scope => { + expect(getIsolationScope()).toBe(scope); + done(); + }); + }); + + it('will pass an isolation scope that is different from the current active scope', done => { + withIsolationScope(scope => { + expect(getCurrentScope()).not.toBe(scope); + done(); + }); + }); + + it('will always make the inner most passed scope the current scope when nesting calls', done => { + withIsolationScope(_scope1 => { + withIsolationScope(scope2 => { + expect(getIsolationScope()).toBe(scope2); + done(); + }); + }); + }); + + it('forks the isolation scope when not passing any isolation scope', done => { + const initialScope = getIsolationScope(); + initialScope.setTag('aa', 'aa'); + + withIsolationScope(scope => { + expect(getIsolationScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('forks the isolation scope when passing undefined', done => { + const initialScope = getIsolationScope(); + initialScope.setTag('aa', 'aa'); + + withIsolationScope(undefined, scope => { + expect(getIsolationScope()).toBe(scope); + scope.setTag('bb', 'bb'); + expect(scope).not.toBe(initialScope); + expect(scope.getScopeData().tags).toEqual({ aa: 'aa', bb: 'bb' }); + done(); + }); + }); + + it('sets the passed in isolation scope as active isolation scope', done => { + const initialScope = getIsolationScope(); + initialScope.setTag('aa', 'aa'); + + const customScope = new Scope(); + + withIsolationScope(customScope, scope => { + expect(getIsolationScope()).toBe(customScope); + expect(scope).toBe(customScope); + done(); + }); + }); +});