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): Move tracing options to Http integration #6191

Merged
merged 5 commits into from
Nov 15, 2022
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/nextjs/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ function addServerIntegrations(options: NextjsOptions): void {
if (hasTracingEnabled(options)) {
const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true });
integrations = addOrUpdateIntegration(defaultHttpTracingIntegration, integrations, {
_tracing: true,
_tracing: {},
});
}

Expand Down
10 changes: 5 additions & 5 deletions packages/nextjs/test/index.server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ describe('Server init()', () => {
const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');

expect(httpIntegration).toBeDefined();
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: true }));
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: {} }));
});

it('adds `Http` integration with tracing enabled if `tracesSampler` is set', () => {
Expand All @@ -179,7 +179,7 @@ describe('Server init()', () => {
const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');

expect(httpIntegration).toBeDefined();
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: true }));
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: {} }));
});

it('does not add `Http` integration if tracing not enabled in SDK', () => {
Expand All @@ -201,7 +201,7 @@ describe('Server init()', () => {
const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');

expect(httpIntegration).toBeDefined();
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: true }));
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: {} }));
});

it('forces `_tracing = true` if `tracesSampler` is set', () => {
Expand All @@ -214,7 +214,7 @@ describe('Server init()', () => {
const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');

expect(httpIntegration).toBeDefined();
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: true }));
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: {} }));
});

