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

feat(node): Add tracePropagationTargets option #5521

Merged
merged 6 commits into from Aug 4, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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
18 changes: 13 additions & 5 deletions packages/nextjs/test/integration/test/utils/server.js
@@ -1,3 +1,4 @@
// @ts-check
const { get } = require('http');
const nock = require('nock');
const nodeSDK = require('@sentry/node');
Expand Down Expand Up @@ -132,14 +133,16 @@ function ensureWrappedGet(importedGet, url) {
// As of Next 12.1, creating a `NextServer` instance (which we do immediately upon starting this test runner) loads
// `_app`, which has the effect of initializing the SDK. So, unless something's gone wrong, we should always be able
// to find the integration
let httpIntegration;
try {
httpIntegration = nodeSDK.getCurrentHub().getClient().getIntegration(nodeSDK.Integrations.Http);
} catch (err) {
const hub = nodeSDK.getCurrentHub();
const client = hub.getClient();

if (!client) {
console.warn(`Warning: Sentry SDK not set up at \`NextServer\` initialization. Request URL: ${url}`);
return importedGet;
}

const httpIntegration = client.getIntegration(nodeSDK.Integrations.Http);

// This rewraps `http.get` and `http.request`, which, at this point, look like `nockWrapper(sentryWrapper(get))` and
// `nockWrapper(sentryWrapper(request))`. By the time we're done with this function, they'll look like
// `sentryWrapper(nockWrapper(sentryWrapper(get)))` and `sentryWrapper(nockWrapper(sentryWrapper(request)))`,
Expand All @@ -160,7 +163,12 @@ function ensureWrappedGet(importedGet, url) {
//
// TODO: add in a "don't do this twice" check (in `fill`, maybe moved from `wrap`), so that we don't wrap the outer
// wrapper with a third wrapper
httpIntegration.setupOnce();
if (httpIntegration) {
httpIntegration.setupOnce(
() => undefined,
() => hub,
);
}

// now that we've rewrapped it, grab the correct version of `get` for use in our tests
const httpModule = require('http');
Expand Down
@@ -0,0 +1,26 @@
// eslint-disable-next-line @typescript-eslint/no-unused-vars
import '@sentry/tracing';

import * as Sentry from '@sentry/node';
import * as http from 'http';

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
release: '1.0',
tracesSampleRate: 1.0,
tracePropagationTargets: [/\/v0/, 'v1'],
integrations: [new Sentry.Integrations.Http({ tracing: true })],
});

const transaction = Sentry.startTransaction({ name: 'test_transaction' });

Sentry.configureScope(scope => {
scope.setSpan(transaction);
});

http.get('http://match-this-url.com/api/v0');
http.get('http://match-this-url.com/api/v1');
http.get('http://dont-match-this-url.com/api/v2');
http.get('http://dont-match-this-url.com/api/v3');

transaction.finish();
@@ -0,0 +1,37 @@
import nock from 'nock';

import { runScenario, runServer } from '../../../utils';

test('HttpIntegration should instrument correct requests when tracePropagationTargets option is provided', async () => {
const match1 = nock('http://match-this-url.com')
.get('/api/v0')
.matchHeader('baggage', val => typeof val === 'string')
.matchHeader('sentry-trace', val => typeof val === 'string')
.reply(200);

const match2 = nock('http://match-this-url.com')
.get('/api/v1')
.matchHeader('baggage', val => typeof val === 'string')
.matchHeader('sentry-trace', val => typeof val === 'string')
.reply(200);

const match3 = nock('http://dont-match-this-url.com')
.get('/api/v2')
.matchHeader('baggage', val => val === undefined)
.matchHeader('sentry-trace', val => val === undefined)
.reply(200);

const match4 = nock('http://dont-match-this-url.com')
.get('/api/v3')
.matchHeader('baggage', val => val === undefined)
.matchHeader('sentry-trace', val => val === undefined)
.reply(200);

const serverUrl = await runServer(__dirname);
await runScenario(serverUrl);

expect(match1.isDone()).toBe(true);
Lms24 marked this conversation as resolved.
Show resolved Hide resolved
expect(match2.isDone()).toBe(true);
expect(match3.isDone()).toBe(true);
expect(match4.isDone()).toBe(true);
});
21 changes: 18 additions & 3 deletions packages/node-integration-tests/utils/index.ts
Expand Up @@ -152,12 +152,16 @@ export async function runServer(testDir: string, serverPath?: string, scenarioPa
const url = `http://localhost:${port}/test`;
const defaultServerPath = path.resolve(process.cwd(), 'utils', 'defaults', 'server');

await new Promise(resolve => {
await new Promise<void>(resolve => {
// eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-unsafe-member-access
const app = require(serverPath || defaultServerPath).default as Express;

app.get('/test', async () => {
require(scenarioPath || `${testDir}/scenario`);
app.get('/test', (_req, res) => {
try {
require(scenarioPath || `${testDir}/scenario`);
} finally {
res.status(200).end();
}
});

const server = app.listen(port, () => {
Expand All @@ -170,3 +174,14 @@ export async function runServer(testDir: string, serverPath?: string, scenarioPa

return url;
}

export async function runScenario(serverUrl: string): Promise<void> {
return new Promise<void>(resolve => {
http
.get(serverUrl, res => {
res.on('data', () => undefined);
res.on('end', resolve);
})
.end();
});
}
81 changes: 61 additions & 20 deletions packages/node/src/integrations/http.ts
@@ -1,9 +1,10 @@
import { getCurrentHub } from '@sentry/core';
import { Integration, Span } from '@sentry/types';
import { fill, logger, mergeAndSerializeBaggage, parseSemver } from '@sentry/utils';
import { getCurrentHub, Hub } from '@sentry/core';
import { EventProcessor, Integration, Span } from '@sentry/types';
import { fill, isMatchingPattern, logger, mergeAndSerializeBaggage, parseSemver } from '@sentry/utils';
import * as http from 'http';
import * as https from 'https';

import { NodeClientOptions } from '../types';
import {
cleanSpanDescription,
extractUrl,
Expand All @@ -15,7 +16,10 @@ import {

const NODE_VERSION = parseSemver(process.versions.node);

/** http module integration */
/**
* The http module integration instruments nodes internal http module. It creates breadcrumbs, transactions for outgoing
lforst marked this conversation as resolved.
Show resolved Hide resolved
* http requests and attaches trace data when tracing is enabled via its `tracing` option.
*/
export class Http implements Integration {
/**
* @inheritDoc
Expand Down Expand Up @@ -48,13 +52,22 @@ export class Http implements Integration {
/**
* @inheritDoc
*/
public setupOnce(): void {
public setupOnce(
_addGlobalEventProcessor: (callback: EventProcessor) => void,
setupOnceGetCurrentHub: () => Hub,
): void {
// No need to instrument if we don't want to track anything
if (!this._breadcrumbs && !this._tracing) {
return;
}

const wrappedHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, this._tracing);
const clientOptions = setupOnceGetCurrentHub().getClient()?.getOptions() as NodeClientOptions | undefined;

const wrappedHandlerMaker = _createWrappedRequestMethodFactory(
this._breadcrumbs,
this._tracing,
clientOptions?.tracePropagationTargets,
);

// eslint-disable-next-line @typescript-eslint/no-var-requires
const httpModule = require('http');
Expand Down Expand Up @@ -90,7 +103,26 @@ type WrappedRequestMethodFactory = (original: OriginalRequestMethod) => WrappedR
function _createWrappedRequestMethodFactory(
breadcrumbsEnabled: boolean,
tracingEnabled: boolean,
tracePropagationTargets: (string | RegExp)[] | undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional/question: Does it make sense to extract (string | RegExp)[] | undefined to a type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes a lot of sense! Can you check whether the location where I extracted it makes sense?: 0aa153f

Also I extracted it without | undefined because that works better with the optional operator in the init options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it sort of makes sense but I can see why this might look a little off. Seems like we don't really have a tracing-related file in the types package. Adding it to the transaction, span or baggage files also doesn't make more sense...

So wdyt about adding a new tracing.ts file? Or we add it to the misc.ts file ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a tracing.ts file. Thanks for the input!

): WrappedRequestMethodFactory {
// We're caching results so we dont have to recompute regexp everytime we create a request.
const urlMap: Record<string, boolean> = {};
const shouldAttachTraceData = (url: string): boolean => {
if (tracePropagationTargets === undefined) {
return true;
}
Lms24 marked this conversation as resolved.
Show resolved Hide resolved

if (urlMap[url]) {
return urlMap[url];
}

urlMap[url] = tracePropagationTargets.some(tracePropagationTarget =>
isMatchingPattern(url, tracePropagationTarget),
);

return urlMap[url];
};

return function wrappedRequestMethodFactory(originalRequestMethod: OriginalRequestMethod): WrappedRequestMethod {
return function wrappedMethod(this: typeof http | typeof https, ...args: RequestMethodArgs): http.ClientRequest {
// eslint-disable-next-line @typescript-eslint/no-this-alias
Expand All @@ -109,28 +141,37 @@ function _createWrappedRequestMethodFactory(
let parentSpan: Span | undefined;

const scope = getCurrentHub().getScope();

if (scope && tracingEnabled) {
parentSpan = scope.getSpan();

if (parentSpan) {
span = parentSpan.startChild({
description: `${requestOptions.method || 'GET'} ${requestUrl}`,
op: 'http.client',
});

const sentryTraceHeader = span.toTraceparent();
__DEBUG_BUILD__ &&
logger.log(
`[Tracing] Adding sentry-trace header ${sentryTraceHeader} to outgoing request to ${requestUrl}: `,
);

const baggage = parentSpan.transaction && parentSpan.transaction.getBaggage();
const headerBaggageString = requestOptions.headers && requestOptions.headers.baggage;

requestOptions.headers = {
...requestOptions.headers,
'sentry-trace': sentryTraceHeader,
baggage: mergeAndSerializeBaggage(baggage, headerBaggageString),
};
if (shouldAttachTraceData(requestUrl)) {
const sentryTraceHeader = span.toTraceparent();
__DEBUG_BUILD__ &&
logger.log(
`[Tracing] Adding sentry-trace header ${sentryTraceHeader} to outgoing request to "${requestUrl}": `,
);

const baggage = parentSpan.transaction && parentSpan.transaction.getBaggage();
const headerBaggageString = requestOptions.headers && requestOptions.headers.baggage;

requestOptions.headers = {
...requestOptions.headers,
'sentry-trace': sentryTraceHeader,
baggage: mergeAndSerializeBaggage(baggage, headerBaggageString),
};
} else {
__DEBUG_BUILD__ &&
logger.log(
`[Tracing] Not adding sentry-trace header to outgoing request (${requestUrl}) due to mismatching tracePropagationTargets option.`,
);
}
}
}

Expand Down
13 changes: 13 additions & 0 deletions packages/node/src/types.ts
Expand Up @@ -6,6 +6,19 @@ export interface BaseNodeOptions {
/** Sets an optional server name (device name) */
serverName?: string;

// We have this option seperately in both the node options and the browser options, so that we can have different JSDoc
lforst marked this conversation as resolved.
Show resolved Hide resolved
// comments, since this has different behaviour in the Browser and Node SDKs.
/**
* List of strings/regex controlling to which outgoing requests
* the SDK will attach tracing headers.
*
* By default the SDK will attach those headers to all outgoing
* requests. If this option is provided, the SDK will match the
* request URL of outgoing requests against the items in this
* array, and only attach tracing headers if a match was found.
*/
tracePropagationTargets?: (string | RegExp)[];

/** Callback that is executed when a fatal global error occurs. */
onFatalError?(error: Error): void;
}
Expand Down