Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(serverless): Use RequestData integration in GCP wrapper #5991

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/node/src/integrations/requestdata.ts
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
@@ -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
@@ -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
@@ -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
@@ -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