From f613d336d17bc1ac4fc611a1369982a8bc66c06d Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 3 Jan 2025 14:39:54 +0100 Subject: [PATCH 1/4] feat(core): Add `normalizedRequest` to `samplingContext` --- .../scenario-normalizedRequest.js | 35 +++++++++++++++++++ .../express/tracing/tracesSampler/test.ts | 20 +++++++++++ packages/core/src/tracing/sampling.ts | 14 ++++++-- .../core/src/types-hoist/samplingcontext.ts | 7 ++++ 4 files changed, 73 insertions(+), 3 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/scenario-normalizedRequest.js diff --git a/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/scenario-normalizedRequest.js b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/scenario-normalizedRequest.js new file mode 100644 index 000000000000..7c00cb8d72f7 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/scenario-normalizedRequest.js @@ -0,0 +1,35 @@ +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', + transport: loggingTransport, + tracesSampler: samplingContext => { + // The sampling decision is based on whether the data in `normalizedRequest` is available --> this is what we want to test for + return ( + samplingContext.normalizedRequest.url.includes('/test-normalized-request?query=123') && + samplingContext.normalizedRequest.method && + samplingContext.normalizedRequest.query_string === 'query=123' && + !!samplingContext.normalizedRequest.headers + ); + }, + debug: true, +}); + +// express must be required after Sentry is initialized +const express = require('express'); +const cors = require('cors'); +const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); + +const app = express(); + +app.use(cors()); + +app.get('/test-normalized-request', (_req, res) => { + res.send('Success'); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/test.ts index a19299787f91..07cc8d094d8f 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/test.ts +++ b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/test.ts @@ -22,3 +22,23 @@ describe('express tracesSampler', () => { }); }); }); + +describe('express tracesSampler includes normalizedRequest data', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + describe('CJS', () => { + test('correctly samples & passes data to tracesSampler', done => { + const runner = createRunner(__dirname, 'scenario-normalizedRequest.js') + .expect({ + transaction: { + transaction: 'GET /test-normalized-request', + }, + }) + .start(done); + + runner.makeRequest('get', '/test-normalized-request?query=123'); + }); + }); +}); diff --git a/packages/core/src/tracing/sampling.ts b/packages/core/src/tracing/sampling.ts index 9109e78e0343..39b3f130e4dc 100644 --- a/packages/core/src/tracing/sampling.ts +++ b/packages/core/src/tracing/sampling.ts @@ -1,5 +1,6 @@ import type { Options, SamplingContext } from '../types-hoist'; +import { getIsolationScope } from '../currentScopes'; import { DEBUG_BUILD } from '../debug-build'; import { logger } from '../utils-hoist/logger'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; @@ -20,13 +21,20 @@ export function sampleSpan( return [false]; } + const normalizedRequest = getIsolationScope().getScopeData().sdkProcessingMetadata.normalizedRequest; + + const enhancedSamplingContext = { + ...samplingContext, + normalizedRequest: samplingContext.normalizedRequest || normalizedRequest, + }; + // we would have bailed already if neither `tracesSampler` nor `tracesSampleRate` nor `enableTracing` were defined, so one of these should // work; prefer the hook if so let sampleRate; if (typeof options.tracesSampler === 'function') { - sampleRate = options.tracesSampler(samplingContext); - } else if (samplingContext.parentSampled !== undefined) { - sampleRate = samplingContext.parentSampled; + sampleRate = options.tracesSampler(enhancedSamplingContext); + } else if (enhancedSamplingContext.parentSampled !== undefined) { + sampleRate = enhancedSamplingContext.parentSampled; } else if (typeof options.tracesSampleRate !== 'undefined') { sampleRate = options.tracesSampleRate; } else { diff --git a/packages/core/src/types-hoist/samplingcontext.ts b/packages/core/src/types-hoist/samplingcontext.ts index b7657b68ba92..1cb15490e5b2 100644 --- a/packages/core/src/types-hoist/samplingcontext.ts +++ b/packages/core/src/types-hoist/samplingcontext.ts @@ -1,3 +1,4 @@ +import type { RequestEventData } from '../types-hoist/request'; import type { ExtractedNodeRequestData, WorkerLocation } from './misc'; import type { SpanAttributes } from './span'; @@ -36,9 +37,15 @@ export interface SamplingContext extends CustomSamplingContext { /** * Object representing the incoming request to a node server. + * @deprecated This attribute is currently never defined and will be removed in v9. Use `normalizedRequest` instead */ request?: ExtractedNodeRequestData; + /** + * Object representing the incoming request to a node server in a normalized format. + */ + normalizedRequest?: RequestEventData; + /** The name of the span being sampled. */ name: string; From fe2ef55287f997f50f78dd9c2a07fb4d6b9b9b5e Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 3 Jan 2025 14:53:20 +0100 Subject: [PATCH 2/4] add migration notice --- docs/migration/v8-to-v9.md | 1 + packages/core/src/types-hoist/samplingcontext.ts | 8 +------- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index b8af294ec72f..24fcf14f9d15 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -217,6 +217,7 @@ Since v9, the types have been merged into `@sentry/core`, which removed some of - The `TransactionNamingScheme` type has been removed. There is no replacement. - The `Request` type has been removed. Use `RequestEventData` type instead. - The `IntegrationClass` type is no longer exported - it was not used anymore. Instead, use `Integration` or `IntegrationFn`. +- - The `samplingContext.request` attribute in the `tracesSampler` has been removed. Use `samplingContext.normalizedRequest` instead. # No Version Support Timeline diff --git a/packages/core/src/types-hoist/samplingcontext.ts b/packages/core/src/types-hoist/samplingcontext.ts index 1cb15490e5b2..0c73ba0968c2 100644 --- a/packages/core/src/types-hoist/samplingcontext.ts +++ b/packages/core/src/types-hoist/samplingcontext.ts @@ -1,5 +1,5 @@ import type { RequestEventData } from '../types-hoist/request'; -import type { ExtractedNodeRequestData, WorkerLocation } from './misc'; +import type { WorkerLocation } from './misc'; import type { SpanAttributes } from './span'; /** @@ -35,12 +35,6 @@ export interface SamplingContext extends CustomSamplingContext { */ location?: WorkerLocation; - /** - * Object representing the incoming request to a node server. - * @deprecated This attribute is currently never defined and will be removed in v9. Use `normalizedRequest` instead - */ - request?: ExtractedNodeRequestData; - /** * Object representing the incoming request to a node server in a normalized format. */ From db8d0722b68ee1f582a73930af8ac256f6d445aa Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 3 Jan 2025 15:39:19 +0100 Subject: [PATCH 3/4] review suggestions --- docs/migration/v8-to-v9.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index 24fcf14f9d15..63b3f3d4f2f7 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -217,7 +217,7 @@ Since v9, the types have been merged into `@sentry/core`, which removed some of - The `TransactionNamingScheme` type has been removed. There is no replacement. - The `Request` type has been removed. Use `RequestEventData` type instead. - The `IntegrationClass` type is no longer exported - it was not used anymore. Instead, use `Integration` or `IntegrationFn`. -- - The `samplingContext.request` attribute in the `tracesSampler` has been removed. Use `samplingContext.normalizedRequest` instead. +- The `samplingContext.request` attribute in the `tracesSampler` has been removed. Use `samplingContext.normalizedRequest` instead. Note that the type of `normalizedRequest` differs from `request`. # No Version Support Timeline From 939b4a46db391353febf5de554a7e67f94caf04d Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Fri, 3 Jan 2025 15:51:46 +0100 Subject: [PATCH 4/4] remove debug --- .../express/tracing/tracesSampler/scenario-normalizedRequest.js | 1 - .../suites/express/tracing/tracesSampler/server.js | 1 - 2 files changed, 2 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/scenario-normalizedRequest.js b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/scenario-normalizedRequest.js index 7c00cb8d72f7..da31780f2c5f 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/scenario-normalizedRequest.js +++ b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/scenario-normalizedRequest.js @@ -14,7 +14,6 @@ Sentry.init({ !!samplingContext.normalizedRequest.headers ); }, - debug: true, }); // express must be required after Sentry is initialized diff --git a/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/server.js b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/server.js index c096871cb755..b60ea07b636f 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/server.js +++ b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/server.js @@ -15,7 +15,6 @@ Sentry.init({ samplingContext.attributes['http.method'] === 'GET' ); }, - debug: true, }); // express must be required after Sentry is initialized