From fdb20a03e194eaf2aa8fcaa68d9465a69cac5bc2 Mon Sep 17 00:00:00 2001 From: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com> Date: Tue, 6 Aug 2024 13:03:18 +0200 Subject: [PATCH 01/20] feat(astro): Always add BrowserTracing (#13244) The bundler-plugins still refer to the option as `excludePerformanceMonitoring` ([here](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/main/packages/bundler-plugin-core/src/types.ts#L260)), but this is going to be renamed to `excludeTracing`, so I already used the new naming as discussed with @Lms24 and @mydea. closes https://github.com/getsentry/sentry-javascript/issues/13013 --- packages/astro/src/client/sdk.ts | 14 ++++++-------- packages/astro/test/client/sdk.test.ts | 17 +---------------- 2 files changed, 7 insertions(+), 24 deletions(-) diff --git a/packages/astro/src/client/sdk.ts b/packages/astro/src/client/sdk.ts index e38a552feb39..8a4617c652cf 100644 --- a/packages/astro/src/client/sdk.ts +++ b/packages/astro/src/client/sdk.ts @@ -4,7 +4,7 @@ import { getDefaultIntegrations as getBrowserDefaultIntegrations, init as initBrowserSdk, } from '@sentry/browser'; -import { applySdkMetadata, hasTracingEnabled } from '@sentry/core'; +import { applySdkMetadata } from '@sentry/core'; import type { Client, Integration } from '@sentry/types'; // Tree-shakable guard to remove all code related to tracing @@ -26,14 +26,12 @@ export function init(options: BrowserOptions): Client | undefined { return initBrowserSdk(opts); } -function getDefaultIntegrations(options: BrowserOptions): Integration[] | undefined { +function getDefaultIntegrations(options: BrowserOptions): Integration[] { // This evaluates to true unless __SENTRY_TRACING__ is text-replaced with "false", - // in which case everything inside will get treeshaken away + // in which case everything inside will get tree-shaken away if (typeof __SENTRY_TRACING__ === 'undefined' || __SENTRY_TRACING__) { - if (hasTracingEnabled(options)) { - return [...getBrowserDefaultIntegrations(options), browserTracingIntegration()]; - } + return [...getBrowserDefaultIntegrations(options), browserTracingIntegration()]; + } else { + return getBrowserDefaultIntegrations(options); } - - return undefined; } diff --git a/packages/astro/test/client/sdk.test.ts b/packages/astro/test/client/sdk.test.ts index 1ef31131cb77..41ff4bbae061 100644 --- a/packages/astro/test/client/sdk.test.ts +++ b/packages/astro/test/client/sdk.test.ts @@ -53,6 +53,7 @@ describe('Sentry client SDK', () => { ['tracesSampleRate', { tracesSampleRate: 0 }], ['tracesSampler', { tracesSampler: () => 1.0 }], ['enableTracing', { enableTracing: true }], + ['no tracing option set', {}], ])('adds browserTracingIntegration if tracing is enabled via %s', (_, tracingOptions) => { init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', @@ -66,22 +67,6 @@ describe('Sentry client SDK', () => { expect(browserTracing).toBeDefined(); }); - it.each([ - ['enableTracing', { enableTracing: false }], - ['no tracing option set', {}], - ])("doesn't add browserTracingIntegration if tracing is disabled via %s", (_, tracingOptions) => { - init({ - dsn: 'https://public@dsn.ingest.sentry.io/1337', - ...tracingOptions, - }); - - const integrationsToInit = browserInit.mock.calls[0]![0]?.defaultIntegrations || []; - const browserTracing = getClient()?.getIntegrationByName('BrowserTracing'); - - expect(integrationsToInit).not.toContainEqual(expect.objectContaining({ name: 'BrowserTracing' })); - expect(browserTracing).toBeUndefined(); - }); - it("doesn't add browserTracingIntegration if `__SENTRY_TRACING__` is set to false", () => { (globalThis as any).__SENTRY_TRACING__ = false; From 05684d45222782c41a0ffb6da2af16708ff65d32 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 6 Aug 2024 14:37:58 +0200 Subject: [PATCH 02/20] feat(core): Add `getTraceMetaTags` function (#13201) Export function `getTraceMetaTags` that gives users an easy way to get stringified Html meta tags for server->client trace propagation. --------- Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com> --- .../suites/tracing/meta-tags/server.js | 33 +++++++++++++++++++ .../suites/tracing/meta-tags/test.ts | 26 +++++++++++++++ packages/astro/src/index.server.ts | 1 + packages/astro/src/server/middleware.ts | 12 +++---- packages/astro/test/server/middleware.test.ts | 10 +++--- packages/aws-serverless/src/index.ts | 1 + packages/bun/src/index.ts | 1 + packages/cloudflare/src/index.ts | 1 + packages/core/src/index.ts | 1 + packages/core/src/utils/meta.ts | 29 ++++++++++++++++ packages/core/test/lib/utils/meta.test.ts | 30 +++++++++++++++++ packages/deno/src/index.ts | 1 + packages/google-cloud-serverless/src/index.ts | 1 + packages/node/src/index.ts | 1 + packages/sveltekit/src/server/index.ts | 1 + packages/vercel-edge/src/index.ts | 1 + 16 files changed, 138 insertions(+), 12 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/meta-tags/server.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/meta-tags/test.ts create mode 100644 packages/core/src/utils/meta.ts create mode 100644 packages/core/test/lib/utils/meta.test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags/server.js b/dev-packages/node-integration-tests/suites/tracing/meta-tags/server.js new file mode 100644 index 000000000000..35e204a05d6b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags/server.js @@ -0,0 +1,33 @@ +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + tracesSampleRate: 1.0, + transport: loggingTransport, +}); + +// express must be required after Sentry is initialized +const express = require('express'); +const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); + +const app = express(); + +app.get('/test', (_req, res) => { + res.send({ + response: ` + + + ${Sentry.getTraceMetaTags()} + + + Hi :) + + + `, + }); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags/test.ts b/dev-packages/node-integration-tests/suites/tracing/meta-tags/test.ts new file mode 100644 index 000000000000..22bef9d2b1b9 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags/test.ts @@ -0,0 +1,26 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +describe('getTraceMetaTags', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + test('injects sentry tracing tags', async () => { + const traceId = 'cd7ee7a6fe3ebe7ab9c3271559bc203c'; + const parentSpanId = '100ff0980e7a4ead'; + + const runner = createRunner(__dirname, 'server.js').start(); + + const response = await runner.makeRequest('get', '/test', { + 'sentry-trace': `${traceId}-${parentSpanId}-1`, + baggage: 'sentry-environment=production', + }); + + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const html = response?.response as unknown as string; + + expect(html).toMatch(//); + expect(html).toContain(''); + }); +}); diff --git a/packages/astro/src/index.server.ts b/packages/astro/src/index.server.ts index 1084643584d6..7f52ad0dc0bb 100644 --- a/packages/astro/src/index.server.ts +++ b/packages/astro/src/index.server.ts @@ -56,6 +56,7 @@ export { getSpanDescendants, getSpanStatusFromHttpCode, getTraceData, + getTraceMetaTags, graphqlIntegration, hapiIntegration, httpIntegration, diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index 6b668f462489..4b2f15eb3be4 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -6,6 +6,7 @@ import { getActiveSpan, getClient, getCurrentScope, + getTraceMetaTags, setHttpStatus, startSpan, withIsolationScope, @@ -14,8 +15,6 @@ import type { Client, Scope, Span, SpanAttributes } from '@sentry/types'; import { addNonEnumerableProperty, objectify, stripUrlQueryAndFragment } from '@sentry/utils'; import type { APIContext, MiddlewareResponseHandler } from 'astro'; -import { getTraceData } from '@sentry/node'; - type MiddlewareOptions = { /** * If true, the client IP will be attached to the event by calling `setUser`. @@ -189,16 +188,13 @@ function addMetaTagToHead(htmlChunk: string, scope: Scope, client: Client, span? if (typeof htmlChunk !== 'string') { return htmlChunk; } - const { 'sentry-trace': sentryTrace, baggage } = getTraceData(span, scope, client); + const metaTags = getTraceMetaTags(span, scope, client); - if (!sentryTrace) { + if (!metaTags) { return htmlChunk; } - const sentryTraceMeta = ``; - const baggageMeta = baggage && ``; - - const content = `\n${sentryTraceMeta}`.concat(baggageMeta ? `\n${baggageMeta}` : '', '\n'); + const content = `${metaTags}`; return htmlChunk.replace('', content); } diff --git a/packages/astro/test/server/middleware.test.ts b/packages/astro/test/server/middleware.test.ts index 58405c8d1c12..bf96f6ef9046 100644 --- a/packages/astro/test/server/middleware.test.ts +++ b/packages/astro/test/server/middleware.test.ts @@ -34,10 +34,12 @@ describe('sentryMiddleware', () => { }); vi.spyOn(SentryNode, 'getActiveSpan').mockImplementation(getSpanMock); vi.spyOn(SentryNode, 'getClient').mockImplementation(() => ({}) as Client); - vi.spyOn(SentryNode, 'getTraceData').mockImplementation(() => ({ - 'sentry-trace': '123', - baggage: 'abc', - })); + vi.spyOn(SentryNode, 'getTraceMetaTags').mockImplementation( + () => ` + + + `, + ); vi.spyOn(SentryCore, 'getDynamicSamplingContextFromSpan').mockImplementation(() => ({ transaction: 'test', })); diff --git a/packages/aws-serverless/src/index.ts b/packages/aws-serverless/src/index.ts index 95b2d553f2d4..20ef9eeaf09f 100644 --- a/packages/aws-serverless/src/index.ts +++ b/packages/aws-serverless/src/index.ts @@ -21,6 +21,7 @@ export { getGlobalScope, getIsolationScope, getTraceData, + getTraceMetaTags, setCurrentClient, Scope, SDK_VERSION, diff --git a/packages/bun/src/index.ts b/packages/bun/src/index.ts index 287dbc26eeee..4de55fd1c5f7 100644 --- a/packages/bun/src/index.ts +++ b/packages/bun/src/index.ts @@ -41,6 +41,7 @@ export { getGlobalScope, getIsolationScope, getTraceData, + getTraceMetaTags, setCurrentClient, Scope, SDK_VERSION, diff --git a/packages/cloudflare/src/index.ts b/packages/cloudflare/src/index.ts index 2f77f96f4e33..0c02fb8ca810 100644 --- a/packages/cloudflare/src/index.ts +++ b/packages/cloudflare/src/index.ts @@ -56,6 +56,7 @@ export { getActiveSpan, getRootSpan, getTraceData, + getTraceMetaTags, startSpan, startInactiveSpan, startSpanManual, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 5c21c8e484ed..73295f7df64c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -83,6 +83,7 @@ export { export { parseSampleRate } from './utils/parseSampleRate'; export { applySdkMetadata } from './utils/sdkMetadata'; export { getTraceData } from './utils/traceData'; +export { getTraceMetaTags } from './utils/meta'; export { DEFAULT_ENVIRONMENT } from './constants'; export { addBreadcrumb } from './breadcrumbs'; export { functionToStringIntegration } from './integrations/functiontostring'; diff --git a/packages/core/src/utils/meta.ts b/packages/core/src/utils/meta.ts new file mode 100644 index 000000000000..339dfcee2f28 --- /dev/null +++ b/packages/core/src/utils/meta.ts @@ -0,0 +1,29 @@ +import type { Client, Scope, Span } from '@sentry/types'; +import { getTraceData } from './traceData'; + +/** + * Returns a string of meta tags that represent the current trace data. + * + * You can use this to propagate a trace from your server-side rendered Html to the browser. + * This function returns up to two meta tags, `sentry-trace` and `baggage`, depending on the + * current trace data state. + * + * @example + * Usage example: + * + * ```js + * function renderHtml() { + * return ` + * + * ${getTraceMetaTags()} + * + * `; + * } + * ``` + * + */ +export function getTraceMetaTags(span?: Span, scope?: Scope, client?: Client): string { + return Object.entries(getTraceData(span, scope, client)) + .map(([key, value]) => ``) + .join('\n'); +} diff --git a/packages/core/test/lib/utils/meta.test.ts b/packages/core/test/lib/utils/meta.test.ts new file mode 100644 index 000000000000..3d78247b8951 --- /dev/null +++ b/packages/core/test/lib/utils/meta.test.ts @@ -0,0 +1,30 @@ +import { getTraceMetaTags } from '../../../src/utils/meta'; +import * as TraceDataModule from '../../../src/utils/traceData'; + +describe('getTraceMetaTags', () => { + it('renders baggage and sentry-trace values to stringified Html meta tags', () => { + jest.spyOn(TraceDataModule, 'getTraceData').mockReturnValueOnce({ + 'sentry-trace': '12345678901234567890123456789012-1234567890123456-1', + baggage: 'sentry-environment=production', + }); + + expect(getTraceMetaTags()).toBe(` +`); + }); + + it('renders just sentry-trace values to stringified Html meta tags', () => { + jest.spyOn(TraceDataModule, 'getTraceData').mockReturnValueOnce({ + 'sentry-trace': '12345678901234567890123456789012-1234567890123456-1', + }); + + expect(getTraceMetaTags()).toBe( + '', + ); + }); + + it('returns an empty string if neither sentry-trace nor baggage values are available', () => { + jest.spyOn(TraceDataModule, 'getTraceData').mockReturnValueOnce({}); + + expect(getTraceMetaTags()).toBe(''); + }); +}); diff --git a/packages/deno/src/index.ts b/packages/deno/src/index.ts index 69b26bb1729a..1f983b476b74 100644 --- a/packages/deno/src/index.ts +++ b/packages/deno/src/index.ts @@ -56,6 +56,7 @@ export { getActiveSpan, getRootSpan, getTraceData, + getTraceMetaTags, startSpan, startInactiveSpan, startSpanManual, diff --git a/packages/google-cloud-serverless/src/index.ts b/packages/google-cloud-serverless/src/index.ts index 351f843d2c2d..73f24f9cf39e 100644 --- a/packages/google-cloud-serverless/src/index.ts +++ b/packages/google-cloud-serverless/src/index.ts @@ -21,6 +21,7 @@ export { getGlobalScope, getIsolationScope, getTraceData, + getTraceMetaTags, setCurrentClient, Scope, SDK_VERSION, diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index badd1f1a27bf..1fdc32d3d77a 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -96,6 +96,7 @@ export { getCurrentScope, getIsolationScope, getTraceData, + getTraceMetaTags, withScope, withIsolationScope, captureException, diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index a74e5bb89dc0..99813d01ceba 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -52,6 +52,7 @@ export { getSpanDescendants, getSpanStatusFromHttpCode, getTraceData, + getTraceMetaTags, graphqlIntegration, hapiIntegration, httpIntegration, diff --git a/packages/vercel-edge/src/index.ts b/packages/vercel-edge/src/index.ts index a96fc15e35d2..8be93345fa3d 100644 --- a/packages/vercel-edge/src/index.ts +++ b/packages/vercel-edge/src/index.ts @@ -56,6 +56,7 @@ export { getActiveSpan, getRootSpan, getTraceData, + getTraceMetaTags, startSpan, startInactiveSpan, startSpanManual, From 6a8e89d88f9b3373e14f7c608c0ca2d7f462c72f Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 6 Aug 2024 10:32:01 -0400 Subject: [PATCH 03/20] chore: Update sdk list in issue template and use issue types (#13211) ref https://github.com/getsentry/sentry-javascript/issues/13210 1. Update sdk list and issue labeling workflow to be up-to-date. 2. Instead of attaching labels, categorize issues with issue types. --- .github/ISSUE_TEMPLATE/bug.yml | 9 +++-- .github/ISSUE_TEMPLATE/feature.yml | 2 +- .github/ISSUE_TEMPLATE/flaky.yml | 1 + .github/ISSUE_TEMPLATE/internal.yml | 4 ++ .github/workflows/issue-package-label.yml | 45 +++++++++++++++++------ 5 files changed, 45 insertions(+), 16 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug.yml b/.github/ISSUE_TEMPLATE/bug.yml index 437191889954..43e4e82c61b0 100644 --- a/.github/ISSUE_TEMPLATE/bug.yml +++ b/.github/ISSUE_TEMPLATE/bug.yml @@ -1,6 +1,6 @@ name: 🐞 Bug Report description: Tell us about something that's not working the way we (probably) intend. -labels: ['Type: Bug'] +type: 'bug' body: - type: checkboxes attributes: @@ -31,20 +31,23 @@ body: setup. options: - '@sentry/browser' - - '@sentry/astro' + - '@sentry/node' - '@sentry/angular' + - '@sentry/astro' - '@sentry/aws-serverless' - '@sentry/bun' + - '@sentry/cloudflare' - '@sentry/deno' - '@sentry/ember' - '@sentry/gatsby' - '@sentry/google-cloud-serverless' - '@sentry/nestjs' - '@sentry/nextjs' - - '@sentry/node' + - '@sentry/nuxt' - '@sentry/react' - '@sentry/remix' - '@sentry/solid' + - '@sentry/solidstart' - '@sentry/svelte' - '@sentry/sveltekit' - '@sentry/vue' diff --git a/.github/ISSUE_TEMPLATE/feature.yml b/.github/ISSUE_TEMPLATE/feature.yml index 7749deee1d44..185de4888de1 100644 --- a/.github/ISSUE_TEMPLATE/feature.yml +++ b/.github/ISSUE_TEMPLATE/feature.yml @@ -1,6 +1,6 @@ name: 💡 Feature Request description: Create a feature request for a sentry-javascript SDK. -labels: ['Type: Improvement'] +type: 'enhancement' body: - type: markdown attributes: diff --git a/.github/ISSUE_TEMPLATE/flaky.yml b/.github/ISSUE_TEMPLATE/flaky.yml index 48e6ce41c047..a679cf98d328 100644 --- a/.github/ISSUE_TEMPLATE/flaky.yml +++ b/.github/ISSUE_TEMPLATE/flaky.yml @@ -1,6 +1,7 @@ name: ❅ Flaky Test description: Report a flaky test in CI title: '[Flaky CI]: ' +type: 'task' labels: ['Type: Tests'] body: - type: dropdown diff --git a/.github/ISSUE_TEMPLATE/internal.yml b/.github/ISSUE_TEMPLATE/internal.yml index bd5b1d1f1970..308d04db7eb5 100644 --- a/.github/ISSUE_TEMPLATE/internal.yml +++ b/.github/ISSUE_TEMPLATE/internal.yml @@ -1,6 +1,10 @@ name: 💡 [Internal] Blank Issue description: Only for Sentry Employees! Create an issue without a template. +type: 'task' body: + - type: markdown + attributes: + value: Make sure to apply relevant labels and issue types before submitting. - type: textarea id: description attributes: diff --git a/.github/workflows/issue-package-label.yml b/.github/workflows/issue-package-label.yml index 2e2f41904f7c..ad5edbd3b398 100644 --- a/.github/workflows/issue-package-label.yml +++ b/.github/workflows/issue-package-label.yml @@ -29,17 +29,26 @@ jobs: # Note: Since this is handled as a regex, and JSON parse wrangles slashes /, we just use `.` instead map: | { + "@sentry.angular": { + "label": "Package: angular" + }, "@sentry.astro": { - "label": "Package: Astro" + "label": "Package: astro" }, - "@sentry.browser": { - "label": "Package: Browser" + "@sentry.aws-serverless": { + "label": "Package: aws-serverless" }, - "@sentry.angular": { - "label": "Package: Angular" + "@sentry.browser": { + "label": "Package: browser" }, "@sentry.bun": { - "label": "Package: Bun" + "label": "Package: bun" + }, + "@sentry.cloudflare": { + "label": "Package: cloudflare" + }, + "@sentry.deno": { + "label": "Package: deno" }, "@sentry.ember": { "label": "Package: ember" @@ -47,11 +56,20 @@ jobs: "@sentry.gatsby": { "label": "Package: gatbsy" }, + "@sentry.google-cloud-serverless": { + "label": "Package: google-cloud-serverless" + }, + "@sentry.nestjs": { + "label": "Package: nestjs" + }, "@sentry.nextjs": { - "label": "Package: Nextjs" + "label": "Package: nextjs" }, "@sentry.node": { - "label": "Package: Node" + "label": "Package: node" + }, + "@sentry.nuxt": { + "label": "Package: nuxt" }, "@sentry.react": { "label": "Package: react" @@ -59,15 +77,18 @@ jobs: "@sentry.remix": { "label": "Package: remix" }, - "@sentry.serverless": { - "label": "Package: Serverless" + "@sentry.solid": { + "label": "Package: solid" }, - "@sentry.sveltekit": { - "label": "Package: SvelteKit" + "@sentry.solid": { + "label": "Package: solidstart" }, "@sentry.svelte": { "label": "Package: svelte" }, + "@sentry.sveltekit": { + "label": "Package: sveltekit" + }, "@sentry.vue": { "label": "Package: vue" }, From a09998029e2ebe55c1864559a6176e9e18e0d4a0 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 6 Aug 2024 10:32:11 -0400 Subject: [PATCH 04/20] test(browser): Fix flaky test in test/utils/lazyLoadIntegration.test.ts (#13213) Fixes https://github.com/getsentry/sentry-javascript/issues/13184 We can now await here because we use `vitest`, so there should be no jsdom problems :) But really these tests should live in playwright instead. --- packages/browser/test/utils/lazyLoadIntegration.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/browser/test/utils/lazyLoadIntegration.test.ts b/packages/browser/test/utils/lazyLoadIntegration.test.ts index ec88ae49a1a9..82548a59faec 100644 --- a/packages/browser/test/utils/lazyLoadIntegration.test.ts +++ b/packages/browser/test/utils/lazyLoadIntegration.test.ts @@ -71,9 +71,11 @@ describe('lazyLoadIntegration', () => { httpClientIntegration: undefined, }; - // We do not await here, as this this does not seem to work with JSDOM :( - // We have browser integration tests to check that this actually works - void lazyLoadIntegration('httpClientIntegration'); + try { + await lazyLoadIntegration('httpClientIntegration'); + } catch { + // skip + } expect(global.document.querySelectorAll('script')).toHaveLength(1); expect(global.document.querySelector('script')?.src).toEqual( From 0666eb5e886d909b48d010c7028385d118589b5d Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 6 Aug 2024 10:33:34 -0400 Subject: [PATCH 05/20] test(nestjs): Switch to explicit vitest imports (#13214) 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 Nestjs SDK tests to do so. ref https://github.com/getsentry/sentry-javascript/issues/11084 --- packages/nestjs/test/sdk.test.ts | 3 ++- packages/nestjs/tsconfig.test.json | 3 --- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/nestjs/test/sdk.test.ts b/packages/nestjs/test/sdk.test.ts index c6bf7166444d..77d19fa2c797 100644 --- a/packages/nestjs/test/sdk.test.ts +++ b/packages/nestjs/test/sdk.test.ts @@ -1,7 +1,8 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + import * as SentryNode from '@sentry/node'; import { SDK_VERSION } from '@sentry/utils'; -import { vi } from 'vitest'; import { init as nestInit } from '../src/sdk'; const nodeInit = vi.spyOn(SentryNode, 'init'); diff --git a/packages/nestjs/tsconfig.test.json b/packages/nestjs/tsconfig.test.json index fc9e549d35ce..00cada2d8bcf 100644 --- a/packages/nestjs/tsconfig.test.json +++ b/packages/nestjs/tsconfig.test.json @@ -4,9 +4,6 @@ "include": ["test/**/*", "vite.config.ts"], "compilerOptions": { - // should include all types from `./tsconfig.json` plus types for all test frameworks used - "types": ["vitest/globals"] - // other package-specific, test-specific options } } From b0d4926c5459f74bd9f59cc085fbdaffa0cda159 Mon Sep 17 00:00:00 2001 From: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com> Date: Tue, 6 Aug 2024 16:47:15 +0200 Subject: [PATCH 06/20] feat(astro): Add `bundleSizeOptimizations` vite options to integration (#13250) The bundler-plugins still refer to the option as `excludePerformanceMonitoring` ([here](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/main/packages/bundler-plugin-core/src/types.ts#L260)), but this is going to be renamed to `excludeTracing`, so I already used the new naming as discussed with @Lms24 and @mydea. part of https://github.com/getsentry/sentry-javascript/issues/13013 --- packages/astro/src/integration/index.ts | 37 +++-- packages/astro/src/integration/snippets.ts | 4 +- packages/astro/src/integration/types.ts | 149 ++++++++++++------ packages/astro/test/integration/index.test.ts | 4 + .../astro/test/integration/snippets.test.ts | 22 ++- 5 files changed, 147 insertions(+), 69 deletions(-) diff --git a/packages/astro/src/integration/index.ts b/packages/astro/src/integration/index.ts index 9baee4d9dc39..0a367a1bdf5d 100644 --- a/packages/astro/src/integration/index.ts +++ b/packages/astro/src/integration/index.ts @@ -3,6 +3,7 @@ import * as path from 'path'; import { sentryVitePlugin } from '@sentry/vite-plugin'; import type { AstroConfig, AstroIntegration } from 'astro'; +import { dropUndefinedKeys } from '@sentry/utils'; import { buildClientSnippet, buildSdkInitFileImportSnippet, buildServerSnippet } from './snippets'; import type { SentryOptions } from './types'; @@ -40,21 +41,29 @@ export const sentryAstro = (options: SentryOptions = {}): AstroIntegration => { sourcemap: true, }, plugins: [ - sentryVitePlugin({ - org: uploadOptions.org ?? env.SENTRY_ORG, - project: uploadOptions.project ?? env.SENTRY_PROJECT, - authToken: uploadOptions.authToken ?? env.SENTRY_AUTH_TOKEN, - telemetry: uploadOptions.telemetry ?? true, - sourcemaps: { - assets: uploadOptions.assets ?? [getSourcemapsAssetsGlob(config)], - }, - _metaOptions: { - telemetry: { - metaFramework: 'astro', + sentryVitePlugin( + dropUndefinedKeys({ + org: uploadOptions.org ?? env.SENTRY_ORG, + project: uploadOptions.project ?? env.SENTRY_PROJECT, + authToken: uploadOptions.authToken ?? env.SENTRY_AUTH_TOKEN, + telemetry: uploadOptions.telemetry ?? true, + sourcemaps: { + assets: uploadOptions.assets ?? [getSourcemapsAssetsGlob(config)], }, - }, - debug: options.debug ?? false, - }), + bundleSizeOptimizations: { + ...options.bundleSizeOptimizations, + // TODO: with a future version of the vite plugin (probably 2.22.0) this re-mapping is not needed anymore + // ref: https://github.com/getsentry/sentry-javascript-bundler-plugins/pull/582 + excludePerformanceMonitoring: options.bundleSizeOptimizations?.excludeTracing, + }, + _metaOptions: { + telemetry: { + metaFramework: 'astro', + }, + }, + debug: options.debug ?? false, + }), + ), ], }, }); diff --git a/packages/astro/src/integration/snippets.ts b/packages/astro/src/integration/snippets.ts index c46267080aa8..4b784170d330 100644 --- a/packages/astro/src/integration/snippets.ts +++ b/packages/astro/src/integration/snippets.ts @@ -47,7 +47,7 @@ const buildCommonInitOptions = (options: SentryOptions): string => `dsn: ${ }`; /** - * We don't include the `BrowserTracing` integration if the tracesSampleRate is set to 0. + * We don't include the `BrowserTracing` integration if `bundleSizeOptimizations.excludeTracing` is falsy. * Likewise, we don't include the `Replay` integration if the replaysSessionSampleRate * and replaysOnErrorSampleRate are set to 0. * @@ -56,7 +56,7 @@ const buildCommonInitOptions = (options: SentryOptions): string => `dsn: ${ const buildClientIntegrations = (options: SentryOptions): string => { const integrations: string[] = []; - if (options.tracesSampleRate == null || options.tracesSampleRate) { + if (!options.bundleSizeOptimizations?.excludeTracing) { integrations.push('Sentry.browserTracingIntegration()'); } diff --git a/packages/astro/src/integration/types.ts b/packages/astro/src/integration/types.ts index f51a020bb290..026fcd01d8c4 100644 --- a/packages/astro/src/integration/types.ts +++ b/packages/astro/src/integration/types.ts @@ -25,62 +25,95 @@ type SdkInitPaths = { type SourceMapsOptions = { /** - * Options for the Sentry Vite plugin to customize the source maps upload process. + * If this flag is `true`, and an auth token is detected, the Sentry integration will + * automatically generate and upload source maps to Sentry during a production build. * - * These options are always read from the `sentryAstro` integration. - * Do not define them in the `sentry.client.config.(js|ts)` or `sentry.server.config.(js|ts)` files. + * @default true */ - sourceMapsUploadOptions?: { - /** - * If this flag is `true`, and an auth token is detected, the Sentry integration will - * automatically generate and upload source maps to Sentry during a production build. - * - * @default true - */ - enabled?: boolean; + enabled?: boolean; - /** - * The auth token to use when uploading source maps to Sentry. - * - * Instead of specifying this option, you can also set the `SENTRY_AUTH_TOKEN` environment variable. - * - * To create an auth token, follow this guide: - * @see https://docs.sentry.io/product/accounts/auth-tokens/#organization-auth-tokens - */ - authToken?: string; + /** + * The auth token to use when uploading source maps to Sentry. + * + * Instead of specifying this option, you can also set the `SENTRY_AUTH_TOKEN` environment variable. + * + * To create an auth token, follow this guide: + * @see https://docs.sentry.io/product/accounts/auth-tokens/#organization-auth-tokens + */ + authToken?: string; - /** - * The organization slug of your Sentry organization. - * Instead of specifying this option, you can also set the `SENTRY_ORG` environment variable. - */ - org?: string; + /** + * The organization slug of your Sentry organization. + * Instead of specifying this option, you can also set the `SENTRY_ORG` environment variable. + */ + org?: string; - /** - * The project slug of your Sentry project. - * Instead of specifying this option, you can also set the `SENTRY_PROJECT` environment variable. - */ - project?: string; + /** + * The project slug of your Sentry project. + * Instead of specifying this option, you can also set the `SENTRY_PROJECT` environment variable. + */ + project?: string; - /** - * If this flag is `true`, the Sentry plugin will collect some telemetry data and send it to Sentry. - * It will not collect any sensitive or user-specific data. - * - * @default true - */ - telemetry?: boolean; + /** + * If this flag is `true`, the Sentry plugin will collect some telemetry data and send it to Sentry. + * It will not collect any sensitive or user-specific data. + * + * @default true + */ + telemetry?: boolean; - /** - * A glob or an array of globs that specify the build artifacts and source maps that will be uploaded to Sentry. - * - * If this option is not specified, sensible defaults based on your `outDir`, `rootDir` and `adapter` - * config will be used. Use this option to override these defaults, for instance if you have a - * customized build setup that diverges from Astro's defaults. - * - * The globbing patterns must follow the implementation of the `glob` package. - * @see https://www.npmjs.com/package/glob#glob-primer - */ - assets?: string | Array; - }; + /** + * A glob or an array of globs that specify the build artifacts and source maps that will be uploaded to Sentry. + * + * If this option is not specified, sensible defaults based on your `outDir`, `rootDir` and `adapter` + * config will be used. Use this option to override these defaults, for instance if you have a + * customized build setup that diverges from Astro's defaults. + * + * The globbing patterns must follow the implementation of the `glob` package. + * @see https://www.npmjs.com/package/glob#glob-primer + */ + assets?: string | Array; +}; + +type BundleSizeOptimizationOptions = { + /** + * If set to `true`, the plugin will attempt to tree-shake (remove) any debugging code within the Sentry SDK. + * Note that the success of this depends on tree shaking being enabled in your build tooling. + * + * Setting this option to `true` will disable features like the SDK's `debug` option. + */ + excludeDebugStatements?: boolean; + + /** + * If set to true, the plugin will try to tree-shake performance monitoring statements out. + * Note that the success of this depends on tree shaking generally being enabled in your build. + * Attention: DO NOT enable this when you're using any performance monitoring-related SDK features (e.g. Sentry.startTransaction()). + */ + excludeTracing?: boolean; + + /** + * If set to `true`, the plugin will attempt to tree-shake (remove) code related to the Sentry SDK's Session Replay Shadow DOM recording functionality. + * Note that the success of this depends on tree shaking being enabled in your build tooling. + * + * This option is safe to be used when you do not want to capture any Shadow DOM activity via Sentry Session Replay. + */ + excludeReplayShadowDom?: boolean; + + /** + * If set to `true`, the plugin will attempt to tree-shake (remove) code related to the Sentry SDK's Session Replay `iframe` recording functionality. + * Note that the success of this depends on tree shaking being enabled in your build tooling. + * + * You can safely do this when you do not want to capture any `iframe` activity via Sentry Session Replay. + */ + excludeReplayIframe?: boolean; + + /** + * If set to `true`, the plugin will attempt to tree-shake (remove) code related to the Sentry SDK's Session Replay's Compression Web Worker. + * Note that the success of this depends on tree shaking being enabled in your build tooling. + * + * **Notice:** You should only do use this option if you manually host a compression worker and configure it in your Sentry Session Replay integration config via the `workerUrl` option. + */ + excludeReplayWorker?: boolean; }; type InstrumentationOptions = { @@ -138,6 +171,20 @@ type SdkEnabledOptions = { export type SentryOptions = SdkInitPaths & Pick & Pick & - SourceMapsOptions & InstrumentationOptions & - SdkEnabledOptions; + SdkEnabledOptions & { + /** + * Options for the Sentry Vite plugin to customize the source maps upload process. + * + * These options are always read from the `sentryAstro` integration. + * Do not define them in the `sentry.client.config.(js|ts)` or `sentry.server.config.(js|ts)` files. + */ + sourceMapsUploadOptions?: SourceMapsOptions; + /** + * Options for the Sentry Vite plugin to customize bundle size optimizations. + * + * These options are always read from the `sentryAstro` integration. + * Do not define them in the `sentry.client.config.(js|ts)` or `sentry.server.config.(js|ts)` files. + */ + bundleSizeOptimizations?: BundleSizeOptimizationOptions; + }; diff --git a/packages/astro/test/integration/index.test.ts b/packages/astro/test/integration/index.test.ts index 008132264602..eb6bdf555ae3 100644 --- a/packages/astro/test/integration/index.test.ts +++ b/packages/astro/test/integration/index.test.ts @@ -57,6 +57,7 @@ describe('sentryAstro integration', () => { project: 'my-project', telemetry: false, debug: false, + bundleSizeOptimizations: {}, sourcemaps: { assets: ['out/**/*'], }, @@ -82,6 +83,7 @@ describe('sentryAstro integration', () => { project: 'my-project', telemetry: false, debug: false, + bundleSizeOptimizations: {}, sourcemaps: { assets: ['dist/**/*'], }, @@ -114,6 +116,7 @@ describe('sentryAstro integration', () => { project: 'my-project', telemetry: false, debug: false, + bundleSizeOptimizations: {}, sourcemaps: { assets: ['{.vercel,dist}/**/*'], }, @@ -151,6 +154,7 @@ describe('sentryAstro integration', () => { project: 'my-project', telemetry: true, debug: false, + bundleSizeOptimizations: {}, sourcemaps: { assets: ['dist/server/**/*, dist/client/**/*'], }, diff --git a/packages/astro/test/integration/snippets.test.ts b/packages/astro/test/integration/snippets.test.ts index 04aaa866aee9..66a3b15efd58 100644 --- a/packages/astro/test/integration/snippets.test.ts +++ b/packages/astro/test/integration/snippets.test.ts @@ -52,7 +52,25 @@ describe('buildClientSnippet', () => { `); }); - it('does not include browserTracingIntegration if tracesSampleRate is 0', () => { + it('does not include browserTracingIntegration if bundleSizeOptimizations.excludeTracing is true', () => { + const snippet = buildClientSnippet({ bundleSizeOptimizations: { excludeTracing: true } }); + expect(snippet).toMatchInlineSnapshot(` + "import * as Sentry from "@sentry/astro"; + + Sentry.init({ + dsn: import.meta.env.PUBLIC_SENTRY_DSN, + debug: false, + environment: import.meta.env.PUBLIC_VERCEL_ENV, + release: import.meta.env.PUBLIC_VERCEL_GIT_COMMIT_SHA, + tracesSampleRate: 1, + integrations: [Sentry.replayIntegration()], + replaysSessionSampleRate: 0.1, + replaysOnErrorSampleRate: 1, + });" + `); + }); + + it('still include browserTracingIntegration if tracesSampleRate is 0', () => { const snippet = buildClientSnippet({ tracesSampleRate: 0 }); expect(snippet).toMatchInlineSnapshot(` "import * as Sentry from "@sentry/astro"; @@ -63,7 +81,7 @@ describe('buildClientSnippet', () => { environment: import.meta.env.PUBLIC_VERCEL_ENV, release: import.meta.env.PUBLIC_VERCEL_GIT_COMMIT_SHA, tracesSampleRate: 0, - integrations: [Sentry.replayIntegration()], + integrations: [Sentry.browserTracingIntegration(), Sentry.replayIntegration()], replaysSessionSampleRate: 0.1, replaysOnErrorSampleRate: 1, });" From 8fb3f241b8ae845824354dfd933193993e2dcac3 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 6 Aug 2024 16:52:57 +0200 Subject: [PATCH 07/20] test: Fix flakey integration test (#13253) --- .../suites/express/multiple-init/server.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/dev-packages/node-integration-tests/suites/express/multiple-init/server.ts b/dev-packages/node-integration-tests/suites/express/multiple-init/server.ts index aade87fe3bcf..4d1625035ebf 100644 --- a/dev-packages/node-integration-tests/suites/express/multiple-init/server.ts +++ b/dev-packages/node-integration-tests/suites/express/multiple-init/server.ts @@ -52,7 +52,17 @@ app.get('/test/error/:id', (req, res) => { Sentry.captureException(new Error(`This is an exception ${id}`)); - res.send({}); + setTimeout(() => { + // We flush to ensure we are sending exceptions in a certain order + Sentry.flush(3000).then( + () => { + res.send({}); + }, + () => { + res.send({}); + }, + ); + }, 1000); }); Sentry.setupExpressErrorHandler(app); From 69d48239cf11b20557748fa82b207fbfc2abd2a5 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 6 Aug 2024 17:50:12 +0200 Subject: [PATCH 08/20] chore(lint): Allow `ts-ignore` in Node integration tests (#13254) Discovered today that TS 3.8 doesn't understand `// @ts-expect-error`. Since we test on Node 22 also with TS 3.8, we shouldn't use `ts-expect-error` in test files as TS 3.8 would simply ignore the comment and throw a type error. Instead, it's fine to use `@ts-ignore` in these test files. --- dev-packages/node-integration-tests/.eslintrc.js | 9 +++++++++ .../suites/tracing/meta-tags/test.ts | 3 +-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/dev-packages/node-integration-tests/.eslintrc.js b/dev-packages/node-integration-tests/.eslintrc.js index df04aa267446..51b7dfbb7ed3 100644 --- a/dev-packages/node-integration-tests/.eslintrc.js +++ b/dev-packages/node-integration-tests/.eslintrc.js @@ -20,6 +20,15 @@ module.exports = { }, rules: { '@typescript-eslint/typedef': 'off', + // Explicitly allow ts-ignore with description for Node integration tests + // Reason: We run these tests on TS3.8 which doesn't support `@ts-expect-error` + '@typescript-eslint/ban-ts-comment': [ + 'error', + { + 'ts-ignore': 'allow-with-description', + 'ts-expect-error': true, + }, + ], }, }, ], diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags/test.ts b/dev-packages/node-integration-tests/suites/tracing/meta-tags/test.ts index 22bef9d2b1b9..c42269dd8504 100644 --- a/dev-packages/node-integration-tests/suites/tracing/meta-tags/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags/test.ts @@ -16,8 +16,7 @@ describe('getTraceMetaTags', () => { baggage: 'sentry-environment=production', }); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore + // @ts-ignore - response is defined, types just don't reflect it const html = response?.response as unknown as string; expect(html).toMatch(//); From 9ed211288650d3d4740edba5ed1f609bb75b2480 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 6 Aug 2024 17:57:32 +0200 Subject: [PATCH 09/20] test: Unflake slow click test (#13252) --- .github/workflows/flaky-test-detector.yml | 4 +- .../suites/replay/slowClick/mutation/test.ts | 298 +++++++++--------- 2 files changed, 154 insertions(+), 148 deletions(-) diff --git a/.github/workflows/flaky-test-detector.yml b/.github/workflows/flaky-test-detector.yml index d2238ce58e71..5d0d5af5d247 100644 --- a/.github/workflows/flaky-test-detector.yml +++ b/.github/workflows/flaky-test-detector.yml @@ -68,10 +68,10 @@ jobs: CHANGED_TEST_PATHS: ${{ steps.changed.outputs.browser_integration_files }} TEST_RUN_COUNT: 'AUTO' - - name: Artifacts upload + - name: Upload Playwright Traces uses: actions/upload-artifact@v4 if: failure() && steps.test.outcome == 'failure' with: name: playwright-test-results - path: test-results + path: dev-packages/browser-integration-tests/test-results retention-days: 5 diff --git a/dev-packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts b/dev-packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts index ba3dd43ac3d3..aafdced81505 100644 --- a/dev-packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts @@ -18,49 +18,51 @@ sentryTest('mutation after threshold results in slow click', async ({ forceFlush const url = await getLocalTestUrl({ testDir: __dirname }); - await Promise.all([waitForReplayRequest(page, 0), page.goto(url)]); - await forceFlushReplay(); + const replayRequestPromise = waitForReplayRequest(page, 0); + + const segmentReqWithSlowClickBreadcrumbPromise = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); + }); - const [req1] = await Promise.all([ - waitForReplayRequest(page, (event, res) => { - const { breadcrumbs } = getCustomRecordingEvents(res); + await page.goto(url); + await replayRequestPromise; - return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); - }), + await forceFlushReplay(); + + await page.locator('#mutationButton').click(); - page.locator('#mutationButton').click(), - ]); + const segmentReqWithSlowClick = await segmentReqWithSlowClickBreadcrumbPromise; - const { breadcrumbs } = getCustomRecordingEvents(req1); + const { breadcrumbs } = getCustomRecordingEvents(segmentReqWithSlowClick); const slowClickBreadcrumbs = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); - expect(slowClickBreadcrumbs).toEqual([ - { - category: 'ui.slowClickDetected', - type: 'default', - data: { - endReason: 'mutation', - clickCount: 1, - node: { - attributes: { - id: 'mutationButton', - }, - id: expect.any(Number), - tagName: 'button', - textContent: '******* ********', + expect(slowClickBreadcrumbs).toContainEqual({ + category: 'ui.slowClickDetected', + type: 'default', + data: { + endReason: 'mutation', + clickCount: 1, + node: { + attributes: { + id: 'mutationButton', }, - nodeId: expect.any(Number), - timeAfterClickMs: expect.any(Number), - url: 'http://sentry-test.io/index.html', + id: expect.any(Number), + tagName: 'button', + textContent: '******* ********', }, - message: 'body > button#mutationButton', - timestamp: expect.any(Number), + nodeId: expect.any(Number), + timeAfterClickMs: expect.any(Number), + url: 'http://sentry-test.io/index.html', }, - ]); + message: 'body > button#mutationButton', + timestamp: expect.any(Number), + }); expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeGreaterThan(3000); - expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeLessThan(3500); + expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeLessThan(3501); }); sentryTest('multiple clicks are counted', async ({ getLocalTestUrl, page }) => { @@ -78,49 +80,50 @@ sentryTest('multiple clicks are counted', async ({ getLocalTestUrl, page }) => { const url = await getLocalTestUrl({ testDir: __dirname }); - await Promise.all([waitForReplayRequest(page, 0), page.goto(url)]); + const replayRequestPromise = waitForReplayRequest(page, 0); + const segmentReqWithSlowClickBreadcrumbPromise = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); - const [req1] = await Promise.all([ - waitForReplayRequest(page, (event, res) => { - const { breadcrumbs } = getCustomRecordingEvents(res); + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); + }); - return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); - }), - page.locator('#mutationButton').click({ clickCount: 4 }), - ]); + await page.goto(url); + await replayRequestPromise; - const { breadcrumbs } = getCustomRecordingEvents(req1); + await page.locator('#mutationButton').click({ clickCount: 4 }); + + const segmentReqWithSlowClick = await segmentReqWithSlowClickBreadcrumbPromise; + + const { breadcrumbs } = getCustomRecordingEvents(segmentReqWithSlowClick); const slowClickBreadcrumbs = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.slowClickDetected'); const multiClickBreadcrumbs = breadcrumbs.filter(breadcrumb => breadcrumb.category === 'ui.multiClick'); - expect(slowClickBreadcrumbs).toEqual([ - { - category: 'ui.slowClickDetected', - type: 'default', - data: { - endReason: 'mutation', - clickCount: 4, - node: { - attributes: { - id: 'mutationButton', - }, - id: expect.any(Number), - tagName: 'button', - textContent: '******* ********', + expect(slowClickBreadcrumbs).toContainEqual({ + category: 'ui.slowClickDetected', + type: 'default', + data: { + endReason: expect.stringMatching(/^(mutation|timeout)$/), + clickCount: 4, + node: { + attributes: { + id: 'mutationButton', }, - nodeId: expect.any(Number), - timeAfterClickMs: expect.any(Number), - url: 'http://sentry-test.io/index.html', + id: expect.any(Number), + tagName: 'button', + textContent: '******* ********', }, - message: 'body > button#mutationButton', - timestamp: expect.any(Number), + nodeId: expect.any(Number), + timeAfterClickMs: expect.any(Number), + url: 'http://sentry-test.io/index.html', }, - ]); + message: 'body > button#mutationButton', + timestamp: expect.any(Number), + }); expect(multiClickBreadcrumbs.length).toEqual(0); expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeGreaterThan(3000); - expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeLessThan(3500); + expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeLessThan(3501); }); sentryTest('immediate mutation does not trigger slow click', async ({ forceFlushReplay, getLocalTestUrl, page }) => { @@ -138,7 +141,15 @@ sentryTest('immediate mutation does not trigger slow click', async ({ forceFlush const url = await getLocalTestUrl({ testDir: __dirname }); - await Promise.all([waitForReplayRequest(page, 0), page.goto(url)]); + const replayRequestPromise = waitForReplayRequest(page, 0); + const segmentReqWithClickBreadcrumbPromise = waitForReplayRequest(page, (_event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); + }); + + await page.goto(url); + await replayRequestPromise; await forceFlushReplay(); let slowClickCount = 0; @@ -150,36 +161,29 @@ sentryTest('immediate mutation does not trigger slow click', async ({ forceFlush slowClickCount += slowClicks.length; }); - const [req1] = await Promise.all([ - waitForReplayRequest(page, (_event, res) => { - const { breadcrumbs } = getCustomRecordingEvents(res); - - return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); - }), - page.locator('#mutationButtonImmediately').click(), - ]); - - const { breadcrumbs } = getCustomRecordingEvents(req1); - - expect(breadcrumbs).toEqual([ - { - category: 'ui.click', - data: { - node: { - attributes: { - id: 'mutationButtonImmediately', - }, - id: expect.any(Number), - tagName: 'button', - textContent: '******* ******** ***********', + await page.locator('#mutationButtonImmediately').click(); + + const segmentReqWithSlowClick = await segmentReqWithClickBreadcrumbPromise; + + const { breadcrumbs } = getCustomRecordingEvents(segmentReqWithSlowClick); + + expect(breadcrumbs).toContainEqual({ + category: 'ui.click', + data: { + node: { + attributes: { + id: 'mutationButtonImmediately', }, - nodeId: expect.any(Number), + id: expect.any(Number), + tagName: 'button', + textContent: '******* ******** ***********', }, - message: 'body > button#mutationButtonImmediately', - timestamp: expect.any(Number), - type: 'default', + nodeId: expect.any(Number), }, - ]); + message: 'body > button#mutationButtonImmediately', + timestamp: expect.any(Number), + type: 'default', + }); // Ensure we wait for timeout, to make sure no slow click is created // Waiting for 3500 + 1s rounding room @@ -204,39 +208,41 @@ sentryTest('inline click handler does not trigger slow click', async ({ forceFlu const url = await getLocalTestUrl({ testDir: __dirname }); - await Promise.all([waitForReplayRequest(page, 0), page.goto(url)]); + const replayRequestPromise = waitForReplayRequest(page, 0); + const segmentReqWithClickBreadcrumbPromise = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); + }); + + await page.goto(url); + await replayRequestPromise; + await forceFlushReplay(); - const [req1] = await Promise.all([ - waitForReplayRequest(page, (event, res) => { - const { breadcrumbs } = getCustomRecordingEvents(res); - - return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); - }), - page.locator('#mutationButtonInline').click(), - ]); - - const { breadcrumbs } = getCustomRecordingEvents(req1); - - expect(breadcrumbs).toEqual([ - { - category: 'ui.click', - data: { - node: { - attributes: { - id: 'mutationButtonInline', - }, - id: expect.any(Number), - tagName: 'button', - textContent: '******* ******** ***********', + await page.locator('#mutationButtonInline').click(); + + const segmentReqWithClick = await segmentReqWithClickBreadcrumbPromise; + + const { breadcrumbs } = getCustomRecordingEvents(segmentReqWithClick); + + expect(breadcrumbs).toContainEqual({ + category: 'ui.click', + data: { + node: { + attributes: { + id: 'mutationButtonInline', }, - nodeId: expect.any(Number), + id: expect.any(Number), + tagName: 'button', + textContent: '******* ******** ***********', }, - message: 'body > button#mutationButtonInline', - timestamp: expect.any(Number), - type: 'default', + nodeId: expect.any(Number), }, - ]); + message: 'body > button#mutationButtonInline', + timestamp: expect.any(Number), + type: 'default', + }); }); sentryTest('mouseDown events are considered', async ({ getLocalTestUrl, page }) => { @@ -254,36 +260,36 @@ sentryTest('mouseDown events are considered', async ({ getLocalTestUrl, page }) const url = await getLocalTestUrl({ testDir: __dirname }); - await Promise.all([waitForReplayRequest(page, 0), page.goto(url)]); - - const [req1] = await Promise.all([ - waitForReplayRequest(page, (event, res) => { - const { breadcrumbs } = getCustomRecordingEvents(res); - - return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); - }), - page.locator('#mouseDownButton').click(), - ]); - - const { breadcrumbs } = getCustomRecordingEvents(req1); - - expect(breadcrumbs).toEqual([ - { - category: 'ui.click', - data: { - node: { - attributes: { - id: 'mouseDownButton', - }, - id: expect.any(Number), - tagName: 'button', - textContent: '******* ******** ** ***** ****', + const replayRequestPromise = waitForReplayRequest(page, 0); + const segmentReqWithClickBreadcrumbPromise = waitForReplayRequest(page, (event, res) => { + const { breadcrumbs } = getCustomRecordingEvents(res); + + return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click'); + }); + + await page.goto(url); + await replayRequestPromise; + + await page.locator('#mouseDownButton').click(); + const segmentReqWithClick = await segmentReqWithClickBreadcrumbPromise; + + const { breadcrumbs } = getCustomRecordingEvents(segmentReqWithClick); + + expect(breadcrumbs).toContainEqual({ + category: 'ui.click', + data: { + node: { + attributes: { + id: 'mouseDownButton', }, - nodeId: expect.any(Number), + id: expect.any(Number), + tagName: 'button', + textContent: '******* ******** ** ***** ****', }, - message: 'body > button#mouseDownButton', - timestamp: expect.any(Number), - type: 'default', + nodeId: expect.any(Number), }, - ]); + message: 'body > button#mouseDownButton', + timestamp: expect.any(Number), + type: 'default', + }); }); From 061042a4fc43ca4c51bbf9e33f5f91b106b99351 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 7 Aug 2024 14:50:39 +0200 Subject: [PATCH 10/20] fix(browser): Initialize default integration if `defaultIntegrations: undefined` (#13261) If users or our higher-level SDKs pass `defaultIntegrations: undefined` to the Browser SDK's init options, it deactivates all default integrations. As per our docs, this should only happen if you explicitly pass `defaultIntegrations: false`. This PR fixes this by removing the `defaultIntegrations` key from the user options object before merging the options together. --------- Co-authored-by: Francesco Novy --- .size-limit.js | 2 +- packages/browser/src/sdk.ts | 8 ++++++++ packages/browser/test/sdk.test.ts | 14 ++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/.size-limit.js b/.size-limit.js index 72050f7225f3..437e466a89e1 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -22,7 +22,7 @@ module.exports = [ path: 'packages/browser/build/npm/esm/index.js', import: createImport('init', 'browserTracingIntegration', 'replayIntegration'), gzip: true, - limit: '72 KB', + limit: '73 KB', }, { name: '@sentry/browser (incl. Tracing, Replay) - with treeshaking flags', diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index 1421814ae9e5..04aa82b5f0e6 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -57,6 +57,14 @@ function applyDefaultOptions(optionsArg: BrowserOptions = {}): BrowserOptions { sendClientReports: true, }; + // TODO: Instead of dropping just `defaultIntegrations`, we should simply + // call `dropUndefinedKeys` on the entire `optionsArg`. + // However, for this to work we need to adjust the `hasTracingEnabled()` logic + // first as it differentiates between `undefined` and the key not being in the object. + if (optionsArg.defaultIntegrations == null) { + delete optionsArg.defaultIntegrations; + } + return { ...defaultOptions, ...optionsArg }; } diff --git a/packages/browser/test/sdk.test.ts b/packages/browser/test/sdk.test.ts index 80e54e3d49d2..618333532a09 100644 --- a/packages/browser/test/sdk.test.ts +++ b/packages/browser/test/sdk.test.ts @@ -6,6 +6,7 @@ import type { Mock } from 'vitest'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as SentryCore from '@sentry/core'; import { Scope, createTransport } from '@sentry/core'; import type { Client, Integration } from '@sentry/types'; import { resolvedSyncPromise } from '@sentry/utils'; @@ -79,6 +80,18 @@ describe('init', () => { expect(DEFAULT_INTEGRATIONS[1]!.setupOnce as Mock).toHaveBeenCalledTimes(1); }); + it('installs default integrations if `defaultIntegrations: undefined`', () => { + // @ts-expect-error this is fine for testing + const initAndBindSpy = vi.spyOn(SentryCore, 'initAndBind').mockImplementationOnce(() => {}); + const options = getDefaultBrowserOptions({ dsn: PUBLIC_DSN, defaultIntegrations: undefined }); + init(options); + + expect(initAndBindSpy).toHaveBeenCalledTimes(1); + + const optionsPassed = initAndBindSpy.mock.calls[0]?.[1]; + expect(optionsPassed?.integrations?.length).toBeGreaterThan(0); + }); + test("doesn't install default integrations if told not to", () => { const DEFAULT_INTEGRATIONS: Integration[] = [ new MockIntegration('MockIntegration 0.3'), @@ -150,6 +163,7 @@ describe('init', () => { Object.defineProperty(WINDOW, 'browser', { value: undefined, writable: true }); Object.defineProperty(WINDOW, 'nw', { value: undefined, writable: true }); Object.defineProperty(WINDOW, 'window', { value: WINDOW, writable: true }); + vi.clearAllMocks(); }); it('logs a browser extension error if executed inside a Chrome extension', () => { From b71d0fd15aff0c8788434c9dc99aebe14a9af79f Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Wed, 7 Aug 2024 16:27:26 +0200 Subject: [PATCH 11/20] feat(nestjs): Automatic instrumentation of nestjs exception filters (#13230) Adds automatic instrumentation of exception filters to `@sentry/nestjs`. Exception filters in nest have a `@Catch` decorator and implement a `catch` function. So we can use that to attach an instrumentation proxy. --- .../tests/transactions.test.ts | 74 +++++++++++++++++++ packages/nestjs/src/setup.ts | 4 + .../src/integrations/tracing/nest/helpers.ts | 6 +- .../nest/sentry-nest-instrumentation.ts | 58 ++++++++++++++- .../src/integrations/tracing/nest/types.ts | 12 +++ 5 files changed, 147 insertions(+), 7 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nestjs-with-submodules/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-with-submodules/tests/transactions.test.ts index 887284585ae1..9217249faad0 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-with-submodules/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-with-submodules/tests/transactions.test.ts @@ -121,3 +121,77 @@ test('Sends an API route transaction from module', async ({ baseURL }) => { }), ); }); + +test('API route transaction includes exception filter span for global filter', async ({ baseURL }) => { + const transactionEventPromise = waitForTransaction('nestjs-with-submodules', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /example-module/expected-exception' && + transactionEvent?.request?.url?.includes('/example-module/expected-exception') + ); + }); + + const response = await fetch(`${baseURL}/example-module/expected-exception`); + expect(response.status).toBe(400); + + const transactionEvent = await transactionEventPromise; + + 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: 'ExampleExceptionFilter', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); +}); + +test('API route transaction includes exception filter span for local filter', async ({ baseURL }) => { + const transactionEventPromise = waitForTransaction('nestjs-with-submodules', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /example-module-local-filter/expected-exception' && + transactionEvent?.request?.url?.includes('/example-module-local-filter/expected-exception') + ); + }); + + const response = await fetch(`${baseURL}/example-module-local-filter/expected-exception`); + expect(response.status).toBe(400); + + const transactionEvent = await transactionEventPromise; + + 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: 'LocalExampleExceptionFilter', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); +}); diff --git a/packages/nestjs/src/setup.ts b/packages/nestjs/src/setup.ts index b068ed052a91..5e76f5cbe912 100644 --- a/packages/nestjs/src/setup.ts +++ b/packages/nestjs/src/setup.ts @@ -64,6 +64,8 @@ export { SentryTracingInterceptor }; * Global filter to handle exceptions and report them to Sentry. */ class SentryGlobalFilter extends BaseExceptionFilter { + public static readonly __SENTRY_INTERNAL__ = true; + /** * Catches exceptions and reports them to Sentry unless they are expected errors. */ @@ -84,6 +86,8 @@ export { SentryGlobalFilter }; * Service to set up Sentry performance tracing for Nest.js applications. */ class SentryService implements OnModuleInit { + public static readonly __SENTRY_INTERNAL__ = true; + /** * Initializes the Sentry service and registers span attributes. */ diff --git a/packages/node/src/integrations/tracing/nest/helpers.ts b/packages/node/src/integrations/tracing/nest/helpers.ts index 32eb3a0d5a39..babf80022c1f 100644 --- a/packages/node/src/integrations/tracing/nest/helpers.ts +++ b/packages/node/src/integrations/tracing/nest/helpers.ts @@ -1,6 +1,6 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import { addNonEnumerableProperty } from '@sentry/utils'; -import type { InjectableTarget } from './types'; +import type { CatchTarget, InjectableTarget } from './types'; const sentryPatched = 'sentryPatched'; @@ -10,7 +10,7 @@ const sentryPatched = 'sentryPatched'; * 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 { +export function isPatched(target: InjectableTarget | CatchTarget): boolean { if (target.sentryPatched) { return true; } @@ -23,7 +23,7 @@ export function isPatched(target: InjectableTarget): boolean { * Returns span options for nest middleware spans. */ // eslint-disable-next-line @typescript-eslint/explicit-function-return-type -export function getMiddlewareSpanOptions(target: InjectableTarget) { +export function getMiddlewareSpanOptions(target: InjectableTarget | CatchTarget) { return { name: target.name, attributes: { diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index 52c3a4ad6b40..28d5a74ef63d 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -9,7 +9,7 @@ import { getActiveSpan, startSpan, startSpanManual, withActiveSpan } from '@sent import type { Span } from '@sentry/types'; import { SDK_VERSION } from '@sentry/utils'; import { getMiddlewareSpanOptions, isPatched } from './helpers'; -import type { InjectableTarget } from './types'; +import type { CatchTarget, InjectableTarget } from './types'; const supportedVersions = ['>=8.0.0 <11']; @@ -34,7 +34,10 @@ export class SentryNestInstrumentation extends InstrumentationBase { public init(): InstrumentationNodeModuleDefinition { const moduleDef = new InstrumentationNodeModuleDefinition(SentryNestInstrumentation.COMPONENT, supportedVersions); - moduleDef.files.push(this._getInjectableFileInstrumentation(supportedVersions)); + moduleDef.files.push( + this._getInjectableFileInstrumentation(supportedVersions), + this._getCatchFileInstrumentation(supportedVersions), + ); return moduleDef; } @@ -58,10 +61,28 @@ export class SentryNestInstrumentation extends InstrumentationBase { ); } + /** + * Wraps the @Catch decorator. + */ + private _getCatchFileInstrumentation(versions: string[]): InstrumentationNodeModuleFile { + return new InstrumentationNodeModuleFile( + '@nestjs/common/decorators/core/catch.decorator.js', + versions, + (moduleExports: { Catch: CatchTarget }) => { + if (isWrapped(moduleExports.Catch)) { + this._unwrap(moduleExports, 'Catch'); + } + this._wrap(moduleExports, 'Catch', this._createWrapCatch()); + return moduleExports; + }, + (moduleExports: { Catch: CatchTarget }) => { + this._unwrap(moduleExports, 'Catch'); + }, + ); + } + /** * 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 @@ -177,4 +198,33 @@ export class SentryNestInstrumentation extends InstrumentationBase { }; }; } + + /** + * Creates a wrapper function for the @Catch decorator. Used to instrument exception filters. + */ + private _createWrapCatch() { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return function wrapCatch(original: any) { + return function wrappedCatch(...exceptions: unknown[]) { + return function (target: CatchTarget) { + if (typeof target.prototype.catch === 'function' && !target.__SENTRY_INTERNAL__) { + // patch only once + if (isPatched(target)) { + return original(...exceptions)(target); + } + + target.prototype.catch = new Proxy(target.prototype.catch, { + apply: (originalCatch, thisArgCatch, argsCatch) => { + return startSpan(getMiddlewareSpanOptions(target), () => { + return originalCatch.apply(thisArgCatch, argsCatch); + }); + }, + }); + } + + return original(...exceptions)(target); + }; + }; + }; + } } diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index 2cdd1b6aefaf..42aa0b003315 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -55,3 +55,15 @@ export interface InjectableTarget { intercept?: (context: unknown, next: CallHandler, ...args: any[]) => Observable; }; } + +/** + * Represents a target class in NestJS annotated with @Catch. + */ +export interface CatchTarget { + name: string; + sentryPatched?: boolean; + __SENTRY_INTERNAL__?: boolean; + prototype: { + catch?: (...args: any[]) => any; + }; +} From 7b170d293e4dea4dcfcc827d86f49e3e31a1a201 Mon Sep 17 00:00:00 2001 From: Andrei <168741329+andreiborza@users.noreply.github.com> Date: Wed, 7 Aug 2024 22:03:03 +0200 Subject: [PATCH 12/20] chore(solidstart): Add sourcemap instructions to README (#13268) --------- Co-authored-by: Charly Gomez --- packages/solidstart/README.md | 56 +++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/packages/solidstart/README.md b/packages/solidstart/README.md index 61aa3b2793da..b654b9bdf744 100644 --- a/packages/solidstart/README.md +++ b/packages/solidstart/README.md @@ -161,3 +161,59 @@ render( document.getElementById('root'), ); ``` + +# Sourcemaps and Releases + +To generate and upload source maps of your Solid Start app use our Vite bundler plugin. + +1. Install the Sentry Vite plugin + +```bash +# Using npm +npm install @sentry/vite-plugin --save-dev + +# Using yarn +yarn add @sentry/vite-plugin --dev +``` + +2. Configure the vite plugin + +To upload source maps you have to configure an auth token. Auth tokens can be passed to the plugin explicitly with the +`authToken` option, with a `SENTRY_AUTH_TOKEN` environment variable, or with an `.env.sentry-build-plugin` file in the +working directory when building your project. We recommend you add the auth token to your CI/CD environment as an +environment variable. + +Learn more about configuring the plugin in our +[Sentry Vite Plugin documentation](https://www.npmjs.com/package/@sentry/vite-plugin). + +```bash +// .env.sentry-build-plugin +SENTRY_AUTH_TOKEN= +SENTRY_ORG= +SENTRY_PROJECT= +``` + +3. Finally, add the plugin to your `app.config.ts` file. + +```javascript +import { defineConfig } from '@solidjs/start/config'; +import { sentryVitePlugin } from '@sentry/vite-plugin'; + +export default defineConfig({ + // rest of your config + // ... + + vite: { + build: { + sourcemap: true, + }, + plugins: [ + sentryVitePlugin({ + org: process.env.SENTRY_ORG, + project: process.env.SENTRY_PROJECT, + authToken: process.env.SENTRY_AUTH_TOKEN, + }), + ], + }, +}); +``` From 6a08d901e0037960c9df7db7779710ba1a988ec8 Mon Sep 17 00:00:00 2001 From: Andrei <168741329+andreiborza@users.noreply.github.com> Date: Thu, 8 Aug 2024 10:05:09 +0200 Subject: [PATCH 13/20] chore(solidstart): Add .craft.yml entry for Solid Start SDK (#13269) Closes: https://github.com/getsentry/sentry-javascript/issues/12550 --- .craft.yml | 3 +++ .github/workflows/build.yml | 1 + 2 files changed, 4 insertions(+) diff --git a/.craft.yml b/.craft.yml index d387e917307d..185fa2fd0510 100644 --- a/.craft.yml +++ b/.craft.yml @@ -114,6 +114,9 @@ targets: - name: npm id: '@sentry/remix' includeNames: /^sentry-remix-\d.*\.tgz$/ + - name: npm + id: '@sentry/solidstart' + includeNames: /^sentry-solidstart-\d.*\.tgz$/ - name: npm id: '@sentry/sveltekit' includeNames: /^sentry-sveltekit-\d.*\.tgz$/ diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 6eea92c884ef..e5e9be1422e2 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -878,6 +878,7 @@ jobs: 'react-router-5', 'react-router-6', 'solid', + 'solidstart', 'svelte-5', 'sveltekit', 'sveltekit-2', From 6cbc416120979f2bb2f197c1821ed5f026f84cc2 Mon Sep 17 00:00:00 2001 From: Andrei <168741329+andreiborza@users.noreply.github.com> Date: Thu, 8 Aug 2024 10:39:55 +0200 Subject: [PATCH 14/20] ref(solidstart): Use core's getTraceMetaTags over own implementation (#13274) ref: #13273 --- packages/solidstart/src/middleware.ts | 14 ++++---------- packages/solidstart/test/middleware.test.ts | 8 ++++---- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/packages/solidstart/src/middleware.ts b/packages/solidstart/src/middleware.ts index 0113cce8f988..65287d23fa0b 100644 --- a/packages/solidstart/src/middleware.ts +++ b/packages/solidstart/src/middleware.ts @@ -1,4 +1,4 @@ -import { getTraceData } from '@sentry/core'; +import { getTraceMetaTags } from '@sentry/core'; import { addNonEnumerableProperty } from '@sentry/utils'; import type { ResponseMiddleware } from '@solidjs/start/middleware'; import type { FetchEvent } from '@solidjs/start/server'; @@ -8,19 +8,13 @@ export type ResponseMiddlewareResponse = Parameters[1] & { }; function addMetaTagToHead(html: string): string { - const { 'sentry-trace': sentryTrace, baggage } = getTraceData(); + const metaTags = getTraceMetaTags(); - if (!sentryTrace) { + if (!metaTags) { return html; } - const metaTags = [``]; - - if (baggage) { - metaTags.push(``); - } - - const content = `\n${metaTags.join('\n')}\n`; + const content = `\n${metaTags}\n`; return html.replace('', content); } diff --git a/packages/solidstart/test/middleware.test.ts b/packages/solidstart/test/middleware.test.ts index 888a0fbc702d..c025dc38af97 100644 --- a/packages/solidstart/test/middleware.test.ts +++ b/packages/solidstart/test/middleware.test.ts @@ -5,10 +5,10 @@ import type { ResponseMiddlewareResponse } from '../src/middleware'; describe('middleware', () => { describe('sentryBeforeResponseMiddleware', () => { - vi.spyOn(SentryCore, 'getTraceData').mockReturnValue({ - 'sentry-trace': '123', - baggage: 'abc', - }); + vi.spyOn(SentryCore, 'getTraceMetaTags').mockReturnValue(` + , + + `); const mockFetchEvent = { request: {}, From b17ac598bb18996a517f4ad6144cab0fd27f04fd Mon Sep 17 00:00:00 2001 From: Andrei <168741329+andreiborza@users.noreply.github.com> Date: Thu, 8 Aug 2024 10:56:58 +0200 Subject: [PATCH 15/20] fix(aws-serverless): Extract sentry trace data from handler `context` over `event` (#13266) Currently, the AWS otel integration (and our `wrapHandler` fallback) try to extract sentry trace data from the `event` object passed to a Lambda call. The aws-sdk integration, however, places tracing data onto `context.clientContext.Custom`. This PR adds a custom `eventContextExtractor` that attempts extracting sentry trace data from the `context`, with a fallback to `event` to enable distributed tracing among Lambda invocations. Traces are now connected. Here an example: `Lambda-A` calling `Lambda-B`: ``` import { LambdaClient, InvokeCommand } from "@aws-sdk/client-lambda"; import * as Sentry from "@sentry/aws-serverless"; export const handler = Sentry.wrapHandler(async (event, context) => { const client = new LambdaClient(); const command = new InvokeCommand({ FunctionName: `Lambda-B`, InvocationType: "RequestResponse", Payload: new Uint16Array(), }) return client.send(command); }); ``` `Lambda-B`: ``` import * as Sentry from "@sentry/aws-serverless"; Sentry.addIntegration(Sentry.postgresIntegration()) export const handler = Sentry.wrapHandler(async (event) => { const queryString = "select count(*) from myTable;"; return await Sentry.startSpan({ name: queryString, op: "db.sql.execute" }, async (span) => { console.log('executing query', queryString); }) }) ``` ![CleanShot 2024-08-07 at 16 34 51@2x](https://github.com/user-attachments/assets/43f5dd9e-e5af-4667-9551-05fac90f03a6) Closes: #13146 --- packages/aws-serverless/rollup.npm.config.mjs | 4 + .../src/integration/awslambda.ts | 44 ++++++-- packages/aws-serverless/src/sdk.ts | 14 +-- packages/aws-serverless/src/utils.ts | 74 ++++++++++++- packages/aws-serverless/test/utils.test.ts | 102 ++++++++++++++++++ 5 files changed, 217 insertions(+), 21 deletions(-) create mode 100644 packages/aws-serverless/test/utils.test.ts diff --git a/packages/aws-serverless/rollup.npm.config.mjs b/packages/aws-serverless/rollup.npm.config.mjs index 46e006f70b95..0ac3218144d5 100644 --- a/packages/aws-serverless/rollup.npm.config.mjs +++ b/packages/aws-serverless/rollup.npm.config.mjs @@ -9,6 +9,10 @@ export default [ entrypoints: ['src/index.ts', 'src/awslambda-auto.ts'], // packages with bundles have a different build directory structure hasBundles: true, + packageSpecificConfig: { + // Used for our custom eventContextExtractor + external: ['@opentelemetry/api'], + }, }), ), ...makeOtelLoaders('./build', 'sentry-node'), diff --git a/packages/aws-serverless/src/integration/awslambda.ts b/packages/aws-serverless/src/integration/awslambda.ts index c6516603b6b8..2e1479914471 100644 --- a/packages/aws-serverless/src/integration/awslambda.ts +++ b/packages/aws-serverless/src/integration/awslambda.ts @@ -1,20 +1,44 @@ import { AwsLambdaInstrumentation } from '@opentelemetry/instrumentation-aws-lambda'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, defineIntegration } from '@sentry/core'; -import { addOpenTelemetryInstrumentation } from '@sentry/node'; +import { generateInstrumentOnce } from '@sentry/node'; import type { IntegrationFn } from '@sentry/types'; +import { eventContextExtractor } from '../utils'; -const _awsLambdaIntegration = (() => { +interface AwsLambdaOptions { + /** + * Disables the AWS context propagation and instead uses + * Sentry's context. Defaults to `true`, in order for + * Sentry trace propagation to take precedence, but can + * be disabled if you want AWS propagation to take take + * precedence. + */ + disableAwsContextPropagation?: boolean; +} + +export const instrumentAwsLambda = generateInstrumentOnce( + 'AwsLambda', + (_options: AwsLambdaOptions = {}) => { + const options = { + disableAwsContextPropagation: true, + ..._options, + }; + + return new AwsLambdaInstrumentation({ + ...options, + eventContextExtractor, + requestHook(span) { + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.otel.aws-lambda'); + span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'function.aws.lambda'); + }, + }); + }, +); + +const _awsLambdaIntegration = ((options: AwsLambdaOptions = {}) => { return { name: 'AwsLambda', setupOnce() { - addOpenTelemetryInstrumentation( - new AwsLambdaInstrumentation({ - requestHook(span) { - span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.otel.aws-lambda'); - span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'function.aws.lambda'); - }, - }), - ); + instrumentAwsLambda(options); }, }; }) satisfies IntegrationFn; diff --git a/packages/aws-serverless/src/sdk.ts b/packages/aws-serverless/src/sdk.ts index 8ed86d9f23d4..e052782d50eb 100644 --- a/packages/aws-serverless/src/sdk.ts +++ b/packages/aws-serverless/src/sdk.ts @@ -16,7 +16,7 @@ import { withScope, } from '@sentry/node'; import type { Integration, Options, Scope, SdkMetadata, Span } from '@sentry/types'; -import { isString, logger } from '@sentry/utils'; +import { logger } from '@sentry/utils'; import type { Context, Handler } from 'aws-lambda'; import { performance } from 'perf_hooks'; @@ -25,7 +25,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } fr import { DEBUG_BUILD } from './debug-build'; import { awsIntegration } from './integration/aws'; import { awsLambdaIntegration } from './integration/awslambda'; -import { markEventUnhandled } from './utils'; +import { getAwsTraceData, markEventUnhandled } from './utils'; const { isPromise } = types; @@ -334,15 +334,9 @@ export function wrapHandler( // Otherwise, we create two root spans (one from otel, one from our wrapper). // If Otel instrumentation didn't work or was filtered by users, we still want to trace the handler. if (options.startTrace && !isWrappedByOtel(handler)) { - const eventWithHeaders = event as { headers?: { [key: string]: string } }; + const traceData = getAwsTraceData(event as { headers?: Record }, context); - const sentryTrace = - eventWithHeaders.headers && isString(eventWithHeaders.headers['sentry-trace']) - ? eventWithHeaders.headers['sentry-trace'] - : undefined; - const baggage = eventWithHeaders.headers?.baggage; - - return continueTrace({ sentryTrace, baggage }, () => { + return continueTrace({ sentryTrace: traceData['sentry-trace'], baggage: traceData.baggage }, () => { return startSpanManual( { name: context.functionName, diff --git a/packages/aws-serverless/src/utils.ts b/packages/aws-serverless/src/utils.ts index 259388bb193c..f6461030c1a7 100644 --- a/packages/aws-serverless/src/utils.ts +++ b/packages/aws-serverless/src/utils.ts @@ -1,5 +1,29 @@ +import type { TextMapGetter } from '@opentelemetry/api'; +import type { Context as OtelContext } from '@opentelemetry/api'; +import { context as otelContext, propagation } from '@opentelemetry/api'; import type { Scope } from '@sentry/types'; -import { addExceptionMechanism } from '@sentry/utils'; +import { addExceptionMechanism, isString } from '@sentry/utils'; +import type { Handler } from 'aws-lambda'; +import type { APIGatewayProxyEventHeaders } from 'aws-lambda'; + +type HandlerEvent = Parameters }>>[0]; +type HandlerContext = Parameters[1]; + +type TraceData = { + 'sentry-trace'?: string; + baggage?: string; +}; + +// vendored from +// https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts#L65-L72 +const headerGetter: TextMapGetter = { + keys(carrier): string[] { + return Object.keys(carrier); + }, + get(carrier, key: string) { + return carrier[key]; + }, +}; /** * Marks an event as unhandled by adding a span processor to the passed scope. @@ -12,3 +36,51 @@ export function markEventUnhandled(scope: Scope): Scope { return scope; } + +/** + * Extracts sentry trace data from the handler `context` if available and falls + * back to the `event`. + * + * When instrumenting the Lambda function with Sentry, the sentry trace data + * is placed on `context.clientContext.Custom`. Users are free to modify context + * tho and provide this data via `event` or `context`. + */ +export function getAwsTraceData(event: HandlerEvent, context?: HandlerContext): TraceData { + const headers = event.headers || {}; + + const traceData: TraceData = { + 'sentry-trace': headers['sentry-trace'], + baggage: headers.baggage, + }; + + if (context && context.clientContext && context.clientContext.Custom) { + const customContext: Record = context.clientContext.Custom; + const sentryTrace = isString(customContext['sentry-trace']) ? customContext['sentry-trace'] : undefined; + + if (sentryTrace) { + traceData['sentry-trace'] = sentryTrace; + traceData.baggage = isString(customContext.baggage) ? customContext.baggage : undefined; + } + } + + return traceData; +} + +/** + * A custom event context extractor for the aws integration. It takes sentry trace data + * from the context rather than the event, with the event being a fallback. + * + * Is only used when the handler was successfully wrapped by otel and the integration option + * `disableAwsContextPropagation` is `true`. + */ +export function eventContextExtractor(event: HandlerEvent, context?: HandlerContext): OtelContext { + // The default context extractor tries to get sampled trace headers from HTTP headers + // The otel aws integration packs these onto the context, so we try to extract them from + // there instead. + const httpHeaders = { + ...(event.headers || {}), + ...getAwsTraceData(event, context), + }; + + return propagation.extract(otelContext.active(), httpHeaders, headerGetter); +} diff --git a/packages/aws-serverless/test/utils.test.ts b/packages/aws-serverless/test/utils.test.ts new file mode 100644 index 000000000000..197c6ebdf90f --- /dev/null +++ b/packages/aws-serverless/test/utils.test.ts @@ -0,0 +1,102 @@ +import { eventContextExtractor, getAwsTraceData } from '../src/utils'; + +const mockExtractContext = jest.fn(); +jest.mock('@opentelemetry/api', () => { + const actualApi = jest.requireActual('@opentelemetry/api'); + return { + ...actualApi, + propagation: { + extract: (...args: unknown[]) => mockExtractContext(args), + }, + }; +}); + +const mockContext = { + clientContext: { + Custom: { + 'sentry-trace': '12345678901234567890123456789012-1234567890123456-1', + baggage: 'sentry-environment=production', + }, + }, +}; +const mockEvent = { + headers: { + 'sentry-trace': '12345678901234567890123456789012-1234567890123456-2', + baggage: 'sentry-environment=staging', + }, +}; + +describe('getTraceData', () => { + test('gets sentry trace data from the context', () => { + // @ts-expect-error, a partial context object is fine here + const traceData = getAwsTraceData({}, mockContext); + + expect(traceData['sentry-trace']).toEqual('12345678901234567890123456789012-1234567890123456-1'); + expect(traceData.baggage).toEqual('sentry-environment=production'); + }); + + test('gets sentry trace data from the context even if event has data', () => { + // @ts-expect-error, a partial context object is fine here + const traceData = getAwsTraceData(mockEvent, mockContext); + + expect(traceData['sentry-trace']).toEqual('12345678901234567890123456789012-1234567890123456-1'); + expect(traceData.baggage).toEqual('sentry-environment=production'); + }); + + test('gets sentry trace data from the event if no context is passed', () => { + const traceData = getAwsTraceData(mockEvent); + + expect(traceData['sentry-trace']).toEqual('12345678901234567890123456789012-1234567890123456-2'); + expect(traceData.baggage).toEqual('sentry-environment=staging'); + }); + + test('gets sentry trace data from the event if the context sentry trace is undefined', () => { + const traceData = getAwsTraceData(mockEvent, { + // @ts-expect-error, a partial context object is fine here + clientContext: { Custom: { 'sentry-trace': undefined, baggage: '' } }, + }); + + expect(traceData['sentry-trace']).toEqual('12345678901234567890123456789012-1234567890123456-2'); + expect(traceData.baggage).toEqual('sentry-environment=staging'); + }); +}); + +describe('eventContextExtractor', () => { + afterEach(() => { + jest.clearAllMocks(); + }); + + test('passes sentry trace data to the propagation extractor', () => { + // @ts-expect-error, a partial context object is fine here + eventContextExtractor(mockEvent, mockContext); + + // @ts-expect-error, a partial context object is fine here + const expectedTraceData = getAwsTraceData(mockEvent, mockContext); + + expect(mockExtractContext).toHaveBeenCalledTimes(1); + expect(mockExtractContext).toHaveBeenCalledWith(expect.arrayContaining([expectedTraceData])); + }); + + test('passes along non-sentry trace headers along', () => { + eventContextExtractor( + { + ...mockEvent, + headers: { + ...mockEvent.headers, + 'X-Custom-Header': 'Foo', + }, + }, + // @ts-expect-error, a partial context object is fine here + mockContext, + ); + + const expectedHeaders = { + 'X-Custom-Header': 'Foo', + // @ts-expect-error, a partial context object is fine here + ...getAwsTraceData(mockEvent, mockContext), + }; + + expect(mockExtractContext).toHaveBeenCalledTimes(1); + expect(mockExtractContext).toHaveBeenCalledWith(expect.arrayContaining([expectedHeaders])); + }); +}); From 0ca88216ddb4bb5b7b483b237b2055ccd76a8906 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Aug 2024 14:15:21 +0200 Subject: [PATCH 16/20] feat(node): Add `useOperationNameForRootSpan` to`graphqlIntegration` (#13248) This introduces a new option for the `graphqlIntegration`, `useOperationNameForRootSpan`, which is by default `true` but can be disabled in integration settings like this: `Sentry.graphqlIntegration({ useOperationNameForRootSpan: true })`. With this setting enabled, the graphql instrumentation will update the `http.server` root span it is in (if there is one) will be appended to the span name. So instead of having all root spans be `POST /graphql`, the names will now be e.g. `POST /graphql (query MyQuery)`. If there are multiple operations in a single http request, they will be appended like `POST /graphql (query Query1, query Query2)`, up to a limit of 5, at which point they will be appended as `+2` or similar. Closes https://github.com/getsentry/sentry-javascript/issues/13238 --- .../tracing/apollo-graphql/apollo-server.js | 33 ++++ .../apollo-graphql/scenario-mutation.js | 26 +-- .../tracing/apollo-graphql/scenario-query.js | 17 +- .../suites/tracing/apollo-graphql/test.ts | 19 ++- .../scenario-invalid-root-span.js | 34 ++++ .../scenario-multiple-operations-many.js | 43 +++++ .../scenario-multiple-operations.js | 45 ++++++ .../scenario-mutation.js | 45 ++++++ .../scenario-no-operation-name.js | 41 +++++ .../scenario-query.js | 41 +++++ .../useOperationNameForRootSpan/test.ts | 152 ++++++++++++++++++ .../node/src/integrations/tracing/graphql.ts | 51 +++++- packages/opentelemetry/src/index.ts | 2 + .../opentelemetry/src/semanticAttributes.ts | 3 + .../src/utils/parseSpanDescription.ts | 29 +++- packages/opentelemetry/src/utils/spanTypes.ts | 2 +- .../test/utils/spanTypes.test.ts | 4 +- 17 files changed, 536 insertions(+), 51 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/apollo-graphql/apollo-server.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-invalid-root-span.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-multiple-operations-many.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-multiple-operations.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-mutation.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-no-operation-name.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-query.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/apollo-server.js b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/apollo-server.js new file mode 100644 index 000000000000..8c1817564196 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/apollo-server.js @@ -0,0 +1,33 @@ +const { ApolloServer, gql } = require('apollo-server'); +const Sentry = require('@sentry/node'); + +module.exports = () => { + return Sentry.startSpan({ name: 'Test Server Start' }, () => { + return new ApolloServer({ + typeDefs: gql`type Query { + hello: String + world: String + } + type Mutation { + login(email: String): String + }`, + resolvers: { + Query: { + hello: () => { + return 'Hello!'; + }, + world: () => { + return 'World!'; + }, + }, + Mutation: { + login: async (_, { email }) => { + return `${email}--token`; + }, + }, + }, + introspection: false, + debug: false, + }); + }); +}; diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/scenario-mutation.js b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/scenario-mutation.js index 9cecf2302315..6defe777d464 100644 --- a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/scenario-mutation.js +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/scenario-mutation.js @@ -12,7 +12,8 @@ Sentry.init({ setInterval(() => {}, 1000); async function run() { - const { ApolloServer, gql } = require('apollo-server'); + const { gql } = require('apollo-server'); + const server = require('./apollo-server')(); await Sentry.startSpan( { @@ -20,29 +21,6 @@ async function run() { op: 'transaction', }, async span => { - const server = new ApolloServer({ - typeDefs: gql` - type Query { - hello: String - } - type Mutation { - login(email: String): String - } - `, - resolvers: { - Query: { - hello: () => { - return 'Hello world!'; - }, - }, - Mutation: { - login: async (_, { email }) => { - return `${email}--token`; - }, - }, - }, - }); - // Ref: https://www.apollographql.com/docs/apollo-server/testing/testing/#testing-using-executeoperation await server.executeOperation({ query: gql`mutation Mutation($email: String){ diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/scenario-query.js b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/scenario-query.js index f0c140fd4b24..b9a05c4b1c3c 100644 --- a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/scenario-query.js +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/scenario-query.js @@ -12,7 +12,7 @@ Sentry.init({ setInterval(() => {}, 1000); async function run() { - const { ApolloServer, gql } = require('apollo-server'); + const server = require('./apollo-server')(); await Sentry.startSpan( { @@ -20,21 +20,6 @@ async function run() { op: 'transaction', }, async span => { - const typeDefs = gql`type Query { hello: String }`; - - const resolvers = { - Query: { - hello: () => { - return 'Hello world!'; - }, - }, - }; - - const server = new ApolloServer({ - typeDefs, - resolvers, - }); - // Ref: https://www.apollographql.com/docs/apollo-server/testing/testing/#testing-using-executeoperation await server.executeOperation({ query: '{hello}', diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/test.ts b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/test.ts index 5bf91f7653c1..46e05acf940e 100644 --- a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/test.ts @@ -1,7 +1,12 @@ import { createRunner } from '../../../utils/runner'; +// Graphql Instrumentation emits some spans by default on server start +const EXPECTED_START_SERVER_TRANSACTION = { + transaction: 'Test Server Start', +}; + describe('GraphQL/Apollo Tests', () => { - test('CJS - should instrument GraphQL queries used from Apollo Server.', done => { + test('should instrument GraphQL queries used from Apollo Server.', done => { const EXPECTED_TRANSACTION = { transaction: 'Test Transaction', spans: expect.arrayContaining([ @@ -18,10 +23,13 @@ describe('GraphQL/Apollo Tests', () => { ]), }; - createRunner(__dirname, 'scenario-query.js').expect({ transaction: EXPECTED_TRANSACTION }).start(done); + createRunner(__dirname, 'scenario-query.js') + .expect({ transaction: EXPECTED_START_SERVER_TRANSACTION }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); }); - test('CJS - should instrument GraphQL mutations used from Apollo Server.', done => { + test('should instrument GraphQL mutations used from Apollo Server.', done => { const EXPECTED_TRANSACTION = { transaction: 'Test Transaction', spans: expect.arrayContaining([ @@ -39,6 +47,9 @@ describe('GraphQL/Apollo Tests', () => { ]), }; - createRunner(__dirname, 'scenario-mutation.js').expect({ transaction: EXPECTED_TRANSACTION }).start(done); + createRunner(__dirname, 'scenario-mutation.js') + .expect({ transaction: EXPECTED_START_SERVER_TRANSACTION }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); }); }); diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-invalid-root-span.js b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-invalid-root-span.js new file mode 100644 index 000000000000..840a5551b98a --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-invalid-root-span.js @@ -0,0 +1,34 @@ +const Sentry = require('@sentry/node'); +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); + +const client = Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [Sentry.graphqlIntegration({ useOperationNameForRootSpan: true })], + transport: loggingTransport, +}); + +const tracer = client.tracer; + +// Stop the process from exiting before the transaction is sent +setInterval(() => {}, 1000); + +async function run() { + const server = require('../apollo-server')(); + + await tracer.startActiveSpan('test span name', async span => { + // Ref: https://www.apollographql.com/docs/apollo-server/testing/testing/#testing-using-executeoperation + await server.executeOperation({ + query: 'query GetHello {hello}', + }); + + setTimeout(() => { + span.end(); + server.stop(); + }, 500); + }); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-multiple-operations-many.js b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-multiple-operations-many.js new file mode 100644 index 000000000000..992ff5337b46 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-multiple-operations-many.js @@ -0,0 +1,43 @@ +const Sentry = require('@sentry/node'); +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); + +const client = Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [Sentry.graphqlIntegration({ useOperationNameForRootSpan: true })], + transport: loggingTransport, +}); + +const tracer = client.tracer; + +// Stop the process from exiting before the transaction is sent +setInterval(() => {}, 1000); + +async function run() { + const server = require('../apollo-server')(); + + await tracer.startActiveSpan( + 'test span name', + { + kind: 1, + attributes: { 'http.method': 'GET', 'http.route': '/test-graphql' }, + }, + async span => { + for (let i = 1; i < 10; i++) { + // Ref: https://www.apollographql.com/docs/apollo-server/testing/testing/#testing-using-executeoperation + await server.executeOperation({ + query: `query GetHello${i} {hello}`, + }); + } + + setTimeout(() => { + span.end(); + server.stop(); + }, 500); + }, + ); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-multiple-operations.js b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-multiple-operations.js new file mode 100644 index 000000000000..d9eeca63ae10 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-multiple-operations.js @@ -0,0 +1,45 @@ +const Sentry = require('@sentry/node'); +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); + +const client = Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [Sentry.graphqlIntegration({ useOperationNameForRootSpan: true })], + transport: loggingTransport, +}); + +const tracer = client.tracer; + +// Stop the process from exiting before the transaction is sent +setInterval(() => {}, 1000); + +async function run() { + const server = require('../apollo-server')(); + + await tracer.startActiveSpan( + 'test span name', + { + kind: 1, + attributes: { 'http.method': 'GET', 'http.route': '/test-graphql' }, + }, + async span => { + // Ref: https://www.apollographql.com/docs/apollo-server/testing/testing/#testing-using-executeoperation + await server.executeOperation({ + query: 'query GetWorld {world}', + }); + + await server.executeOperation({ + query: 'query GetHello {hello}', + }); + + setTimeout(() => { + span.end(); + server.stop(); + }, 500); + }, + ); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-mutation.js b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-mutation.js new file mode 100644 index 000000000000..8ee9154c0e51 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-mutation.js @@ -0,0 +1,45 @@ +const Sentry = require('@sentry/node'); +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); + +const client = Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [Sentry.graphqlIntegration({ useOperationNameForRootSpan: true })], + transport: loggingTransport, +}); + +const tracer = client.tracer; + +// Stop the process from exiting before the transaction is sent +setInterval(() => {}, 1000); + +async function run() { + const { gql } = require('apollo-server'); + const server = require('../apollo-server')(); + + await tracer.startActiveSpan( + 'test span name', + { + kind: 1, + attributes: { 'http.method': 'GET', 'http.route': '/test-graphql' }, + }, + async span => { + // Ref: https://www.apollographql.com/docs/apollo-server/testing/testing/#testing-using-executeoperation + await server.executeOperation({ + query: gql`mutation TestMutation($email: String){ + login(email: $email) + }`, + variables: { email: 'test@email.com' }, + }); + + setTimeout(() => { + span.end(); + server.stop(); + }, 500); + }, + ); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-no-operation-name.js b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-no-operation-name.js new file mode 100644 index 000000000000..14879bc0e79d --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-no-operation-name.js @@ -0,0 +1,41 @@ +const Sentry = require('@sentry/node'); +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); + +const client = Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [Sentry.graphqlIntegration({ useOperationNameForRootSpan: true })], + transport: loggingTransport, +}); + +const tracer = client.tracer; + +// Stop the process from exiting before the transaction is sent +setInterval(() => {}, 1000); + +async function run() { + const server = require('../apollo-server')(); + + await tracer.startActiveSpan( + 'test span name', + { + kind: 1, + attributes: { 'http.method': 'GET', 'http.route': '/test-graphql' }, + }, + async span => { + // Ref: https://www.apollographql.com/docs/apollo-server/testing/testing/#testing-using-executeoperation + await server.executeOperation({ + query: 'query {hello}', + }); + + setTimeout(() => { + span.end(); + server.stop(); + }, 500); + }, + ); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-query.js b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-query.js new file mode 100644 index 000000000000..4dc3357ab17f --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/scenario-query.js @@ -0,0 +1,41 @@ +const Sentry = require('@sentry/node'); +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); + +const client = Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [Sentry.graphqlIntegration({ useOperationNameForRootSpan: true })], + transport: loggingTransport, +}); + +const tracer = client.tracer; + +// Stop the process from exiting before the transaction is sent +setInterval(() => {}, 1000); + +async function run() { + const server = require('../apollo-server')(); + + await tracer.startActiveSpan( + 'test span name', + { + kind: 1, + attributes: { 'http.method': 'GET', 'http.route': '/test-graphql' }, + }, + async span => { + // Ref: https://www.apollographql.com/docs/apollo-server/testing/testing/#testing-using-executeoperation + await server.executeOperation({ + query: 'query GetHello {hello}', + }); + + setTimeout(() => { + span.end(); + server.stop(); + }, 500); + }, + ); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/test.ts b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/test.ts new file mode 100644 index 000000000000..234cc4009b38 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/apollo-graphql/useOperationNameForRootSpan/test.ts @@ -0,0 +1,152 @@ +import { createRunner } from '../../../../utils/runner'; + +// Graphql Instrumentation emits some spans by default on server start +const EXPECTED_START_SERVER_TRANSACTION = { + transaction: 'Test Server Start', +}; + +describe('GraphQL/Apollo Tests > useOperationNameForRootSpan', () => { + test('useOperationNameForRootSpan works with single query operation', done => { + const EXPECTED_TRANSACTION = { + transaction: 'GET /test-graphql (query GetHello)', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: { + 'graphql.operation.name': 'GetHello', + 'graphql.operation.type': 'query', + 'graphql.source': 'query GetHello {hello}', + 'sentry.origin': 'auto.graphql.otel.graphql', + }, + description: 'query GetHello', + status: 'ok', + origin: 'auto.graphql.otel.graphql', + }), + ]), + }; + + createRunner(__dirname, 'scenario-query.js') + .expect({ transaction: EXPECTED_START_SERVER_TRANSACTION }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); + + test('useOperationNameForRootSpan works with single mutation operation', done => { + const EXPECTED_TRANSACTION = { + transaction: 'GET /test-graphql (mutation TestMutation)', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: { + 'graphql.operation.name': 'TestMutation', + 'graphql.operation.type': 'mutation', + 'graphql.source': `mutation TestMutation($email: String) { + login(email: $email) +}`, + 'sentry.origin': 'auto.graphql.otel.graphql', + }, + description: 'mutation TestMutation', + status: 'ok', + origin: 'auto.graphql.otel.graphql', + }), + ]), + }; + + createRunner(__dirname, 'scenario-mutation.js') + .expect({ transaction: EXPECTED_START_SERVER_TRANSACTION }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); + + test('useOperationNameForRootSpan ignores an invalid root span', done => { + const EXPECTED_TRANSACTION = { + transaction: 'test span name', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: { + 'graphql.operation.name': 'GetHello', + 'graphql.operation.type': 'query', + 'graphql.source': 'query GetHello {hello}', + 'sentry.origin': 'auto.graphql.otel.graphql', + }, + description: 'query GetHello', + status: 'ok', + origin: 'auto.graphql.otel.graphql', + }), + ]), + }; + + createRunner(__dirname, 'scenario-invalid-root-span.js') + .expect({ transaction: EXPECTED_START_SERVER_TRANSACTION }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); + + test('useOperationNameForRootSpan works with single query operation without name', done => { + const EXPECTED_TRANSACTION = { + transaction: 'GET /test-graphql (query)', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: { + 'graphql.operation.type': 'query', + 'graphql.source': 'query {hello}', + 'sentry.origin': 'auto.graphql.otel.graphql', + }, + description: 'query', + status: 'ok', + origin: 'auto.graphql.otel.graphql', + }), + ]), + }; + + createRunner(__dirname, 'scenario-no-operation-name.js') + .expect({ transaction: EXPECTED_START_SERVER_TRANSACTION }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); + + test('useOperationNameForRootSpan works with multiple query operations', done => { + const EXPECTED_TRANSACTION = { + transaction: 'GET /test-graphql (query GetHello, query GetWorld)', + spans: expect.arrayContaining([ + expect.objectContaining({ + data: { + 'graphql.operation.name': 'GetHello', + 'graphql.operation.type': 'query', + 'graphql.source': 'query GetHello {hello}', + 'sentry.origin': 'auto.graphql.otel.graphql', + }, + description: 'query GetHello', + status: 'ok', + origin: 'auto.graphql.otel.graphql', + }), + expect.objectContaining({ + data: { + 'graphql.operation.name': 'GetWorld', + 'graphql.operation.type': 'query', + 'graphql.source': 'query GetWorld {world}', + 'sentry.origin': 'auto.graphql.otel.graphql', + }, + description: 'query GetWorld', + status: 'ok', + origin: 'auto.graphql.otel.graphql', + }), + ]), + }; + + createRunner(__dirname, 'scenario-multiple-operations.js') + .expect({ transaction: EXPECTED_START_SERVER_TRANSACTION }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); + + test('useOperationNameForRootSpan works with more than 5 query operations', done => { + const EXPECTED_TRANSACTION = { + transaction: + 'GET /test-graphql (query GetHello1, query GetHello2, query GetHello3, query GetHello4, query GetHello5, +4)', + }; + + createRunner(__dirname, 'scenario-multiple-operations-many.js') + .expect({ transaction: EXPECTED_START_SERVER_TRANSACTION }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); +}); diff --git a/packages/node/src/integrations/tracing/graphql.ts b/packages/node/src/integrations/tracing/graphql.ts index 097ee3ba43f8..914653ac745c 100644 --- a/packages/node/src/integrations/tracing/graphql.ts +++ b/packages/node/src/integrations/tracing/graphql.ts @@ -1,12 +1,17 @@ import { GraphQLInstrumentation } from '@opentelemetry/instrumentation-graphql'; -import { defineIntegration } from '@sentry/core'; +import { defineIntegration, getRootSpan, spanToJSON } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION } from '@sentry/opentelemetry'; import type { IntegrationFn } from '@sentry/types'; import { generateInstrumentOnce } from '../../otel/instrument'; import { addOriginToSpan } from '../../utils/addOriginToSpan'; interface GraphqlOptions { - /** Do not create spans for resolvers. */ + /** + * Do not create spans for resolvers. + * + * Defaults to true. + */ ignoreResolveSpans?: boolean; /** @@ -16,8 +21,18 @@ interface GraphqlOptions { * use the default resolver which just looks for a property with that name on the object. * If the property is not a function, it's not very interesting to trace. * This option can reduce noise and number of spans created. + * + * Defaults to true. + */ + ignoreTrivialResolveSpans?: boolean; + + /** + * If this is enabled, a http.server root span containing this span will automatically be renamed to include the operation name. + * Set this to `false` if you do not want this behavior, and want to keep the default http.server span name. + * + * Defaults to true. */ - ignoreTrivalResolveSpans?: boolean; + useOperationNameForRootSpan?: boolean; } const INTEGRATION_NAME = 'Graphql'; @@ -28,6 +43,7 @@ export const instrumentGraphql = generateInstrumentOnce( const options = { ignoreResolveSpans: true, ignoreTrivialResolveSpans: true, + useOperationNameForRootSpan: true, ..._options, }; @@ -35,6 +51,35 @@ export const instrumentGraphql = generateInstrumentOnce( ...options, responseHook(span) { addOriginToSpan(span, 'auto.graphql.otel.graphql'); + + const attributes = spanToJSON(span).data || {}; + + // If operation.name is not set, we fall back to use operation.type only + const operationType = attributes['graphql.operation.type']; + const operationName = attributes['graphql.operation.name']; + + if (options.useOperationNameForRootSpan && operationType) { + const rootSpan = getRootSpan(span); + + // We guard to only do this on http.server spans + + const rootSpanAttributes = spanToJSON(rootSpan).data || {}; + + const existingOperations = rootSpanAttributes[SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION] || []; + + const newOperation = operationName ? `${operationType} ${operationName}` : `${operationType}`; + + // We keep track of each operation on the root span + // This can either be a string, or an array of strings (if there are multiple operations) + if (Array.isArray(existingOperations)) { + existingOperations.push(newOperation); + rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION, existingOperations); + } else if (existingOperations) { + rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION, [existingOperations, newOperation]); + } else { + rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION, newOperation); + } + } }, }); }, diff --git a/packages/opentelemetry/src/index.ts b/packages/opentelemetry/src/index.ts index ef57ab0fff3d..98460b575c8d 100644 --- a/packages/opentelemetry/src/index.ts +++ b/packages/opentelemetry/src/index.ts @@ -1,3 +1,5 @@ +export { SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION } from './semanticAttributes'; + export { getRequestSpanData } from './utils/getRequestSpanData'; export type { OpenTelemetryClient } from './types'; diff --git a/packages/opentelemetry/src/semanticAttributes.ts b/packages/opentelemetry/src/semanticAttributes.ts index 80a80f87a666..2e14c71bf5e9 100644 --- a/packages/opentelemetry/src/semanticAttributes.ts +++ b/packages/opentelemetry/src/semanticAttributes.ts @@ -1,2 +1,5 @@ /** If this attribute is true, it means that the parent is a remote span. */ export const SEMANTIC_ATTRIBUTE_SENTRY_PARENT_IS_REMOTE = 'sentry.parentIsRemote'; + +// These are not standardized yet, but used by the graphql instrumentation +export const SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION = 'sentry.graphql.operation'; diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index a9d99aa91b8a..6d1c9936899b 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/packages/opentelemetry/src/utils/parseSpanDescription.ts @@ -15,6 +15,7 @@ import type { SpanAttributes, TransactionSource } from '@sentry/types'; import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION } from '../semanticAttributes'; import type { AbstractSpan } from '../types'; import { getSpanKind } from './getSpanKind'; import { spanHasAttributes, spanHasName } from './spanTypes'; @@ -136,8 +137,16 @@ export function descriptionForHttpMethod( return { op: opParts.join('.'), description: name, source: 'custom' }; } - // Ex. description="GET /api/users". - const description = `${httpMethod} ${urlPath}`; + const graphqlOperationsAttribute = attributes[SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION]; + + // Ex. GET /api/users + const baseDescription = `${httpMethod} ${urlPath}`; + + // When the http span has a graphql operation, append it to the description + // We add these in the graphqlIntegration + const description = graphqlOperationsAttribute + ? `${baseDescription} (${getGraphqlOperationNamesFromAttribute(graphqlOperationsAttribute)})` + : baseDescription; // If `httpPath` is a root path, then we can categorize the transaction source as route. const source: TransactionSource = hasRoute || urlPath === '/' ? 'route' : 'url'; @@ -162,6 +171,22 @@ export function descriptionForHttpMethod( }; } +function getGraphqlOperationNamesFromAttribute(attr: AttributeValue): string { + if (Array.isArray(attr)) { + const sorted = attr.slice().sort(); + + // Up to 5 items, we just add all of them + if (sorted.length <= 5) { + return sorted.join(', '); + } else { + // Else, we add the first 5 and the diff of other operations + return `${sorted.slice(0, 5).join(', ')}, +${sorted.length - 5}`; + } + } + + return `${attr}`; +} + /** Exported for tests only */ export function getSanitizedUrl( attributes: Attributes, diff --git a/packages/opentelemetry/src/utils/spanTypes.ts b/packages/opentelemetry/src/utils/spanTypes.ts index f92d411200a1..39c62219d2ad 100644 --- a/packages/opentelemetry/src/utils/spanTypes.ts +++ b/packages/opentelemetry/src/utils/spanTypes.ts @@ -22,7 +22,7 @@ export function spanHasAttributes( */ export function spanHasKind(span: SpanType): span is SpanType & { kind: SpanKind } { const castSpan = span as ReadableSpan; - return !!castSpan.kind; + return typeof castSpan.kind === 'number'; } /** diff --git a/packages/opentelemetry/test/utils/spanTypes.test.ts b/packages/opentelemetry/test/utils/spanTypes.test.ts index 99152204adfa..af07e5c45af5 100644 --- a/packages/opentelemetry/test/utils/spanTypes.test.ts +++ b/packages/opentelemetry/test/utils/spanTypes.test.ts @@ -24,7 +24,9 @@ describe('spanTypes', () => { it.each([ [{}, false], [{ kind: null }, false], - [{ kind: 'TEST_KIND' }, true], + [{ kind: 0 }, true], + [{ kind: 5 }, true], + [{ kind: 'TEST_KIND' }, false], ])('works with %p', (span, expected) => { const castSpan = span as unknown as Span; const actual = spanHasKind(castSpan); From adf7b4026c534051a1ea73e6fbd4e4f483b8e618 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Aug 2024 15:23:18 +0200 Subject: [PATCH 17/20] fix(utils): Streamline IP capturing on incoming requests (#13272) This PR does three things: 1. It ensures we infer the IP (in RequestData integration) from IP-related headers, if available. 2. It ensures we do not send these headers if IP capturing is not enabled (which is the default) 3. It removes the custom handling we had for this in remix, as this should now just be handled generally Closes https://github.com/getsentry/sentry-javascript/issues/13260 --- packages/remix/src/utils/web-fetch.ts | 17 - .../remix/test/utils/getIpAddress.test.ts | 42 --- .../test/utils/normalizeRemixRequest.test.ts | 1 - packages/utils/src/requestdata.ts | 34 +- .../src}/vendor/getIpAddress.ts | 82 +++-- packages/utils/test/requestdata.test.ts | 316 ++++++++++++++++++ 6 files changed, 387 insertions(+), 105 deletions(-) delete mode 100644 packages/remix/test/utils/getIpAddress.test.ts rename packages/{remix/src/utils => utils/src}/vendor/getIpAddress.ts (50%) diff --git a/packages/remix/src/utils/web-fetch.ts b/packages/remix/src/utils/web-fetch.ts index 8450a12eb05d..6e188bd9d440 100644 --- a/packages/remix/src/utils/web-fetch.ts +++ b/packages/remix/src/utils/web-fetch.ts @@ -1,4 +1,3 @@ -/* eslint-disable complexity */ // Based on Remix's implementation of Fetch API // https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/ // The MIT License (MIT) @@ -23,10 +22,6 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. -import { logger } from '@sentry/utils'; - -import { DEBUG_BUILD } from './debug-build'; -import { getClientIPAddress } from './vendor/getIpAddress'; import type { RemixRequest } from './vendor/types'; /* @@ -124,15 +119,6 @@ export const normalizeRemixRequest = (request: RemixRequest): Record = {}; - - get(key: string): string | null { - return this._headers[key] ?? null; - } - - set(key: string, value: string): void { - this._headers[key] = value; - } -} - -describe('getClientIPAddress', () => { - it.each([ - [ - '2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5,2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5, 141.101.69.35', - '2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5', - ], - [ - '2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5, 2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5, 141.101.69.35', - '2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5', - ], - [ - '2a01:cb19:8350:ed00:d0dd:INVALID_IP_ADDR:8be5,141.101.69.35,2a01:cb19:8350:ed00:d0dd:fa5b:de31:8be5', - '141.101.69.35', - ], - [ - '2b01:cb19:8350:ed00:d0dd:fa5b:nope:8be5, 2b01:cb19:NOPE:ed00:d0dd:fa5b:de31:8be5, 141.101.69.35 ', - '141.101.69.35', - ], - ['2b01:cb19:8350:ed00:d0 dd:fa5b:de31:8be5, 141.101.69.35', '141.101.69.35'], - ])('should parse the IP from the X-Forwarded-For header %s', (headerValue, expectedIP) => { - const headers = new Headers(); - headers.set('X-Forwarded-For', headerValue); - - const ip = getClientIPAddress(headers as any); - - expect(ip).toEqual(expectedIP); - }); -}); diff --git a/packages/remix/test/utils/normalizeRemixRequest.test.ts b/packages/remix/test/utils/normalizeRemixRequest.test.ts index b627a34e4f12..64de88510014 100644 --- a/packages/remix/test/utils/normalizeRemixRequest.test.ts +++ b/packages/remix/test/utils/normalizeRemixRequest.test.ts @@ -83,7 +83,6 @@ describe('normalizeRemixRequest', () => { hostname: 'example.com', href: 'https://example.com/api/json?id=123', insecureHTTPParser: undefined, - ip: null, method: 'GET', originalUrl: 'https://example.com/api/json?id=123', path: '/api/json?id=123', diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 85133521ef76..a4eae547edb1 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -13,6 +13,7 @@ import { isPlainObject, isString } from './is'; import { logger } from './logger'; import { normalize } from './normalize'; import { stripUrlQueryAndFragment } from './url'; +import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress'; const DEFAULT_INCLUDES = { ip: false, @@ -98,7 +99,6 @@ export function extractPathForTransaction( return [name, source]; } -/** JSDoc */ function extractTransaction(req: PolymorphicRequest, type: boolean | TransactionNamingScheme): string { switch (type) { case 'path': { @@ -116,7 +116,6 @@ function extractTransaction(req: PolymorphicRequest, type: boolean | Transaction } } -/** JSDoc */ function extractUserData( user: { [key: string]: unknown; @@ -146,17 +145,16 @@ function extractUserData( */ export function extractRequestData( req: PolymorphicRequest, - options?: { + options: { include?: string[]; - }, + } = {}, ): ExtractedNodeRequestData { - const { include = DEFAULT_REQUEST_INCLUDES } = options || {}; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const requestData: { [key: string]: any } = {}; + const { include = DEFAULT_REQUEST_INCLUDES } = options; + const requestData: { [key: string]: unknown } = {}; // headers: // node, express, koa, nextjs: req.headers - const headers = (req.headers || {}) as { + const headers = (req.headers || {}) as typeof req.headers & { host?: string; cookie?: string; }; @@ -191,6 +189,14 @@ export function extractRequestData( delete (requestData.headers as { cookie?: string }).cookie; } + // Remove IP headers in case IP data should not be included in the event + if (!include.includes('ip')) { + ipHeaderNames.forEach(ipHeaderName => { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete (requestData.headers as Record)[ipHeaderName]; + }); + } + break; } case 'method': { @@ -264,9 +270,12 @@ export function addRequestDataToEvent( }; if (include.request) { - const extractedRequestData = Array.isArray(include.request) - ? extractRequestData(req, { include: include.request }) - : extractRequestData(req); + const includeRequest = Array.isArray(include.request) ? [...include.request] : [...DEFAULT_REQUEST_INCLUDES]; + if (include.ip) { + includeRequest.push('ip'); + } + + const extractedRequestData = extractRequestData(req, { include: includeRequest }); event.request = { ...event.request, @@ -288,8 +297,9 @@ export function addRequestDataToEvent( // client ip: // node, nextjs: req.socket.remoteAddress // express, koa: req.ip + // It may also be sent by proxies as specified in X-Forwarded-For or similar headers if (include.ip) { - const ip = req.ip || (req.socket && req.socket.remoteAddress); + const ip = (req.headers && getClientIPAddress(req.headers)) || req.ip || (req.socket && req.socket.remoteAddress); if (ip) { event.user = { ...event.user, diff --git a/packages/remix/src/utils/vendor/getIpAddress.ts b/packages/utils/src/vendor/getIpAddress.ts similarity index 50% rename from packages/remix/src/utils/vendor/getIpAddress.ts rename to packages/utils/src/vendor/getIpAddress.ts index d63e31779aac..8b96fe2146af 100644 --- a/packages/remix/src/utils/vendor/getIpAddress.ts +++ b/packages/utils/src/vendor/getIpAddress.ts @@ -23,7 +23,21 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. -import { isIP } from 'net'; +// The headers to check, in priority order +export const ipHeaderNames = [ + 'X-Client-IP', + 'X-Forwarded-For', + 'Fly-Client-IP', + 'CF-Connecting-IP', + 'Fastly-Client-Ip', + 'True-Client-Ip', + 'X-Real-IP', + 'X-Cluster-Client-IP', + 'X-Forwarded', + 'Forwarded-For', + 'Forwarded', + 'X-Vercel-Forwarded-For', +]; /** * Get the IP address of the client sending a request. @@ -31,50 +45,24 @@ import { isIP } from 'net'; * It receives a Request headers object and use it to get the * IP address from one of the following headers in order. * - * - X-Client-IP - * - X-Forwarded-For - * - Fly-Client-IP - * - CF-Connecting-IP - * - Fastly-Client-Ip - * - True-Client-Ip - * - X-Real-IP - * - X-Cluster-Client-IP - * - X-Forwarded - * - Forwarded-For - * - Forwarded - * * If the IP address is valid, it will be returned. Otherwise, null will be * returned. * * If the header values contains more than one IP address, the first valid one * will be returned. */ -export function getClientIPAddress(headers: Headers): string | null { - // The headers to check, in priority order - const headerNames = [ - 'X-Client-IP', - 'X-Forwarded-For', - 'Fly-Client-IP', - 'CF-Connecting-IP', - 'Fastly-Client-Ip', - 'True-Client-Ip', - 'X-Real-IP', - 'X-Cluster-Client-IP', - 'X-Forwarded', - 'Forwarded-For', - 'Forwarded', - ]; - +export function getClientIPAddress(headers: { [key: string]: string | string[] | undefined }): string | null { // This will end up being Array because of the various possible values a header // can take - const headerValues = headerNames.map((headerName: string) => { - const value = headers.get(headerName); + const headerValues = ipHeaderNames.map((headerName: string) => { + const rawValue = headers[headerName]; + const value = Array.isArray(rawValue) ? rawValue.join(';') : rawValue; if (headerName === 'Forwarded') { return parseForwardedHeader(value); } - return value?.split(',').map((v: string) => v.trim()); + return value && value.split(',').map((v: string) => v.trim()); }); // Flatten the array and filter out any falsy entries @@ -92,7 +80,7 @@ export function getClientIPAddress(headers: Headers): string | null { return ipAddress || null; } -function parseForwardedHeader(value: string | null): string | null { +function parseForwardedHeader(value: string | null | undefined): string | null { if (!value) { return null; } @@ -105,3 +93,31 @@ function parseForwardedHeader(value: string | null): string | null { return null; } + +// +/** + * Custom method instead of importing this from `net` package, as this only exists in node + * Accepts: + * 127.0.0.1 + * 192.168.1.1 + * 192.168.1.255 + * 255.255.255.255 + * 10.1.1.1 + * 0.0.0.0 + * 2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5 + * + * Rejects: + * 1.1.1.01 + * 30.168.1.255.1 + * 127.1 + * 192.168.1.256 + * -1.2.3.4 + * 1.1.1.1. + * 3...3 + * 192.168.1.099 + */ +function isIP(str: string): boolean { + const regex = + /(?:^(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}$)|(?:^(?:(?:[a-fA-F\d]{1,4}:){7}(?:[a-fA-F\d]{1,4}|:)|(?:[a-fA-F\d]{1,4}:){6}(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|:[a-fA-F\d]{1,4}|:)|(?:[a-fA-F\d]{1,4}:){5}(?::(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,2}|:)|(?:[a-fA-F\d]{1,4}:){4}(?:(?::[a-fA-F\d]{1,4}){0,1}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,3}|:)|(?:[a-fA-F\d]{1,4}:){3}(?:(?::[a-fA-F\d]{1,4}){0,2}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,4}|:)|(?:[a-fA-F\d]{1,4}:){2}(?:(?::[a-fA-F\d]{1,4}){0,3}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,5}|:)|(?:[a-fA-F\d]{1,4}:){1}(?:(?::[a-fA-F\d]{1,4}){0,4}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,6}|:)|(?::(?:(?::[a-fA-F\d]{1,4}){0,5}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,7}|:)))(?:%[0-9a-zA-Z]{1,})?$)/; + return regex.test(str); +} diff --git a/packages/utils/test/requestdata.test.ts b/packages/utils/test/requestdata.test.ts index 7e44f703c62a..570f80647b6b 100644 --- a/packages/utils/test/requestdata.test.ts +++ b/packages/utils/test/requestdata.test.ts @@ -1,6 +1,7 @@ import type * as net from 'net'; import type { Event, PolymorphicRequest, TransactionSource, User } from '@sentry/types'; import { addRequestDataToEvent, extractPathForTransaction, extractRequestData } from '@sentry/utils'; +import { getClientIPAddress } from '../src/vendor/getIpAddress'; describe('addRequestDataToEvent', () => { let mockEvent: Event; @@ -107,6 +108,227 @@ describe('addRequestDataToEvent', () => { expect(parsedRequest.user!.ip_address).toEqual('321'); }); + + test.each([ + 'X-Client-IP', + 'X-Forwarded-For', + 'Fly-Client-IP', + 'CF-Connecting-IP', + 'Fastly-Client-Ip', + 'True-Client-Ip', + 'X-Real-IP', + 'X-Cluster-Client-IP', + 'X-Forwarded', + 'Forwarded-For', + 'X-Vercel-Forwarded-For', + ])('can be extracted from %s header', headerName => { + const reqWithIPInHeader = { + ...mockReq, + headers: { + [headerName]: '123.5.6.1', + }, + }; + + const optionsWithIP = { + include: { + ip: true, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); + + expect(parsedRequest.user!.ip_address).toEqual('123.5.6.1'); + }); + + it('can be extracted from Forwarded header', () => { + const reqWithIPInHeader = { + ...mockReq, + headers: { + Forwarded: 'by=111;for=123.5.6.1;for=123.5.6.2;', + }, + }; + + const optionsWithIP = { + include: { + ip: true, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); + + expect(parsedRequest.user!.ip_address).toEqual('123.5.6.1'); + }); + + test('it ignores invalid IP in header', () => { + const reqWithIPInHeader = { + ...mockReq, + headers: { + 'X-Client-IP': 'invalid', + }, + }; + + const optionsWithIP = { + include: { + ip: true, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); + + expect(parsedRequest.user!.ip_address).toEqual(undefined); + }); + + test('IP from header takes presedence over socket', () => { + const reqWithIPInHeader = { + ...mockReq, + headers: { + 'X-Client-IP': '123.5.6.1', + }, + socket: { + remoteAddress: '321', + } as net.Socket, + }; + + const optionsWithIP = { + include: { + ip: true, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); + + expect(parsedRequest.user!.ip_address).toEqual('123.5.6.1'); + }); + + test('IP from header takes presedence over req.ip', () => { + const reqWithIPInHeader = { + ...mockReq, + headers: { + 'X-Client-IP': '123.5.6.1', + }, + ip: '123', + }; + + const optionsWithIP = { + include: { + ip: true, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); + + expect(parsedRequest.user!.ip_address).toEqual('123.5.6.1'); + }); + + test('does not add IP if ip=false', () => { + const reqWithIPInHeader = { + ...mockReq, + headers: { + 'X-Client-IP': '123.5.6.1', + }, + ip: '123', + }; + + const optionsWithoutIP = { + include: { + ip: false, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithoutIP); + + expect(parsedRequest.user!.ip_address).toEqual(undefined); + }); + + test('does not add IP by default', () => { + const reqWithIPInHeader = { + ...mockReq, + headers: { + 'X-Client-IP': '123.5.6.1', + }, + ip: '123', + }; + + const optionsWithoutIP = {}; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithoutIP); + + expect(parsedRequest.user!.ip_address).toEqual(undefined); + }); + + test('removes IP headers if `ip` is not set in the options', () => { + const reqWithIPInHeader = { + ...mockReq, + headers: { + otherHeader: 'hello', + 'X-Client-IP': '123', + 'X-Forwarded-For': '123', + 'Fly-Client-IP': '123', + 'CF-Connecting-IP': '123', + 'Fastly-Client-Ip': '123', + 'True-Client-Ip': '123', + 'X-Real-IP': '123', + 'X-Cluster-Client-IP': '123', + 'X-Forwarded': '123', + 'Forwarded-For': '123', + Forwarded: '123', + 'X-Vercel-Forwarded-For': '123', + }, + }; + + const optionsWithoutIP = { + include: {}, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithoutIP); + + expect(parsedRequest.request?.headers).toEqual({ otherHeader: 'hello' }); + }); + + test('keeps IP headers if `ip=true`', () => { + const reqWithIPInHeader = { + ...mockReq, + headers: { + otherHeader: 'hello', + 'X-Client-IP': '123', + 'X-Forwarded-For': '123', + 'Fly-Client-IP': '123', + 'CF-Connecting-IP': '123', + 'Fastly-Client-Ip': '123', + 'True-Client-Ip': '123', + 'X-Real-IP': '123', + 'X-Cluster-Client-IP': '123', + 'X-Forwarded': '123', + 'Forwarded-For': '123', + Forwarded: '123', + 'X-Vercel-Forwarded-For': '123', + }, + }; + + const optionsWithoutIP = { + include: { + ip: true, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithoutIP); + + expect(parsedRequest.request?.headers).toEqual({ + otherHeader: 'hello', + 'X-Client-IP': '123', + 'X-Forwarded-For': '123', + 'Fly-Client-IP': '123', + 'CF-Connecting-IP': '123', + 'Fastly-Client-Ip': '123', + 'True-Client-Ip': '123', + 'X-Real-IP': '123', + 'X-Cluster-Client-IP': '123', + 'X-Forwarded': '123', + 'Forwarded-For': '123', + Forwarded: '123', + 'X-Vercel-Forwarded-For': '123', + }); + }); }); describe('request properties', () => { @@ -269,6 +491,70 @@ describe('extractRequestData', () => { cookies: { foo: 'bar' }, }); }); + + it('removes IP-related headers from requestdata.headers, if `ip` is not set in the options', () => { + const mockReq = { + headers: { + otherHeader: 'hello', + 'X-Client-IP': '123', + 'X-Forwarded-For': '123', + 'Fly-Client-IP': '123', + 'CF-Connecting-IP': '123', + 'Fastly-Client-Ip': '123', + 'True-Client-Ip': '123', + 'X-Real-IP': '123', + 'X-Cluster-Client-IP': '123', + 'X-Forwarded': '123', + 'Forwarded-For': '123', + Forwarded: '123', + 'X-Vercel-Forwarded-For': '123', + }, + }; + const options = { include: ['headers'] }; + + expect(extractRequestData(mockReq, options)).toStrictEqual({ + headers: { otherHeader: 'hello' }, + }); + }); + + it('keeps IP-related headers from requestdata.headers, if `ip` is enabled in options', () => { + const mockReq = { + headers: { + otherHeader: 'hello', + 'X-Client-IP': '123', + 'X-Forwarded-For': '123', + 'Fly-Client-IP': '123', + 'CF-Connecting-IP': '123', + 'Fastly-Client-Ip': '123', + 'True-Client-Ip': '123', + 'X-Real-IP': '123', + 'X-Cluster-Client-IP': '123', + 'X-Forwarded': '123', + 'Forwarded-For': '123', + Forwarded: '123', + 'X-Vercel-Forwarded-For': '123', + }, + }; + const options = { include: ['headers', 'ip'] }; + + expect(extractRequestData(mockReq, options)).toStrictEqual({ + headers: { + otherHeader: 'hello', + 'X-Client-IP': '123', + 'X-Forwarded-For': '123', + 'Fly-Client-IP': '123', + 'CF-Connecting-IP': '123', + 'Fastly-Client-Ip': '123', + 'True-Client-Ip': '123', + 'X-Real-IP': '123', + 'X-Cluster-Client-IP': '123', + 'X-Forwarded': '123', + 'Forwarded-For': '123', + Forwarded: '123', + 'X-Vercel-Forwarded-For': '123', + }, + }); + }); }); describe('cookies', () => { @@ -502,3 +788,33 @@ describe('extractPathForTransaction', () => { expect(source).toEqual('route'); }); }); + +describe('getClientIPAddress', () => { + it.each([ + [ + '2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5,2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5, 141.101.69.35', + '2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5', + ], + [ + '2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5, 2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5, 141.101.69.35', + '2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5', + ], + [ + '2a01:cb19:8350:ed00:d0dd:INVALID_IP_ADDR:8be5,141.101.69.35,2a01:cb19:8350:ed00:d0dd:fa5b:de31:8be5', + '141.101.69.35', + ], + [ + '2b01:cb19:8350:ed00:d0dd:fa5b:nope:8be5, 2b01:cb19:NOPE:ed00:d0dd:fa5b:de31:8be5, 141.101.69.35 ', + '141.101.69.35', + ], + ['2b01:cb19:8350:ed00:d0 dd:fa5b:de31:8be5, 141.101.69.35', '141.101.69.35'], + ])('should parse the IP from the X-Forwarded-For header %s', (headerValue, expectedIP) => { + const headers = { + 'X-Forwarded-For': headerValue, + }; + + const ip = getClientIPAddress(headers); + + expect(ip).toEqual(expectedIP); + }); +}); From 21830b1115bd85fcdc536f39a3153f874d725881 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Aug 2024 16:06:11 +0200 Subject: [PATCH 18/20] ci: Streamline CI browser playwright tests (#13276) This PR streamlines our browser integration tests a bit more: 1. We now only install chromium for playwright tests in most places. This may help cut down test run time a bit, as we do not need to install native dependencies for webkit and firefox everywhere. 2. I changed how we split the matrix jobs for the browser integration tests. Previously, we sharded this via playwright. Now, we only do this for esm tests, which we now only run in chromium, not all browsers. Only the full bundle tests are run in all browsers (they are considerably faster than the esm tests...), and there we now do not use sharding, but have one matrix job for chromium (default), one for webkit and one for firefox (where each can only install the native dependencies it needs). 3. I renamed the jobs a bit so that we can see a bit more of the job name in the Github UI... it still truncates but you see more than before, at least... 4. For all other playwright tests (e.g. e2e tests etc) we only install chromium, AFAIK we do not have any non-chromium tests anywhere there. Some data points: * E2E tests seem to be about 10-20% faster than before, by looking through traces here: https://sentry.sentry.io/performance/trace/c5c1c643f1db0e8dd97df658405c2035/?field=title&field=event.type&field=project&field=user.display&field=timestamp&name=All+Events&node=txn-8892bc908791478aba163dd3edd373b4&project=5899451&query=&sort=-timestamp&source=discover&statsPeriod=1h×tamp=1723118364&yAxis=count%28%29 * For the playwright steps it's a bit harder to say as they are renamed, also they are not super consistent in execution time, we'll have to monitor it a bit afterwards, but subjectively it appears a bit shorter in most cases too. --- .github/actions/install-playwright/action.yml | 7 ++- .github/workflows/build.yml | 48 ++++++++++++------- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/.github/actions/install-playwright/action.yml b/.github/actions/install-playwright/action.yml index 29ecbcfbd2d1..7f85f5e743ba 100644 --- a/.github/actions/install-playwright/action.yml +++ b/.github/actions/install-playwright/action.yml @@ -1,5 +1,9 @@ name: "Install Playwright dependencies" description: "Installs Playwright dependencies and caches them." +inputs: + browsers: + description: 'What browsers to install.' + default: 'chromium webkit firefox' runs: using: "composite" @@ -17,12 +21,13 @@ runs: ~/.cache/ms-playwright key: playwright-${{ runner.os }}-${{ steps.playwright-version.outputs.version }} + # We always install all browsers, if uncached - name: Install Playwright dependencies (uncached) run: npx playwright install chromium webkit firefox --with-deps if: steps.playwright-cache.outputs.cache-hit != 'true' shell: bash - name: Install Playwright system dependencies only (cached) - run: npx playwright install-deps chromium webkit firefox + run: npx playwright install-deps ${{ inputs.browsers || 'chromium webkit firefox' }} if: steps.playwright-cache.outputs.cache-hit == 'true' shell: bash diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e5e9be1422e2..8ab03a313253 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -528,7 +528,7 @@ jobs: run: yarn lerna run test --scope @sentry/profiling-node job_browser_playwright_tests: - name: Playwright (${{ matrix.bundle }}${{ matrix.shard && format(' {0}/{1}', matrix.shard, matrix.shards) || ''}}) Tests + name: Playwright ${{ matrix.bundle }}${{ matrix.project && matrix.project != 'chromium' && format(' {0}', matrix.project) || ''}}${{ matrix.shard && format(' ({0}/{1})', matrix.shard, matrix.shards) || ''}} Tests needs: [job_get_metadata, job_build] if: needs.job_build.outputs.changed_browser_integration == 'true' || github.event_name != 'pull_request' runs-on: ubuntu-20.04-large-js @@ -548,31 +548,30 @@ jobs: project: - chromium include: - # Only check all projects for esm & full bundle + # Only check all projects for full bundle # We also shard the tests as they take the longest - bundle: bundle_tracing_replay_feedback_min - project: '' - shard: 1 - shards: 2 + project: 'webkit' - bundle: bundle_tracing_replay_feedback_min - project: '' - shard: 2 - shards: 2 + project: 'firefox' - bundle: esm - project: '' + project: chromium shard: 1 - shards: 3 + shards: 4 - bundle: esm + project: chromium shard: 2 - shards: 3 + shards: 4 - bundle: esm - project: '' + project: chromium shard: 3 - shards: 3 + shards: 4 + - bundle: esm + project: chromium + shard: 4 + shards: 4 exclude: - # Do not run the default chromium-only tests - - bundle: bundle_tracing_replay_feedback_min - project: 'chromium' + # Do not run the un-sharded esm tests - bundle: esm project: 'chromium' @@ -592,12 +591,15 @@ jobs: - name: Install Playwright uses: ./.github/actions/install-playwright + with: + browsers: ${{ matrix.project }} - name: Run Playwright tests env: PW_BUNDLE: ${{ matrix.bundle }} working-directory: dev-packages/browser-integration-tests run: yarn test:ci${{ matrix.project && format(' --project={0}', matrix.project) || '' }}${{ matrix.shard && format(' --shard={0}/{1}', matrix.shard, matrix.shards) || '' }} + - name: Upload Playwright Traces uses: actions/upload-artifact@v3 if: always() @@ -606,7 +608,7 @@ jobs: path: dev-packages/browser-integration-tests/test-results job_browser_loader_tests: - name: Playwright Loader (${{ matrix.bundle }}) Tests + name: PW ${{ matrix.bundle }} Tests needs: [job_get_metadata, job_build] if: needs.job_build.outputs.changed_browser_integration == 'true' || github.event_name != 'pull_request' runs-on: ubuntu-20.04 @@ -639,6 +641,8 @@ jobs: - name: Install Playwright uses: ./.github/actions/install-playwright + with: + browsers: chromium - name: Run Playwright Loader tests env: @@ -750,8 +754,12 @@ jobs: uses: ./.github/actions/restore-cache env: DEPENDENCY_CACHE_KEY: ${{ needs.job_build.outputs.dependency_cache_key }} + - name: Install Playwright uses: ./.github/actions/install-playwright + with: + browsers: chromium + - name: Run integration tests env: NODE_VERSION: ${{ matrix.node }} @@ -953,6 +961,8 @@ jobs: - name: Install Playwright uses: ./.github/actions/install-playwright + with: + browsers: chromium - name: Get node version id: versions @@ -1050,6 +1060,8 @@ jobs: - name: Install Playwright uses: ./.github/actions/install-playwright + with: + browsers: chromium - name: Get node version id: versions @@ -1150,6 +1162,8 @@ jobs: - name: Install Playwright uses: ./.github/actions/install-playwright + with: + browsers: chromium - name: Get node version id: versions From a67a69ecad5f2f6925d25cc4e44137c86db255ae Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 8 Aug 2024 18:37:53 +0200 Subject: [PATCH 19/20] feat(sveltekit): Add `wrapServerRouteWithSentry` wrapper (#13247) Add a wrapper for SvelteKit server routes. The reason is that some errors (e.g. sveltekit `error()` calls) are not caught within server (API) routes, as reported in #13224 because in contrast to `load` function we don't directly try/catch the function invokation. For now, users will have to add this wrapper manually. At a later time we can think about auto instrumentation, similarly to `load` functions but for now this will remain manual. --- .../sveltekit-2/src/routes/+page.svelte | 3 + .../src/routes/wrap-server-route/+page.svelte | 7 + .../src/routes/wrap-server-route/+page.ts | 7 + .../routes/wrap-server-route/api/+server.ts | 6 + .../sveltekit-2/tests/errors.server.test.ts | 33 +++++ packages/sveltekit/src/server/handle.ts | 40 +----- packages/sveltekit/src/server/index.ts | 1 + packages/sveltekit/src/server/load.ts | 46 +----- packages/sveltekit/src/server/serverRoute.ts | 67 +++++++++ packages/sveltekit/src/server/utils.ts | 43 +++++- .../sveltekit/test/server/serverRoute.test.ts | 133 ++++++++++++++++++ 11 files changed, 307 insertions(+), 79 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.svelte create mode 100644 dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.ts create mode 100644 dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/api/+server.ts create mode 100644 packages/sveltekit/src/server/serverRoute.ts create mode 100644 packages/sveltekit/test/server/serverRoute.test.ts diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte index e7788b6433cd..0cfae1c54741 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte @@ -38,4 +38,7 @@
  • Component Tracking
    • +
  • + server routes +
    • diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.svelte new file mode 100644 index 000000000000..adc04d52c0ea --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.svelte @@ -0,0 +1,7 @@ + + +

    + Message from API: {data.myMessage} +

    diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.ts new file mode 100644 index 000000000000..3f3f5942366e --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.ts @@ -0,0 +1,7 @@ +export const load = async ({ fetch }) => { + const res = await fetch('/wrap-server-route/api'); + const myMessage = await res.json(); + return { + myMessage: myMessage.myMessage, + }; +}; diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/api/+server.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/api/+server.ts new file mode 100644 index 000000000000..6ba210690ad5 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/api/+server.ts @@ -0,0 +1,6 @@ +import { wrapServerRouteWithSentry } from '@sentry/sveltekit'; +import { error } from '@sveltejs/kit'; + +export const GET = wrapServerRouteWithSentry(async () => { + error(500, 'error() error'); +}); diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/errors.server.test.ts index c9dc56b9c96b..fd2e58e9c2a3 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/errors.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/errors.server.test.ts @@ -58,4 +58,37 @@ test.describe('server-side errors', () => { expect(errorEvent.transaction).toEqual('GET /server-route-error'); }); + + test('captures error() thrown in server route with `wrapServerRouteWithSentry`', async ({ page }) => { + const errorEventPromise = waitForError('sveltekit-2', errorEvent => { + return errorEvent?.exception?.values?.[0]?.value === "'HttpError' captured as exception with keys: body, status"; + }); + + await page.goto('/wrap-server-route'); + + expect(await errorEventPromise).toMatchObject({ + exception: { + values: [ + { + value: "'HttpError' captured as exception with keys: body, status", + mechanism: { + handled: false, + data: { + function: 'serverRoute', + }, + }, + stacktrace: { frames: expect.any(Array) }, + }, + ], + }, + extra: { + __serialized__: { + body: { + message: 'error() error', + }, + status: 500, + }, + }, + }); + }); }); diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts index 56ddc23e1885..7f4b8d980ad8 100644 --- a/packages/sveltekit/src/server/handle.ts +++ b/packages/sveltekit/src/server/handle.ts @@ -11,21 +11,15 @@ import { withIsolationScope, } from '@sentry/core'; import { startSpan } from '@sentry/core'; -import { captureException, continueTrace } from '@sentry/node'; +import { continueTrace } from '@sentry/node'; import type { Span } from '@sentry/types'; -import { - dynamicSamplingContextToSentryBaggageHeader, - logger, - objectify, - winterCGRequestToRequestData, -} from '@sentry/utils'; +import { dynamicSamplingContextToSentryBaggageHeader, logger, winterCGRequestToRequestData } from '@sentry/utils'; import type { Handle, ResolveOptions } from '@sveltejs/kit'; import { getDynamicSamplingContextFromSpan } from '@sentry/opentelemetry'; import { DEBUG_BUILD } from '../common/debug-build'; -import { isHttpError, isRedirect } from '../common/utils'; -import { flushIfServerless, getTracePropagationData } from './utils'; +import { flushIfServerless, getTracePropagationData, sendErrorToSentry } from './utils'; export type SentryHandleOptions = { /** @@ -62,32 +56,6 @@ export type SentryHandleOptions = { fetchProxyScriptNonce?: string; }; -function sendErrorToSentry(e: unknown): unknown { - // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can - // store a seen flag on it. - const objectifiedErr = objectify(e); - - // similarly to the `load` function, we don't want to capture 4xx errors or redirects - if ( - isRedirect(objectifiedErr) || - (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) - ) { - return objectifiedErr; - } - - captureException(objectifiedErr, { - mechanism: { - type: 'sveltekit', - handled: false, - data: { - function: 'handle', - }, - }, - }); - - return objectifiedErr; -} - /** * Exported only for testing */ @@ -225,7 +193,7 @@ async function instrumentHandle( ); return resolveResult; } catch (e: unknown) { - sendErrorToSentry(e); + sendErrorToSentry(e, 'handle'); throw e; } finally { await flushIfServerless(); diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index 99813d01ceba..32dd6627d7a6 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -129,6 +129,7 @@ export { init } from './sdk'; export { handleErrorWithSentry } from './handleError'; export { wrapLoadWithSentry, wrapServerLoadWithSentry } from './load'; export { sentryHandle } from './handle'; +export { wrapServerRouteWithSentry } from './serverRoute'; /** * Tracks the Svelte component's initialization and mounting operation as well as diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index fe61ed0913bd..82a8c548c6ec 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -1,49 +1,13 @@ -import { - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - captureException, - startSpan, -} from '@sentry/node'; -import { addNonEnumerableProperty, objectify } from '@sentry/utils'; +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, startSpan } from '@sentry/node'; +import { addNonEnumerableProperty } from '@sentry/utils'; import type { LoadEvent, ServerLoadEvent } from '@sveltejs/kit'; import type { SentryWrappedFlag } from '../common/utils'; -import { isHttpError, isRedirect } from '../common/utils'; -import { flushIfServerless } from './utils'; +import { flushIfServerless, sendErrorToSentry } from './utils'; type PatchedLoadEvent = LoadEvent & SentryWrappedFlag; type PatchedServerLoadEvent = ServerLoadEvent & SentryWrappedFlag; -function sendErrorToSentry(e: unknown): unknown { - // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can - // store a seen flag on it. - const objectifiedErr = objectify(e); - - // The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error - // If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they - // could be noisy. - // Also the `redirect(...)` helper is used to redirect users from one page to another. We don't want to capture thrown - // `Redirect`s as they're not errors but expected behaviour - if ( - isRedirect(objectifiedErr) || - (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) - ) { - return objectifiedErr; - } - - captureException(objectifiedErr, { - mechanism: { - type: 'sveltekit', - handled: false, - data: { - function: 'load', - }, - }, - }); - - return objectifiedErr; -} - /** * @inheritdoc */ @@ -81,7 +45,7 @@ export function wrapLoadWithSentry any>(origLoad: T) () => wrappingTarget.apply(thisArg, args), ); } catch (e) { - sendErrorToSentry(e); + sendErrorToSentry(e, 'load'); throw e; } finally { await flushIfServerless(); @@ -146,7 +110,7 @@ export function wrapServerLoadWithSentry any>(origSe () => wrappingTarget.apply(thisArg, args), ); } catch (e: unknown) { - sendErrorToSentry(e); + sendErrorToSentry(e, 'load'); throw e; } finally { await flushIfServerless(); diff --git a/packages/sveltekit/src/server/serverRoute.ts b/packages/sveltekit/src/server/serverRoute.ts new file mode 100644 index 000000000000..a5f13f9a73ca --- /dev/null +++ b/packages/sveltekit/src/server/serverRoute.ts @@ -0,0 +1,67 @@ +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, startSpan } from '@sentry/node'; +import { addNonEnumerableProperty } from '@sentry/utils'; +import type { RequestEvent } from '@sveltejs/kit'; +import { flushIfServerless, sendErrorToSentry } from './utils'; + +type PatchedServerRouteEvent = RequestEvent & { __sentry_wrapped__?: boolean }; + +/** + * Wraps a server route handler for API or server routes registered in `+server.(js|js)` files. + * + * This function will automatically capture any errors that occur during the execution of the route handler + * and it will start a span for the duration of your route handler. + * + * @example + * ```js + * import { wrapServerRouteWithSentry } from '@sentry/sveltekit'; + * + * const get = async event => { + * return new Response(JSON.stringify({ message: 'hello world' })); + * } + * + * export const GET = wrapServerRouteWithSentry(get); + * ``` + * + * @param originalRouteHandler your server route handler + * @param httpMethod the HTTP method of your route handler + * + * @returns a wrapped version of your server route handler + */ +export function wrapServerRouteWithSentry( + originalRouteHandler: (request: RequestEvent) => Promise, +): (requestEvent: RequestEvent) => Promise { + return new Proxy(originalRouteHandler, { + apply: async (wrappingTarget, thisArg, args) => { + const event = args[0] as PatchedServerRouteEvent; + + if (event.__sentry_wrapped__) { + return wrappingTarget.apply(thisArg, args); + } + + const routeId = event.route && event.route.id; + const httpMethod = event.request.method; + + addNonEnumerableProperty(event as unknown as Record, '__sentry_wrapped__', true); + + try { + return await startSpan( + { + name: `${httpMethod} ${routeId || 'Server Route'}`, + op: `function.sveltekit.server.${httpMethod.toLowerCase()}`, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.sveltekit', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + onlyIfParent: true, + }, + () => wrappingTarget.apply(thisArg, args), + ); + } catch (e) { + sendErrorToSentry(e, 'serverRoute'); + throw e; + } finally { + await flushIfServerless(); + } + }, + }); +} diff --git a/packages/sveltekit/src/server/utils.ts b/packages/sveltekit/src/server/utils.ts index 0db8ee893783..8d7f2c649331 100644 --- a/packages/sveltekit/src/server/utils.ts +++ b/packages/sveltekit/src/server/utils.ts @@ -1,8 +1,9 @@ -import { flush } from '@sentry/node'; -import { logger } from '@sentry/utils'; +import { captureException, flush } from '@sentry/node'; +import { logger, objectify } from '@sentry/utils'; import type { RequestEvent } from '@sveltejs/kit'; import { DEBUG_BUILD } from '../common/debug-build'; +import { isHttpError, isRedirect } from '../common/utils'; /** * Takes a request event and extracts traceparent and DSC data @@ -31,3 +32,41 @@ export async function flushIfServerless(): Promise { } } } + +/** + * Extracts a server-side sveltekit error, filters a couple of known errors we don't want to capture + * and captures the error via `captureException`. + * + * @param e error + * + * @returns an objectified version of @param e + */ +export function sendErrorToSentry(e: unknown, handlerFn: 'handle' | 'load' | 'serverRoute'): object { + // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can + // store a seen flag on it. + const objectifiedErr = objectify(e); + + // The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error + // If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they + // could be noisy. + // Also the `redirect(...)` helper is used to redirect users from one page to another. We don't want to capture thrown + // `Redirect`s as they're not errors but expected behaviour + if ( + isRedirect(objectifiedErr) || + (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) + ) { + return objectifiedErr; + } + + captureException(objectifiedErr, { + mechanism: { + type: 'sveltekit', + handled: false, + data: { + function: handlerFn, + }, + }, + }); + + return objectifiedErr; +} diff --git a/packages/sveltekit/test/server/serverRoute.test.ts b/packages/sveltekit/test/server/serverRoute.test.ts new file mode 100644 index 000000000000..de99db5a548e --- /dev/null +++ b/packages/sveltekit/test/server/serverRoute.test.ts @@ -0,0 +1,133 @@ +import * as SentryNode from '@sentry/node'; +import type { NumericRange } from '@sveltejs/kit'; +import { type RequestEvent, error, redirect } from '@sveltejs/kit'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, + wrapServerRouteWithSentry, +} from '../../src/server'; + +describe('wrapServerRouteWithSentry', () => { + const originalRouteHandler = vi.fn(); + + const getRequestEventMock = () => + ({ + request: { + method: 'GET', + }, + route: { + id: '/api/users/:id', + }, + }) as RequestEvent; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('wraps a server route span around the original server route handler', () => { + const startSpanSpy = vi.spyOn(SentryNode, 'startSpan'); + + it('assigns the route id as name if available', () => { + const wrappedRouteHandler = wrapServerRouteWithSentry(originalRouteHandler); + + wrappedRouteHandler(getRequestEventMock() as RequestEvent); + + expect(startSpanSpy).toHaveBeenCalledWith( + { + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.sveltekit', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + name: 'GET /api/users/:id', + onlyIfParent: true, + op: 'function.sveltekit.server.get', + }, + expect.any(Function), + ); + + expect(originalRouteHandler).toHaveBeenCalledTimes(1); + }); + + it('falls back to a generic name if route id is not available', () => { + const wrappedRouteHandler = wrapServerRouteWithSentry(originalRouteHandler); + + wrappedRouteHandler({ ...getRequestEventMock(), route: undefined } as unknown as RequestEvent); + + expect(startSpanSpy).toHaveBeenCalledWith( + { + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.sveltekit', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + name: 'GET Server Route', + onlyIfParent: true, + op: 'function.sveltekit.server.get', + }, + expect.any(Function), + ); + + expect(originalRouteHandler).toHaveBeenCalledTimes(1); + }); + }); + + const captureExceptionSpy = vi.spyOn(SentryNode, 'captureException'); + describe('captures server route errors', () => { + it('captures and rethrows normal server route error', async () => { + const error = new Error('Server Route Error'); + + const wrappedRouteHandler = wrapServerRouteWithSentry(() => { + throw error; + }); + + await expect(async () => { + await wrappedRouteHandler(getRequestEventMock() as RequestEvent); + }).rejects.toThrowError('Server Route Error'); + + expect(captureExceptionSpy).toHaveBeenCalledWith(error, { + mechanism: { type: 'sveltekit', handled: false, data: { function: 'serverRoute' } }, + }); + }); + + it.each([500, 501, 599])('captures and rethrows %s error() calls', async status => { + const wrappedRouteHandler = wrapServerRouteWithSentry(() => { + error(status as NumericRange<400, 599>, `error(${status}) error`); + }); + + await expect(async () => { + await wrappedRouteHandler(getRequestEventMock() as RequestEvent); + }).rejects.toThrow(); + + expect(captureExceptionSpy).toHaveBeenCalledWith( + { body: { message: `error(${status}) error` }, status }, + { + mechanism: { type: 'sveltekit', handled: false, data: { function: 'serverRoute' } }, + }, + ); + }); + + it.each([400, 401, 499])("doesn't capture but rethrows %s error() calls", async status => { + const wrappedRouteHandler = wrapServerRouteWithSentry(() => { + error(status as NumericRange<400, 599>, `error(${status}) error`); + }); + + await expect(async () => { + await wrappedRouteHandler(getRequestEventMock() as RequestEvent); + }).rejects.toThrow(); + + expect(captureExceptionSpy).not.toHaveBeenCalled(); + }); + + it.each([300, 301, 308])("doesn't capture redirect(%s) calls", async status => { + const wrappedRouteHandler = wrapServerRouteWithSentry(() => { + redirect(status as NumericRange<300, 308>, '/redirect'); + }); + + await expect(async () => { + await wrappedRouteHandler(getRequestEventMock() as RequestEvent); + }).rejects.toThrow(); + + expect(captureExceptionSpy).not.toHaveBeenCalled(); + }); + }); +}); From 476a51bab888e455dbd5afdf7ebfacdbdc9b1f0a Mon Sep 17 00:00:00 2001 From: Andrei Borza Date: Thu, 8 Aug 2024 18:52:21 +0200 Subject: [PATCH 20/20] meta(changelog): Update changelog for 8.25.0 --- CHANGELOG.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ad5a1920aae..2e8d141efd95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,28 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +## 8.25.0 + +### Important Changes + +- **Alpha release of Official Solid Start SDK** + +This release contains the alpha version of `@sentry/solidstart`, our SDK for [Solid Start](https://start.solidjs.com/)! +For details on how to use it, please see the [README](./packages/solidstart/README.md). Any feedback/bug reports are +greatly appreciated, please [reach out on GitHub](https://github.com/getsentry/sentry-javascript/issues/12538). + +### Other Changes + +- feat(astro): Add `bundleSizeOptimizations` vite options to integration (#13250) +- feat(astro): Always add BrowserTracing (#13244) +- feat(core): Add `getTraceMetaTags` function (#13201) +- feat(nestjs): Automatic instrumentation of nestjs exception filters (#13230) +- feat(node): Add `useOperationNameForRootSpan` to`graphqlIntegration` (#13248) +- feat(sveltekit): Add `wrapServerRouteWithSentry` wrapper (#13247) +- fix(aws-serverless): Extract sentry trace data from handler `context` over `event` (#13266) +- fix(browser): Initialize default integration if `defaultIntegrations: undefined` (#13261) +- fix(utils): Streamline IP capturing on incoming requests (#13272) + ## 8.24.0 - feat(nestjs): Filter RPC exceptions (#13227)