From 71af30f5150e5dbb670932482d0d901fc3ae5a74 Mon Sep 17 00:00:00 2001 From: Andrei <168741329+andreiborza@users.noreply.github.com> Date: Tue, 30 Jul 2024 14:56:49 +0200 Subject: [PATCH 1/7] feat(solidstart): Add server action instrumentation helper (#13035) Can be used like this: ```js const getUserData = async () => { 'use server'; return await withServerActionInstrumentation('getData', () => { return { prefecture: 'Kanagawa' }; }); }; ``` Can also be used for api routes like this: ```js export async function GET() { return await withServerActionInstrumentation('getUser', () => { return json({ prefecture: 'Akita' }) }) } ``` --- .../test-applications/solidstart/package.json | 3 +- .../solidstart/src/entry-client.tsx | 1 + .../solidstart/src/instrument.server.mjs | 1 + .../solidstart/src/routes/index.tsx | 3 + .../solidstart/src/routes/server-error.tsx | 17 ++ .../solidstart/src/routes/users/[id].tsx | 19 +- .../solidstart/tests/errors.server.test.ts | 31 ++++ .../tests/performance.server.test.ts | 55 ++++++ packages/solidstart/package.json | 3 +- packages/solidstart/rollup.npm.config.mjs | 2 +- packages/solidstart/src/index.types.ts | 2 +- packages/solidstart/src/server/index.ts | 2 + packages/solidstart/src/server/utils.ts | 33 ++++ .../server/withServerActionInstrumentation.ts | 58 ++++++ .../withServerActionInstrumentation.test.ts | 167 ++++++++++++++++++ scripts/node-unit-tests.ts | 2 +- 16 files changed, 392 insertions(+), 7 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/solidstart/src/routes/server-error.tsx create mode 100644 dev-packages/e2e-tests/test-applications/solidstart/tests/errors.server.test.ts create mode 100644 dev-packages/e2e-tests/test-applications/solidstart/tests/performance.server.test.ts create mode 100644 packages/solidstart/src/server/utils.ts create mode 100644 packages/solidstart/src/server/withServerActionInstrumentation.ts create mode 100644 packages/solidstart/test/server/withServerActionInstrumentation.test.ts diff --git a/dev-packages/e2e-tests/test-applications/solidstart/package.json b/dev-packages/e2e-tests/test-applications/solidstart/package.json index 6409d191de5b..dfcf8a47402a 100644 --- a/dev-packages/e2e-tests/test-applications/solidstart/package.json +++ b/dev-packages/e2e-tests/test-applications/solidstart/package.json @@ -11,6 +11,7 @@ "This is currently not an issue outside of our repo. See: https://github.com/nksaraf/vinxi/issues/177" ], "preview": "HOST=localhost PORT=3030 NODE_OPTIONS='--import ./src/instrument.server.mjs' vinxi dev", + "start": "HOST=localhost PORT=3030 NODE_OPTIONS='--import ./src/instrument.server.mjs' vinxi start", "test:prod": "TEST_ENV=production playwright test", "test:build": "pnpm install && npx playwright install && pnpm build", "test:assert": "pnpm test:prod" @@ -31,7 +32,7 @@ "jsdom": "^24.0.0", "solid-js": "1.8.17", "typescript": "^5.4.5", - "vinxi": "^0.3.12", + "vinxi": "^0.4.0", "vite": "^5.2.8", "vite-plugin-solid": "^2.10.2", "vitest": "^1.5.0" diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/entry-client.tsx b/dev-packages/e2e-tests/test-applications/solidstart/src/entry-client.tsx index 9391faa9652d..11087fbb5918 100644 --- a/dev-packages/e2e-tests/test-applications/solidstart/src/entry-client.tsx +++ b/dev-packages/e2e-tests/test-applications/solidstart/src/entry-client.tsx @@ -12,6 +12,7 @@ Sentry.init({ tunnel: 'http://localhost:3031/', // proxy server // Performance Monitoring tracesSampleRate: 1.0, // Capture 100% of the transactions + debug: !!import.meta.env.DEBUG, }); mount(() => , document.getElementById('app')!); diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/instrument.server.mjs b/dev-packages/e2e-tests/test-applications/solidstart/src/instrument.server.mjs index 4146470295e1..3dd5d8933b7b 100644 --- a/dev-packages/e2e-tests/test-applications/solidstart/src/instrument.server.mjs +++ b/dev-packages/e2e-tests/test-applications/solidstart/src/instrument.server.mjs @@ -5,4 +5,5 @@ Sentry.init({ environment: 'qa', // dynamic sampling bias to keep transactions tracesSampleRate: 1.0, // Capture 100% of the transactions tunnel: 'http://localhost:3031/', // proxy server + debug: !!process.env.DEBUG, }); diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/routes/index.tsx b/dev-packages/e2e-tests/test-applications/solidstart/src/routes/index.tsx index f1635dee3b63..873d542e4bae 100644 --- a/dev-packages/e2e-tests/test-applications/solidstart/src/routes/index.tsx +++ b/dev-packages/e2e-tests/test-applications/solidstart/src/routes/index.tsx @@ -11,6 +11,9 @@ export default function Home() {
  • Client error
  • +
  • + Server error +
  • User 5 diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/routes/server-error.tsx b/dev-packages/e2e-tests/test-applications/solidstart/src/routes/server-error.tsx new file mode 100644 index 000000000000..05dce5e10a56 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/solidstart/src/routes/server-error.tsx @@ -0,0 +1,17 @@ +import { withServerActionInstrumentation } from '@sentry/solidstart'; +import { createAsync } from '@solidjs/router'; + +const getPrefecture = async () => { + 'use server'; + return await withServerActionInstrumentation('getPrefecture', () => { + throw new Error('Error thrown from Solid Start E2E test app server route'); + + return { prefecture: 'Kanagawa' }; + }); +}; + +export default function ServerErrorPage() { + const data = createAsync(() => getPrefecture()); + + return
    Prefecture: {data()?.prefecture}
    ; +} diff --git a/dev-packages/e2e-tests/test-applications/solidstart/src/routes/users/[id].tsx b/dev-packages/e2e-tests/test-applications/solidstart/src/routes/users/[id].tsx index 639ab0be8118..22abd3ba8803 100644 --- a/dev-packages/e2e-tests/test-applications/solidstart/src/routes/users/[id].tsx +++ b/dev-packages/e2e-tests/test-applications/solidstart/src/routes/users/[id].tsx @@ -1,6 +1,21 @@ -import { useParams } from '@solidjs/router'; +import { withServerActionInstrumentation } from '@sentry/solidstart'; +import { createAsync, useParams } from '@solidjs/router'; +const getPrefecture = async () => { + 'use server'; + return await withServerActionInstrumentation('getPrefecture', () => { + return { prefecture: 'Ehime' }; + }); +}; export default function User() { const params = useParams(); - return
    User ID: {params.id}
    ; + const userData = createAsync(() => getPrefecture()); + + return ( +
    + User ID: {params.id} +
    + Prefecture: {userData()?.prefecture} +
    + ); } diff --git a/dev-packages/e2e-tests/test-applications/solidstart/tests/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/solidstart/tests/errors.server.test.ts new file mode 100644 index 000000000000..0ccea7d3767e --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/solidstart/tests/errors.server.test.ts @@ -0,0 +1,31 @@ +import { expect, test } from '@playwright/test'; +import { waitForError } from '@sentry-internal/test-utils'; + +test.describe('server-side errors', () => { + test('captures server action error', async ({ page }) => { + const errorEventPromise = waitForError('solidstart', errorEvent => { + return errorEvent?.exception?.values?.[0]?.value === 'Error thrown from Solid Start E2E test app server route'; + }); + + await page.goto(`/server-error`); + + const error = await errorEventPromise; + + expect(error.tags).toMatchObject({ runtime: 'node' }); + expect(error).toMatchObject({ + exception: { + values: [ + { + type: 'Error', + value: 'Error thrown from Solid Start E2E test app server route', + mechanism: { + type: 'solidstart', + handled: false, + }, + }, + ], + }, + transaction: 'GET /server-error', + }); + }); +}); diff --git a/dev-packages/e2e-tests/test-applications/solidstart/tests/performance.server.test.ts b/dev-packages/e2e-tests/test-applications/solidstart/tests/performance.server.test.ts new file mode 100644 index 000000000000..bfd53bbb6bfa --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/solidstart/tests/performance.server.test.ts @@ -0,0 +1,55 @@ +import { expect, test } from '@playwright/test'; +import { waitForTransaction } from '@sentry-internal/test-utils'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, +} from '@sentry/core'; + +test('sends a server action transaction on pageload', async ({ page }) => { + const transactionPromise = waitForTransaction('solidstart', transactionEvent => { + return transactionEvent?.transaction === 'GET /users/6'; + }); + + await page.goto('/users/6'); + + const transaction = await transactionPromise; + + expect(transaction.spans).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + description: 'getPrefecture', + data: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.server_action', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.solidstart', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + }, + }), + ]), + ); +}); + +test('sends a server action transaction on client navigation', async ({ page }) => { + const transactionPromise = waitForTransaction('solidstart', transactionEvent => { + return transactionEvent?.transaction === 'POST getPrefecture'; + }); + + await page.goto('/'); + await page.locator('#navLink').click(); + await page.waitForURL('/users/5'); + + const transaction = await transactionPromise; + + expect(transaction.spans).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + description: 'getPrefecture', + data: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.server_action', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.solidstart', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + }, + }), + ]), + ); +}); diff --git a/packages/solidstart/package.json b/packages/solidstart/package.json index 4dd4d345f5e7..a22c235576f9 100644 --- a/packages/solidstart/package.json +++ b/packages/solidstart/package.json @@ -72,7 +72,8 @@ "@sentry/solid": "8.20.0", "@sentry/types": "8.20.0", "@sentry/utils": "8.20.0", - "@sentry/vite-plugin": "2.19.0" + "@sentry/vite-plugin": "2.19.0", + "@opentelemetry/instrumentation": "^0.52.1" }, "devDependencies": { "@solidjs/router": "^0.13.4", diff --git a/packages/solidstart/rollup.npm.config.mjs b/packages/solidstart/rollup.npm.config.mjs index 5a36889c70f0..b0087a93c6fe 100644 --- a/packages/solidstart/rollup.npm.config.mjs +++ b/packages/solidstart/rollup.npm.config.mjs @@ -16,7 +16,7 @@ export default makeNPMConfigVariants( // prevent this internal code from ending up in our built package (this doesn't happen automatially because // the name doesn't match an SDK dependency) packageSpecificConfig: { - external: ['solid-js', '@sentry/solid', '@sentry/solid/solidrouter'], + external: ['solid-js/web', 'solid-js', '@sentry/solid', '@sentry/solid/solidrouter'], output: { dynamicImportInCjs: true, }, diff --git a/packages/solidstart/src/index.types.ts b/packages/solidstart/src/index.types.ts index a2c4d1ab7c06..89eaa14662e3 100644 --- a/packages/solidstart/src/index.types.ts +++ b/packages/solidstart/src/index.types.ts @@ -1,5 +1,5 @@ // We export everything from both the client part of the SDK and from the server part. -// Some of the exports collide, which is not allowed, unless we redifine the colliding +// Some of the exports collide, which is not allowed, unless we redefine the colliding // exports in this file - which we do below. export * from './client'; export * from './server'; diff --git a/packages/solidstart/src/server/index.ts b/packages/solidstart/src/server/index.ts index 8b6ab6148fb8..75a67d3bb847 100644 --- a/packages/solidstart/src/server/index.ts +++ b/packages/solidstart/src/server/index.ts @@ -126,3 +126,5 @@ export { withSentryErrorBoundary } from '@sentry/solid'; // ------------------------- // Solid Start SDK exports: export { init } from './sdk'; + +export * from './withServerActionInstrumentation'; diff --git a/packages/solidstart/src/server/utils.ts b/packages/solidstart/src/server/utils.ts new file mode 100644 index 000000000000..f3d26e5d3a26 --- /dev/null +++ b/packages/solidstart/src/server/utils.ts @@ -0,0 +1,33 @@ +import { flush } from '@sentry/node'; +import { logger } from '@sentry/utils'; +import { DEBUG_BUILD } from '../common/debug-build'; + +/** Flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda ends */ +export async function flushIfServerless(): Promise { + const isServerless = !!process.env.LAMBDA_TASK_ROOT || !!process.env.VERCEL; + + if (isServerless) { + try { + DEBUG_BUILD && logger.log('Flushing events...'); + await flush(2000); + DEBUG_BUILD && logger.log('Done flushing events'); + } catch (e) { + DEBUG_BUILD && logger.log('Error while flushing events:\n', e); + } + } +} + +/** + * Determines if a thrown "error" is a redirect Response which Solid Start users can throw to redirect to another route. + * see: https://docs.solidjs.com/solid-router/reference/data-apis/response-helpers#redirect + * @param error the potential redirect error + */ +export function isRedirect(error: unknown): boolean { + if (error == null || !(error instanceof Response)) { + return false; + } + + const hasValidLocation = typeof error.headers.get('location') === 'string'; + const hasValidStatus = error.status >= 300 && error.status <= 308; + return hasValidLocation && hasValidStatus; +} diff --git a/packages/solidstart/src/server/withServerActionInstrumentation.ts b/packages/solidstart/src/server/withServerActionInstrumentation.ts new file mode 100644 index 000000000000..4e976322c6e4 --- /dev/null +++ b/packages/solidstart/src/server/withServerActionInstrumentation.ts @@ -0,0 +1,58 @@ +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, handleCallbackErrors } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, captureException, getActiveSpan, spanToJSON, startSpan } from '@sentry/node'; +import { flushIfServerless, isRedirect } from './utils'; + +/** + * Wraps a server action (functions that use the 'use server' directive) + * function body with Sentry Error and Performance instrumentation. + */ +export async function withServerActionInstrumentation
    unknown>( + serverActionName: string, + callback: A, +): Promise> { + const activeSpan = getActiveSpan(); + + if (activeSpan) { + const spanData = spanToJSON(activeSpan).data; + + // In solid start, server function calls are made to `/_server` which doesn't tell us + // a lot. We rewrite the span's route to be that of the sever action name but only + // if the target is `/_server`, otherwise we'd overwrite pageloads on routes that use + // server actions (which are more meaningful, e.g. a request to `GET /users/5` is more + // meaningful than overwriting it with `GET doSomeFunctionCall`). + if (spanData && !spanData['http.route'] && spanData['http.target'] === '/_server') { + activeSpan.setAttribute('http.route', serverActionName); + activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'component'); + } + } + + try { + return await startSpan( + { + op: 'function.server_action', + name: serverActionName, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.solidstart', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + }, + }, + async span => { + const result = await handleCallbackErrors(callback, error => { + if (!isRedirect(error)) { + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + captureException(error, { + mechanism: { + handled: false, + type: 'solidstart', + }, + }); + } + }); + + return result; + }, + ); + } finally { + await flushIfServerless(); + } +} diff --git a/packages/solidstart/test/server/withServerActionInstrumentation.test.ts b/packages/solidstart/test/server/withServerActionInstrumentation.test.ts new file mode 100644 index 000000000000..9a5b1e0c2b51 --- /dev/null +++ b/packages/solidstart/test/server/withServerActionInstrumentation.test.ts @@ -0,0 +1,167 @@ +import * as SentryNode from '@sentry/node'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, + createTransport, + getCurrentScope, + getIsolationScope, + setCurrentClient, + spanToJSON, +} from '@sentry/node'; +import { NodeClient } from '@sentry/node'; +import { solidRouterBrowserTracingIntegration } from '@sentry/solidstart/solidrouter'; +import { redirect } from '@solidjs/router'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const mockCaptureException = vi.spyOn(SentryNode, 'captureException').mockImplementation(() => ''); +const mockFlush = vi.spyOn(SentryNode, 'flush').mockImplementation(async () => true); +const mockGetActiveSpan = vi.spyOn(SentryNode, 'getActiveSpan'); + +const mockGetRequestEvent = vi.fn(); +vi.mock('solid-js/web', async () => { + const original = await vi.importActual('solid-js/web'); + return { + ...original, + getRequestEvent: (...args: unknown[]) => mockGetRequestEvent(...args), + }; +}); + +import { SentrySpan } from '@sentry/core'; +import { withServerActionInstrumentation } from '../../src/server'; + +describe('withServerActionInstrumentation', () => { + function createMockNodeClient(): NodeClient { + return new NodeClient({ + integrations: [], + tracesSampleRate: 1, + transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => Promise.resolve({})), + stackParser: () => [], + _metadata: { + sdk: { + name: 'sentry.javascript.solidstart', + }, + }, + }); + } + + // Mimics a server action function using sentry instrumentation + const serverActionGetPrefecture = async function getPrefecture() { + return withServerActionInstrumentation('getPrefecture', () => { + return { prefecture: 'Kagoshima' }; + }); + }; + + beforeEach(() => { + vi.clearAllMocks(); + vi.unstubAllEnvs(); + getCurrentScope().setClient(undefined); + getCurrentScope().clear(); + getIsolationScope().clear(); + }); + + afterEach(() => { + mockCaptureException.mockClear(); + }); + + it('calls captureException', async () => { + const error = new Error('Sample server action error'); + const serverAction = async function getData() { + return withServerActionInstrumentation('getData', () => { + throw error; + }); + }; + + const res = serverAction(); + await expect(res).rejects.toThrow(); + + expect(mockCaptureException).toHaveBeenCalledTimes(1); + expect(mockCaptureException).toHaveBeenCalledWith(error, { mechanism: { handled: false, type: 'solidstart' } }); + }); + + it("doesn't call captureException for thrown redirects", async () => { + const serverRedirectAction = async function getData() { + return withServerActionInstrumentation('getData', () => { + throw redirect('/'); + }); + }; + + const res = serverRedirectAction(); + await expect(res).rejects.toThrow(); + + expect(mockCaptureException).not.toHaveBeenCalled(); + }); + + it('calls `startSpan`', async () => { + const spanStartMock = vi.fn(); + const client = createMockNodeClient(); + setCurrentClient(client); + + client.on('spanStart', span => spanStartMock(spanToJSON(span))); + client.addIntegration(solidRouterBrowserTracingIntegration()); + + await serverActionGetPrefecture(); + expect(spanStartMock).toHaveBeenCalledWith( + expect.objectContaining({ + op: 'function.server_action', + description: 'getPrefecture', + data: expect.objectContaining({ + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.server_action', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.solidstart', + }), + }), + ); + }); + + it('calls `flush` if lambda', async () => { + vi.stubEnv('LAMBDA_TASK_ROOT', '1'); + + await serverActionGetPrefecture(); + expect(mockFlush).toHaveBeenCalledTimes(1); + }); + + it('calls `flush` if vercel', async () => { + vi.stubEnv('VERCEL', '1'); + + await serverActionGetPrefecture(); + expect(mockFlush).toHaveBeenCalledTimes(1); + }); + + it('sets a server action name on the active span', async () => { + const span = new SentrySpan(); + span.setAttribute('http.target', '/_server'); + mockGetActiveSpan.mockReturnValue(span); + const mockSpanSetAttribute = vi.spyOn(span, 'setAttribute'); + + const getPrefecture = async function load() { + return withServerActionInstrumentation('getPrefecture', () => { + return { prefecture: 'Kagoshima' }; + }); + }; + + await getPrefecture(); + + expect(mockGetActiveSpan).to.toHaveBeenCalledTimes(1); + expect(mockSpanSetAttribute).to.toHaveBeenCalledWith('http.route', 'getPrefecture'); + expect(mockSpanSetAttribute).to.toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'component'); + }); + + it('does not set a server action name if the active span had a non `/_server` target', async () => { + const span = new SentrySpan(); + span.setAttribute('http.target', '/users/5'); + mockGetActiveSpan.mockReturnValue(span); + const mockSpanSetAttribute = vi.spyOn(span, 'setAttribute'); + + const getPrefecture = async function load() { + return withServerActionInstrumentation('getPrefecture', () => { + return { prefecture: 'Kagoshima' }; + }); + }; + + await getPrefecture(); + + expect(mockGetActiveSpan).to.toHaveBeenCalledTimes(1); + expect(mockSpanSetAttribute).not.toHaveBeenCalled(); + }); +}); diff --git a/scripts/node-unit-tests.ts b/scripts/node-unit-tests.ts index 81ea2b0badf3..d5fe581f2a82 100644 --- a/scripts/node-unit-tests.ts +++ b/scripts/node-unit-tests.ts @@ -41,7 +41,7 @@ const SKIP_TEST_PACKAGES: Record = { ], }, '16': { - ignoredPackages: ['@sentry/cloudflare', '@sentry/vercel-edge', '@sentry/astro'], + ignoredPackages: ['@sentry/cloudflare', '@sentry/vercel-edge', '@sentry/astro', '@sentry/solidstart'], }, '18': { ignoredPackages: [], From b7e62c4d259b7bffed496410a6206475ab553475 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Jul 2024 09:08:00 -0400 Subject: [PATCH 2/7] test(astro): Switch to explicit vitest imports (#13093) As per https://vitest.dev/config/#globals > By default, vitest does not provide global APIs for explicitness I think we should follow vitest defaults here and explicitly import in the APIs that we need. This refactors our Astro SDK tests to do so. I also went ahead and fixed up some TS errors in some tests. This change also removes `environment: 'jsdom'` from the vite config as it seems nothing needs this for astro. This should means that our tests are not polluted with jsdom globals, and that future writers have to explicitly opt-in to the behaviour. --- packages/astro/test/client/sdk.test.ts | 15 ++++++++------- .../astro/test/integration/index.files.test.ts | 8 +++----- packages/astro/test/integration/index.test.ts | 4 ++-- .../test/integration/middleware/index.test.ts | 2 +- packages/astro/test/integration/snippets.test.ts | 2 ++ packages/astro/tsconfig.test.json | 2 +- packages/astro/vite.config.ts | 1 - 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/astro/test/client/sdk.test.ts b/packages/astro/test/client/sdk.test.ts index 4e7882b33e32..55381f52be17 100644 --- a/packages/astro/test/client/sdk.test.ts +++ b/packages/astro/test/client/sdk.test.ts @@ -1,3 +1,5 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + import type { BrowserClient } from '@sentry/browser'; import { browserTracingIntegration, @@ -8,9 +10,8 @@ import { } from '@sentry/browser'; import * as SentryBrowser from '@sentry/browser'; import { SDK_VERSION, getClient } from '@sentry/browser'; -import { vi } from 'vitest'; -import { init } from '../../../astro/src/client/sdk'; +import { init } from '../../src/client/sdk'; const browserInit = vi.spyOn(SentryBrowser, 'init'); @@ -66,7 +67,7 @@ describe('Sentry client SDK', () => { ...tracingOptions, }); - const integrationsToInit = browserInit.mock.calls[0][0]?.defaultIntegrations; + const integrationsToInit = browserInit.mock.calls[0]![0]?.defaultIntegrations; const browserTracing = getClient()?.getIntegrationByName('BrowserTracing'); expect(integrationsToInit).toContainEqual(expect.objectContaining({ name: 'BrowserTracing' })); @@ -82,7 +83,7 @@ describe('Sentry client SDK', () => { ...tracingOptions, }); - const integrationsToInit = browserInit.mock.calls[0][0]?.defaultIntegrations || []; + const integrationsToInit = browserInit.mock.calls[0]![0]?.defaultIntegrations || []; const browserTracing = getClient()?.getIntegrationByName('BrowserTracing'); expect(integrationsToInit).not.toContainEqual(expect.objectContaining({ name: 'BrowserTracing' })); @@ -90,20 +91,20 @@ describe('Sentry client SDK', () => { }); it("doesn't add browserTracingIntegration if `__SENTRY_TRACING__` is set to false", () => { - globalThis.__SENTRY_TRACING__ = false; + (globalThis as any).__SENTRY_TRACING__ = false; init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', enableTracing: true, }); - const integrationsToInit = browserInit.mock.calls[0][0]?.defaultIntegrations || []; + const integrationsToInit = browserInit.mock.calls[0]![0]?.defaultIntegrations || []; const browserTracing = getClient()?.getIntegrationByName('BrowserTracing'); expect(integrationsToInit).not.toContainEqual(expect.objectContaining({ name: 'BrowserTracing' })); expect(browserTracing).toBeUndefined(); - delete globalThis.__SENTRY_TRACING__; + delete (globalThis as any).__SENTRY_TRACING__; }); it('Overrides the automatically default browserTracingIntegration instance with a a user-provided browserTracingIntegration instance', () => { diff --git a/packages/astro/test/integration/index.files.test.ts b/packages/astro/test/integration/index.files.test.ts index f0b15f6a48c2..0b28b2a0550d 100644 --- a/packages/astro/test/integration/index.files.test.ts +++ b/packages/astro/test/integration/index.files.test.ts @@ -1,12 +1,10 @@ -import { vi } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { sentryAstro } from '../../src/integration'; -vi.mock('fs', async () => { - const actual = await vi.importActual('fs'); +vi.mock('fs', async requireActual => { return { - // @ts-expect-error - just mocking around - ...actual, + ...(await requireActual()), existsSync: vi.fn(p => p.endsWith('js')), }; }); diff --git a/packages/astro/test/integration/index.test.ts b/packages/astro/test/integration/index.test.ts index 886e77ada2dd..008132264602 100644 --- a/packages/astro/test/integration/index.test.ts +++ b/packages/astro/test/integration/index.test.ts @@ -1,4 +1,4 @@ -import { vi } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { sentryAstro } from '../../src/integration'; @@ -294,7 +294,7 @@ describe('sentryAstro integration', () => { it.each([{ output: 'static' }, { output: undefined }])( "doesn't add middleware if in static mode (config %s)", - async config => { + async (config: any) => { const integration = sentryAstro({}); const addMiddleware = vi.fn(); const updateConfig = vi.fn(); diff --git a/packages/astro/test/integration/middleware/index.test.ts b/packages/astro/test/integration/middleware/index.test.ts index b9d1273261de..3b12508feaa7 100644 --- a/packages/astro/test/integration/middleware/index.test.ts +++ b/packages/astro/test/integration/middleware/index.test.ts @@ -1,4 +1,4 @@ -import { vi } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { onRequest } from '../../../src/integration/middleware'; diff --git a/packages/astro/test/integration/snippets.test.ts b/packages/astro/test/integration/snippets.test.ts index 5911372c6647..04aaa866aee9 100644 --- a/packages/astro/test/integration/snippets.test.ts +++ b/packages/astro/test/integration/snippets.test.ts @@ -1,3 +1,5 @@ +import { describe, expect, it } from 'vitest'; + import { buildClientSnippet, buildSdkInitFileImportSnippet, buildServerSnippet } from '../../src/integration/snippets'; const allSdkOptions = { diff --git a/packages/astro/tsconfig.test.json b/packages/astro/tsconfig.test.json index 3fbe012384ee..c41efeacd92f 100644 --- a/packages/astro/tsconfig.test.json +++ b/packages/astro/tsconfig.test.json @@ -5,6 +5,6 @@ "compilerOptions": { // should include all types from `./tsconfig.json` plus types for all test frameworks used - "types": ["node", "vitest/globals"] + "types": ["node"] } } diff --git a/packages/astro/vite.config.ts b/packages/astro/vite.config.ts index 1094fe0d79da..f18ec92095bc 100644 --- a/packages/astro/vite.config.ts +++ b/packages/astro/vite.config.ts @@ -4,6 +4,5 @@ export default { ...baseConfig, test: { ...baseConfig.test, - environment: 'jsdom', }, }; From e3af1ce9cc6aa1d82128e7d84ba5f9ae03803523 Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Tue, 30 Jul 2024 16:14:56 +0200 Subject: [PATCH 3/7] feat(nestjs): Automatic instrumentation of nestjs middleware (#13065) Adds middleware instrumentation to the `@sentry/nestjs`. The implementation lives in `@sentry/node` so that both users using `@sentry/nestjs` directly as well as users still on `@sentry/node` benefit. The instrumentation is automatic without requiring any additional setup. The idea is to hook into the Injectable decorator (every class middleware is annotated with `@Injectable` and patch the `use` method if it is implemented. Caveat: This implementation only works for class middleware, which implements the `use` method, which seems to be the standard for implementing middleware in nest. However, nest also provides functional middleware, for which this implementation does not work. --- .../nestjs-basic/src/app.controller.ts | 5 + .../nestjs-basic/src/app.module.ts | 9 +- .../nestjs-basic/src/app.service.ts | 5 + .../nestjs-basic/src/example.middleware.ts | 12 ++ .../nestjs-basic/tests/transactions.test.ts | 79 +++++++++ .../node-nestjs-basic/src/app.controller.ts | 5 + .../node-nestjs-basic/src/app.module.ts | 9 +- .../node-nestjs-basic/src/app.service.ts | 5 + .../src/example.middleware.ts | 12 ++ .../tests/transactions.test.ts | 79 +++++++++ .../node/src/integrations/tracing/nest.ts | 163 +++++++++++++++++- .../test/integrations/tracing/nest.test.ts | 17 ++ 12 files changed, 394 insertions(+), 6 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.middleware.ts create mode 100644 dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.middleware.ts create mode 100644 packages/node/test/integrations/tracing/nest.test.ts diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts index b54604d999cb..2a4f14cae541 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts @@ -10,6 +10,11 @@ export class AppController { return this.appService.testTransaction(); } + @Get('test-middleware-instrumentation') + testMiddlewareInstrumentation() { + return this.appService.testMiddleware(); + } + @Get('test-exception/:id') async testException(@Param('id') id: string) { return this.appService.testException(id); diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts index f4c5ceb0cc5a..b2aad014c745 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts @@ -1,12 +1,17 @@ -import { Module } from '@nestjs/common'; +import { MiddlewareConsumer, Module } from '@nestjs/common'; import { ScheduleModule } from '@nestjs/schedule'; import { SentryModule } from '@sentry/nestjs/setup'; import { AppController } from './app.controller'; import { AppService } from './app.service'; +import { ExampleMiddleware } from './example.middleware'; @Module({ imports: [SentryModule.forRoot(), ScheduleModule.forRoot()], controllers: [AppController], providers: [AppService], }) -export class AppModule {} +export class AppModule { + configure(consumer: MiddlewareConsumer): void { + consumer.apply(ExampleMiddleware).forRoutes('test-middleware-instrumentation'); + } +} diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts index 3afb7b5147bd..9a47f0e08e7a 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts @@ -21,6 +21,11 @@ export class AppService { }); } + testMiddleware() { + // span that should not be a child span of the middleware span + Sentry.startSpan({ name: 'test-controller-span' }, () => {}); + } + testException(id: string) { throw new Error(`This is an exception with id ${id}`); } diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.middleware.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.middleware.ts new file mode 100644 index 000000000000..31d15c9372ea --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.middleware.ts @@ -0,0 +1,12 @@ +import { Injectable, NestMiddleware } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; +import { NextFunction, Request, Response } from 'express'; + +@Injectable() +export class ExampleMiddleware implements NestMiddleware { + use(req: Request, res: Response, next: NextFunction) { + // span that should be a child span of the middleware span + Sentry.startSpan({ name: 'test-middleware-span' }, () => {}); + next(); + } +} diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts index ae5d8b63b16d..b7017b72dbf5 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts @@ -121,3 +121,82 @@ test('Sends an API route transaction', async ({ baseURL }) => { }), ); }); + +test('API route transaction includes nest middleware span. Spans created in and after middleware are nested correctly', async ({ + baseURL, +}) => { + const pageloadTransactionEventPromise = waitForTransaction('nestjs', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-middleware-instrumentation' + ); + }); + + await fetch(`${baseURL}/test-middleware-instrumentation`); + + const transactionEvent = await pageloadTransactionEventPromise; + + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + 'sentry.op': 'middleware.nestjs', + 'sentry.origin': 'auto.middleware.nestjs', + }, + description: 'ExampleMiddleware', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + const exampleMiddlewareSpan = transactionEvent.spans.find(span => span.description === 'ExampleMiddleware'); + const exampleMiddlewareSpanId = exampleMiddlewareSpan?.span_id; + + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-controller-span', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + origin: 'manual', + }, + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-middleware-span', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + origin: 'manual', + }, + ]), + }), + ); + + // verify correct span parent-child relationships + const testMiddlewareSpan = transactionEvent.spans.find(span => span.description === 'test-middleware-span'); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'ExampleMiddleware' is the parent of 'test-middleware-span' + expect(testMiddlewareSpan.parent_span_id).toBe(exampleMiddlewareSpanId); + + // 'ExampleMiddleware' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleMiddlewareSpanId); +}); diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.controller.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.controller.ts index b54604d999cb..2a4f14cae541 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.controller.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.controller.ts @@ -10,6 +10,11 @@ export class AppController { return this.appService.testTransaction(); } + @Get('test-middleware-instrumentation') + testMiddlewareInstrumentation() { + return this.appService.testMiddleware(); + } + @Get('test-exception/:id') async testException(@Param('id') id: string) { return this.appService.testException(id); diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts index ceb7199a99cf..567dbefeadb7 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts @@ -1,11 +1,16 @@ -import { Module } from '@nestjs/common'; +import { MiddlewareConsumer, Module } from '@nestjs/common'; import { ScheduleModule } from '@nestjs/schedule'; import { AppController } from './app.controller'; import { AppService } from './app.service'; +import { ExampleMiddleware } from './example.middleware'; @Module({ imports: [ScheduleModule.forRoot()], controllers: [AppController], providers: [AppService], }) -export class AppModule {} +export class AppModule { + configure(consumer: MiddlewareConsumer): void { + consumer.apply(ExampleMiddleware).forRoutes('test-middleware-instrumentation'); + } +} diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.service.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.service.ts index 3afb7b5147bd..9a47f0e08e7a 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.service.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.service.ts @@ -21,6 +21,11 @@ export class AppService { }); } + testMiddleware() { + // span that should not be a child span of the middleware span + Sentry.startSpan({ name: 'test-controller-span' }, () => {}); + } + testException(id: string) { throw new Error(`This is an exception with id ${id}`); } diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.middleware.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.middleware.ts new file mode 100644 index 000000000000..31d15c9372ea --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.middleware.ts @@ -0,0 +1,12 @@ +import { Injectable, NestMiddleware } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; +import { NextFunction, Request, Response } from 'express'; + +@Injectable() +export class ExampleMiddleware implements NestMiddleware { + use(req: Request, res: Response, next: NextFunction) { + // span that should be a child span of the middleware span + Sentry.startSpan({ name: 'test-middleware-span' }, () => {}); + next(); + } +} diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts index ae5d8b63b16d..b7017b72dbf5 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts @@ -121,3 +121,82 @@ test('Sends an API route transaction', async ({ baseURL }) => { }), ); }); + +test('API route transaction includes nest middleware span. Spans created in and after middleware are nested correctly', async ({ + baseURL, +}) => { + const pageloadTransactionEventPromise = waitForTransaction('nestjs', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-middleware-instrumentation' + ); + }); + + await fetch(`${baseURL}/test-middleware-instrumentation`); + + const transactionEvent = await pageloadTransactionEventPromise; + + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + 'sentry.op': 'middleware.nestjs', + 'sentry.origin': 'auto.middleware.nestjs', + }, + description: 'ExampleMiddleware', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + const exampleMiddlewareSpan = transactionEvent.spans.find(span => span.description === 'ExampleMiddleware'); + const exampleMiddlewareSpanId = exampleMiddlewareSpan?.span_id; + + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-controller-span', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + origin: 'manual', + }, + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-middleware-span', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + origin: 'manual', + }, + ]), + }), + ); + + // verify correct span parent-child relationships + const testMiddlewareSpan = transactionEvent.spans.find(span => span.description === 'test-middleware-span'); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'ExampleMiddleware' is the parent of 'test-middleware-span' + expect(testMiddlewareSpan.parent_span_id).toBe(exampleMiddlewareSpanId); + + // 'ExampleMiddleware' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleMiddlewareSpanId); +}); diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index ab6a66fdb895..2ec5fa840387 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -1,16 +1,27 @@ +import { isWrapped } from '@opentelemetry/core'; +import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; +import { + InstrumentationBase, + InstrumentationNodeModuleDefinition, + InstrumentationNodeModuleFile, +} from '@opentelemetry/instrumentation'; import { NestInstrumentation } from '@opentelemetry/instrumentation-nestjs-core'; import { + SDK_VERSION, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, captureException, defineIntegration, + getActiveSpan, getClient, getDefaultIsolationScope, getIsolationScope, spanToJSON, + startSpanManual, + withActiveSpan, } from '@sentry/core'; import type { IntegrationFn, Span } from '@sentry/types'; -import { logger } from '@sentry/utils'; +import { addNonEnumerableProperty, logger } from '@sentry/utils'; import { generateInstrumentOnce } from '../../otel/instrument'; interface MinimalNestJsExecutionContext { @@ -44,7 +55,155 @@ interface MinimalNestJsApp { const INTEGRATION_NAME = 'Nest'; -export const instrumentNest = generateInstrumentOnce(INTEGRATION_NAME, () => new NestInstrumentation()); +const supportedVersions = ['>=8.0.0 <11']; + +const sentryPatched = 'sentryPatched'; + +/** + * Represents an injectable target class in NestJS. + */ +export interface InjectableTarget { + name: string; + sentryPatched?: boolean; + prototype: { + use?: (req: unknown, res: unknown, next: () => void) => void; + }; +} + +/** + * Helper checking if a concrete target class is already patched. + * + * We already guard duplicate patching with isWrapped. However, isWrapped checks whether a file has been patched, whereas we use this check for concrete target classes. + * This check might not be necessary, but better to play it safe. + */ +export function isPatched(target: InjectableTarget): boolean { + if (target.sentryPatched) { + return true; + } + + addNonEnumerableProperty(target, sentryPatched, true); + return false; +} + +/** + * Custom instrumentation for nestjs. + * + * This hooks into the @Injectable decorator, which is applied on class middleware, interceptors and guards. + */ +export class SentryNestInstrumentation extends InstrumentationBase { + public static readonly COMPONENT = '@nestjs/common'; + public static readonly COMMON_ATTRIBUTES = { + component: SentryNestInstrumentation.COMPONENT, + }; + + public constructor(config: InstrumentationConfig = {}) { + super('sentry-nestjs', SDK_VERSION, config); + } + + /** + * Initializes the instrumentation by defining the modules to be patched. + */ + public init(): InstrumentationNodeModuleDefinition { + const moduleDef = new InstrumentationNodeModuleDefinition(SentryNestInstrumentation.COMPONENT, supportedVersions); + + moduleDef.files.push(this._getInjectableFileInstrumentation(supportedVersions)); + return moduleDef; + } + + /** + * Wraps the @Injectable decorator. + */ + private _getInjectableFileInstrumentation(versions: string[]): InstrumentationNodeModuleFile { + return new InstrumentationNodeModuleFile( + '@nestjs/common/decorators/core/injectable.decorator.js', + versions, + (moduleExports: { Injectable: InjectableTarget }) => { + if (isWrapped(moduleExports.Injectable)) { + this._unwrap(moduleExports, 'Injectable'); + } + this._wrap(moduleExports, 'Injectable', this._createWrapInjectable()); + return moduleExports; + }, + (moduleExports: { Injectable: InjectableTarget }) => { + this._unwrap(moduleExports, 'Injectable'); + }, + ); + } + + /** + * Creates a wrapper function for the @Injectable decorator. + * + * Wraps the use method to instrument nest class middleware. + */ + private _createWrapInjectable() { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return function wrapInjectable(original: any) { + return function wrappedInjectable(options?: unknown) { + return function (target: InjectableTarget) { + // patch middleware + if (typeof target.prototype.use === 'function') { + // patch only once + if (isPatched(target)) { + return original(options)(target); + } + + target.prototype.use = new Proxy(target.prototype.use, { + apply: (originalUse, thisArgUse, argsUse) => { + const [req, res, next, ...args] = argsUse; + const prevSpan = getActiveSpan(); + + startSpanManual( + { + name: target.name, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'middleware.nestjs', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs', + }, + }, + (span: Span) => { + const nextProxy = new Proxy(next, { + apply: (originalNext, thisArgNext, argsNext) => { + span.end(); + + if (prevSpan) { + withActiveSpan(prevSpan, () => { + Reflect.apply(originalNext, thisArgNext, argsNext); + }); + } else { + Reflect.apply(originalNext, thisArgNext, argsNext); + } + }, + }); + + originalUse.apply(thisArgUse, [req, res, nextProxy, args]); + }, + ); + }, + }); + } + + return original(options)(target); + }; + }; + }; + } +} + +const instrumentNestCore = generateInstrumentOnce('Nest-Core', () => { + return new NestInstrumentation(); +}); + +const instrumentNestCommon = generateInstrumentOnce('Nest-Common', () => { + return new SentryNestInstrumentation(); +}); + +export const instrumentNest = Object.assign( + (): void => { + instrumentNestCore(); + instrumentNestCommon(); + }, + { id: INTEGRATION_NAME }, +); const _nestIntegration = (() => { return { diff --git a/packages/node/test/integrations/tracing/nest.test.ts b/packages/node/test/integrations/tracing/nest.test.ts new file mode 100644 index 000000000000..3dc321f28008 --- /dev/null +++ b/packages/node/test/integrations/tracing/nest.test.ts @@ -0,0 +1,17 @@ +import type { InjectableTarget } from '../../../src/integrations/tracing/nest'; +import { isPatched } from '../../../src/integrations/tracing/nest'; + +describe('Nest', () => { + describe('isPatched', () => { + it('should return true if target is already patched', () => { + const target = { name: 'TestTarget', sentryPatched: true, prototype: {} }; + expect(isPatched(target)).toBe(true); + }); + + it('should add the sentryPatched property and return false if target is not patched', () => { + const target: InjectableTarget = { name: 'TestTarget', prototype: {} }; + expect(isPatched(target)).toBe(false); + expect(target.sentryPatched).toBe(true); + }); + }); +}); From ce536675468281b4dd6c5446b195ce7d82abbfc6 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 30 Jul 2024 16:47:39 +0200 Subject: [PATCH 4/7] fix(nextjs): Only delete clientside bundle source maps with `sourcemaps.deleteFilesAfterUpload` (#13102) --- .../test-applications/nextjs-app-dir/assert-build.ts | 4 ++-- packages/nextjs/src/config/webpackPluginOptions.ts | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/assert-build.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/assert-build.ts index 58453223a4cb..955988101724 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/assert-build.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/assert-build.ts @@ -22,10 +22,10 @@ assert.match(buildStdout, /(λ|ƒ) \/server-component\/parameter\/\[\.\.\.parame assert.match(buildStdout, /(λ|ƒ) \/server-component\/parameter\/\[parameter\]/); // Read the contents of the directory -const files = fs.readdirSync(path.join(process.cwd(), '.next', 'server')); +const files = fs.readdirSync(path.join(process.cwd(), '.next', 'static')); const mapFiles = files.filter(file => path.extname(file) === '.map'); if (mapFiles.length > 0) { - throw new Error('.map files found even though `sourcemaps.deleteSourcemapsAfterUpload` option is set!'); + throw new Error('Client bundle .map files found even though `sourcemaps.deleteSourcemapsAfterUpload` option is set!'); } export {}; diff --git a/packages/nextjs/src/config/webpackPluginOptions.ts b/packages/nextjs/src/config/webpackPluginOptions.ts index 6b755cf2c839..f70862bfe484 100644 --- a/packages/nextjs/src/config/webpackPluginOptions.ts +++ b/packages/nextjs/src/config/webpackPluginOptions.ts @@ -76,9 +76,12 @@ export function getWebpackPluginOptions( ignore: sentryBuildOptions.sourcemaps?.ignore ?? sourcemapUploadIgnore, filesToDeleteAfterUpload: sentryBuildOptions.sourcemaps?.deleteSourcemapsAfterUpload ? [ - path.join(distDirAbsPath, '**', '*.js.map'), - path.join(distDirAbsPath, '**', '*.mjs.map'), - path.join(distDirAbsPath, '**', '*.cjs.map'), + // We only care to delete client bundle source maps because they would be the ones being served. + // Removing the server source maps crashes Vercel builds for (thus far) unknown reasons: + // https://github.com/getsentry/sentry-javascript/issues/13099 + path.join(distDirAbsPath, 'static', '**', '*.js.map'), + path.join(distDirAbsPath, 'static', '**', '*.mjs.map'), + path.join(distDirAbsPath, 'static', '**', '*.cjs.map'), ] : undefined, ...sentryBuildOptions.unstable_sentryWebpackPluginOptions?.sourcemaps, From d73a5677f4452d1862484712da4491ed9ae43c43 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 30 Jul 2024 16:53:14 +0200 Subject: [PATCH 5/7] fix(browser-integration-tests): Fix feedback addon CDN bundle integration test setup (#13108) - Add symlinks to lazy-loaded feedback sub-integrations whenever we discover that `feedbackIntegration` is imported in a test app - Stop forwarding the CDN bundle request to the actual CDN but throw a hard error instead --- .../browser-integration-tests/utils/fixtures.ts | 10 ++++++++-- .../utils/generatePlugin.ts | 13 +++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/dev-packages/browser-integration-tests/utils/fixtures.ts b/dev-packages/browser-integration-tests/utils/fixtures.ts index f6c989a53778..cf34c9b7e693 100644 --- a/dev-packages/browser-integration-tests/utils/fixtures.ts +++ b/dev-packages/browser-integration-tests/utils/fixtures.ts @@ -68,12 +68,18 @@ const sentryTest = base.extend({ // Ensure feedback can be lazy loaded await page.route(`https://browser.sentry-cdn.com/${SDK_VERSION}/feedback-modal.min.js`, route => { const filePath = path.resolve(testDir, './dist/feedback-modal.bundle.js'); - return fs.existsSync(filePath) ? route.fulfill({ path: filePath }) : route.continue(); + if (!fs.existsSync(filePath)) { + throw new Error(`Feedback modal bundle (${filePath}) not found`); + } + return route.fulfill({ path: filePath }); }); await page.route(`https://browser.sentry-cdn.com/${SDK_VERSION}/feedback-screenshot.min.js`, route => { const filePath = path.resolve(testDir, './dist/feedback-screenshot.bundle.js'); - return fs.existsSync(filePath) ? route.fulfill({ path: filePath }) : route.continue(); + if (!fs.existsSync(filePath)) { + throw new Error(`Feedback screenshot bundle (${filePath}) not found`); + } + return route.fulfill({ path: filePath }); }); } diff --git a/dev-packages/browser-integration-tests/utils/generatePlugin.ts b/dev-packages/browser-integration-tests/utils/generatePlugin.ts index 69e8f946fc89..30939c40c955 100644 --- a/dev-packages/browser-integration-tests/utils/generatePlugin.ts +++ b/dev-packages/browser-integration-tests/utils/generatePlugin.ts @@ -272,6 +272,19 @@ class SentryScenarioGenerationPlugin { fileName, ); + if (integration === 'feedback') { + addStaticAssetSymlink( + this.localOutPath, + path.resolve(PACKAGES_DIR, 'feedback', 'build/bundles/feedback-modal.js'), + 'feedback-modal.bundle.js', + ); + addStaticAssetSymlink( + this.localOutPath, + path.resolve(PACKAGES_DIR, 'feedback', 'build/bundles/feedback-screenshot.js'), + 'feedback-screenshot.bundle.js', + ); + } + const integrationObject = createHtmlTagObject('script', { src: fileName, }); From 51f85e645110eaf80940949264b4ff2135f13779 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Jul 2024 10:58:41 -0400 Subject: [PATCH 6/7] feat(node): Upgrade import-in-the-middle to 1.11.0 (#13107) ref https://github.com/nodejs/import-in-the-middle/releases/tag/import-in-the-middle-v1.11.0 Bumping this for the bug fix, we can expose the `registerOptions` in another PR. --- packages/node/package.json | 2 +- packages/nuxt/README.md | 12 ------------ yarn.lock | 8 ++++---- 3 files changed, 5 insertions(+), 17 deletions(-) diff --git a/packages/node/package.json b/packages/node/package.json index 41e321c66f53..5390c57bd0a3 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -92,7 +92,7 @@ "@sentry/opentelemetry": "8.20.0", "@sentry/types": "8.20.0", "@sentry/utils": "8.20.0", - "import-in-the-middle": "^1.10.0" + "import-in-the-middle": "^1.11.0" }, "devDependencies": { "@types/node": "^14.18.0" diff --git a/packages/nuxt/README.md b/packages/nuxt/README.md index a2f9d9d0d22e..1227aa5e2c82 100644 --- a/packages/nuxt/README.md +++ b/packages/nuxt/README.md @@ -126,18 +126,6 @@ other imports: } ``` -If you are getting an `import-in-the-middle` error message, add the package with a minimum version of `1.10.0` as a -dependency to your `package.json` -([issue reference](https://github.com/getsentry/sentry-javascript-examples/pull/38#issuecomment-2245259327)): - -```json -{ - "dependencies": { - "import-in-the-middle": "1.10.0" - } -} -``` - ## Uploading Source Maps To upload source maps, you can use the `sourceMapsUploadOptions` option inside the `sentry` options of your diff --git a/yarn.lock b/yarn.lock index 6d6f295e4049..650c78d792aa 100644 --- a/yarn.lock +++ b/yarn.lock @@ -20493,10 +20493,10 @@ import-in-the-middle@1.7.1: cjs-module-lexer "^1.2.2" module-details-from-path "^1.0.3" -import-in-the-middle@^1.10.0, import-in-the-middle@^1.8.1: - version "1.10.0" - resolved "https://registry.yarnpkg.com/import-in-the-middle/-/import-in-the-middle-1.10.0.tgz#f15b0841950ded8d899b635058da5646256949b1" - integrity sha512-Z1jumVdF2GwnnYfM0a/y2ts7mZbwFMgt5rRuVmLgobgahC6iKgN5MBuXjzfTIOUpq5LSU10vJIPpVKe0X89fIw== +import-in-the-middle@^1.11.0, import-in-the-middle@^1.8.1: + version "1.11.0" + resolved "https://registry.yarnpkg.com/import-in-the-middle/-/import-in-the-middle-1.11.0.tgz#a94c4925b8da18256cde3b3b7b38253e6ca5e708" + integrity sha512-5DimNQGoe0pLUHbR9qK84iWaWjjbsxiqXnw6Qz64+azRgleqv9k2kTt5fw7QsOpmaGYtuxxursnPPsnTKEx10Q== dependencies: acorn "^8.8.2" acorn-import-attributes "^1.9.5" From f3a4109dad24be685653f2094e07e6b0533bb4f0 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 30 Jul 2024 17:00:22 +0200 Subject: [PATCH 7/7] meta: Update CHANGELOG for 8.21.0 --- CHANGELOG.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9566c79ebf66..208ec7eb2a68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,11 +31,15 @@ upcoming release. - feat(feedback): Make cropped screenshot area draggable (#13071) - feat(core): Adapt spans for client-side fetch to streaming responses (#12723) - feat(core): Capture # of dropped spans through `beforeSendTransaction` (#13022) -- feat(deps): bump @opentelemetry/instrumentation-aws-sdk from 0.43.0 to 0.43.1 (#13089) -- feat(deps): bump @opentelemetry/instrumentation-express from 0.41.0 to 0.41.1 (#13090) +- feat(deps): bump `@opentelemetry/instrumentation-aws-sdk` from 0.43.0 to 0.43.1 (#13089) +- feat(deps): bump `@opentelemetry/instrumentation-express` from 0.41.0 to 0.41.1 (#13090) +- feat(nestjs): Automatic instrumentation of nestjs middleware (#13065) +- feat(node): Upgrade `import-in-the-middle` to 1.11.0 (#13107) - feat(nuxt): Add connected tracing meta tags (#13098) - feat(nuxt): Add vue-router instrumentation (#13054) +- feat(solidstart): Add server action instrumentation helper (#13035) - fix(feedback): Ensure pluggable feedback CDN bundle is correctly built (#13081) +- fix(nextjs): Only delete clientside bundle source maps with `sourcemaps.deleteFilesAfterUpload` (#13102) - fix(node): Improve OTEL validation logic (#13079) ## 8.20.0