diff --git a/CHANGELOG.md b/CHANGELOG.md index 67a3c9fc8247..de5310507d05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -238,6 +238,10 @@ Work in this release was contributed by @4ktLuffy, @adityareddy-dev, @dasilvalui These integrations need the vite plugin. Without it, they record nothing, and with `debug: true` the SDK logs a warning. +- **fix(cloudflare): Do not add back channel integrations the user removed** + + When an instrumented package loaded after `init()` (for example through a dynamic `import()`), the SDK added its integration even if the user had removed it. This applied to an `integrations` function that filtered it out, `defaultIntegrations: false`, and a custom `defaultIntegrations` list. The SDK now applies the same rules to a late-loaded integration as to the defaults at `init()`. To do this, it calls an `integrations` function again, once for each package that loads after `init()`. + ## 11.8.0 ### Important Changes diff --git a/packages/cloudflare/src/baseSdk.ts b/packages/cloudflare/src/baseSdk.ts index 0375e7f94601..0a9f6a1cf709 100644 --- a/packages/cloudflare/src/baseSdk.ts +++ b/packages/cloudflare/src/baseSdk.ts @@ -1,5 +1,5 @@ import type { Integration } from '@sentry/core'; -import { _INTERNAL_isClientClosed, debug, getCurrentScope, setCurrentClient } from '@sentry/core'; +import { _INTERNAL_isClientClosed, debug, getCurrentScope, safeCallback, setCurrentClient } from '@sentry/core'; import { consoleIntegration, conversationIdIntegration, @@ -107,6 +107,9 @@ export function initWithDefaultIntegrations( return cached; } + // `false` or a custom list turns off the SDK defaults, late-loaded ones + // included, unless an SDK built the list from the defaults. + const usesSdkDefaults = options.defaultIntegrations === undefined || !!options.extendsDefaultIntegrations; if (options.defaultIntegrations === undefined) { options.defaultIntegrations = getDefaultIntegrationsImpl(options); } @@ -153,12 +156,36 @@ export function initWithDefaultIntegrations( // lazily required on first use) stores its subscriber factory on the global // marker too late for the default-integrations snapshot above. Its injected // snippet emits this event right after storing the factory, so install the - // integration on the live client here. `addIntegration` dedupes by - // integration name, so already-installed integrations are no-ops. + // integration on the live client here, with the same rules as the defaults. + // `addIntegration` dedupes by integration name, so an integration the user + // configured stays in place. + const userIntegrations = options.integrations; client.on('orchestrion.module-injected', moduleName => { const factory = GLOBAL_OBJ.__SENTRY_ORCHESTRION__?.integrations?.get(moduleName); - if (factory) { - client.addIntegration(factory()); + if (!usesSdkDefaults || !factory) { + return; + } + + const integration = factory(); + if (typeof userIntegrations !== 'function') { + client.addIntegration(integration); + return; + } + + // Pass the late default through the user's `integrations` function, so a + // filter that removed it at init removes it here too. This calls the + // function again, once for each late module. If the function throws, skip + // the integration, so the module load that emitted the event still works. + const kept = safeCallback( + DEBUG_BUILD ? 'Error in the `integrations` function, skipping a late-loaded integration:' : '', + () => + getIntegrationsToSetup({ defaultIntegrations: [integration], integrations: userIntegrations }).find( + ({ name }) => name === integration.name, + ), + () => undefined, + ); + if (kept) { + client.addIntegration(kept); } }); diff --git a/packages/cloudflare/src/client.ts b/packages/cloudflare/src/client.ts index 051ffd8627d2..be45095e1481 100644 --- a/packages/cloudflare/src/client.ts +++ b/packages/cloudflare/src/client.ts @@ -351,6 +351,16 @@ interface BaseCloudflareOptions { */ enableDedupe?: boolean; + /** + * @ignore Used internally by SDKs that build `defaultIntegrations` from the Cloudflare defaults. + * @hidden Used internally by SDKs that build `defaultIntegrations` from the Cloudflare defaults. + * + * Marks the `defaultIntegrations` list as an extension of the SDK defaults, so the SDK also adds + * the integrations of instrumented modules that load after `init()`. + * @default false + */ + extendsDefaultIntegrations?: boolean; + /** * The Cloudflare SDK is not OpenTelemetry native. By default (`false`) it does not set up a tracer * provider; spans are emitted via the SDK's own instrumentation and scopes are isolated with diff --git a/packages/cloudflare/test/sdk.test.ts b/packages/cloudflare/test/sdk.test.ts index d1be4d2d4599..60254f504a66 100644 --- a/packages/cloudflare/test/sdk.test.ts +++ b/packages/cloudflare/test/sdk.test.ts @@ -458,6 +458,108 @@ describe('getDefaultIntegrations', () => { expect(client?.getIntegrationByName('Mysql')).toBe(userInstance); }); + describe('when a channel integration module loads after init', () => { + const DSN = 'https://public@dsn.ingest.sentry.io/1337'; + + async function loadMysqlLate(client: CloudflareClient | undefined): Promise { + const { mysqlIntegration } = await import('@sentry/server-utils'); + globalThis.__SENTRY_ORCHESTRION__ = { + bundler: new Set(['mysql']), + integrations: new Map([['mysql', mysqlIntegration]]), + }; + client?.emit('orchestrion.module-injected', 'mysql'); + } + + test('does not add it when the integrations function filtered it out', async () => { + resetSdk(); + const client = init({ + dsn: DSN, + integrations: defaults => defaults.filter(integration => integration.name !== 'Mysql'), + }); + + await loadMysqlLate(client); + + expect(client?.getIntegrationByName('Mysql')).toBeUndefined(); + }); + + test('adds it when the integrations function keeps it', async () => { + resetSdk(); + const client = init({ dsn: DSN, integrations: defaults => defaults }); + + await loadMysqlLate(client); + + expect(client?.getIntegrationByName('Mysql')).toBeDefined(); + }); + + test('keeps the instance the integrations function configured', async () => { + resetSdk(); + const client = init({ + dsn: DSN, + integrations: defaults => [...defaults, exportedMysqlIntegration()], + }); + const configured = client?.getIntegrationByName('Mysql'); + expect(configured).toBeDefined(); + + await loadMysqlLate(client); + + expect(client?.getIntegrationByName('Mysql')).toBe(configured); + }); + + test('does not add it and does not throw when the integrations function throws', async () => { + resetSdk(); + let calls = 0; + const client = init({ + dsn: DSN, + integrations: defaults => { + calls++; + if (calls > 1) { + throw new Error('integrations failed'); + } + return defaults; + }, + }); + + await expect(loadMysqlLate(client)).resolves.toBeUndefined(); + + expect(client?.getIntegrationByName('Mysql')).toBeUndefined(); + }); + + test('adds it when a custom defaultIntegrations list extends the SDK defaults', async () => { + resetSdk(); + const client = init({ dsn: DSN, defaultIntegrations: [], extendsDefaultIntegrations: true }); + + await loadMysqlLate(client); + + expect(client?.getIntegrationByName('Mysql')).toBeDefined(); + }); + + test('does not add it when the integrations function filtered it out of a list that extends the SDK defaults', async () => { + resetSdk(); + const client = init({ + dsn: DSN, + defaultIntegrations: [], + extendsDefaultIntegrations: true, + integrations: defaults => defaults.filter(integration => integration.name !== 'Mysql'), + }); + + await loadMysqlLate(client); + + expect(client?.getIntegrationByName('Mysql')).toBeUndefined(); + }); + + test.each([ + ['false', false as const], + ['a custom list', [] as Integration[]], + ])('does not add it when defaultIntegrations is %s', async (_label, defaultIntegrations) => { + resetSdk(); + const client = init({ dsn: DSN, defaultIntegrations }); + + await loadMysqlLate(client); + + expect(client?.getIntegrationByName('Mysql')).toBeUndefined(); + }); + }); + test('keeps a channel integration the user configured when its module loads after init', async () => { resetSdk(); const { mysqlIntegration } = await import('@sentry/server-utils'); diff --git a/packages/nuxt/src/runtime/plugins/sentry-cloudflare.server.ts b/packages/nuxt/src/runtime/plugins/sentry-cloudflare.server.ts index cc2fcb1c3315..46c18d6f4aa8 100644 --- a/packages/nuxt/src/runtime/plugins/sentry-cloudflare.server.ts +++ b/packages/nuxt/src/runtime/plugins/sentry-cloudflare.server.ts @@ -71,7 +71,11 @@ export const sentryCloudflareNitroPlugin = const requestHandlerOptions = { // `wrapRequestHandler` only sets up the integrations that work without `nodejs_compat`. // Nitro's Cloudflare presets require the flag anyway, so opt into the full set. - options: { defaultIntegrations: getDefaultIntegrations(cloudflareOptions), ...cloudflareOptions }, + options: { + defaultIntegrations: getDefaultIntegrations(cloudflareOptions), + extendsDefaultIntegrations: cloudflareOptions.defaultIntegrations === undefined, + ...cloudflareOptions, + }, request, context: getCloudflareProperties(event).context, }; diff --git a/packages/nuxt/test/runtime/plugins/sentry-cloudflare.server.test.ts b/packages/nuxt/test/runtime/plugins/sentry-cloudflare.server.test.ts new file mode 100644 index 000000000000..b2e720545c8a --- /dev/null +++ b/packages/nuxt/test/runtime/plugins/sentry-cloudflare.server.test.ts @@ -0,0 +1,56 @@ +import type { CloudflareOptions } from '@sentry/cloudflare'; +import { wrapRequestHandler } from '@sentry/cloudflare/request'; +import type { NitroApp } from 'nitropack'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { sentryCloudflareNitroPlugin } from '../../../src/runtime/plugins/sentry-cloudflare.server'; + +vi.mock('@sentry/cloudflare/request', () => ({ + wrapRequestHandler: vi.fn((_options: unknown, callback: () => unknown) => callback()), +})); + +const DSN = 'https://public@dsn.ingest.sentry.io/1337'; + +async function handleRequest(options: CloudflareOptions): Promise { + const nitroApp = { localFetch: vi.fn(), hooks: { hook: vi.fn() } } as unknown as NitroApp; + sentryCloudflareNitroPlugin(options)(nitroApp); + + const event = { + protocol: 'https:', + host: 'example.com', + method: 'GET', + headers: {}, + context: { cf: {}, cloudflare: { context: {} } }, + }; + await nitroApp.localFetch('/api/db-mysql', event as unknown as RequestInit); +} + +describe('sentryCloudflareNitroPlugin', () => { + beforeEach(() => { + vi.mocked(wrapRequestHandler).mockClear(); + }); + + it('passes a defaultIntegrations list that extends the SDK defaults', async () => { + await handleRequest({ dsn: DSN }); + + expect(wrapRequestHandler).toHaveBeenCalledWith( + expect.objectContaining({ + options: expect.objectContaining({ + defaultIntegrations: expect.any(Array), + extendsDefaultIntegrations: true, + }), + }), + expect.any(Function), + ); + }); + + it('does not mark a defaultIntegrations list the user passed as extending the SDK defaults', async () => { + await handleRequest({ dsn: DSN, defaultIntegrations: [] }); + + expect(wrapRequestHandler).toHaveBeenCalledWith( + expect.objectContaining({ + options: expect.objectContaining({ defaultIntegrations: [], extendsDefaultIntegrations: false }), + }), + expect.any(Function), + ); + }); +}); diff --git a/packages/sveltekit/src/worker/cloudflare.ts b/packages/sveltekit/src/worker/cloudflare.ts index 651cc6af6f37..f7298f1539b5 100644 --- a/packages/sveltekit/src/worker/cloudflare.ts +++ b/packages/sveltekit/src/worker/cloudflare.ts @@ -23,6 +23,7 @@ export function initCloudflareSentryHandle(options: CloudflareOptions): Handle { rewriteFramesIntegration(), svelteKitSpansIntegration(), ], + extendsDefaultIntegrations: options.defaultIntegrations === undefined, // SvelteKit emits its own OpenTelemetry spans (Kit tracing), so, like the Node SvelteKit SDK, it // defaults to registering the tracer provider instead of inheriting Cloudflare's no-provider default. // Only the `init`-backed wrapper from the main entry point honors this; `@sentry/cloudflare/request` diff --git a/packages/sveltekit/test/worker/cloudflare.test.ts b/packages/sveltekit/test/worker/cloudflare.test.ts index dea694b292ec..f4b24b641b4e 100644 --- a/packages/sveltekit/test/worker/cloudflare.test.ts +++ b/packages/sveltekit/test/worker/cloudflare.test.ts @@ -63,7 +63,11 @@ describe('initCloudflareSentryHandle', () => { { // SvelteKit emits its own OpenTelemetry spans, so it opts into the tracer provider rather than // inheriting Cloudflare's no-provider default. - options: expect.objectContaining({ dsn: options.dsn, enableOpenTelemetrySetup: true }), + options: expect.objectContaining({ + dsn: options.dsn, + enableOpenTelemetrySetup: true, + extendsDefaultIntegrations: true, + }), request, context, captureErrors: false, @@ -74,6 +78,25 @@ describe('initCloudflareSentryHandle', () => { expect(resolve).toHaveBeenCalledTimes(1); }); + it('does not mark a defaultIntegrations list the user passed as extending the SDK defaults', async () => { + const { options, event, resolve } = getHandlerInput(); + + // @ts-expect-error - resolving an empty object is enough for this test + vi.mocked(wrapRequestHandler).mockImplementationOnce((_, cb) => cb()); + + const handle = initCloudflareSentryHandle({ ...options, defaultIntegrations: [] }); + + // @ts-expect-error - only passing a partial event object + await handle({ event, resolve }); + + expect(wrapRequestHandler).toHaveBeenCalledWith( + expect.objectContaining({ + options: expect.objectContaining({ defaultIntegrations: [], extendsDefaultIntegrations: false }), + }), + expect.any(Function), + ); + }); + it('adds flag to skip request isolation in subsequent sentry handler', async () => { const { options, event, resolve } = getHandlerInput(); const locals = {};