Skip to content

Commit

Permalink
feat(node): Move tracing options to Http integration (#6191)
Browse files Browse the repository at this point in the history 
Since both `tracing` and `shouldCreateSpanForRequest` effectively control whether tracing is enabled and have opposing defaults (`tracing: false` and `shouldCreateSpanForRequest: _ => true`), this patch makes `tracing: TracingOptions | boolean` so its not a breaking change and the options don't end up conflicting. 

```ts
interface TracingOptions {
  tracePropagationTargets?: TracePropagationTargets;
  shouldCreateSpanForRequest?: (url: string) => boolean;
}

interface HttpOptions {
  breadcrumbs?: boolean;
  tracing?: TracingOptions | boolean;
}
```
  • Loading branch information
@timfish
timfish committed Nov 15, 2022
1 parent b4e89de commit 23c19bf
Show file tree
Hide file tree
Showing 5 changed files with 248 additions and 102 deletions.
2 changes: 1 addition & 1 deletion packages/nextjs/src/index.server.ts
View file
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
View file
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
View file
@@ -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
View file
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

0 comments on commit 23c19bf

Please sign in to comment.