From 89377da232017b19b5200f8fc5dc2331dcffe99f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 09:51:19 +0200 Subject: [PATCH 01/13] test(node): Add test for errors-only ESM app (#12046) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This tests a node app that uses ESM, but no `--import` flag. Somehow this works for `http` (but not other packages...) but this is fine for errors-only mode, for now. Missing: We do show the warning for missing express instrumentation there, still 😬 we may need to tweak this... --- .github/workflows/build.yml | 1 + .../node-express-esm-without-loader/.npmrc | 2 + .../package.json | 24 +++++++ .../playwright.config.ts | 70 +++++++++++++++++++ .../src/app.mjs | 46 ++++++++++++ .../src/instrument.mjs | 7 ++ .../start-event-proxy.mjs | 6 ++ .../tests/server.test.ts | 35 ++++++++++ 8 files changed, 191 insertions(+) create mode 100644 dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/.npmrc create mode 100644 dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/package.json create mode 100644 dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/playwright.config.ts create mode 100644 dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/src/app.mjs create mode 100644 dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/src/instrument.mjs create mode 100644 dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/start-event-proxy.mjs create mode 100644 dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/tests/server.test.ts diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d617b09bea01..bcbef8e06a8e 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1004,6 +1004,7 @@ jobs: 'create-remix-app-express-vite-dev', 'debug-id-sourcemaps', 'node-express-esm-loader', + 'node-express-esm-without-loader', 'nextjs-app-dir', 'nextjs-14', 'react-create-hash-router', diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/.npmrc b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/.npmrc new file mode 100644 index 000000000000..070f80f05092 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/.npmrc @@ -0,0 +1,2 @@ +@sentry:registry=http://127.0.0.1:4873 +@sentry-internal:registry=http://127.0.0.1:4873 diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/package.json b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/package.json new file mode 100644 index 000000000000..b339fa65d2a2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/package.json @@ -0,0 +1,24 @@ +{ + "name": "node-express-esm-without-loader", + "version": "1.0.0", + "private": true, + "scripts": { + "start": "node src/app.mjs", + "clean": "npx rimraf node_modules pnpm-lock.yaml", + "test:build": "pnpm install", + "test:assert": "playwright test" + }, + "dependencies": { + "@sentry/node": "latest || *", + "@sentry/opentelemetry": "latest || *", + "express": "4.19.2" + }, + "devDependencies": { + "@sentry-internal/event-proxy-server": "link:../../../event-proxy-server", + "@playwright/test": "^1.27.1" + }, + "volta": { + "extends": "../../package.json", + "node": "18.19.1" + } +} diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/playwright.config.ts b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/playwright.config.ts new file mode 100644 index 000000000000..5e672ed97676 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/playwright.config.ts @@ -0,0 +1,70 @@ +import type { PlaywrightTestConfig } from '@playwright/test'; +import { devices } from '@playwright/test'; + +// Fix urls not resolving to localhost on Node v17+ +// See: https://github.com/axios/axios/issues/3821#issuecomment-1413727575 +import { setDefaultResultOrder } from 'dns'; +setDefaultResultOrder('ipv4first'); + +const eventProxyPort = 3031; +const expressPort = 3030; + +/** + * See https://playwright.dev/docs/test-configuration. + */ +const config: PlaywrightTestConfig = { + testDir: './tests', + /* Maximum time one test can run for. */ + timeout: 150_000, + expect: { + /** + * Maximum time expect() should wait for the condition to be met. + * For example in `await expect(locator).toHaveText();` + */ + timeout: 5000, + }, + /* Run tests in files in parallel */ + fullyParallel: true, + /* Fail the build on CI if you accidentally left test.only in the source code. */ + forbidOnly: !!process.env.CI, + /* Retry on CI only */ + retries: 0, + /* Reporter to use. See https://playwright.dev/docs/test-reporters */ + reporter: 'list', + /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */ + use: { + /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */ + actionTimeout: 0, + + /* Base URL to use in actions like `await page.goto('/')`. */ + baseURL: `http://localhost:${expressPort}`, + }, + + /* Configure projects for major browsers */ + projects: [ + { + name: 'chromium', + use: { + ...devices['Desktop Chrome'], + }, + }, + ], + + /* Run your local dev server before starting the tests */ + webServer: [ + { + command: 'node start-event-proxy.mjs', + port: eventProxyPort, + stdout: 'pipe', + stderr: 'pipe', + }, + { + command: 'pnpm start', + port: expressPort, + stdout: 'pipe', + stderr: 'pipe', + }, + ], +}; + +export default config; diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/src/app.mjs b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/src/app.mjs new file mode 100644 index 000000000000..0d318ab5fc13 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/src/app.mjs @@ -0,0 +1,46 @@ +import './instrument.mjs'; + +// Below other imports +import * as Sentry from '@sentry/node'; +import express from 'express'; + +const app = express(); +const port = 3030; + +app.get('/test-success', function (req, res) { + setTimeout(() => { + res.status(200).end(); + }, 100); +}); + +app.get('/test-params/:param', function (req, res) { + const { param } = req.params; + Sentry.setTag(`param-${param}`, 'yes'); + Sentry.captureException(new Error(`Error for param ${param}`)); + + setTimeout(() => { + res.status(200).end(); + }, 100); +}); + +app.get('/test-error', function (req, res) { + Sentry.captureException(new Error('This is an error')); + setTimeout(() => { + Sentry.flush(2000).then(() => { + res.status(200).end(); + }); + }, 100); +}); + +Sentry.setupExpressErrorHandler(app); + +app.use(function onError(err, req, res, next) { + // The error id is attached to `res.sentry` to be returned + // and optionally displayed to the user for support. + res.statusCode = 500; + res.end(res.sentry + '\n'); +}); + +app.listen(port, () => { + console.log(`Example app listening on port ${port}`); +}); diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/src/instrument.mjs b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/src/instrument.mjs new file mode 100644 index 000000000000..1636d10e836c --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/src/instrument.mjs @@ -0,0 +1,7 @@ +import * as Sentry from '@sentry/node'; + +Sentry.init({ + environment: 'qa', // dynamic sampling bias to keep transactions + dsn: process.env.E2E_TEST_DSN, + tunnel: `http://localhost:3031/`, // proxy server +}); diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/start-event-proxy.mjs b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/start-event-proxy.mjs new file mode 100644 index 000000000000..df0fdb65c929 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/start-event-proxy.mjs @@ -0,0 +1,6 @@ +import { startEventProxyServer } from '@sentry-internal/event-proxy-server'; + +startEventProxyServer({ + port: 3031, + proxyServerName: 'node-express-esm-without-loader', +}); diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/tests/server.test.ts b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/tests/server.test.ts new file mode 100644 index 000000000000..c2c076f13e4e --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/tests/server.test.ts @@ -0,0 +1,35 @@ +import { expect, test } from '@playwright/test'; +import { waitForError } from '@sentry-internal/event-proxy-server'; + +test('Should record exceptions captured inside handlers', async ({ request }) => { + const errorEventPromise = waitForError('node-express-esm-without-loader', errorEvent => { + return !!errorEvent?.exception?.values?.[0]?.value?.includes('This is an error'); + }); + + await request.get('/test-error'); + + await expect(errorEventPromise).resolves.toBeDefined(); +}); + +test('Isolates requests', async ({ request }) => { + const errorEventPromise = waitForError('node-express-esm-without-loader', errorEvent => { + return !!errorEvent?.exception?.values?.[0]?.value?.includes('Error for param 1'); + }); + + const errorEventPromise2 = waitForError('node-express-esm-without-loader', errorEvent => { + return !!errorEvent?.exception?.values?.[0]?.value?.includes('Error for param 2'); + }); + + await request.get('/test-params/1'); + await request.get('/test-params/2'); + + const errorEvent1 = await errorEventPromise; + const errorEvent2 = await errorEventPromise2; + + expect(errorEvent1.tags).toEqual({ 'param-1': 'yes' }); + expect(errorEvent2.tags).toEqual({ 'param-2': 'yes' }); + + // Transaction is not set, since we have no expressIntegration by default + expect(errorEvent1.transaction).toBeUndefined(); + expect(errorEvent2.transaction).toBeUndefined(); +}); From ccb98ffc27221ea1c8faa16b9dba1753329f104f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 11:12:28 +0200 Subject: [PATCH 02/13] feat(browser): Improve idle span handling (#12065) Closes https://github.com/getsentry/sentry-javascript/issues/12051 This PR does two things: 1. Ensures we add helpful attributes to idle spans more consistently. We now add the `sentry.idle_span_finish_reason` attribute always for idle spans, not only when op == `ui.action.click`. This should also make it easier to debug stuff etc, and I see no reason to not always set this. Additionally, we also keep the number of spans we discarded (if any) for easier debugging too (as `sentry.idle_span_discarded_spans`). 2. We ensure that idle spans cannot exceed the configured `finalTimeout`. Previously, due to the order of things, it was possible that we ended a span very late, if it had a child span with a very late end timestamp (as we took the last child span timestamp). Possibly this could lead to overly long transactions. I think 2, combined with the fact that later we _do_ filter out child spans that ended after idle span, lead to the incorrect-length spans. --- packages/core/src/tracing/idleSpan.ts | 16 +- .../core/test/lib/tracing/idleSpan.test.ts | 177 +++++++++++++++++- 2 files changed, 183 insertions(+), 10 deletions(-) diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 0a1a279f7b5c..2f9230b7dd4b 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -129,15 +129,17 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti const latestSpanEndTimestamp = childEndTimestamps.length ? Math.max(...childEndTimestamps) : undefined; const spanEndTimestamp = spanTimeInputToSeconds(timestamp); + // In reality this should always exist here, but type-wise it may be undefined... const spanStartTimestamp = spanToJSON(span).start_timestamp; // The final endTimestamp should: // * Never be before the span start timestamp // * Be the latestSpanEndTimestamp, if there is one, and it is smaller than the passed span end timestamp // * Otherwise be the passed end timestamp - const endTimestamp = Math.max( - spanStartTimestamp || -Infinity, - Math.min(spanEndTimestamp, latestSpanEndTimestamp || Infinity), + // Final timestamp can never be after finalTimeout + const endTimestamp = Math.min( + spanStartTimestamp ? spanStartTimestamp + finalTimeout / 1000 : Infinity, + Math.max(spanStartTimestamp || -Infinity, Math.min(spanEndTimestamp, latestSpanEndTimestamp || Infinity)), ); span.end(endTimestamp); @@ -240,7 +242,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti } const attributes: SpanAttributes = spanJSON.data || {}; - if (spanJSON.op === 'ui.action.click' && !attributes[SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON]) { + if (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON]) { span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON, _finishReason); } @@ -248,6 +250,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti const childSpans = getSpanDescendants(span).filter(child => child !== span); + let discardedSpans = 0; childSpans.forEach(childSpan => { // We cancel all pending spans with status "cancelled" to indicate the idle span was finished early if (childSpan.isRecording()) { @@ -277,8 +280,13 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti if (!spanEndedBeforeFinalTimeout || !spanStartedBeforeIdleSpanEnd) { removeChildSpanFromSpan(span, childSpan); + discardedSpans++; } }); + + if (discardedSpans > 0) { + span.setAttribute('sentry.idle_span_discarded_spans', discardedSpans); + } } client.on('spanStart', startedSpan => { diff --git a/packages/core/test/lib/tracing/idleSpan.test.ts b/packages/core/test/lib/tracing/idleSpan.test.ts index 5896e80654e5..1624160736ea 100644 --- a/packages/core/test/lib/tracing/idleSpan.test.ts +++ b/packages/core/test/lib/tracing/idleSpan.test.ts @@ -230,6 +230,81 @@ describe('startIdleSpan', () => { ); }); + it('Ensures idle span cannot exceed finalTimeout', () => { + const transactions: Event[] = []; + const beforeSendTransaction = jest.fn(event => { + transactions.push(event); + return null; + }); + const options = getDefaultTestClientOptions({ + dsn, + tracesSampleRate: 1, + beforeSendTransaction, + }); + const client = new TestClient(options); + setCurrentClient(client); + client.init(); + + // We want to accomodate a bit of drift there, so we ensure this starts earlier... + const finalTimeout = 99_999; + const baseTimeInSeconds = Math.floor(Date.now() / 1000) - 9999; + + const idleSpan = startIdleSpan({ name: 'idle span', startTime: baseTimeInSeconds }, { finalTimeout: finalTimeout }); + expect(idleSpan).toBeDefined(); + + // regular child - should be kept + const regularSpan = startInactiveSpan({ + name: 'regular span', + startTime: baseTimeInSeconds + 2, + }); + regularSpan.end(baseTimeInSeconds + 4); + + // very late ending span + const discardedSpan = startInactiveSpan({ name: 'discarded span', startTime: baseTimeInSeconds + 99 }); + discardedSpan.end(baseTimeInSeconds + finalTimeout + 100); + + // Should be cancelled - will not finish + const cancelledSpan = startInactiveSpan({ + name: 'cancelled span', + startTime: baseTimeInSeconds + 4, + }); + + jest.runOnlyPendingTimers(); + + expect(regularSpan.isRecording()).toBe(false); + expect(idleSpan.isRecording()).toBe(false); + expect(discardedSpan.isRecording()).toBe(false); + expect(cancelledSpan.isRecording()).toBe(false); + + expect(beforeSendTransaction).toHaveBeenCalledTimes(1); + const transaction = transactions[0]; + + // End time is based on idle time etc. + const idleSpanEndTime = transaction.timestamp!; + expect(idleSpanEndTime).toEqual(baseTimeInSeconds + finalTimeout / 1000); + + expect(transaction.spans).toHaveLength(2); + expect(transaction.spans).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + description: 'regular span', + timestamp: baseTimeInSeconds + 4, + start_timestamp: baseTimeInSeconds + 2, + }), + ]), + ); + expect(transaction.spans).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + description: 'cancelled span', + timestamp: idleSpanEndTime, + start_timestamp: baseTimeInSeconds + 4, + status: 'cancelled', + }), + ]), + ); + }); + it('emits span hooks', () => { const client = getClient()!; @@ -274,6 +349,27 @@ describe('startIdleSpan', () => { expect(recordDroppedEventSpy).toHaveBeenCalledWith('sample_rate', 'transaction'); }); + it('sets finish reason when span is ended manually', () => { + let transaction: Event | undefined; + const beforeSendTransaction = jest.fn(event => { + transaction = event; + return null; + }); + const options = getDefaultTestClientOptions({ dsn, tracesSampleRate: 1, beforeSendTransaction }); + const client = new TestClient(options); + setCurrentClient(client); + client.init(); + + const span = startIdleSpan({ name: 'foo' }); + span.end(); + jest.runOnlyPendingTimers(); + + expect(beforeSendTransaction).toHaveBeenCalledTimes(1); + expect(transaction?.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON]).toEqual( + 'externalFinish', + ); + }); + it('sets finish reason when span ends', () => { let transaction: Event | undefined; const beforeSendTransaction = jest.fn(event => { @@ -285,8 +381,7 @@ describe('startIdleSpan', () => { setCurrentClient(client); client.init(); - // This is only set when op === 'ui.action.click' - startIdleSpan({ name: 'foo', op: 'ui.action.click' }); + startIdleSpan({ name: 'foo' }); startSpan({ name: 'inner' }, () => {}); jest.runOnlyPendingTimers(); @@ -296,6 +391,57 @@ describe('startIdleSpan', () => { ); }); + it('sets finish reason when span ends via expired heartbeat timeout', () => { + let transaction: Event | undefined; + const beforeSendTransaction = jest.fn(event => { + transaction = event; + return null; + }); + const options = getDefaultTestClientOptions({ dsn, tracesSampleRate: 1, beforeSendTransaction }); + const client = new TestClient(options); + setCurrentClient(client); + client.init(); + + startIdleSpan({ name: 'foo' }); + startSpanManual({ name: 'inner' }, () => {}); + jest.runOnlyPendingTimers(); + + expect(beforeSendTransaction).toHaveBeenCalledTimes(1); + expect(transaction?.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON]).toEqual( + 'heartbeatFailed', + ); + }); + + it('sets finish reason when span ends via final timeout', () => { + let transaction: Event | undefined; + const beforeSendTransaction = jest.fn(event => { + transaction = event; + return null; + }); + const options = getDefaultTestClientOptions({ dsn, tracesSampleRate: 1, beforeSendTransaction }); + const client = new TestClient(options); + setCurrentClient(client); + client.init(); + + startIdleSpan({ name: 'foo' }, { finalTimeout: TRACING_DEFAULTS.childSpanTimeout * 2 }); + + const span1 = startInactiveSpan({ name: 'inner' }); + jest.advanceTimersByTime(TRACING_DEFAULTS.childSpanTimeout - 1); + span1.end(); + + const span2 = startInactiveSpan({ name: 'inner2' }); + jest.advanceTimersByTime(TRACING_DEFAULTS.childSpanTimeout - 1); + span2.end(); + + startInactiveSpan({ name: 'inner3' }); + jest.runOnlyPendingTimers(); + + expect(beforeSendTransaction).toHaveBeenCalledTimes(1); + expect(transaction?.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON]).toEqual( + 'finalTimeout', + ); + }); + it('uses finish reason set outside when span ends', () => { let transaction: Event | undefined; const beforeSendTransaction = jest.fn(event => { @@ -307,8 +453,7 @@ describe('startIdleSpan', () => { setCurrentClient(client); client.init(); - // This is only set when op === 'ui.action.click' - const span = startIdleSpan({ name: 'foo', op: 'ui.action.click' }); + const span = startIdleSpan({ name: 'foo' }); span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON, 'custom reason'); startSpan({ name: 'inner' }, () => {}); jest.runOnlyPendingTimers(); @@ -496,7 +641,7 @@ describe('startIdleSpan', () => { describe('trim end timestamp', () => { it('trims end to highest child span end', () => { - const idleSpan = startIdleSpan({ name: 'foo', startTime: 1000 }); + const idleSpan = startIdleSpan({ name: 'foo', startTime: 1000 }, { finalTimeout: 99_999_999 }); expect(idleSpan).toBeDefined(); const span1 = startInactiveSpan({ name: 'span1', startTime: 1001 }); @@ -515,8 +660,28 @@ describe('startIdleSpan', () => { expect(spanToJSON(idleSpan!).timestamp).toBe(1100); }); + it('trims end to final timeout', () => { + const idleSpan = startIdleSpan({ name: 'foo', startTime: 1000 }, { finalTimeout: 30_000 }); + expect(idleSpan).toBeDefined(); + + const span1 = startInactiveSpan({ name: 'span1', startTime: 1001 }); + span1?.end(1005); + + const span2 = startInactiveSpan({ name: 'span2', startTime: 1002 }); + span2?.end(1100); + + const span3 = startInactiveSpan({ name: 'span1', startTime: 1050 }); + span3?.end(1060); + + expect(getActiveSpan()).toBe(idleSpan); + + jest.runAllTimers(); + + expect(spanToJSON(idleSpan!).timestamp).toBe(1030); + }); + it('keeps lower span endTime than highest child span end', () => { - const idleSpan = startIdleSpan({ name: 'foo', startTime: 1000 }); + const idleSpan = startIdleSpan({ name: 'foo', startTime: 1000 }, { finalTimeout: 99_999_999 }); expect(idleSpan).toBeDefined(); const span1 = startInactiveSpan({ name: 'span1', startTime: 999_999_999 }); From d63438564738188d1d928c89c7b95ae781319615 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 11:12:44 +0200 Subject: [PATCH 03/13] fix(node): Fix check for performance integrations (#12043) ref https://github.com/getsentry/sentry-javascript/issues/12034 cc @AbhiPrasad when you're back, let's look again at `hasTracingEnabled()` and see if we want to adjust this maybe. --- packages/node/src/sdk/init.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/node/src/sdk/init.ts b/packages/node/src/sdk/init.ts index 5ca3a743531a..c03af9ec09d5 100644 --- a/packages/node/src/sdk/init.ts +++ b/packages/node/src/sdk/init.ts @@ -78,10 +78,23 @@ export function getDefaultIntegrationsWithoutPerformance(): Integration[] { export function getDefaultIntegrations(options: Options): Integration[] { return [ ...getDefaultIntegrationsWithoutPerformance(), - ...(hasTracingEnabled(options) ? getAutoPerformanceIntegrations() : []), + // We only add performance integrations if tracing is enabled + // Note that this means that without tracing enabled, e.g. `expressIntegration()` will not be added + // This means that generally request isolation will work (because that is done by httpIntegration) + // But `transactionName` will not be set automatically + ...(shouldAddPerformanceIntegrations(options) ? getAutoPerformanceIntegrations() : []), ]; } +function shouldAddPerformanceIntegrations(options: Options): boolean { + if (!hasTracingEnabled(options)) { + return false; + } + + // We want to ensure `tracesSampleRate` is not just undefined/null here + return options.enableTracing || options.tracesSampleRate != null || 'tracesSampler' in options; +} + declare const __IMPORT_META_URL_REPLACEMENT__: string; /** From 677602f86231cf1cdc3285e5c24203e1c8524a18 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 11:12:55 +0200 Subject: [PATCH 04/13] fix(browser): Use consistent timestamps (#12063) In some places we used `Date.now()`, instead of `timestampInSeconds()` which used performance.now(). There are still other places where we use `Date.now()`, esp. in replay and node, but I focused on regular browser stuff for now (because this is most likely to be affected by this). --- packages/browser-utils/src/instrument/xhr.ts | 8 ++++---- packages/browser/src/profiling/utils.ts | 13 ++++++++++--- packages/core/src/integrations/sessiontiming.ts | 5 +++-- packages/utils/src/instrument/fetch.ts | 7 ++++--- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/packages/browser-utils/src/instrument/xhr.ts b/packages/browser-utils/src/instrument/xhr.ts index c504c8dce5f6..c3fa2ec6a62c 100644 --- a/packages/browser-utils/src/instrument/xhr.ts +++ b/packages/browser-utils/src/instrument/xhr.ts @@ -1,6 +1,6 @@ import type { HandlerDataXhr, SentryWrappedXMLHttpRequest, WrappedFunction } from '@sentry/types'; -import { addHandler, fill, isString, maybeInstrument, triggerHandlers } from '@sentry/utils'; +import { addHandler, fill, isString, maybeInstrument, timestampInSeconds, triggerHandlers } from '@sentry/utils'; import { WINDOW } from '../metrics/types'; export const SENTRY_XHR_DATA_KEY = '__sentry_xhr_v3__'; @@ -31,7 +31,7 @@ export function instrumentXHR(): void { fill(xhrproto, 'open', function (originalOpen: () => void): () => void { return function (this: XMLHttpRequest & SentryWrappedXMLHttpRequest, ...args: unknown[]): void { - const startTimestamp = Date.now(); + const startTimestamp = timestampInSeconds() * 1000; // open() should always be called with two or more arguments // But to be on the safe side, we actually validate this and bail out if we don't have a method & url @@ -71,7 +71,7 @@ export function instrumentXHR(): void { } const handlerData: HandlerDataXhr = { - endTimestamp: Date.now(), + endTimestamp: timestampInSeconds() * 1000, startTimestamp, xhr: this, }; @@ -124,7 +124,7 @@ export function instrumentXHR(): void { } const handlerData: HandlerDataXhr = { - startTimestamp: Date.now(), + startTimestamp: timestampInSeconds() * 1000, xhr: this, }; triggerHandlers('xhr', handlerData); diff --git a/packages/browser/src/profiling/utils.ts b/packages/browser/src/profiling/utils.ts index e13c731644b5..7c22f938f619 100644 --- a/packages/browser/src/profiling/utils.ts +++ b/packages/browser/src/profiling/utils.ts @@ -12,7 +12,14 @@ import type { StackParser, ThreadCpuProfile, } from '@sentry/types'; -import { GLOBAL_OBJ, browserPerformanceTimeOrigin, forEachEnvelopeItem, logger, uuid4 } from '@sentry/utils'; +import { + GLOBAL_OBJ, + browserPerformanceTimeOrigin, + forEachEnvelopeItem, + logger, + timestampInSeconds, + uuid4, +} from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; import { WINDOW } from '../helpers'; @@ -152,8 +159,8 @@ export function createProfilePayload( ? start_timestamp : typeof event.start_timestamp === 'number' ? event.start_timestamp * 1000 - : Date.now(); - const transactionEndMs = typeof event.timestamp === 'number' ? event.timestamp * 1000 : Date.now(); + : timestampInSeconds() * 1000; + const transactionEndMs = typeof event.timestamp === 'number' ? event.timestamp * 1000 : timestampInSeconds() * 1000; const profile: Profile = { event_id: profile_id, diff --git a/packages/core/src/integrations/sessiontiming.ts b/packages/core/src/integrations/sessiontiming.ts index 6f01e5d48541..4664893915e0 100644 --- a/packages/core/src/integrations/sessiontiming.ts +++ b/packages/core/src/integrations/sessiontiming.ts @@ -1,15 +1,16 @@ import type { IntegrationFn } from '@sentry/types'; +import { timestampInSeconds } from '@sentry/utils'; import { defineIntegration } from '../integration'; const INTEGRATION_NAME = 'SessionTiming'; const _sessionTimingIntegration = (() => { - const startTime = Date.now(); + const startTime = timestampInSeconds() * 1000; return { name: INTEGRATION_NAME, processEvent(event) { - const now = Date.now(); + const now = timestampInSeconds() * 1000; return { ...event, diff --git a/packages/utils/src/instrument/fetch.ts b/packages/utils/src/instrument/fetch.ts index 9c663f88baf0..4c502334024a 100644 --- a/packages/utils/src/instrument/fetch.ts +++ b/packages/utils/src/instrument/fetch.ts @@ -3,6 +3,7 @@ import type { HandlerDataFetch } from '@sentry/types'; import { fill } from '../object'; import { supportsNativeFetch } from '../supports'; +import { timestampInSeconds } from '../time'; import { GLOBAL_OBJ } from '../worldwide'; import { addHandler, maybeInstrument, triggerHandlers } from './handlers'; @@ -37,7 +38,7 @@ function instrumentFetch(): void { method, url, }, - startTimestamp: Date.now(), + startTimestamp: timestampInSeconds() * 1000, }; triggerHandlers('fetch', { @@ -49,7 +50,7 @@ function instrumentFetch(): void { (response: Response) => { const finishedHandlerData: HandlerDataFetch = { ...handlerData, - endTimestamp: Date.now(), + endTimestamp: timestampInSeconds() * 1000, response, }; @@ -59,7 +60,7 @@ function instrumentFetch(): void { (error: Error) => { const erroredHandlerData: HandlerDataFetch = { ...handlerData, - endTimestamp: Date.now(), + endTimestamp: timestampInSeconds() * 1000, error, }; From 07c6838307a2dc265b4caf59fb5a97065b63db25 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 11:29:21 +0200 Subject: [PATCH 05/13] fix(core): Avoid looking up client for `hasTracingEnabled()` if possible (#12066) This is a bit weird in that we access the current scope & client etc., even if options are passed. Since we also use this method in `init()` before we have setup stuff, it seems safer to avoid calling this at all if possible. This is related to https://github.com/getsentry/sentry-javascript/issues/12054, but not really the cause of it. --- packages/core/src/utils/hasTracingEnabled.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/core/src/utils/hasTracingEnabled.ts b/packages/core/src/utils/hasTracingEnabled.ts index a4a854edf314..97463d9d5e5e 100644 --- a/packages/core/src/utils/hasTracingEnabled.ts +++ b/packages/core/src/utils/hasTracingEnabled.ts @@ -16,7 +16,11 @@ export function hasTracingEnabled( return false; } - const client = getClient(); - const options = maybeOptions || (client && client.getOptions()); + const options = maybeOptions || getClientOptions(); return !!options && (options.enableTracing || 'tracesSampleRate' in options || 'tracesSampler' in options); } + +function getClientOptions(): Options | undefined { + const client = getClient(); + return client && client.getOptions(); +} From 4a1f2ac09f67ab98a1159a33a0c3ffbeef1d96ab Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 13:52:03 +0200 Subject: [PATCH 06/13] fix(core): Export Scope interface as `Scope` (#12067) To make interop with e.g. importing `Scope` from `@sentry/node` easier. Closes https://github.com/getsentry/sentry-javascript/issues/12053 --- packages/core/src/scope.ts | 20 +++++++++++++++++--- packages/core/test/lib/scope.test.ts | 11 +++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts index 4bf17b936563..724c9b621ce9 100644 --- a/packages/core/src/scope.ts +++ b/packages/core/src/scope.ts @@ -34,7 +34,7 @@ const DEFAULT_MAX_BREADCRUMBS = 100; /** * Holds additional event information. */ -export class Scope implements ScopeInterface { +class ScopeClass implements ScopeInterface { /** Flag if notifying is happening. */ protected _notifyingListeners: boolean; @@ -116,8 +116,8 @@ export class Scope implements ScopeInterface { /** * @inheritDoc */ - public clone(): Scope { - const newScope = new Scope(); + public clone(): ScopeClass { + const newScope = new ScopeClass(); newScope._breadcrumbs = [...this._breadcrumbs]; newScope._tags = { ...this._tags }; newScope._extra = { ...this._extra }; @@ -587,6 +587,20 @@ export class Scope implements ScopeInterface { } } +// NOTE: By exporting this here as const & type, instead of doing `export class`, +// We can get the correct class when importing from `@sentry/core`, but the original type (from `@sentry/types`) +// This is helpful for interop, e.g. when doing `import type { Scope } from '@sentry/node';` (which re-exports this) + +/** + * Holds additional event information. + */ +export const Scope = ScopeClass; + +/** + * Holds additional event information. + */ +export type Scope = ScopeInterface; + function generatePropagationContext(): PropagationContext { return { traceId: uuid4(), diff --git a/packages/core/test/lib/scope.test.ts b/packages/core/test/lib/scope.test.ts index aadc26856c6e..a58370a1b632 100644 --- a/packages/core/test/lib/scope.test.ts +++ b/packages/core/test/lib/scope.test.ts @@ -985,4 +985,15 @@ describe('withIsolationScope()', () => { done(); }); }); + + it('Scope type is equal to Scope from @sentry/types', () => { + // We pass the Scope _class_ here to the callback, + // Which actually is typed as using the Scope from @sentry/types + // This should not TS-error, as we export the type from core as well + const scope = withScope((scope: Scope) => { + return scope; + }); + + expect(scope).toBeDefined(); + }); }); From 4003f7ecc742d289f48e7516fbebb801e1214cad Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 14:44:47 +0200 Subject: [PATCH 07/13] meta: Fix changelog script (#12069) Actually we should check for the changelog commit, which we look for by checking `-meta` and `changelog` in the commit message. (Not perfect, but good enough hopefully...) I noticed it was not correctly picking up all commits while a release was in progress before. --- scripts/get-commit-list.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/scripts/get-commit-list.ts b/scripts/get-commit-list.ts index e64645692ed4..3992694cf8f0 100644 --- a/scripts/get-commit-list.ts +++ b/scripts/get-commit-list.ts @@ -3,11 +3,19 @@ import { execSync } from 'child_process'; function run(): void { const commits = execSync('git log --format="- %s"').toString().split('\n'); - const lastReleasePos = commits.findIndex(commit => commit.includes("Merge branch 'release")); + const lastReleasePos = commits.findIndex(commit => /- meta(.*)changelog/i.test(commit)); const newCommits = commits.splice(0, lastReleasePos).filter(commit => { - // Filter out master/develop merges - if (/Merge pull request #(\d+) from getsentry\/(master|develop)/.test(commit)) { + // Filter out merge commits + if (/Merge pull request/.test(commit)) { + return false; + } + // Filter release branch merged + if (/Merge branch/.test(commit)) { + return false; + } + // Filter release commit itself + if (/release:/.test(commit)) { return false; } From c594d6acbbcdccd0671d1203fd75d46bed7bf742 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 14:48:16 +0200 Subject: [PATCH 08/13] fix(node): Set transactionName for unsampled spans in httpIntegration (#12071) We noticed that in http integration, we were only setting `transactionName` when we had a sampled span. We can actually set this based on the request object we get either way, making this more robust for error-only mode. --- .../node-express-esm-without-loader/tests/server.test.ts | 5 ++--- .../suites/express/without-tracing/server.ts | 1 - .../suites/express/without-tracing/test.ts | 6 ++---- packages/node/src/integrations/http.ts | 9 ++------- 4 files changed, 6 insertions(+), 15 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/tests/server.test.ts b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/tests/server.test.ts index c2c076f13e4e..eeeb033a42df 100644 --- a/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/tests/server.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-express-esm-without-loader/tests/server.test.ts @@ -29,7 +29,6 @@ test('Isolates requests', async ({ request }) => { expect(errorEvent1.tags).toEqual({ 'param-1': 'yes' }); expect(errorEvent2.tags).toEqual({ 'param-2': 'yes' }); - // Transaction is not set, since we have no expressIntegration by default - expect(errorEvent1.transaction).toBeUndefined(); - expect(errorEvent2.transaction).toBeUndefined(); + expect(errorEvent1.transaction).toBe('GET /test-params/1'); + expect(errorEvent2.transaction).toBe('GET /test-params/2'); }); diff --git a/dev-packages/node-integration-tests/suites/express/without-tracing/server.ts b/dev-packages/node-integration-tests/suites/express/without-tracing/server.ts index 6b8af4270430..2a85d39b83b8 100644 --- a/dev-packages/node-integration-tests/suites/express/without-tracing/server.ts +++ b/dev-packages/node-integration-tests/suites/express/without-tracing/server.ts @@ -18,7 +18,6 @@ app.get('/test/isolationScope/:id', (req, res) => { const id = req.params.id; Sentry.setTag('isolation-scope', 'tag'); Sentry.setTag(`isolation-scope-${id}`, id); - Sentry.setTag('isolation-scope-transactionName', `${Sentry.getIsolationScope().getScopeData().transactionName}`); Sentry.captureException(new Error('This is an exception')); diff --git a/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts b/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts index 85c037dc0df9..73d6a322ed28 100644 --- a/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts @@ -9,12 +9,11 @@ test('correctly applies isolation scope even without tracing', done => { .ignore('session', 'sessions') .expect({ event: { + transaction: 'GET /test/isolationScope/1', tags: { global: 'tag', 'isolation-scope': 'tag', 'isolation-scope-1': '1', - // We can't properly test non-existance of fields here, so we cast this to a string to test it here - 'isolation-scope-transactionName': 'undefined', }, // Request is correctly set request: { @@ -27,12 +26,11 @@ test('correctly applies isolation scope even without tracing', done => { }) .expect({ event: { + transaction: 'GET /test/isolationScope/2', tags: { global: 'tag', 'isolation-scope': 'tag', 'isolation-scope-2': '2', - // We can't properly test non-existance of fields here, so we cast this to a string to test it here - 'isolation-scope-transactionName': 'undefined', }, // Request is correctly set request: { diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index ce6916da4dcd..8022227c6c4e 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -12,7 +12,6 @@ import { getIsolationScope, isSentryRequestUrl, setCapturedScopesOnSpan, - spanToJSON, } from '@sentry/core'; import { getClient, getRequestSpanData, getSpanKind } from '@sentry/opentelemetry'; import type { IntegrationFn } from '@sentry/types'; @@ -121,13 +120,9 @@ const _httpIntegration = ((options: HttpOptions = {}) => { // attempt to update the scope's `transactionName` based on the request URL // Ideally, framework instrumentations coming after the HttpInstrumentation // update the transactionName once we get a parameterized route. - const attributes = spanToJSON(span).data; - if (!attributes) { - return; - } + const httpMethod = (req.method || 'GET').toUpperCase(); + const httpTarget = stripUrlQueryAndFragment(req.url || '/'); - const httpMethod = String(attributes['http.method']).toUpperCase() || 'GET'; - const httpTarget = stripUrlQueryAndFragment(String(attributes['http.target'])) || '/'; const bestEffortTransactionName = `${httpMethod} ${httpTarget}`; isolationScope.setTransactionName(bestEffortTransactionName); From bc29d88ec561a910be5eaa4f671e97f86184fda3 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 16 May 2024 15:10:43 +0200 Subject: [PATCH 09/13] ref(sveltekit): Warn to delete source maps if Sentry plugin enabled source maps generation (#12072) Now we can at least recommend to use `filesToDeleteAfterUpload` since we bumped to vite plugin 2.x with 8.0.0 --- packages/sveltekit/src/vite/sourceMaps.ts | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/sveltekit/src/vite/sourceMaps.ts b/packages/sveltekit/src/vite/sourceMaps.ts index eb97a3e4a7c6..5b9e2d355cfa 100644 --- a/packages/sveltekit/src/vite/sourceMaps.ts +++ b/packages/sveltekit/src/vite/sourceMaps.ts @@ -103,8 +103,18 @@ export async function makeCustomSentryVitePlugins(options?: CustomSentryVitePlug // Modify the config to generate source maps config: config => { - // eslint-disable-next-line no-console - debug && console.log('[Source Maps Plugin] Enabeling source map generation'); + const sourceMapsPreviouslyEnabled = !config.build?.sourcemap; + if (debug && sourceMapsPreviouslyEnabled) { + // eslint-disable-next-line no-console + console.log('[Source Maps Plugin] Enabeling source map generation'); + if (!mergedOptions.sourcemaps?.filesToDeleteAfterUpload) { + // eslint-disable-next-line no-console + console.warn( + `[Source Maps Plugin] We recommend setting the \`sourceMapsUploadOptions.sourcemaps.filesToDeleteAfterUpload\` option to clean up source maps after uploading. +[Source Maps Plugin] Otherwise, source maps might be deployed to production, depending on your configuration`, + ); + } + } return { ...config, build: { From 6c37df12b9c0968252a663aaed6cb10853a89bcc Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 16 May 2024 15:48:19 +0200 Subject: [PATCH 10/13] doc(migration): Add entry for `interactionsSampleRate` (#12064) As discussed internally in Slack and #12006, we decided to remove `interactionsSampleRate` from v8 for good (it never was added in a publicly released `8.0.0*` version). This PR now updates the migration guide to retroactively reflect this change. closes #12006 --- MIGRATION.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/MIGRATION.md b/MIGRATION.md index edb03fa7f1c2..14e5bdd79a93 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -647,6 +647,38 @@ The `BrowserTracing` integration, together with the custom routing instrumentati Instead, you should use `Sentry.browserTracingIntegration()`. See examples [below](./MIGRATION.md#deprecated-browsertracing-integration) +#### Removal of `interactionsSampleRate` in `browserTracingIntegration` options + +The `interactionsSampleRate` option that could be passed to `browserTracingIntegration` or `new BrowserTracing()` was +removed in v8, due to the option being redundant and in favour of bundle size minimization. + +It's important to note that this sample rate only ever was applied when collecting INP (Interaction To Next Paint) +values. You most likely don't need to replace this option. Furthermore, INP values are already sampled by the +[`tracesSampleRate` SDK option](https://docs.sentry.io/platforms/javascript/configuration/options/#traces-sampler), like +any regular span. At the time of writing, INP value collection does not deplete your span or transaction quota. + +If you used `interactionsSampleRate` before, and still want to reduce INP value collection, we recommend using the +`tracesSampler` SDK option instead: + +```javascript +// v7 +Sentry.init({ + integrations: [new BrowserTracing({ interactionsSampleRate: 0.1 })], +}); +``` + +```javascript +// v8 - please read the text above, you most likely don't need this :) +Sentry.init({ + tracesSampler: (ctx) => { + if (ctx.attributes?['sentry.op']?.startsWith('ui.interaction')) { + return 0.1; + } + return 0.5; + } +}) +``` + #### Removal of the `Offline` integration The `Offline` integration has been removed in favor of the From f1d1a16952b8e53a8c78b5dada79472440d60f88 Mon Sep 17 00:00:00 2001 From: Andrei <168741329+andreiborza@users.noreply.github.com> Date: Thu, 16 May 2024 15:56:33 +0200 Subject: [PATCH 11/13] feat(core): Add `beforeSendSpan` hook (#11886) (#11918) --- packages/core/src/baseclient.ts | 20 +++- packages/core/src/envelope.ts | 22 +++- packages/core/src/tracing/sentrySpan.ts | 18 ++- packages/core/test/lib/base.test.ts | 103 ++++++++++++++++++ packages/core/test/lib/envelope.test.ts | 67 ++++++++++++ .../core/test/lib/tracing/sentrySpan.test.ts | 53 +++++++++ packages/types/src/options.ts | 10 ++ 7 files changed, 285 insertions(+), 8 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 096b288c8a21..d29c7f61af47 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -24,6 +24,7 @@ import type { Span, SpanAttributes, SpanContextData, + SpanJSON, StartSpanOptions, TransactionEvent, Transport, @@ -896,14 +897,27 @@ function processBeforeSend( event: Event, hint: EventHint, ): PromiseLike | Event | null { - const { beforeSend, beforeSendTransaction } = options; + const { beforeSend, beforeSendTransaction, beforeSendSpan } = options; if (isErrorEvent(event) && beforeSend) { return beforeSend(event, hint); } - if (isTransactionEvent(event) && beforeSendTransaction) { - return beforeSendTransaction(event, hint); + if (isTransactionEvent(event)) { + if (event.spans && beforeSendSpan) { + const processedSpans: SpanJSON[] = []; + for (const span of event.spans) { + const processedSpan = beforeSendSpan(span); + if (processedSpan) { + processedSpans.push(processedSpan); + } + } + event.spans = processedSpans; + } + + if (beforeSendTransaction) { + return beforeSendTransaction(event, hint); + } } return event; diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index 2aef194b069e..258216b290d4 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -1,4 +1,5 @@ import type { + Client, DsnComponents, DynamicSamplingContext, Event, @@ -11,6 +12,8 @@ import type { SessionEnvelope, SessionItem, SpanEnvelope, + SpanItem, + SpanJSON, } from '@sentry/types'; import { createEnvelope, @@ -94,8 +97,10 @@ export function createEventEnvelope( /** * Create envelope from Span item. + * + * Takes an optional client and runs spans through `beforeSendSpan` if available. */ -export function createSpanEnvelope(spans: SentrySpan[]): SpanEnvelope { +export function createSpanEnvelope(spans: SentrySpan[], client?: Client): SpanEnvelope { function dscHasRequiredProps(dsc: Partial): dsc is DynamicSamplingContext { return !!dsc.trace_id && !!dsc.public_key; } @@ -109,6 +114,19 @@ export function createSpanEnvelope(spans: SentrySpan[]): SpanEnvelope { sent_at: new Date().toISOString(), ...(dscHasRequiredProps(dsc) && { trace: dsc }), }; - const items = spans.map(span => createSpanEnvelopeItem(spanToJSON(span))); + + const beforeSendSpan = client && client.getOptions().beforeSendSpan; + const convertToSpanJSON = beforeSendSpan + ? (span: SentrySpan) => beforeSendSpan(spanToJSON(span) as SpanJSON) + : (span: SentrySpan) => spanToJSON(span); + + const items: SpanItem[] = []; + for (const span of spans) { + const spanJson = convertToSpanJSON(span); + if (spanJson) { + items.push(createSpanEnvelopeItem(spanJson)); + } + } + return createEnvelope(headers, items); } diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index e9e503150fcd..b2dd49f32b2a 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -97,12 +97,12 @@ export class SentrySpan implements Span { this._events = []; + this._isStandaloneSpan = spanContext.isStandalone; + // If the span is already ended, ensure we finalize the span immediately if (this._endTime) { this._onSpanEnded(); } - - this._isStandaloneSpan = spanContext.isStandalone; } /** @inheritdoc */ @@ -259,7 +259,7 @@ export class SentrySpan implements Span { // if this is a standalone span, we send it immediately if (this._isStandaloneSpan) { - sendSpanEnvelope(createSpanEnvelope([this])); + sendSpanEnvelope(createSpanEnvelope([this], client)); return; } @@ -357,12 +357,24 @@ function isStandaloneSpan(span: Span): boolean { return span instanceof SentrySpan && span.isStandaloneSpan(); } +/** + * Sends a `SpanEnvelope`. + * + * Note: If the envelope's spans are dropped, e.g. via `beforeSendSpan`, + * the envelope will not be sent either. + */ function sendSpanEnvelope(envelope: SpanEnvelope): void { const client = getClient(); if (!client) { return; } + const spanItems = envelope[1]; + if (!spanItems || spanItems.length === 0) { + client.recordDroppedEvent('before_send', 'span'); + return; + } + const transport = client.getTransport(); if (transport) { transport.send(envelope).then(null, reason => { diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 3cfb230da5cf..1ee834e0d8ae 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -978,6 +978,38 @@ describe('BaseClient', () => { expect(TestClient.instance!.event!.transaction).toBe('/dogs/are/great'); }); + test('calls `beforeSendSpan` and uses original spans without any changes', () => { + expect.assertions(2); + + const beforeSendSpan = jest.fn(span => span); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); + const client = new TestClient(options); + + const transaction: Event = { + transaction: '/cats/are/great', + type: 'transaction', + spans: [ + { + description: 'first span', + span_id: '9e15bf99fbe4bc80', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + { + description: 'second span', + span_id: 'aa554c1f506b0783', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + ], + }; + client.captureEvent(transaction); + + expect(beforeSendSpan).toHaveBeenCalledTimes(2); + const capturedEvent = TestClient.instance!.event!; + expect(capturedEvent.spans).toEqual(transaction.spans); + }); + test('calls `beforeSend` and uses the modified event', () => { expect.assertions(2); @@ -1010,6 +1042,45 @@ describe('BaseClient', () => { expect(TestClient.instance!.event!.transaction).toBe('/adopt/dont/shop'); }); + test('calls `beforeSendSpan` and uses the modified spans', () => { + expect.assertions(3); + + const beforeSendSpan = jest.fn(span => { + span.data = { version: 'bravo' }; + return span; + }); + + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); + const client = new TestClient(options); + const transaction: Event = { + transaction: '/cats/are/great', + type: 'transaction', + spans: [ + { + description: 'first span', + span_id: '9e15bf99fbe4bc80', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + { + description: 'second span', + span_id: 'aa554c1f506b0783', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + ], + }; + + client.captureEvent(transaction); + + expect(beforeSendSpan).toHaveBeenCalledTimes(2); + const capturedEvent = TestClient.instance!.event!; + for (const [idx, span] of capturedEvent.spans!.entries()) { + const originalSpan = transaction.spans![idx]; + expect(span).toEqual({ ...originalSpan, data: { version: 'bravo' } }); + } + }); + test('calls `beforeSend` and discards the event', () => { expect.assertions(4); @@ -1048,6 +1119,38 @@ describe('BaseClient', () => { expect(loggerWarnSpy).toBeCalledWith('before send for type `transaction` returned `null`, will not send event.'); }); + test('calls `beforeSendSpan` and discards the span', () => { + expect.assertions(2); + + const beforeSendSpan = jest.fn(() => null); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); + const client = new TestClient(options); + + const transaction: Event = { + transaction: '/cats/are/great', + type: 'transaction', + spans: [ + { + description: 'first span', + span_id: '9e15bf99fbe4bc80', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + { + description: 'second span', + span_id: 'aa554c1f506b0783', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + ], + }; + client.captureEvent(transaction); + + expect(beforeSendSpan).toHaveBeenCalledTimes(2); + const capturedEvent = TestClient.instance!.event!; + expect(capturedEvent.spans).toHaveLength(0); + }); + test('calls `beforeSend` and logs info about invalid return value', () => { const invalidValues = [undefined, false, true, [], 1]; expect.assertions(invalidValues.length * 3); diff --git a/packages/core/test/lib/envelope.test.ts b/packages/core/test/lib/envelope.test.ts index f5240e826604..7d5a2c5f7740 100644 --- a/packages/core/test/lib/envelope.test.ts +++ b/packages/core/test/lib/envelope.test.ts @@ -157,4 +157,71 @@ describe('createSpanEnvelope', () => { sent_at: expect.any(String), }); }); + + it('calls `beforeSendSpan` and uses original span without any changes', () => { + const beforeSendSpan = jest.fn(span => span); + const options = getDefaultTestClientOptions({ dsn: 'https://domain/123', beforeSendSpan }); + const client = new TestClient(options); + + const span = new SentrySpan({ + name: 'test', + isStandalone: true, + startTimestamp: 1, + endTimestamp: 2, + }); + + const spanEnvelope = createSpanEnvelope([span], client); + + expect(beforeSendSpan).toHaveBeenCalled(); + + const spanItem = spanEnvelope[1][0][1]; + expect(spanItem).toEqual({ + data: { + 'sentry.origin': 'manual', + }, + description: 'test', + is_segment: true, + origin: 'manual', + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + segment_id: spanItem.segment_id, + start_timestamp: 1, + timestamp: 2, + trace_id: expect.stringMatching(/^[0-9a-f]{32}$/), + }); + }); + + it('calls `beforeSendSpan` and uses the modified span', () => { + const beforeSendSpan = jest.fn(span => { + span.description = `mutated description: ${span.description}`; + return span; + }); + const options = getDefaultTestClientOptions({ dsn: 'https://domain/123', beforeSendSpan }); + const client = new TestClient(options); + + const span = new SentrySpan({ + name: 'test', + isStandalone: true, + startTimestamp: 1, + endTimestamp: 2, + }); + + const spanEnvelope = createSpanEnvelope([span], client); + + expect(beforeSendSpan).toHaveBeenCalled(); + + const spanItem = spanEnvelope[1][0][1]; + expect(spanItem).toEqual({ + data: { + 'sentry.origin': 'manual', + }, + description: 'mutated description: test', + is_segment: true, + origin: 'manual', + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + segment_id: spanItem.segment_id, + start_timestamp: 1, + timestamp: 2, + trace_id: expect.stringMatching(/^[0-9a-f]{32}$/), + }); + }); }); diff --git a/packages/core/test/lib/tracing/sentrySpan.test.ts b/packages/core/test/lib/tracing/sentrySpan.test.ts index 13d52149bb8b..5d6895f34026 100644 --- a/packages/core/test/lib/tracing/sentrySpan.test.ts +++ b/packages/core/test/lib/tracing/sentrySpan.test.ts @@ -1,7 +1,9 @@ import { timestampInSeconds } from '@sentry/utils'; +import { setCurrentClient } from '../../../src'; import { SentrySpan } from '../../../src/tracing/sentrySpan'; import { SPAN_STATUS_ERROR } from '../../../src/tracing/spanstatus'; import { TRACE_FLAG_NONE, TRACE_FLAG_SAMPLED, spanToJSON } from '../../../src/utils/spanUtils'; +import { TestClient, getDefaultTestClientOptions } from '../../mocks/client'; describe('SentrySpan', () => { describe('name', () => { @@ -88,6 +90,57 @@ describe('SentrySpan', () => { span.end(); expect(spanToJSON(span).timestamp).toBeGreaterThan(1); }); + + test('sends the span if `beforeSendSpan` does not modify the span ', () => { + const beforeSendSpan = jest.fn(span => span); + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://username@domain/123', + enableSend: true, + beforeSendSpan, + }), + ); + setCurrentClient(client); + + // @ts-expect-error Accessing private transport API + const mockSend = jest.spyOn(client._transport, 'send'); + const span = new SentrySpan({ + name: 'test', + isStandalone: true, + startTimestamp: 1, + endTimestamp: 2, + sampled: true, + }); + span.end(); + expect(mockSend).toHaveBeenCalled(); + }); + + test('does not send the span if `beforeSendSpan` drops the span', () => { + const beforeSendSpan = jest.fn(() => null); + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://username@domain/123', + enableSend: true, + beforeSendSpan, + }), + ); + setCurrentClient(client); + + const recordDroppedEventSpy = jest.spyOn(client, 'recordDroppedEvent'); + // @ts-expect-error Accessing private transport API + const mockSend = jest.spyOn(client._transport, 'send'); + const span = new SentrySpan({ + name: 'test', + isStandalone: true, + startTimestamp: 1, + endTimestamp: 2, + sampled: true, + }); + span.end(); + + expect(mockSend).not.toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'span'); + }); }); describe('end', () => { diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index bf41b16fc595..26a043647dcc 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -3,6 +3,7 @@ import type { ErrorEvent, EventHint, TransactionEvent } from './event'; import type { Integration } from './integration'; import type { CaptureContext } from './scope'; import type { SdkMetadata } from './sdkmetadata'; +import type { SpanJSON } from './span'; import type { StackLineParser, StackParser } from './stacktrace'; import type { TracePropagationTargets } from './tracing'; import type { SamplingContext } from './transaction'; @@ -281,6 +282,15 @@ export interface ClientOptions PromiseLike | ErrorEvent | null; + /** + * An event-processing callback for spans. This allows a span to be modified before it's sent. + * + * Returning `null` will cause this span to be dropped. + * @param span The span generated by the SDK. + * @returns A new span that will be sent | null. + */ + beforeSendSpan?: (span: SpanJSON) => SpanJSON | null; + /** * An event-processing callback for transaction events, guaranteed to be invoked after all other event * processors. This allows an event to be modified or dropped before it's sent. From 257bcb05ddc80044d2d5bb92a1dc0778f5c323a3 Mon Sep 17 00:00:00 2001 From: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com> Date: Thu, 16 May 2024 16:34:17 +0200 Subject: [PATCH 12/13] feat(redis-cache): Create cache-span with prefixed keys (get/set commands) (#12070) Populates the OTel span with cache attributes. Currently, `get` and `set` commands are considered. --------- Co-authored-by: Francesco Novy --- .../tracing/redis-cache/docker-compose.yml | 9 ++ .../tracing/redis-cache/scenario-ioredis.js | 41 +++++++++ .../suites/tracing/redis-cache/test.ts | 92 +++++++++++++++++++ .../suites/tracing/redis/scenario-ioredis.js | 4 +- .../suites/tracing/redis/test.ts | 4 +- packages/core/src/semanticAttributes.ts | 6 ++ .../node/src/integrations/tracing/redis.ts | 81 +++++++++++++++- 7 files changed, 231 insertions(+), 6 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/redis-cache/docker-compose.yml create mode 100644 dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/docker-compose.yml b/dev-packages/node-integration-tests/suites/tracing/redis-cache/docker-compose.yml new file mode 100644 index 000000000000..164d5977e33d --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/docker-compose.yml @@ -0,0 +1,9 @@ +version: '3.9' + +services: + db: + image: redis:latest + restart: always + container_name: integration-tests-redis + ports: + - '6379:6379' diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js b/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js new file mode 100644 index 000000000000..22385f81b064 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/scenario-ioredis.js @@ -0,0 +1,41 @@ +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, + integrations: [Sentry.redisIntegration({ cachePrefixes: ['ioredis-cache:'] })], +}); + +// Stop the process from exiting before the transaction is sent +setInterval(() => {}, 1000); + +const Redis = require('ioredis'); + +const redis = new Redis({ port: 6379 }); + +async function run() { + await Sentry.startSpan( + { + name: 'Test Span', + op: 'test-span', + }, + async () => { + try { + await redis.set('test-key', 'test-value'); + await redis.set('ioredis-cache:test-key', 'test-value'); + + await redis.get('test-key'); + await redis.get('ioredis-cache:test-key'); + await redis.get('ioredis-cache:unavailable-data'); + } finally { + await redis.disconnect(); + } + }, + ); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts new file mode 100644 index 000000000000..0c2beaf7d4c8 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts @@ -0,0 +1,92 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +describe('redis auto instrumentation', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + test('should not add cache spans when key is not prefixed', done => { + const EXPECTED_TRANSACTION = { + transaction: 'Test Span', + spans: expect.arrayContaining([ + expect.objectContaining({ + description: 'set test-key [1 other arguments]', + op: 'db', + data: expect.objectContaining({ + 'sentry.op': 'db', + 'db.system': 'redis', + 'net.peer.name': 'localhost', + 'net.peer.port': 6379, + 'db.statement': 'set test-key [1 other arguments]', + }), + }), + expect.objectContaining({ + description: 'get test-key', + op: 'db', + data: expect.objectContaining({ + 'sentry.op': 'db', + 'db.system': 'redis', + 'net.peer.name': 'localhost', + 'net.peer.port': 6379, + 'db.statement': 'get test-key', + }), + }), + ]), + }; + + createRunner(__dirname, 'scenario-ioredis.js') + .withDockerCompose({ workingDirectory: [__dirname], readyMatches: ['port=6379'] }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); + + test('should create cache spans for prefixed keys', done => { + const EXPECTED_TRANSACTION = { + transaction: 'Test Span', + spans: expect.arrayContaining([ + // SET + expect.objectContaining({ + description: 'set ioredis-cache:test-key [1 other arguments]', + op: 'cache.put', + data: expect.objectContaining({ + 'db.statement': 'set ioredis-cache:test-key [1 other arguments]', + 'cache.key': 'ioredis-cache:test-key', + 'cache.item_size': 2, + 'network.peer.address': 'localhost', + 'network.peer.port': 6379, + }), + }), + // GET + expect.objectContaining({ + description: 'get ioredis-cache:test-key', + op: 'cache.get_item', // todo: will be changed to cache.get + data: expect.objectContaining({ + 'db.statement': 'get ioredis-cache:test-key', + 'cache.hit': true, + 'cache.key': 'ioredis-cache:test-key', + 'cache.item_size': 10, + 'network.peer.address': 'localhost', + 'network.peer.port': 6379, + }), + }), + // GET (unavailable) + expect.objectContaining({ + description: 'get ioredis-cache:unavailable-data', + op: 'cache.get_item', // todo: will be changed to cache.get + data: expect.objectContaining({ + 'db.statement': 'get ioredis-cache:unavailable-data', + 'cache.hit': false, + 'cache.key': 'ioredis-cache:unavailable-data', + 'network.peer.address': 'localhost', + 'network.peer.port': 6379, + }), + }), + ]), + }; + + createRunner(__dirname, 'scenario-ioredis.js') + .withDockerCompose({ workingDirectory: [__dirname], readyMatches: ['port=6379'] }) + .expect({ transaction: EXPECTED_TRANSACTION }) + .start(done); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis/scenario-ioredis.js b/dev-packages/node-integration-tests/suites/tracing/redis/scenario-ioredis.js index 52df06b2a386..4c325c3a6c21 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis/scenario-ioredis.js +++ b/dev-packages/node-integration-tests/suites/tracing/redis/scenario-ioredis.js @@ -18,8 +18,8 @@ const redis = new Redis({ port: 6379 }); async function run() { await Sentry.startSpan( { - name: 'Test Transaction', - op: 'transaction', + name: 'Test Span', + op: 'test-span', }, async () => { try { diff --git a/dev-packages/node-integration-tests/suites/tracing/redis/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis/test.ts index fd441201cebc..604b2751f05b 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis/test.ts @@ -7,12 +7,13 @@ describe('redis auto instrumentation', () => { test('should auto-instrument `ioredis` package when using redis.set() and redis.get()', done => { const EXPECTED_TRANSACTION = { - transaction: 'Test Transaction', + transaction: 'Test Span', spans: expect.arrayContaining([ expect.objectContaining({ description: 'set test-key [1 other arguments]', op: 'db', data: expect.objectContaining({ + 'sentry.op': 'db', 'db.system': 'redis', 'net.peer.name': 'localhost', 'net.peer.port': 6379, @@ -23,6 +24,7 @@ describe('redis auto instrumentation', () => { description: 'get test-key', op: 'db', data: expect.objectContaining({ + 'sentry.op': 'db', 'db.system': 'redis', 'net.peer.name': 'localhost', 'net.peer.port': 6379, diff --git a/packages/core/src/semanticAttributes.ts b/packages/core/src/semanticAttributes.ts index 8f46de21a1ca..2c268110854c 100644 --- a/packages/core/src/semanticAttributes.ts +++ b/packages/core/src/semanticAttributes.ts @@ -35,3 +35,9 @@ export const SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE = 'sentry.measurement_v export const SEMANTIC_ATTRIBUTE_PROFILE_ID = 'sentry.profile_id'; export const SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME = 'sentry.exclusive_time'; + +export const SEMANTIC_ATTRIBUTE_CACHE_HIT = 'cache.hit'; + +export const SEMANTIC_ATTRIBUTE_CACHE_KEY = 'cache.key'; + +export const SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE = 'cache.item_size'; diff --git a/packages/node/src/integrations/tracing/redis.ts b/packages/node/src/integrations/tracing/redis.ts index 8e1eebb83b6a..f9309b2de33e 100644 --- a/packages/node/src/integrations/tracing/redis.ts +++ b/packages/node/src/integrations/tracing/redis.ts @@ -1,14 +1,89 @@ import { IORedisInstrumentation } from '@opentelemetry/instrumentation-ioredis'; -import { defineIntegration } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_CACHE_HIT, + SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, + SEMANTIC_ATTRIBUTE_CACHE_KEY, + SEMANTIC_ATTRIBUTE_SENTRY_OP, + defineIntegration, + spanToJSON, +} from '@sentry/core'; import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry'; import type { IntegrationFn } from '@sentry/types'; -const _redisIntegration = (() => { +function keyHasPrefix(key: string, prefixes: string[]): boolean { + return prefixes.some(prefix => key.startsWith(prefix)); +} + +/** Currently, caching only supports 'get' and 'set' commands. More commands will be added (setex, mget, del, expire) */ +function shouldConsiderForCache( + redisCommand: string, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + key: string | number | any[] | Buffer, + prefixes: string[], +): boolean { + return (redisCommand === 'get' || redisCommand === 'set') && typeof key === 'string' && keyHasPrefix(key, prefixes); +} + +function calculateCacheItemSize(response: unknown): number | undefined { + try { + if (Buffer.isBuffer(response)) return response.byteLength; + else if (typeof response === 'string') return response.length; + else if (typeof response === 'number') return response.toString().length; + else if (response === null || response === undefined) return 0; + return JSON.stringify(response).length; + } catch (e) { + return undefined; + } +} + +interface RedisOptions { + cachePrefixes?: string[]; +} + +const _redisIntegration = ((options?: RedisOptions) => { return { name: 'Redis', setupOnce() { addOpenTelemetryInstrumentation([ - new IORedisInstrumentation({}), + new IORedisInstrumentation({ + responseHook: (span, redisCommand, cmdArgs, response) => { + const key = cmdArgs[0]; + + if (!options?.cachePrefixes || !shouldConsiderForCache(redisCommand, key, options.cachePrefixes)) { + // not relevant for cache + return; + } + + // otel/ioredis seems to be using the old standard, as there was a change to those params: https://github.com/open-telemetry/opentelemetry-specification/issues/3199 + // We are using params based on the docs: https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/ + const networkPeerAddress = spanToJSON(span).data?.['net.peer.name']; + const networkPeerPort = spanToJSON(span).data?.['net.peer.port']; + if (networkPeerPort && networkPeerAddress) { + span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort }); + } + + const cacheItemSize = calculateCacheItemSize(response); + if (cacheItemSize) span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); + + if (typeof key === 'string') { + switch (redisCommand) { + case 'get': + span.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.get_item', // todo: will be changed to cache.get + [SEMANTIC_ATTRIBUTE_CACHE_KEY]: key, + }); + if (cacheItemSize !== undefined) span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); + break; + case 'set': + span.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.put', + [SEMANTIC_ATTRIBUTE_CACHE_KEY]: key, + }); + break; + } + } + }, + }), // todo: implement them gradually // new LegacyRedisInstrumentation({}), // new RedisInstrumentation({}), From 46511e51856bf2620e07477e86dc3f8aaa4beb70 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 16 May 2024 16:15:05 +0200 Subject: [PATCH 13/13] meta(changelog): Update changelog for v8.2.0 --- CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 333768dcf17e..5fe7286ed88d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,18 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +## 8.2.0 + +- feat(redis-cache): Create cache-span with prefixed keys (get/set commands) (#12070) +- feat(core): Add `beforeSendSpan` hook (#11886) +- feat(browser): Improve idle span handling (#12065) +- fix(node): Set transactionName for unsampled spans in httpIntegration (#12071) +- fix(core): Export Scope interface as `Scope` (#12067) +- fix(core): Avoid looking up client for `hasTracingEnabled()` if possible (#12066) +- fix(browser): Use consistent timestamps (#12063) +- fix(node): Fix check for performance integrations (#12043) +- ref(sveltekit): Warn to delete source maps if Sentry plugin enabled source maps generation (#12072) + ## 8.1.0 This release mainly fixes a couple of bugs from the initial [8.0.0 release](#800). In addition to the changes below, we