Skip to content

Commit

Permalink
ref(serverless): Use RequestData integration in GCP wrapper (#5991)
Browse files Browse the repository at this point in the history
This switches the GCP wrapper in the serverless SDK to use the new `RequestData` integration for adding request data to events rather than the current event processor. 

Ref: #5756
  • Loading branch information
lobsterkatie authored Oct 20, 2022
1 parent 284184e commit 0576852
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 25 deletions.
3 changes: 2 additions & 1 deletion packages/node/src/integrations/requestdata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,11 @@ export class RequestData implements Integration {
}

// 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 people to use this
// 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);
Expand Down
35 changes: 32 additions & 3 deletions packages/node/test/integrations/requestdata.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getCurrentHub, Hub, makeMain } from '@sentry/core';
import { Event, EventProcessor } from '@sentry/types';
import { Event, EventProcessor, PolymorphicRequest } from '@sentry/types';
import * as http from 'http';

import { NodeClient } from '../../src/client';
Expand Down Expand Up @@ -102,8 +102,8 @@ describe('`RequestData` integration', () => {
});
});

describe('usage with express request handler', () => {
it('uses options from request handler', async () => {
describe('usage with express request handler and GCP wrapper', () => {
it('uses options from Express request handler', async () => {
const sentryRequestMiddleware = requestHandler({ include: { transaction: 'methodPath' } });
const res = new http.ServerResponse(req);
const next = jest.fn();
Expand All @@ -120,5 +120,34 @@ describe('`RequestData` integration', () => {
// `transaction` matches the request middleware's option, not the integration's option
expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'methodPath' }));
});

it('uses options from GCP wrapper', async () => {
type GCPHandler = (req: PolymorphicRequest, res: http.ServerResponse) => void;
const mockGCPWrapper = (origHandler: GCPHandler, options: Record<string, unknown>): GCPHandler => {
const wrappedHandler: GCPHandler = (req, res) => {
getCurrentHub().getScope()?.setSDKProcessingMetadata({
request: req,
requestDataOptionsFromGCPWrapper: options,
});
origHandler(req, res);
};
return wrappedHandler;
};

const wrappedGCPFunction = mockGCPWrapper(jest.fn(), { include: { transaction: 'methodPath' } });
const res = new http.ServerResponse(req);

initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' });

wrappedGCPFunction(req, res);

await getCurrentHub().getScope()!.applyToEvent(event, {});
requestDataEventProcessor(event);

const passedOptions = addRequestDataToEventSpy.mock.calls[0][2];

// `transaction` matches the GCP wrapper's option, not the integration's option
expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'methodPath' }));
});
});
});
13 changes: 5 additions & 8 deletions packages/serverless/src/gcpfunction/http.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,4 @@
import {
addRequestDataToEvent,
AddRequestDataToEventOptions,
captureException,
flush,
getCurrentHub,
} from '@sentry/node';
import { AddRequestDataToEventOptions, captureException, flush, getCurrentHub } from '@sentry/node';
import { extractTraceparentData } from '@sentry/tracing';
import { baggageHeaderToDynamicSamplingContext, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils';

Expand Down Expand Up @@ -97,7 +91,10 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial<HttpFunctionWr
// since functions-framework creates a domain for each incoming request.
// So adding of event processors every time should not lead to memory bloat.
hub.configureScope(scope => {
scope.addEventProcessor(event => addRequestDataToEvent(event, req, options.addRequestDataToEventOptions));
scope.setSDKProcessingMetadata({
request: req,
requestDataOptionsFromGCPWrapper: options.addRequestDataToEventOptions,
});
// We put the transaction on the scope so users can attach children to it
scope.setSpan(transaction);
});
Expand Down
2 changes: 2 additions & 0 deletions packages/serverless/test/__mocks__/@sentry/node.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const origSentry = jest.requireActual('@sentry/node');
export const defaultIntegrations = origSentry.defaultIntegrations; // eslint-disable-line @typescript-eslint/no-unsafe-member-access
export const Handlers = origSentry.Handlers; // eslint-disable-line @typescript-eslint/no-unsafe-member-access
export const Integrations = origSentry.Integrations;
export const addRequestDataToEvent = origSentry.addRequestDataToEvent;
export const SDK_VERSION = '6.6.6';
export const Severity = {
Expand All @@ -20,6 +21,7 @@ export const fakeScope = {
setContext: jest.fn(),
setSpan: jest.fn(),
getTransaction: jest.fn(() => fakeTransaction),
setSDKProcessingMetadata: jest.fn(),
};
export const fakeSpan = {
finish: jest.fn(),
Expand Down
40 changes: 27 additions & 13 deletions packages/serverless/test/gcpfunction.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Event } from '@sentry/types';
import * as SentryNode from '@sentry/node';
import * as domain from 'domain';

import * as Sentry from '../src';
Expand Down Expand Up @@ -230,23 +230,37 @@ describe('GCPFunction', () => {
});
});

test('wrapHttpFunction request data', async () => {
expect.assertions(6);
// This tests that the necessary pieces are in place for request data to get added to event - the `RequestData`
// 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) => {
res.end();
};
const wrappedHandler = wrapHttpFunction(handler);
const event: Event = {};
// @ts-ignore see "Why @ts-ignore" note
Sentry.fakeScope.addEventProcessor.mockImplementation(cb => cb(event));
const wrappedHandler = wrapHttpFunction(handler, { addRequestDataToEventOptions: { include: { ip: true } } });

await handleHttp(wrappedHandler);
expect(event.transaction).toEqual('POST /path');
expect(event.request?.method).toEqual('POST');
expect(event.request?.url).toEqual('http://hostname/path?q=query');
expect(event.request?.query_string).toEqual('q=query');
expect(event.request?.headers).toEqual({ host: 'hostname', 'content-type': 'application/json' });
expect(event.request?.data).toEqual('{"foo":"bar"}');

expect(SentryNode.init).toHaveBeenCalledWith(
expect.objectContaining({
defaultIntegrations: expect.arrayContaining([expect.any(SentryNode.Integrations.RequestData)]),
}),
);

// @ts-ignore see "Why @ts-ignore" note
expect(Sentry.fakeScope.setSDKProcessingMetadata).toHaveBeenCalledWith({
request: {
method: 'POST',
url: '/path?q=query',
headers: { host: 'hostname', 'content-type': 'application/json' },
body: { foo: 'bar' },
},
requestDataOptionsFromGCPWrapper: { include: { ip: true } },
});
});

describe('wrapEventFunction() without callback', () => {
Expand Down

0 comments on commit 0576852

Please sign in to comment.