Skip to content

Commit

Permalink
ref(core): Refactor core integrations to avoid setupOnce (#9813)
Browse files Browse the repository at this point in the history 
Where possible, we should use the new `processEvent` and/or `setup`
hooks of the integrations, instead of `setupOnce`. This updates this for
the core integrations.
  • Loading branch information
@mydea
mydea committed Dec 14, 2023
1 parent 41c7782 commit f3d54de
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 91 deletions.
21 changes: 12 additions & 9 deletions packages/core/src/integrations/metadata.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}
}
101 changes: 51 additions & 50 deletions packages/core/src/integrations/requestdata.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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;
Expand Down
39 changes: 21 additions & 18 deletions packages/core/test/lib/integrations/requestdata.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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,
});
Expand All @@ -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', () => {
Expand All @@ -58,29 +57,31 @@ 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];

expect(passedOptions?.include).toEqual(expect.objectContaining({ ip: false, user: true }));
});

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];

expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'path' }));
});

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];

Expand All @@ -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];

Expand Down
27 changes: 13 additions & 14 deletions packages/node/test/integrations/requestdata.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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';
Expand All @@ -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,
});
Expand All @@ -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', () => {
Expand All @@ -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];

Expand All @@ -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];

Expand Down

0 comments on commit f3d54de

Please sign in to comment.