From 74a12d90c052fa2bda59db1e1ad9560725b0a64d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 20 Dec 2023 09:26:15 +0100 Subject: [PATCH] ref(core): Refactor core integrations to functional style (#9916) Also make a small adjustment to the legacy converter util to ensure `setupOnce()` is callable with or without arguments, to remain stable. --- packages/core/src/integration.ts | 24 ++- .../core/src/integrations/functiontostring.ts | 60 +++--- .../core/src/integrations/linkederrors.ts | 86 ++++----- packages/core/src/integrations/metadata.ts | 88 ++++----- packages/core/src/integrations/requestdata.ts | 180 ++++++++---------- .../serverless/test/__mocks__/@sentry/node.ts | 2 + packages/serverless/test/gcpfunction.test.ts | 13 +- 7 files changed, 201 insertions(+), 252 deletions(-) diff --git a/packages/core/src/integration.ts b/packages/core/src/integration.ts index b7782fcfa65c..01a55081c04f 100644 --- a/packages/core/src/integration.ts +++ b/packages/core/src/integration.ts @@ -1,4 +1,14 @@ -import type { Client, Event, EventHint, Integration, IntegrationClass, IntegrationFn, Options } from '@sentry/types'; +import type { + Client, + Event, + EventHint, + EventProcessor, + Hub, + Integration, + IntegrationClass, + IntegrationFn, + Options, +} from '@sentry/types'; import { arrayify, logger } from '@sentry/utils'; import { DEBUG_BUILD } from './debug-build'; @@ -165,7 +175,11 @@ function findIndex(arr: T[], callback: (item: T) => boolean): number { export function convertIntegrationFnToClass( name: string, fn: Fn, -): IntegrationClass { +): IntegrationClass< + Integration & { + setupOnce: (addGlobalEventProcessor?: (callback: EventProcessor) => void, getCurrentHub?: () => Hub) => void; + } +> { return Object.assign( // eslint-disable-next-line @typescript-eslint/no-explicit-any function ConvertedIntegration(...rest: any[]) { @@ -176,5 +190,9 @@ export function convertIntegrationFnToClass( }; }, { id: name }, - ) as unknown as IntegrationClass; + ) as unknown as IntegrationClass< + Integration & { + setupOnce: (addGlobalEventProcessor?: (callback: EventProcessor) => void, getCurrentHub?: () => Hub) => void; + } + >; } diff --git a/packages/core/src/integrations/functiontostring.ts b/packages/core/src/integrations/functiontostring.ts index b1db3224bc64..124085c9c6c6 100644 --- a/packages/core/src/integrations/functiontostring.ts +++ b/packages/core/src/integrations/functiontostring.ts @@ -1,41 +1,33 @@ -import type { Integration, WrappedFunction } from '@sentry/types'; +import type { IntegrationFn, WrappedFunction } from '@sentry/types'; import { getOriginalFunction } from '@sentry/utils'; +import { convertIntegrationFnToClass } from '../integration'; let originalFunctionToString: () => void; -/** Patch toString calls to return proper name for wrapped functions */ -export class FunctionToString implements Integration { - /** - * @inheritDoc - */ - public static id: string = 'FunctionToString'; - - /** - * @inheritDoc - */ - public name: string; +const INTEGRATION_NAME = 'FunctionToString'; - public constructor() { - this.name = FunctionToString.id; - } +const functionToStringIntegration: IntegrationFn = () => { + return { + name: INTEGRATION_NAME, + setupOnce() { + // eslint-disable-next-line @typescript-eslint/unbound-method + originalFunctionToString = Function.prototype.toString; - /** - * @inheritDoc - */ - public setupOnce(): void { - // eslint-disable-next-line @typescript-eslint/unbound-method - originalFunctionToString = Function.prototype.toString; + // intrinsics (like Function.prototype) might be immutable in some environments + // e.g. Node with --frozen-intrinsics, XS (an embedded JavaScript engine) or SES (a JavaScript proposal) + try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + Function.prototype.toString = function (this: WrappedFunction, ...args: any[]): string { + const context = getOriginalFunction(this) || this; + return originalFunctionToString.apply(context, args); + }; + } catch { + // ignore errors here, just don't patch this + } + }, + }; +}; - // intrinsics (like Function.prototype) might be immutable in some environments - // e.g. Node with --frozen-intrinsics, XS (an embedded JavaScript engine) or SES (a JavaScript proposal) - try { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - Function.prototype.toString = function (this: WrappedFunction, ...args: any[]): string { - const context = getOriginalFunction(this) || this; - return originalFunctionToString.apply(context, args); - }; - } catch { - // ignore errors here, just don't patch this - } - } -} +/** Patch toString calls to return proper name for wrapped functions */ +// eslint-disable-next-line deprecation/deprecation +export const FunctionToString = convertIntegrationFnToClass(INTEGRATION_NAME, functionToStringIntegration); diff --git a/packages/core/src/integrations/linkederrors.ts b/packages/core/src/integrations/linkederrors.ts index 1b3ef1c39a19..aa9df808e2d8 100644 --- a/packages/core/src/integrations/linkederrors.ts +++ b/packages/core/src/integrations/linkederrors.ts @@ -1,59 +1,39 @@ -import type { Client, Event, EventHint, Integration } from '@sentry/types'; +import type { IntegrationFn } from '@sentry/types'; import { applyAggregateErrorsToEvent, exceptionFromError } from '@sentry/utils'; +import { convertIntegrationFnToClass } from '../integration'; + +interface LinkedErrorsOptions { + key?: string; + limit?: number; +} const DEFAULT_KEY = 'cause'; const DEFAULT_LIMIT = 5; -/** Adds SDK info to an event. */ -export class LinkedErrors implements Integration { - /** - * @inheritDoc - */ - public static id: string = 'LinkedErrors'; - - /** - * @inheritDoc - */ - public readonly name: string; - - /** - * @inheritDoc - */ - private readonly _key: string; - - /** - * @inheritDoc - */ - private readonly _limit: number; +const INTEGRATION_NAME = 'LinkedErrors'; + +const linkedErrorsIntegration: IntegrationFn = (options: LinkedErrorsOptions = {}) => { + const limit = options.limit || DEFAULT_LIMIT; + const key = options.key || DEFAULT_KEY; + + return { + name: INTEGRATION_NAME, + preprocessEvent(event, hint, client) { + const options = client.getOptions(); + + applyAggregateErrorsToEvent( + exceptionFromError, + options.stackParser, + options.maxValueLength, + key, + limit, + event, + hint, + ); + }, + }; +}; - /** - * @inheritDoc - */ - public constructor(options: { key?: string; limit?: number } = {}) { - this._key = options.key || DEFAULT_KEY; - this._limit = options.limit || DEFAULT_LIMIT; - this.name = LinkedErrors.id; - } - - /** @inheritdoc */ - public setupOnce(): void { - // noop - } - - /** - * @inheritDoc - */ - public preprocessEvent(event: Event, hint: EventHint | undefined, client: Client): void { - const options = client.getOptions(); - - applyAggregateErrorsToEvent( - exceptionFromError, - options.stackParser, - options.maxValueLength, - this._key, - this._limit, - event, - hint, - ); - } -} +/** Adds SDK info to an event. */ +// eslint-disable-next-line deprecation/deprecation +export const LinkedErrors = convertIntegrationFnToClass(INTEGRATION_NAME, linkedErrorsIntegration); diff --git a/packages/core/src/integrations/metadata.ts b/packages/core/src/integrations/metadata.ts index 1803640ea4dd..b94f252b5ce0 100644 --- a/packages/core/src/integrations/metadata.ts +++ b/packages/core/src/integrations/metadata.ts @@ -1,8 +1,42 @@ -import type { Client, Event, EventItem, EventProcessor, Hub, Integration } from '@sentry/types'; +import type { Event, EventItem, IntegrationFn } from '@sentry/types'; import { forEachEnvelopeItem } from '@sentry/utils'; +import { convertIntegrationFnToClass } from '../integration'; import { addMetadataToStackFrames, stripMetadataFromStackFrames } from '../metadata'; +const INTEGRATION_NAME = 'ModuleMetadata'; + +const moduleMetadataIntegration: IntegrationFn = () => { + return { + name: INTEGRATION_NAME, + setup(client) { + if (typeof client.on !== 'function') { + return; + } + + // We need to strip metadata from stack frames before sending them to Sentry since these are client side only. + client.on('beforeEnvelope', envelope => { + forEachEnvelopeItem(envelope, (item, type) => { + if (type === 'event') { + const event = Array.isArray(item) ? (item as EventItem)[1] : undefined; + + if (event) { + stripMetadataFromStackFrames(event); + item[1] = event; + } + } + }); + }); + }, + + processEvent(event, _hint, client) { + const stackParser = client.getOptions().stackParser; + addMetadataToStackFrames(stackParser, event); + return event; + }, + }; +}; + /** * Adds module metadata to stack frames. * @@ -12,53 +46,5 @@ import { addMetadataToStackFrames, stripMetadataFromStackFrames } from '../metad * under the `module_metadata` property. This can be used to help in tagging or routing of events from different teams * our sources */ -export class ModuleMetadata implements Integration { - /* - * @inheritDoc - */ - public static id: string = 'ModuleMetadata'; - - /** - * @inheritDoc - */ - public name: string; - - public constructor() { - this.name = ModuleMetadata.id; - } - - /** - * @inheritDoc - */ - public setupOnce(_addGlobalEventProcessor: (processor: EventProcessor) => void, _getCurrentHub: () => Hub): void { - // noop - } - - /** @inheritDoc */ - public setup(client: Client): void { - if (typeof client.on !== 'function') { - return; - } - - // We need to strip metadata from stack frames before sending them to Sentry since these are client side only. - client.on('beforeEnvelope', envelope => { - forEachEnvelopeItem(envelope, (item, type) => { - if (type === 'event') { - const event = Array.isArray(item) ? (item as EventItem)[1] : undefined; - - if (event) { - stripMetadataFromStackFrames(event); - item[1] = event; - } - } - }); - }); - } - - /** @inheritDoc */ - public processEvent(event: Event, _hint: unknown, client: Client): Event { - const stackParser = client.getOptions().stackParser; - addMetadataToStackFrames(stackParser, event); - return event; - } -} +// eslint-disable-next-line deprecation/deprecation +export const ModuleMetadata = convertIntegrationFnToClass(INTEGRATION_NAME, moduleMetadataIntegration); diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 6aded915d854..fcf70ccced1a 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -1,6 +1,7 @@ -import type { Client, Event, EventProcessor, Hub, Integration, PolymorphicRequest, Transaction } from '@sentry/types'; +import type { Client, IntegrationFn, Transaction } from '@sentry/types'; import type { AddRequestDataToEventOptions, TransactionNamingScheme } from '@sentry/utils'; import { addRequestDataToEvent, extractPathForTransaction } from '@sentry/utils'; +import { convertIntegrationFnToClass } from '../integration'; export type RequestDataIntegrationOptions = { /** @@ -43,120 +44,93 @@ const DEFAULT_OPTIONS = { transactionNamingScheme: 'methodPath', }; -/** Add data about a request to an event. Primarily for use in Node-based SDKs, but included in `@sentry/integrations` - * so it can be used in cross-platform SDKs like `@sentry/nextjs`. */ -export class RequestData implements Integration { - /** - * @inheritDoc - */ - public static id: string = 'RequestData'; +const INTEGRATION_NAME = 'RequestData'; - /** - * @inheritDoc - */ - public name: string; +const requestDataIntegration: IntegrationFn = (options: RequestDataIntegrationOptions = {}) => { + const _addRequestData = addRequestDataToEvent; + const _options: Required = { + ...DEFAULT_OPTIONS, + ...options, + include: { + // @ts-expect-error It's mad because `method` isn't a known `include` key. (It's only here and not set by default in + // `addRequestDataToEvent` for legacy reasons. TODO (v8): Change that.) + method: true, + ...DEFAULT_OPTIONS.include, + ...options.include, + user: + options.include && typeof options.include.user === 'boolean' + ? options.include.user + : { + ...DEFAULT_OPTIONS.include.user, + // Unclear why TS still thinks `options.include.user` could be a boolean at this point + ...((options.include || {}).user as Record), + }, + }, + }; - /** - * Function for adding request data to event. Defaults to `addRequestDataToEvent` from `@sentry/node` for now, but - * left as a property so this integration can be moved to `@sentry/core` as a base class in case we decide to use - * something similar in browser-based SDKs in the future. - */ - protected _addRequestData: (event: Event, req: PolymorphicRequest, options?: { [key: string]: unknown }) => Event; + return { + name: INTEGRATION_NAME, - private _options: Required; + 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; - /** - * @inheritDoc - */ - public constructor(options: RequestDataIntegrationOptions = {}) { - this.name = RequestData.id; - this._addRequestData = addRequestDataToEvent; - this._options = { - ...DEFAULT_OPTIONS, - ...options, - include: { - // @ts-expect-error It's mad because `method` isn't a known `include` key. (It's only here and not set by default in - // `addRequestDataToEvent` for legacy reasons. TODO (v8): Change that.) - method: true, - ...DEFAULT_OPTIONS.include, - ...options.include, - user: - options.include && typeof options.include.user === 'boolean' - ? options.include.user - : { - ...DEFAULT_OPTIONS.include.user, - // Unclear why TS still thinks `options.include.user` could be a boolean at this point - ...((options.include || {}).user as Record), - }, - }, - }; - } + const { sdkProcessingMetadata = {} } = event; + const req = sdkProcessingMetadata.request; - /** - * @inheritDoc - */ - 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; + if (!req) { + return event; + } - const { sdkProcessingMetadata = {} } = event; - const req = sdkProcessingMetadata.request; + // 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(_options); - if (!req) { - return event; - } + const processedEvent = _addRequestData(event, req, addRequestDataOptions); - // 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); + // Transaction events already have the right `transaction` value + if (event.type === 'transaction' || transactionNamingScheme === 'handler') { + return processedEvent; + } - const processedEvent = this._addRequestData(event, req, addRequestDataOptions); + // 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; + } - // 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(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; - } -} +/** Add data about a request to an event. Primarily for use in Node-based SDKs, but included in `@sentry/integrations` + * so it can be used in cross-platform SDKs like `@sentry/nextjs`. */ +// eslint-disable-next-line deprecation/deprecation +export const RequestData = convertIntegrationFnToClass(INTEGRATION_NAME, requestDataIntegration); /** Convert this integration's options to match what `addRequestDataToEvent` expects */ /** TODO: Can possibly be deleted once https://github.com/getsentry/sentry-javascript/issues/5718 is fixed */ diff --git a/packages/serverless/test/__mocks__/@sentry/node.ts b/packages/serverless/test/__mocks__/@sentry/node.ts index b9eba4b132a9..f9322057f1d5 100644 --- a/packages/serverless/test/__mocks__/@sentry/node.ts +++ b/packages/serverless/test/__mocks__/@sentry/node.ts @@ -41,6 +41,7 @@ export const captureException = jest.fn(); export const captureMessage = jest.fn(); export const withScope = jest.fn(cb => cb(fakeScope)); export const flush = jest.fn(() => Promise.resolve()); +export const getClient = jest.fn(() => ({})); export const resetMocks = (): void => { fakeTransaction.setHttpStatus.mockClear(); @@ -68,4 +69,5 @@ export const resetMocks = (): void => { captureMessage.mockClear(); withScope.mockClear(); flush.mockClear(); + getClient.mockClear(); }; diff --git a/packages/serverless/test/gcpfunction.test.ts b/packages/serverless/test/gcpfunction.test.ts index 90ff6e082c29..f60c26c00986 100644 --- a/packages/serverless/test/gcpfunction.test.ts +++ b/packages/serverless/test/gcpfunction.test.ts @@ -1,6 +1,6 @@ import * as domain from 'domain'; import * as SentryNode from '@sentry/node'; -import type { Event } from '@sentry/types'; +import type { Event, Integration } from '@sentry/types'; import * as Sentry from '../src'; import { wrapCloudEventFunction, wrapEventFunction, wrapHttpFunction } from '../src/gcpfunction'; @@ -234,8 +234,6 @@ describe('GCPFunction', () => { // integration is included in the defaults and the necessary data is stored in `sdkProcessingMetadata`. The // integration's tests cover testing that it uses that data correctly. test('wrapHttpFunction request data prereqs', async () => { - expect.assertions(2); - Sentry.GCPFunction.init({}); const handler: HttpFunction = (_req, res) => { @@ -245,11 +243,10 @@ describe('GCPFunction', () => { await handleHttp(wrappedHandler); - expect(SentryNode.init).toHaveBeenCalledWith( - expect.objectContaining({ - defaultIntegrations: expect.arrayContaining([expect.any(SentryNode.Integrations.RequestData)]), - }), - ); + const initOptions = (SentryNode.init as unknown as jest.SpyInstance).mock.calls[0]; + const defaultIntegrations = initOptions[0].defaultIntegrations.map((i: Integration) => i.name); + + expect(defaultIntegrations).toContain('RequestData'); // @ts-expect-error see "Why @ts-expect-error" note expect(SentryNode.fakeScope.setSDKProcessingMetadata).toHaveBeenCalledWith({