From e6ee74e6e3465a546839d2ffc50c2f63a64fc699 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 14 Nov 2024 15:52:14 +0100 Subject: [PATCH 1/6] ref(utils): Stop setting `transaction` in `requestDataIntegration` This is not really necessary anymore - it only sets this on transaction events, and those get the `transaction` in different places already anyhow. With this, we can also actually remove some other stuff. One method is exported from utils but not otherwise used, we can also drop this in v9. Finally, this was also the only place that used `route` on the request, so we can also get rid of this in `remix`, which is weird anyhow because we set it for errors there but don't even use it for them. --- packages/remix/src/utils/errors.ts | 19 +------------- packages/remix/src/utils/instrumentServer.ts | 3 --- packages/utils/src/requestdata.ts | 26 ++------------------ 3 files changed, 3 insertions(+), 45 deletions(-) diff --git a/packages/remix/src/utils/errors.ts b/packages/remix/src/utils/errors.ts index 92958c2c3eb3..100dac496c75 100644 --- a/packages/remix/src/utils/errors.ts +++ b/packages/remix/src/utils/errors.ts @@ -1,12 +1,5 @@ import type { AppData, DataFunctionArgs, EntryContext, HandleDocumentRequestFunction } from '@remix-run/node'; -import { - captureException, - getActiveSpan, - getClient, - getRootSpan, - handleCallbackErrors, - spanToJSON, -} from '@sentry/core'; +import { captureException, getClient, handleCallbackErrors } from '@sentry/core'; import type { Span } from '@sentry/types'; import { addExceptionMechanism, isPrimitive, logger, objectify } from '@sentry/utils'; import { DEBUG_BUILD } from './debug-build'; @@ -61,19 +54,9 @@ export async function captureRemixServerException( const objectifiedErr = objectify(err); captureException(isResponse(objectifiedErr) ? await extractResponseError(objectifiedErr) : objectifiedErr, scope => { - const activeSpan = getActiveSpan(); - const rootSpan = activeSpan && getRootSpan(activeSpan); - const activeRootSpanName = rootSpan ? spanToJSON(rootSpan).description : undefined; - scope.setSDKProcessingMetadata({ request: { ...normalizedRequest, - // When `route` is not defined, `RequestData` integration uses the full URL - route: activeRootSpanName - ? { - path: activeRootSpanName, - } - : undefined, }, }); diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index e83c14dfbbc4..666c332afa04 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -314,9 +314,6 @@ function wrapRequestHandler( isolationScope.setSDKProcessingMetadata({ request: { ...normalizedRequest, - route: { - path: name, - }, }, }); diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index edffc6f67da7..dbd0bd49a815 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -21,7 +21,6 @@ import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress'; const DEFAULT_INCLUDES = { ip: false, request: true, - transaction: true, user: true, }; const DEFAULT_REQUEST_INCLUDES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; @@ -35,6 +34,7 @@ export type AddRequestDataToEventOptions = { include?: { ip?: boolean; request?: boolean | Array<(typeof DEFAULT_REQUEST_INCLUDES)[number]>; + /** @deprecated This option will be removed in v9. It does not do anything anymore, the `transcation` is set in other places. */ transaction?: boolean | TransactionNamingScheme; user?: boolean | Array<(typeof DEFAULT_USER_INCLUDES)[number]>; }; @@ -67,6 +67,7 @@ export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler'; * used instead of the request's route) * * @returns A tuple of the fully constructed transaction name [0] and its source [1] (can be either 'route' or 'url') + * @deprecated This method will be removed in v9. It is not in use anymore. */ export function extractPathForTransaction( req: PolymorphicRequest, @@ -102,23 +103,6 @@ export function extractPathForTransaction( return [name, source]; } -function extractTransaction(req: PolymorphicRequest, type: boolean | TransactionNamingScheme): string { - switch (type) { - case 'path': { - return extractPathForTransaction(req, { path: true })[0]; - } - case 'handler': { - return (req.route && req.route.stack && req.route.stack[0] && req.route.stack[0].name) || ''; - } - case 'methodPath': - default: { - // if exist _reconstructedRoute return that path instead of route.path - const customRoute = req._reconstructedRoute ? req._reconstructedRoute : undefined; - return extractPathForTransaction(req, { path: true, method: true, customRoute })[0]; - } - } -} - function extractUserData( user: { [key: string]: unknown; @@ -379,12 +363,6 @@ export function addRequestDataToEvent( } } - if (include.transaction && !event.transaction && event.type === 'transaction') { - // TODO do we even need this anymore? - // TODO make this work for nextjs - event.transaction = extractTransaction(req, include.transaction); - } - return event; } From d8356b52a417a1abc749c8a0a11e48279055c919 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 14 Nov 2024 15:20:17 +0000 Subject: [PATCH 2/6] migration guide --- docs/migration/draft-v8-migration-guide.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/migration/draft-v8-migration-guide.md diff --git a/docs/migration/draft-v8-migration-guide.md b/docs/migration/draft-v8-migration-guide.md new file mode 100644 index 000000000000..d8089d461318 --- /dev/null +++ b/docs/migration/draft-v8-migration-guide.md @@ -0,0 +1,5 @@ + + +# Deprecations + +- Deprecated `AddRequestDataToEventOptions.request`. This option effectively doesn't do anything anymore. From 1e759ba771b5416966eca3f2d4e35c2ceeefddc6 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 15 Nov 2024 09:42:54 +0100 Subject: [PATCH 3/6] update migration guide --- docs/migration/draft-v8-migration-guide.md | 5 ----- docs/migration/draft-v9-migration-guide.md | 8 ++++++++ 2 files changed, 8 insertions(+), 5 deletions(-) delete mode 100644 docs/migration/draft-v8-migration-guide.md create mode 100644 docs/migration/draft-v9-migration-guide.md diff --git a/docs/migration/draft-v8-migration-guide.md b/docs/migration/draft-v8-migration-guide.md deleted file mode 100644 index d8089d461318..000000000000 --- a/docs/migration/draft-v8-migration-guide.md +++ /dev/null @@ -1,5 +0,0 @@ - - -# Deprecations - -- Deprecated `AddRequestDataToEventOptions.request`. This option effectively doesn't do anything anymore. diff --git a/docs/migration/draft-v9-migration-guide.md b/docs/migration/draft-v9-migration-guide.md new file mode 100644 index 000000000000..7f4c795948cc --- /dev/null +++ b/docs/migration/draft-v9-migration-guide.md @@ -0,0 +1,8 @@ + + +# Deprecations + +## `@sentry/utils` + +- Deprecated `AddRequestDataToEventOptions.transaction`. This option effectively doesn't do anything anymore, and will + be removed in v9. From 1025797b01babc1bec829ea016ff2419f7a680e2 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 15 Nov 2024 09:44:06 +0100 Subject: [PATCH 4/6] remove test --- packages/utils/test/requestdata.test.ts | 61 ------------------------- 1 file changed, 61 deletions(-) diff --git a/packages/utils/test/requestdata.test.ts b/packages/utils/test/requestdata.test.ts index 570f80647b6b..20f950add715 100644 --- a/packages/utils/test/requestdata.test.ts +++ b/packages/utils/test/requestdata.test.ts @@ -369,67 +369,6 @@ describe('addRequestDataToEvent', () => { } }); }); - - describe('transaction property', () => { - describe('for transaction events', () => { - beforeEach(() => { - mockEvent.type = 'transaction'; - }); - - test('extracts method and full route path by default`', () => { - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - - expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName'); - }); - - test('extracts method and full path by default when mountpoint is `/`', () => { - mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', ''); - mockReq.baseUrl = ''; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - - // `subpath/` is the full path here, because there's no router mount path - expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName'); - }); - - test('fallback to method and `originalUrl` if route is missing', () => { - delete mockReq.route; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - - expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue'); - }); - - test('can extract path only instead if configured', () => { - const optionsWithPathTransaction = { - include: { - transaction: 'path', - }, - } as const; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithPathTransaction); - - expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName'); - }); - - test('can extract handler name instead if configured', () => { - const optionsWithHandlerTransaction = { - include: { - transaction: 'handler', - }, - } as const; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithHandlerTransaction); - - expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler'); - }); - }); - it('transaction is not applied to non-transaction events', () => { - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - - expect(parsedRequest.transaction).toBeUndefined(); - }); - }); }); describe('extractRequestData', () => { From c2f1130cb086127374de472b6316c064b471fa73 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 15 Nov 2024 09:50:45 +0100 Subject: [PATCH 5/6] fix linting & deprecations --- packages/core/src/integrations/requestdata.ts | 7 ++++++- packages/utils/src/requestdata.ts | 4 ++++ packages/utils/test/requestdata.test.ts | 2 ++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 9475973d8e2c..15be450cf27e 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -24,7 +24,11 @@ export type RequestDataIntegrationOptions = { }; }; - /** Whether to identify transactions by parameterized path, parameterized path with method, or handler name */ + /** + * Whether to identify transactions by parameterized path, parameterized path with method, or handler name. + * @deprecated This option does not do anything anymore, and will be removed in v9. + */ + // eslint-disable-next-line deprecation/deprecation transactionNamingScheme?: TransactionNamingScheme; }; @@ -110,6 +114,7 @@ function convertReqDataIntegrationOptsToAddReqDataOpts( integrationOptions: Required, ): AddRequestDataToEventOptions { const { + // eslint-disable-next-line deprecation/deprecation transactionNamingScheme, include: { ip, user, ...requestOptions }, } = integrationOptions; diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index dbd0bd49a815..26b12b07c69e 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -35,6 +35,7 @@ export type AddRequestDataToEventOptions = { ip?: boolean; request?: boolean | Array<(typeof DEFAULT_REQUEST_INCLUDES)[number]>; /** @deprecated This option will be removed in v9. It does not do anything anymore, the `transcation` is set in other places. */ + // eslint-disable-next-line deprecation/deprecation transaction?: boolean | TransactionNamingScheme; user?: boolean | Array<(typeof DEFAULT_USER_INCLUDES)[number]>; }; @@ -52,6 +53,9 @@ export type AddRequestDataToEventOptions = { }; }; +/** + * @deprecated This type will be removed in v9. It is not in use anymore. + */ export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler'; /** diff --git a/packages/utils/test/requestdata.test.ts b/packages/utils/test/requestdata.test.ts index 20f950add715..90c734f23f2c 100644 --- a/packages/utils/test/requestdata.test.ts +++ b/packages/utils/test/requestdata.test.ts @@ -702,6 +702,7 @@ describe('extractPathForTransaction', () => { expectedRoute: string, expectedSource: TransactionSource, ) => { + // eslint-disable-next-line deprecation/deprecation const [route, source] = extractPathForTransaction(req, options); expect(route).toEqual(expectedRoute); @@ -717,6 +718,7 @@ describe('extractPathForTransaction', () => { originalUrl: '/api/users/123/details', } as PolymorphicRequest; + // eslint-disable-next-line deprecation/deprecation const [route, source] = extractPathForTransaction(req, { path: true, method: true, From 7451ff8f5c82af2f3c666b8f5912958511e9e7df Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 15 Nov 2024 09:04:05 +0000 Subject: [PATCH 6/6] Add deprecations to guide --- docs/migration/draft-v9-migration-guide.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/migration/draft-v9-migration-guide.md b/docs/migration/draft-v9-migration-guide.md index 7f4c795948cc..5630d82ede90 100644 --- a/docs/migration/draft-v9-migration-guide.md +++ b/docs/migration/draft-v9-migration-guide.md @@ -6,3 +6,8 @@ - Deprecated `AddRequestDataToEventOptions.transaction`. This option effectively doesn't do anything anymore, and will be removed in v9. +- Deprecated `TransactionNamingScheme` type. + +## `@sentry/core` + +- Deprecated `transactionNamingScheme` option in `requestDataIntegration`.