From 1b40eb9879e02fdef6e4c8c8d64d0e6d71e5bb09 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 13 Dec 2023 11:21:59 +0100 Subject: [PATCH 1/2] ref(core): Refactor core integrations to avoid `setupOnce` --- packages/core/src/integrations/metadata.ts | 21 ++-- packages/core/src/integrations/requestdata.ts | 101 +++++++++--------- .../test/integrations/requestdata.test.ts | 27 +++-- 3 files changed, 76 insertions(+), 73 deletions(-) diff --git a/packages/core/src/integrations/metadata.ts b/packages/core/src/integrations/metadata.ts index caa9ad972e7d..1803640ea4dd 100644 --- a/packages/core/src/integrations/metadata.ts +++ b/packages/core/src/integrations/metadata.ts @@ -1,4 +1,4 @@ -import type { EventItem, EventProcessor, Hub, Integration } from '@sentry/types'; +import type { Client, Event, EventItem, EventProcessor, Hub, Integration } from '@sentry/types'; import { forEachEnvelopeItem } from '@sentry/utils'; import { addMetadataToStackFrames, stripMetadataFromStackFrames } from '../metadata'; @@ -30,10 +30,13 @@ export class ModuleMetadata implements Integration { /** * @inheritDoc */ - public setupOnce(addGlobalEventProcessor: (processor: EventProcessor) => void, getCurrentHub: () => Hub): void { - const client = getCurrentHub().getClient(); + public setupOnce(_addGlobalEventProcessor: (processor: EventProcessor) => void, _getCurrentHub: () => Hub): void { + // noop + } - if (!client || typeof client.on !== 'function') { + /** @inheritDoc */ + public setup(client: Client): void { + if (typeof client.on !== 'function') { return; } @@ -50,12 +53,12 @@ export class ModuleMetadata implements Integration { } }); }); + } + /** @inheritDoc */ + public processEvent(event: Event, _hint: unknown, client: Client): Event { const stackParser = client.getOptions().stackParser; - - addGlobalEventProcessor(event => { - addMetadataToStackFrames(stackParser, event); - return event; - }); + addMetadataToStackFrames(stackParser, event); + return event; } } diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 4481501d8b8c..6aded915d854 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -1,4 +1,4 @@ -import type { Event, EventProcessor, Hub, Integration, PolymorphicRequest, Transaction } from '@sentry/types'; +import type { Client, Event, EventProcessor, Hub, Integration, PolymorphicRequest, Transaction } from '@sentry/types'; import type { AddRequestDataToEventOptions, TransactionNamingScheme } from '@sentry/utils'; import { addRequestDataToEvent, extractPathForTransaction } from '@sentry/utils'; @@ -95,65 +95,66 @@ export class RequestData implements Integration { /** * @inheritDoc */ - public setupOnce(addGlobalEventProcessor: (eventProcessor: EventProcessor) => void, getCurrentHub: () => Hub): void { + public setupOnce( + _addGlobalEventProcessor: (eventProcessor: EventProcessor) => void, + _getCurrentHub: () => Hub, + ): void { + // noop + } + + /** @inheritdoc */ + public processEvent(event: Event, _hint: unknown, client: Client): 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 // that's happened, it will be easier to add this logic in without worrying about unexpected side effects.) const { transactionNamingScheme } = this._options; - addGlobalEventProcessor(event => { - const hub = getCurrentHub(); - const self = hub.getIntegration(RequestData); - - const { sdkProcessingMetadata = {} } = event; - const req = sdkProcessingMetadata.request; + const { sdkProcessingMetadata = {} } = event; + const req = sdkProcessingMetadata.request; - // If the globally installed instance of this integration isn't associated with the current hub, `self` will be - // undefined - if (!self || !req) { - return event; - } + if (!req) { + return event; + } - // The Express request handler takes a similar `include` option to that which can be passed to this integration. - // If passed there, we store it in `sdkProcessingMetadata`. TODO(v8): Force express and GCP people to use this - // integration, so that all of this passing and conversion isn't necessary - const addRequestDataOptions = - sdkProcessingMetadata.requestDataOptionsFromExpressHandler || - sdkProcessingMetadata.requestDataOptionsFromGCPWrapper || - convertReqDataIntegrationOptsToAddReqDataOpts(this._options); + // The Express request handler takes a similar `include` option to that which can be passed to this integration. + // If passed there, we store it in `sdkProcessingMetadata`. TODO(v8): Force express and GCP people to use this + // integration, so that all of this passing and conversion isn't necessary + const addRequestDataOptions = + sdkProcessingMetadata.requestDataOptionsFromExpressHandler || + sdkProcessingMetadata.requestDataOptionsFromGCPWrapper || + convertReqDataIntegrationOptsToAddReqDataOpts(this._options); - const processedEvent = this._addRequestData(event, req, addRequestDataOptions); + const processedEvent = this._addRequestData(event, req, addRequestDataOptions); - // Transaction events already have the right `transaction` value - if (event.type === 'transaction' || transactionNamingScheme === 'handler') { - return processedEvent; - } + // 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?: Transaction }; - const transaction = reqWithTransaction._sentryTransaction; - if (transaction) { - // 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(hub) === 'sentry.javascript.nextjs' - ? transaction.name.startsWith('/api') - : transactionNamingScheme !== 'path'; - - const [transactionValue] = extractPathForTransaction(req, { - path: true, - method: shouldIncludeMethodInTransactionName, - customRoute: transaction.name, - }); - - processedEvent.transaction = transactionValue; - } + // 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?: Transaction }; + const transaction = reqWithTransaction._sentryTransaction; + if (transaction) { + // 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' + ? transaction.name.startsWith('/api') + : transactionNamingScheme !== 'path'; + + const [transactionValue] = extractPathForTransaction(req, { + path: true, + method: shouldIncludeMethodInTransactionName, + customRoute: transaction.name, + }); + + processedEvent.transaction = transactionValue; + } - return processedEvent; - }); + return processedEvent; } } @@ -199,12 +200,12 @@ function convertReqDataIntegrationOptsToAddReqDataOpts( }; } -function getSDKName(hub: Hub): string | undefined { +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 hub.getClient()!.getOptions()!._metadata!.sdk!.name; + return client.getOptions()._metadata!.sdk!.name; } catch (err) { // In theory we should never get here return undefined; diff --git a/packages/node/test/integrations/requestdata.test.ts b/packages/node/test/integrations/requestdata.test.ts index 4895ef43c678..7b5dc41434db 100644 --- a/packages/node/test/integrations/requestdata.test.ts +++ b/packages/node/test/integrations/requestdata.test.ts @@ -1,6 +1,6 @@ import * as http from 'http'; import type { RequestDataIntegrationOptions } from '@sentry/core'; -import { Hub, RequestData, getCurrentHub, makeMain } from '@sentry/core'; +import { RequestData, getCurrentHub } from '@sentry/core'; import type { Event, EventProcessor, PolymorphicRequest } from '@sentry/types'; import * as sentryUtils from '@sentry/utils'; @@ -9,7 +9,6 @@ import { requestHandler } from '../../src/handlers'; import { getDefaultNodeClientOptions } from '../helper/node-client-options'; const addRequestDataToEventSpy = jest.spyOn(sentryUtils, 'addRequestDataToEvent'); -const requestDataEventProcessor = jest.fn(); const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' }; const method = 'wagging'; @@ -18,10 +17,7 @@ const hostname = 'the.dog.park'; const path = '/by/the/trees/'; const queryString = 'chase=me&please=thankyou'; -function initWithRequestDataIntegrationOptions(integrationOptions: RequestDataIntegrationOptions): void { - const setMockEventProcessor = (eventProcessor: EventProcessor) => - requestDataEventProcessor.mockImplementationOnce(eventProcessor); - +function initWithRequestDataIntegrationOptions(integrationOptions: RequestDataIntegrationOptions): EventProcessor { const requestDataIntegration = new RequestData({ ...integrationOptions, }); @@ -32,12 +28,15 @@ function initWithRequestDataIntegrationOptions(integrationOptions: RequestDataIn integrations: [requestDataIntegration], }), ); - client.setupIntegrations = () => requestDataIntegration.setupOnce(setMockEventProcessor, getCurrentHub); - client.getIntegration = () => requestDataIntegration as any; - const hub = new Hub(client); + getCurrentHub().bindClient(client); + + const eventProcessors = client['_eventProcessors'] as EventProcessor[]; + const eventProcessor = eventProcessors.find(processor => processor.id === 'RequestData'); + + expect(eventProcessor).toBeDefined(); - makeMain(hub); + return eventProcessor!; } describe('`RequestData` integration', () => { @@ -64,12 +63,12 @@ describe('`RequestData` integration', () => { const res = new http.ServerResponse(req); const next = jest.fn(); - initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); + const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); sentryRequestMiddleware(req, res, next); await getCurrentHub().getScope()!.applyToEvent(event, {}); - requestDataEventProcessor(event); + void requestDataEventProcessor(event, {}); const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; @@ -93,12 +92,12 @@ describe('`RequestData` integration', () => { const wrappedGCPFunction = mockGCPWrapper(jest.fn(), { include: { transaction: 'methodPath' } }); const res = new http.ServerResponse(req); - initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); + const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); wrappedGCPFunction(req, res); await getCurrentHub().getScope()!.applyToEvent(event, {}); - requestDataEventProcessor(event); + void requestDataEventProcessor(event, {}); const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; From a6ce41d3d835d29823e91ef3eafb8b72eb0adfe9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 13 Dec 2023 11:54:15 +0100 Subject: [PATCH 2/2] fix core test --- .../test/lib/integrations/requestdata.test.ts | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/packages/core/test/lib/integrations/requestdata.test.ts b/packages/core/test/lib/integrations/requestdata.test.ts index 982d5f57566d..fe80c2ffd623 100644 --- a/packages/core/test/lib/integrations/requestdata.test.ts +++ b/packages/core/test/lib/integrations/requestdata.test.ts @@ -1,13 +1,12 @@ import type { IncomingMessage } from 'http'; import type { RequestDataIntegrationOptions } from '@sentry/core'; -import { Hub, RequestData, getCurrentHub, makeMain } from '@sentry/core'; +import { RequestData, getCurrentHub } from '@sentry/core'; import type { Event, EventProcessor } from '@sentry/types'; import * as sentryUtils from '@sentry/utils'; import { TestClient, getDefaultTestClientOptions } from '../../mocks/client'; const addRequestDataToEventSpy = jest.spyOn(sentryUtils, 'addRequestDataToEvent'); -const requestDataEventProcessor = jest.fn(); const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' }; const method = 'wagging'; @@ -16,10 +15,7 @@ const hostname = 'the.dog.park'; const path = '/by/the/trees/'; const queryString = 'chase=me&please=thankyou'; -function initWithRequestDataIntegrationOptions(integrationOptions: RequestDataIntegrationOptions): void { - const setMockEventProcessor = (eventProcessor: EventProcessor) => - requestDataEventProcessor.mockImplementationOnce(eventProcessor); - +function initWithRequestDataIntegrationOptions(integrationOptions: RequestDataIntegrationOptions): EventProcessor { const requestDataIntegration = new RequestData({ ...integrationOptions, }); @@ -30,12 +26,15 @@ function initWithRequestDataIntegrationOptions(integrationOptions: RequestDataIn integrations: [requestDataIntegration], }), ); - client.setupIntegrations = () => requestDataIntegration.setupOnce(setMockEventProcessor, getCurrentHub); - client.getIntegration = () => requestDataIntegration as any; - const hub = new Hub(client); + getCurrentHub().bindClient(client); + + const eventProcessors = client['_eventProcessors'] as EventProcessor[]; + const eventProcessor = eventProcessors.find(processor => processor.id === 'RequestData'); + + expect(eventProcessor).toBeDefined(); - makeMain(hub); + return eventProcessor!; } describe('`RequestData` integration', () => { @@ -58,9 +57,9 @@ describe('`RequestData` integration', () => { describe('option conversion', () => { it('leaves `ip` and `user` at top level of `include`', () => { - initWithRequestDataIntegrationOptions({ include: { ip: false, user: true } }); + const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ include: { ip: false, user: true } }); - requestDataEventProcessor(event); + void requestDataEventProcessor(event, {}); const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; @@ -68,9 +67,9 @@ describe('`RequestData` integration', () => { }); it('moves `transactionNamingScheme` to `transaction` include', () => { - initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); + const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); - requestDataEventProcessor(event); + void requestDataEventProcessor(event, {}); const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; @@ -78,9 +77,11 @@ describe('`RequestData` integration', () => { }); it('moves `true` request keys into `request` include, but omits `false` ones', async () => { - initWithRequestDataIntegrationOptions({ include: { data: true, cookies: false } }); + const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ + include: { data: true, cookies: false }, + }); - requestDataEventProcessor(event); + void requestDataEventProcessor(event, {}); const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; @@ -89,9 +90,11 @@ describe('`RequestData` integration', () => { }); it('moves `true` user keys into `user` include, but omits `false` ones', async () => { - initWithRequestDataIntegrationOptions({ include: { user: { id: true, email: false } } }); + const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ + include: { user: { id: true, email: false } }, + }); - requestDataEventProcessor(event); + void requestDataEventProcessor(event, {}); const passedOptions = addRequestDataToEventSpy.mock.calls[0][2];