it('does not force `_tracing = true` if tracing not enabled in SDK', () => {
Expand All @@ -226,7 +226,7 @@ describe('Server init()', () => {
const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');

expect(httpIntegration).toBeDefined();
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: false }));
expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: undefined }));
});
});
});
Expand Down
72 changes: 50 additions & 22 deletions packages/node/src/integrations/http.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getCurrentHub, Hub } from '@sentry/core';
import { EventProcessor, Integration, Span } from '@sentry/types';
import { EventProcessor, Integration, Span, TracePropagationTargets } from '@sentry/types';
import {
dynamicSamplingContextToSentryBaggageHeader,
fill,
Expand All @@ -11,7 +11,6 @@ import * as http from 'http';
import * as https from 'https';

import { NodeClient } from '../client';
import { NodeClientOptions } from '../types';
import {
cleanSpanDescription,
extractUrl,
Expand All @@ -23,6 +22,39 @@ import {

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

interface TracingOptions {
/**
* 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?: TracePropagationTargets;

/**
* Function determining whether or not to create spans to track outgoing requests to the given URL.
* By default, spans will be created for all outgoing requests.
*/
shouldCreateSpanForRequest?: (url: string) => boolean;
}

interface HttpOptions {
/**
* Whether breadcrumbs should be recorded for requests
* Defaults to true
*/
breadcrumbs?: boolean;

/**
* Whether tracing spans should be created for requests
* Defaults to false
*/
tracing?: TracingOptions | boolean;
}

/**
* The http module integration instruments Node's internal http module. It creates breadcrumbs, transactions for outgoing
* http requests and attaches trace data when tracing is enabled via its `tracing` option.
Expand All @@ -38,22 +70,15 @@ export class Http implements Integration {
*/
public name: string = Http.id;

/**
* @inheritDoc
*/
private readonly _breadcrumbs: boolean;
private readonly _tracing: TracingOptions | undefined;

/**
* @inheritDoc
*/
private readonly _tracing: boolean;

/**
* @inheritDoc
*/
public constructor(options: { breadcrumbs?: boolean; tracing?: boolean } = {}) {
public constructor(options: HttpOptions = {}) {
this._breadcrumbs = typeof options.breadcrumbs === 'undefined' ? true : options.breadcrumbs;
this._tracing = typeof options.tracing === 'undefined' ? false : options.tracing;
this._tracing = !options.tracing ? undefined : options.tracing === true ? {} : options.tracing;
}

/**
Expand All @@ -76,7 +101,11 @@ export class Http implements Integration {
return;
}

const wrappedHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, this._tracing, clientOptions);
// TODO (v8): `tracePropagationTargets` and `shouldCreateSpanForRequest` will be removed from clientOptions
// and we will no longer have to do this optional merge, we can just pass `this._tracing` directly.
const tracingOptions = this._tracing ? { ...clientOptions, ...this._tracing } : undefined;

const wrappedHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions);

// eslint-disable-next-line @typescript-eslint/no-var-requires
const httpModule = require('http');
Expand Down Expand Up @@ -111,37 +140,36 @@ type WrappedRequestMethodFactory = (original: OriginalRequestMethod) => WrappedR
*/
function _createWrappedRequestMethodFactory(
breadcrumbsEnabled: boolean,
tracingEnabled: boolean,
options: NodeClientOptions | undefined,
tracingOptions: TracingOptions | undefined,
): WrappedRequestMethodFactory {
// We're caching results so we don't have to recompute regexp every time we create a request.
const createSpanUrlMap: Record<string, boolean> = {};
const headersUrlMap: Record<string, boolean> = {};

const shouldCreateSpan = (url: string): boolean => {
if (options?.shouldCreateSpanForRequest === undefined) {
if (tracingOptions?.shouldCreateSpanForRequest === undefined) {
return true;
}

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

createSpanUrlMap[url] = options.shouldCreateSpanForRequest(url);
createSpanUrlMap[url] = tracingOptions.shouldCreateSpanForRequest(url);

return createSpanUrlMap[url];
};

const shouldAttachTraceData = (url: string): boolean => {
if (options?.tracePropagationTargets === undefined) {
if (tracingOptions?.tracePropagationTargets === undefined) {
return true;
}

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

headersUrlMap[url] = options.tracePropagationTargets.some(tracePropagationTarget =>
headersUrlMap[url] = tracingOptions.tracePropagationTargets.some(tracePropagationTarget =>
isMatchingPattern(url, tracePropagationTarget),
);

Expand All @@ -167,7 +195,7 @@ function _createWrappedRequestMethodFactory(

const scope = getCurrentHub().getScope();

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

if (parentSpan) {
Expand Down Expand Up @@ -235,7 +263,7 @@ function _createWrappedRequestMethodFactory(
if (breadcrumbsEnabled) {
addRequestBreadcrumb('response', requestUrl, req, res);
}
if (tracingEnabled && requestSpan) {
if (requestSpan) {
if (res.statusCode) {
requestSpan.setHttpStatus(res.statusCode);
}
Expand All @@ -250,7 +278,7 @@ function _createWrappedRequestMethodFactory(
if (breadcrumbsEnabled) {
addRequestBreadcrumb('error', requestUrl, req);
}
if (tracingEnabled && requestSpan) {
if (requestSpan) {
requestSpan.setHttpStatus(500);
requestSpan.description = cleanSpanDescription(requestSpan.description, requestOptions, req);
requestSpan.finish();
Expand Down
39 changes: 28 additions & 11 deletions packages/node/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,39 @@ export interface BaseNodeOptions {
/** Sets an optional server name (device name) */
serverName?: string;

// We have this option separately in both, the node options and the browser options, so that we can have different JSDoc
// comments, since this has different behaviour in the Browser and Node SDKs.
// TODO (v8): Remove this in v8
/**
* 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.
* @deprecated Moved to constructor options of the `Http` integration.
* @example
* ```js
* Sentry.init({
* integrations: [
* new Sentry.Integrations.Http({
* tracing: {
* tracePropagationTargets: ['api.site.com'],
* }
* });
* ],
* });
* ```
*/
tracePropagationTargets?: TracePropagationTargets;

// TODO (v8): Remove this in v8
/**
* Function determining whether or not to create spans to track outgoing requests to the given URL.
* By default, spans will be created for all outgoing requests.
* @deprecated Moved to constructor options of the `Http` integration.
* @example
* ```js
* Sentry.init({
* integrations: [
* new Sentry.Integrations.Http({
* tracing: {
* shouldCreateSpanForRequest: (url: string) => false,
* }
* });
* ],
* });
* ```
*/
shouldCreateSpanForRequest?(url: string): boolean;

Expand Down
Loading