From f5c0816499ee6be9c824950f552e09730ab341eb Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 9 Apr 2024 17:46:00 +0200 Subject: [PATCH 1/6] feat(core): Remove transaction name extraction from requestDataIntegration --- packages/core/src/integrations/requestdata.ts | 53 ++----------------- 1 file changed, 4 insertions(+), 49 deletions(-) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 23c3ae311533..6fa4e431364f 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -1,8 +1,7 @@ -import type { Client, IntegrationFn, Span } from '@sentry/types'; +import type { Client, IntegrationFn } from '@sentry/types'; import type { AddRequestDataToEventOptions, TransactionNamingScheme } from '@sentry/utils'; -import { addRequestDataToEvent, extractPathForTransaction } from '@sentry/utils'; +import { addRequestDataToEvent } from '@sentry/utils'; import { defineIntegration } from '../integration'; -import { spanToJSON } from '../utils/spanUtils'; export type RequestDataIntegrationOptions = { /** @@ -67,12 +66,11 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = return { name: INTEGRATION_NAME, - processEvent(event, _hint, client) { + processEvent(event, _hint, _client) { // Note: In the long run, most of the logic here should probably move into the request data utility functions. For // the moment it lives here, though, until https://github.com/getsentry/sentry-javascript/issues/5718 is addressed. // (TL;DR: Those functions touch many parts of the repo in many different ways, and need to be clened up. Once // that's happened, it will be easier to add this logic in without worrying about unexpected side effects.) - const { transactionNamingScheme } = _options; const { sdkProcessingMetadata = {} } = event; const req = sdkProcessingMetadata.request; @@ -83,38 +81,7 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); - const processedEvent = addRequestDataToEvent(event, req, addRequestDataOptions); - - // Transaction events already have the right `transaction` value - if (event.type === 'transaction' || transactionNamingScheme === 'handler') { - return processedEvent; - } - - // In all other cases, use the request's associated transaction (if any) to overwrite the event's `transaction` - // value with a high-quality one - const reqWithTransaction = req as { _sentryTransaction?: Span }; - const transaction = reqWithTransaction._sentryTransaction; - if (transaction) { - const name = spanToJSON(transaction).description || ''; - - // TODO (v8): Remove the nextjs check and just base it on `transactionNamingScheme` for all SDKs. (We have to - // keep it the way it is for the moment, because changing the names of transactions in Sentry has the potential - // to break things like alert rules.) - const shouldIncludeMethodInTransactionName = - getSDKName(client) === 'sentry.javascript.nextjs' - ? name.startsWith('/api') - : transactionNamingScheme !== 'path'; - - const [transactionValue] = extractPathForTransaction(req, { - path: true, - method: shouldIncludeMethodInTransactionName, - customRoute: name, - }); - - processedEvent.transaction = transactionValue; - } - - return processedEvent; + return addRequestDataToEvent(event, req, addRequestDataOptions); }, }; }) satisfies IntegrationFn; @@ -166,15 +133,3 @@ function convertReqDataIntegrationOptsToAddReqDataOpts( }, }; } - -function getSDKName(client: Client): string | undefined { - try { - // For a long chain like this, it's fewer bytes to combine a try-catch with assuming everything is there than to - // write out a long chain of `a && a.b && a.b.c && ...` - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return client.getOptions()._metadata!.sdk!.name; - } catch (err) { - // In theory we should never get here - return undefined; - } -} From 65e07a1c08ecf8767cbb86a057ced2d037f08094 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 9 Apr 2024 18:04:11 +0200 Subject: [PATCH 2/6] format --- packages/core/src/integrations/requestdata.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 6fa4e431364f..6ecce5f614cc 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -1,4 +1,4 @@ -import type { Client, IntegrationFn } from '@sentry/types'; +import type { IntegrationFn } from '@sentry/types'; import type { AddRequestDataToEventOptions, TransactionNamingScheme } from '@sentry/utils'; import { addRequestDataToEvent } from '@sentry/utils'; import { defineIntegration } from '../integration'; From f12aa7f6538f3cceba300c0c293ee514bffcf1bb Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 9 Apr 2024 18:34:57 +0200 Subject: [PATCH 3/6] remove another event.transaction assignment --- packages/opentelemetry/src/setupEventContextTrace.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/opentelemetry/src/setupEventContextTrace.ts b/packages/opentelemetry/src/setupEventContextTrace.ts index 1aa1edbbe12c..65d8d2131eef 100644 --- a/packages/opentelemetry/src/setupEventContextTrace.ts +++ b/packages/opentelemetry/src/setupEventContextTrace.ts @@ -1,9 +1,8 @@ -import { getRootSpan } from '@sentry/core'; import type { Client } from '@sentry/types'; import { dropUndefinedKeys } from '@sentry/utils'; import { getActiveSpan } from './utils/getActiveSpan'; -import { spanHasName, spanHasParentId } from './utils/spanTypes'; +import { spanHasParentId } from './utils/spanTypes'; /** Ensure the `trace` context is set on all events. */ export function setupEventContextTrace(client: Client): void { @@ -27,12 +26,6 @@ export function setupEventContextTrace(client: Client): void { ...event.contexts, }; - const rootSpan = getRootSpan(span); - const transactionName = spanHasName(rootSpan) ? rootSpan.name : undefined; - if (transactionName && !event.transaction) { - event.transaction = transactionName; - } - return event; }); } From d76edc607f67f058c354b78e0999775fe8a01a99 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 24 Apr 2024 15:39:26 +0000 Subject: [PATCH 4/6] Remove incorrect assertions --- packages/node/test/integration/scope.test.ts | 4 ---- packages/opentelemetry/test/integration/scope.test.ts | 6 ------ 2 files changed, 10 deletions(-) diff --git a/packages/node/test/integration/scope.test.ts b/packages/node/test/integration/scope.test.ts index ed9f20cf2b6c..6c08ea902c18 100644 --- a/packages/node/test/integration/scope.test.ts +++ b/packages/node/test/integration/scope.test.ts @@ -86,7 +86,6 @@ describe('Integration | Scope', () => { tag3: 'val3', tag4: 'val4', }, - ...(enableTracing ? { transaction: 'outer' } : {}), }), { event_id: expect.any(String), @@ -125,7 +124,6 @@ describe('Integration | Scope', () => { tag4: 'val4', }, timestamp: expect.any(Number), - transaction: 'outer', transaction_info: { source: 'custom' }, type: 'transaction', }), @@ -209,7 +207,6 @@ describe('Integration | Scope', () => { tag3: 'val3a', tag4: 'val4a', }, - ...(enableTracing ? { transaction: 'outer' } : {}), }), { event_id: expect.any(String), @@ -236,7 +233,6 @@ describe('Integration | Scope', () => { tag3: 'val3b', tag4: 'val4b', }, - ...(enableTracing ? { transaction: 'outer' } : {}), }), { event_id: expect.any(String), diff --git a/packages/opentelemetry/test/integration/scope.test.ts b/packages/opentelemetry/test/integration/scope.test.ts index e17516a9d7bf..e6c70fd8c7d4 100644 --- a/packages/opentelemetry/test/integration/scope.test.ts +++ b/packages/opentelemetry/test/integration/scope.test.ts @@ -93,7 +93,6 @@ describe('Integration | Scope', () => { tag3: 'val3', tag4: 'val4', }, - ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), @@ -132,7 +131,6 @@ describe('Integration | Scope', () => { tag4: 'val4', }, timestamp: expect.any(Number), - transaction: 'outer', transaction_info: { source: 'custom' }, type: 'transaction', }), @@ -226,7 +224,6 @@ describe('Integration | Scope', () => { tag3: 'val3a', tag4: 'val4a', }, - ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), @@ -253,7 +250,6 @@ describe('Integration | Scope', () => { tag3: 'val3b', tag4: 'val4b', }, - ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), @@ -358,7 +354,6 @@ describe('Integration | Scope', () => { isolationTag1: 'val1', isolationTag2: 'val2', }, - ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), @@ -385,7 +380,6 @@ describe('Integration | Scope', () => { isolationTag1: 'val1', isolationTag2: 'val2b', }, - ...(enableTracing ? { transaction: 'outer' } : undefined), }), { event_id: expect.any(String), From 2d61a45c9f3f3dedadabb4c243919923d2d42f56 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 25 Apr 2024 08:19:34 +0000 Subject: [PATCH 5/6] Fix nextjs transaction names --- .../src/common/wrapApiHandlerWithSentry.ts | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts index 2a3ecd02bdb5..91f3e8085959 100644 --- a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts @@ -6,7 +6,7 @@ import { startSpanManual, withIsolationScope, } from '@sentry/core'; -import { consoleSandbox, isString, logger, objectify, stripUrlQueryAndFragment } from '@sentry/utils'; +import { consoleSandbox, isString, logger, objectify } from '@sentry/utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import type { AugmentedNextApiRequest, AugmentedNextApiResponse, NextApiHandler } from './types'; @@ -20,7 +20,7 @@ import { escapeNextjsTracing } from './utils/tracingUtils'; * * @param apiHandler The handler exported from the user's API page route file, which may or may not already be * wrapped with `withSentry` - * @param parameterizedRoute The page's route, passed in via the proxy loader + * @param parameterizedRoute The page's parameterized route. * @returns The wrapped handler */ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameterizedRoute: string): NextApiHandler { @@ -62,30 +62,14 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz baggage: req.headers?.baggage, }, () => { - // prefer the parameterized route, if we have it (which we will if we've auto-wrapped the route handler) - let reqPath = parameterizedRoute; - - // If not, fake it by just replacing parameter values with their names, hoping that none of them match either - // each other or any hard-coded parts of the path - if (!reqPath) { - const url = `${req.url}`; - // pull off query string, if any - reqPath = stripUrlQueryAndFragment(url); - // Replace with placeholder - if (req.query) { - for (const [key, value] of Object.entries(req.query)) { - reqPath = reqPath.replace(`${value}`, `[${key}]`); - } - } - } - const reqMethod = `${(req.method || 'GET').toUpperCase()} `; isolationScope.setSDKProcessingMetadata({ request: req }); + isolationScope.setTransactionName(`${reqMethod}${parameterizedRoute}`); return startSpanManual( { - name: `${reqMethod}${reqPath}`, + name: `${reqMethod}${parameterizedRoute}`, op: 'http.server', forceTransaction: true, attributes: { From 49baed53d543ee4f00b83bfdb7d0753c10001c9f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 25 Apr 2024 08:40:33 +0000 Subject: [PATCH 6/6] review comment --- packages/core/src/integrations/requestdata.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 6ecce5f614cc..f7846dec6fea 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -66,10 +66,10 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = return { name: INTEGRATION_NAME, - processEvent(event, _hint, _client) { + processEvent(event) { // Note: In the long run, most of the logic here should probably move into the request data utility functions. For // the moment it lives here, though, until https://github.com/getsentry/sentry-javascript/issues/5718 is addressed. - // (TL;DR: Those functions touch many parts of the repo in many different ways, and need to be clened up. Once + // (TL;DR: Those functions touch many parts of the repo in many different ways, and need to be cleaned up. Once // that's happened, it will be easier to add this logic in without worrying about unexpected side effects.) const { sdkProcessingMetadata = {} } = event;