From df7181af7516b641d572b5c5abb20820e160f1f6 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 13 Aug 2020 17:28:19 -0700 Subject: [PATCH 01/40] add tracessampler option; not yet tested --- packages/gatsby/README.md | 7 ++- packages/gatsby/gatsby-browser.js | 5 +- packages/tracing/src/hubextensions.ts | 68 ++++++++++++++++++++++----- packages/types/src/index.ts | 2 +- packages/types/src/options.ts | 4 ++ packages/types/src/transaction.ts | 8 ++++ packages/utils/src/misc.ts | 13 ++++- 7 files changed, 88 insertions(+), 19 deletions(-) diff --git a/packages/gatsby/README.md b/packages/gatsby/README.md index b08c9852ea86..4743e583f964 100644 --- a/packages/gatsby/README.md +++ b/packages/gatsby/README.md @@ -40,7 +40,7 @@ To automatically capture the `release` value on Vercel you will need to register ## Sentry Performance -To enable Tracing support, supply the `tracesSampleRate` to the options and make sure you have installed the `@sentry/tracing` package. This will also turn on the `BrowserTracing` integration for automatic instrumentation of the browser. +To enable Tracing support, supply either `tracesSampleRate` or `tracesSampler` to the options and make sure you have installed the `@sentry/tracing` package. This will also turn on the `BrowserTracing` integration for automatic instrumentation of the browser. ```javascript { @@ -50,7 +50,10 @@ To enable Tracing support, supply the `tracesSampleRate` to the options and make resolve: "@sentry/gatsby", options: { dsn: process.env.SENTRY_DSN, // this is the default - tracesSampleRate: 1, // this is just to test, you should lower this in production + + // A rate of 1 means all traces will be sent, so it's good for testing. + // In production, you'll likely want to either choose a lower rate or use `tracesSampler` instead. + tracesSampleRate: 1, } }, // ... diff --git a/packages/gatsby/gatsby-browser.js b/packages/gatsby/gatsby-browser.js index 573389128096..bb667df41951 100644 --- a/packages/gatsby/gatsby-browser.js +++ b/packages/gatsby/gatsby-browser.js @@ -1,15 +1,15 @@ const Sentry = require('@sentry/react'); const Tracing = require('@sentry/tracing'); +const Utils = require('@sentry/utils'); exports.onClientEntry = function(_, pluginParams) { if (pluginParams === undefined) { return; } - const tracesSampleRate = pluginParams.tracesSampleRate !== undefined ? pluginParams.tracesSampleRate : 0; const integrations = [...(pluginParams.integrations || [])]; - if (tracesSampleRate && !integrations.some(ele => ele.name === 'BrowserTracing')) { + if (Utils.hasTracingEnabled(pluginParams) && !integrations.some(ele => ele.name === 'BrowserTracing')) { integrations.push(new Tracing.Integrations.BrowserTracing(pluginParams.browserTracingOptions)); } @@ -22,7 +22,6 @@ exports.onClientEntry = function(_, pluginParams) { // eslint-disable-next-line no-undef dsn: __SENTRY_DSN__, ...pluginParams, - tracesSampleRate, integrations, }); diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 872941f7b8bc..a250b68349bb 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -1,9 +1,10 @@ import { getMainCarrier, Hub } from '@sentry/hub'; -import { TransactionContext } from '@sentry/types'; +import { SampleContext, TransactionContext } from '@sentry/types'; import { registerErrorInstrumentation } from './errors'; import { IdleTransaction } from './idletransaction'; import { Transaction } from './transaction'; +import { hasTracingEnabled, logger } from '@sentry/utils'; /** Returns all trace headers that are currently on the top scope. */ function traceHeaders(this: Hub): { [key: string]: string } { @@ -20,25 +21,68 @@ function traceHeaders(this: Hub): { [key: string]: string } { } /** - * Use RNG to generate sampling decision, which all child spans inherit. + * Use sample rate (given in options as either a constant or ) along with a random number generator to make a sampling + * decision, which all child spans and child transactions inherit. + * + * Sample rate is set in SDK config, either as a constant (`tracesSampleRate`) or a function to compute the rate + * (`tracesSampler`). + * + * Called every time a transaction is created. Only transactions which emerge with sampled === true will be sent to + * Sentry. + * + * Mutates the given Transaction object and then returns the mutated object. */ function sample(hub: Hub, transaction: T): T { + // nothing to do if tracing is disabled + if (!hasTracingEnabled(hub)) { + transaction.sampled = false; + return transaction; + } + + logger.log('Tracing enabled'); + const client = hub.getClient(); + const options = (client && client.getOptions()) || {}; + + // we have to test for a pre-existsing sampling decision, in case this transaction is a child transaction and has + // inherited its parent's decision if (transaction.sampled === undefined) { - const sampleRate = (client && client.getOptions().tracesSampleRate) || 0; - // if true = we want to have the transaction - // if false = we don't want to have it - // Math.random (inclusive of 0, but not 1) + let sampleRate; + + // prefer the hook + if (options.tracesSampler) { + // TODO (kmclb) build context object + const sampleContext: SampleContext = {}; + sampleRate = options.tracesSampler(sampleContext); + } + // we would have bailed at the beginning if neither `tracesSampler` nor `tracesSampleRate` were defined, so if the + // former isn't, the latter must be + else { + sampleRate = options.tracesSampleRate; + } + + // if the function returned either 0 or null, it's a sign the transaction should be dropped + if (!sampleRate) { + logger.log('Discarding trace because tracesSampler returned 0 or null'); + transaction.sampled = false; + return transaction; + } + + // now we roll the dice (Math.random is inclusive of 0, but not of 1) transaction.sampled = Math.random() < sampleRate; - } - // We only want to create a span list if we sampled the transaction - // If sampled == false, we will discard the span anyway, so we can save memory by not storing child spans - if (transaction.sampled) { - const experimentsOptions = (client && client.getOptions()._experiments) || {}; - transaction.initSpanRecorder(experimentsOptions.maxSpans as number); + // if we're not going to keep it, we're done + if (!transaction.sampled) { + logger.log(`Discarding trace because it's not included in the random sample (sampling rate = ${sampleRate})`); + return transaction; + } } + // at this point we know we're keeping the transaction, whether because of an inherited decision or because it got + // lucky with the dice roll + const experimentsOptions = (client && client.getOptions()._experiments) || {}; + transaction.initSpanRecorder(experimentsOptions.maxSpans as number); + return transaction; } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 1c6939f60969..9f284415a1d9 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -22,7 +22,7 @@ export { Span, SpanContext } from './span'; export { StackFrame } from './stackframe'; export { Stacktrace } from './stacktrace'; export { Status } from './status'; -export { Transaction, TransactionContext } from './transaction'; +export { SampleContext, Transaction, TransactionContext } from './transaction'; export { Thread } from './thread'; export { Transport, TransportOptions, TransportClass } from './transport'; export { User } from './user'; diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 4cc187b83d73..8d23262da796 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -3,6 +3,7 @@ import { Event, EventHint } from './event'; import { Integration } from './integration'; import { LogLevel } from './loglevel'; import { Transport, TransportClass, TransportOptions } from './transport'; +import { SampleContext } from './transaction'; /** Base configuration options for every SDK. */ export interface Options { @@ -88,6 +89,9 @@ export interface Options { */ tracesSampleRate?: number; + /** Function to compute tracing sample rate dynamically and filter unwanted traces */ + tracesSampler?(traceContext: SampleContext): number | null; + /** Attaches stacktraces to pure capture message / log integrations */ attachStacktrace?: boolean; diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 4606763a7e22..d5d830eebcff 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -48,3 +48,11 @@ export interface Transaction extends TransactionContext, Span { */ setName(name: string): void; } + +/** + * The data passed to the `tracesSampler` function, which forms the basis for whatever decisions it might make. + */ +export interface SampleContext { + [key: string]: string; + // TODO (kmclb) fill this out +} diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 083b69c611e9..605c7f748cb5 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -1,5 +1,5 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { Event, Integration, StackFrame, WrappedFunction } from '@sentry/types'; +import { Event, Hub, Integration, StackFrame, WrappedFunction } from '@sentry/types'; import { isString } from './is'; import { snipLine } from './string'; @@ -520,4 +520,15 @@ export function addContextToFrame(lines: string[], frame: StackFrame, linesOfCon export function stripUrlQueryAndFragment(urlPath: string): string { // eslint-disable-next-line no-useless-escape return urlPath.split(/[\?#]/, 1)[0]; + +/** +* Determines if tracing is currently enabled. + * + * Tracing is enabled when at least one of `tracesSampleRate` and `tracesSampler` is defined in the SDK config. + */ +export function hasTracingEnabled(hub: Hub): boolean { + const client = hub.getClient(); + const options = (client && client.getOptions()) || {}; + + return !!options.tracesSampleRate || !!options.tracesSampler; } From fa72ceb147670e0be5344d052957b4d075ff88d1 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 13 Aug 2020 19:08:02 -0700 Subject: [PATCH 02/40] fix type error and some linting things --- packages/tracing/src/hubextensions.ts | 10 +++++----- packages/types/src/options.ts | 16 +++++++++++----- packages/utils/src/misc.ts | 7 ++----- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index a250b68349bb..7e193558bcc7 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -1,10 +1,10 @@ import { getMainCarrier, Hub } from '@sentry/hub'; import { SampleContext, TransactionContext } from '@sentry/types'; +import { hasTracingEnabled, logger } from '@sentry/utils'; import { registerErrorInstrumentation } from './errors'; import { IdleTransaction } from './idletransaction'; import { Transaction } from './transaction'; -import { hasTracingEnabled, logger } from '@sentry/utils'; /** Returns all trace headers that are currently on the top scope. */ function traceHeaders(this: Hub): { [key: string]: string } { @@ -33,17 +33,17 @@ function traceHeaders(this: Hub): { [key: string]: string } { * Mutates the given Transaction object and then returns the mutated object. */ function sample(hub: Hub, transaction: T): T { + const client = hub.getClient(); + const options = (client && client.getOptions()) || {}; + // nothing to do if tracing is disabled - if (!hasTracingEnabled(hub)) { + if (!hasTracingEnabled(options)) { transaction.sampled = false; return transaction; } logger.log('Tracing enabled'); - const client = hub.getClient(); - const options = (client && client.getOptions()) || {}; - // we have to test for a pre-existsing sampling decision, in case this transaction is a child transaction and has // inherited its parent's decision if (transaction.sampled === undefined) { diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 8d23262da796..4d30dff31b81 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -2,8 +2,8 @@ import { Breadcrumb, BreadcrumbHint } from './breadcrumb'; import { Event, EventHint } from './event'; import { Integration } from './integration'; import { LogLevel } from './loglevel'; -import { Transport, TransportClass, TransportOptions } from './transport'; import { SampleContext } from './transaction'; +import { Transport, TransportClass, TransportOptions } from './transport'; /** Base configuration options for every SDK. */ export interface Options { @@ -85,13 +85,10 @@ export interface Options { * 0.0 = 0% chance of a given trace being sent (send no traces) * 1.0 = 100% chance of a given trace being sent (send all traces) * - * Default: 0.0 + * Can be omitted without disabling tracing if `tracesSampler` is defined. If neither is defined, tracing is disabled. */ tracesSampleRate?: number; - /** Function to compute tracing sample rate dynamically and filter unwanted traces */ - tracesSampler?(traceContext: SampleContext): number | null; - /** Attaches stacktraces to pure capture message / log integrations */ attachStacktrace?: boolean; @@ -148,4 +145,13 @@ export interface Options { * @returns The breadcrumb that will be added | null. */ beforeBreadcrumb?(breadcrumb: Breadcrumb, hint?: BreadcrumbHint): Breadcrumb | null; + + /** + * Function to compute tracing sample rate dynamically and filter unwanted traces. + * + * Can be defined in place of `tracesSampleRate`. If neither is defined, tracing is disabled. + * + * @returns A sample rate or `null` to drop the trace. + */ + tracesSampler?(traceContext: SampleContext): number | null; } diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 605c7f748cb5..32f766789f30 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -1,5 +1,5 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { Event, Hub, Integration, StackFrame, WrappedFunction } from '@sentry/types'; +import { Event, Integration, Options, StackFrame, WrappedFunction } from '@sentry/types'; import { isString } from './is'; import { snipLine } from './string'; @@ -526,9 +526,6 @@ export function stripUrlQueryAndFragment(urlPath: string): string { * * Tracing is enabled when at least one of `tracesSampleRate` and `tracesSampler` is defined in the SDK config. */ -export function hasTracingEnabled(hub: Hub): boolean { - const client = hub.getClient(); - const options = (client && client.getOptions()) || {}; - +export function hasTracingEnabled(options: Options): boolean { return !!options.tracesSampleRate || !!options.tracesSampler; } From 290604dcc9f950c537f6c94537c5cccacef9fe9d Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 14 Aug 2020 08:42:05 -0700 Subject: [PATCH 03/40] respond to PR comments --- packages/gatsby/gatsby-browser.js | 2 +- packages/gatsby/package.json | 3 ++- packages/tracing/src/hubextensions.ts | 32 +++++++++++---------------- 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/packages/gatsby/gatsby-browser.js b/packages/gatsby/gatsby-browser.js index bb667df41951..79b0bc4bd2c1 100644 --- a/packages/gatsby/gatsby-browser.js +++ b/packages/gatsby/gatsby-browser.js @@ -1,6 +1,6 @@ const Sentry = require('@sentry/react'); const Tracing = require('@sentry/tracing'); -const Utils = require('@sentry/utils'); +const Utils = require('@sentry/utils') exports.onClientEntry = function(_, pluginParams) { if (pluginParams === undefined) { diff --git a/packages/gatsby/package.json b/packages/gatsby/package.json index 8976dba30718..8312bc06354d 100644 --- a/packages/gatsby/package.json +++ b/packages/gatsby/package.json @@ -27,7 +27,8 @@ }, "dependencies": { "@sentry/react": "5.23.0", - "@sentry/tracing": "5.23.0" + "@sentry/tracing": "5.23.0", + "@sentry/utils": "5.23.0" }, "peerDependencies": { "gatsby": "*" diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 7e193558bcc7..ecc9c124bb19 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -21,8 +21,8 @@ function traceHeaders(this: Hub): { [key: string]: string } { } /** - * Use sample rate (given in options as either a constant or ) along with a random number generator to make a sampling - * decision, which all child spans and child transactions inherit. + * Use sample rate along with a random number generator to make a sampling decision, which all child spans and child + * transactions inherit. * * Sample rate is set in SDK config, either as a constant (`tracesSampleRate`) or a function to compute the rate * (`tracesSampler`). @@ -36,8 +36,8 @@ function sample(hub: Hub, transaction: T): T { const client = hub.getClient(); const options = (client && client.getOptions()) || {}; - // nothing to do if tracing is disabled - if (!hasTracingEnabled(options)) { + // nothing to do if there's no client or if tracing is disabled + if (!client || !hasTracingEnabled(options)) { transaction.sampled = false; return transaction; } @@ -47,23 +47,17 @@ function sample(hub: Hub, transaction: T): T { // we have to test for a pre-existsing sampling decision, in case this transaction is a child transaction and has // inherited its parent's decision if (transaction.sampled === undefined) { - let sampleRate; + const sampleContext: SampleContext = {}; // TODO (kmclb) build context object - // prefer the hook - if (options.tracesSampler) { - // TODO (kmclb) build context object - const sampleContext: SampleContext = {}; - sampleRate = options.tracesSampler(sampleContext); - } - // we would have bailed at the beginning if neither `tracesSampler` nor `tracesSampleRate` were defined, so if the - // former isn't, the latter must be - else { - sampleRate = options.tracesSampleRate; - } + // we would have bailed at the beginning if neither `tracesSampler` nor `tracesSampleRate` were defined, so one of + // these should work; prefer the hook if so + const sampleRate = + typeof options.tracesSampler === 'function' ? options.tracesSampler(sampleContext) : options.tracesSampleRate; - // if the function returned either 0 or null, it's a sign the transaction should be dropped + // if the function returned either 0 or null, or if the sample rate is set to 0, it's a sign the transaction should + // be dropped if (!sampleRate) { - logger.log('Discarding trace because tracesSampler returned 0 or null'); + logger.log('Discarding trace because tracesSampler returned 0 or null or sampleRate is set to 0'); transaction.sampled = false; return transaction; } @@ -80,7 +74,7 @@ function sample(hub: Hub, transaction: T): T { // at this point we know we're keeping the transaction, whether because of an inherited decision or because it got // lucky with the dice roll - const experimentsOptions = (client && client.getOptions()._experiments) || {}; + const experimentsOptions = options._experiments || {}; transaction.initSpanRecorder(experimentsOptions.maxSpans as number); return transaction; From 05a358eee8cbe7bee1c5af144ce75b7bd327cf39 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 18 Aug 2020 15:42:57 -0700 Subject: [PATCH 04/40] don't return null from tracesSampler --- packages/tracing/src/hubextensions.ts | 5 ++--- packages/types/src/options.ts | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index ecc9c124bb19..3dbe35f8aead 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -54,10 +54,9 @@ function sample(hub: Hub, transaction: T): T { const sampleRate = typeof options.tracesSampler === 'function' ? options.tracesSampler(sampleContext) : options.tracesSampleRate; - // if the function returned either 0 or null, or if the sample rate is set to 0, it's a sign the transaction should - // be dropped + // if the function returned 0, or if the sample rate is set to 0, it's a sign the transaction should be dropped if (!sampleRate) { - logger.log('Discarding trace because tracesSampler returned 0 or null or sampleRate is set to 0'); + logger.log('Discarding trace because tracesSampler returned 0 or tracesSampleRate is set to 0'); transaction.sampled = false; return transaction; } diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 4d30dff31b81..6bc2eb0b4f1f 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -151,7 +151,7 @@ export interface Options { * * Can be defined in place of `tracesSampleRate`. If neither is defined, tracing is disabled. * - * @returns A sample rate or `null` to drop the trace. + * @returns A sample rate between 0 and 1 (0 drops the trace, 1 guarantees it will be sent). */ - tracesSampler?(traceContext: SampleContext): number | null; + tracesSampler?(traceContext: SampleContext): number; } From 90854cc0ad5185904439f2c1dc7330cde3541292 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 18 Aug 2020 15:46:50 -0700 Subject: [PATCH 05/40] random cleanup --- packages/tracing/src/hubextensions.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 3dbe35f8aead..e7d6889408a8 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -27,8 +27,8 @@ function traceHeaders(this: Hub): { [key: string]: string } { * Sample rate is set in SDK config, either as a constant (`tracesSampleRate`) or a function to compute the rate * (`tracesSampler`). * - * Called every time a transaction is created. Only transactions which emerge with sampled === true will be sent to - * Sentry. + * Called every time a transaction is created. Only transactions which emerge with a `sampled` value of `true` will be + * sent to Sentry. * * Mutates the given Transaction object and then returns the mutated object. */ @@ -61,7 +61,7 @@ function sample(hub: Hub, transaction: T): T { return transaction; } - // now we roll the dice (Math.random is inclusive of 0, but not of 1) + // now we roll the dice (Math.random is inclusive of 0, but not of 1, so strict < is safe here) transaction.sampled = Math.random() < sampleRate; // if we're not going to keep it, we're done From cbc216c4c75fbc4936c183b494753b3ecc40582c Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 18 Aug 2020 22:03:53 -0700 Subject: [PATCH 06/40] simplify implementation of extractrequestdata and add docstring --- packages/node/src/handlers.ts | 37 ++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index affaf24a7123..b8b6ca1578ac 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -123,10 +123,19 @@ function extractTransaction(req: { [key: string]: any }, type: boolean | Transac /** Default request keys that'll be used to extract data from the request */ const DEFAULT_REQUEST_KEYS = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; -/** JSDoc */ -function extractRequestData(req: { [key: string]: any }, keys: boolean | string[]): { [key: string]: string } { - const request: { [key: string]: any } = {}; - const attributes = Array.isArray(keys) ? keys : DEFAULT_REQUEST_KEYS; +/** + * Normalizes data from the request object, accounting for framework differences. + * + * @param req The request object from which to extract data + * @param keys An optional array of keys to include in the normalized data. Defaults to DEFAULT_REQUEST_KEYS if not + * provided. + * @returns An object containing normalized request data + */ +export function extractRequestData( + req: { [key: string]: any }, + keys: string[] = DEFAULT_REQUEST_KEYS, +): { [key: string]: string } { + const requestData: { [key: string]: any } = {}; // headers: // node, express: req.headers @@ -157,27 +166,27 @@ function extractRequestData(req: { [key: string]: any }, keys: boolean | string[ // absolute url const absoluteUrl = `${protocol}://${host}${originalUrl}`; - attributes.forEach(key => { + keys.forEach(key => { switch (key) { case 'headers': - request.headers = headers; + requestData.headers = headers; break; case 'method': - request.method = method; + requestData.method = method; break; case 'url': - request.url = absoluteUrl; + requestData.url = absoluteUrl; break; case 'cookies': // cookies: // node, express, koa: req.headers.cookie - request.cookies = cookie.parse(headers.cookie || ''); + requestData.cookies = cookie.parse(headers.cookie || ''); break; case 'query_string': // query string: // node: req.url (raw) // express, koa: req.query - request.query_string = url.parse(originalUrl || '', false).query; + requestData.query_string = url.parse(originalUrl || '', false).query; break; case 'data': if (method === 'GET' || method === 'HEAD') { @@ -186,17 +195,17 @@ function extractRequestData(req: { [key: string]: any }, keys: boolean | string[ // body data: // node, express, koa: req.body if (req.body !== undefined) { - request.data = isString(req.body) ? req.body : JSON.stringify(normalize(req.body)); + requestData.data = isString(req.body) ? req.body : JSON.stringify(normalize(req.body)); } break; default: if ({}.hasOwnProperty.call(req, key)) { - request[key] = (req as { [key: string]: any })[key]; + requestData[key] = (req as { [key: string]: any })[key]; } } }); - return request; + return requestData; } /** Default user keys that'll be used to extract data from the request */ @@ -279,7 +288,7 @@ export function parseRequest( if (options.request) { event.request = { ...event.request, - ...extractRequestData(req, options.request), + ...extractRequestData(req), // extract default keys given in DEFAULT_REQUEST_KEYS }; } From 85a077652d7b8781815cfbff926165b01288553b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 18 Aug 2020 22:11:11 -0700 Subject: [PATCH 07/40] allow startTransaction to take a sampleContext argument which gets passed to tracesSampler --- packages/hub/src/hub.ts | 5 +++-- packages/minimal/src/index.ts | 6 ++++-- packages/tracing/src/hubextensions.ts | 11 +++++------ packages/types/src/hub.ts | 5 +++-- packages/types/src/options.ts | 4 ++++ 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 4f3ab5a6476d..b054621ed48b 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -10,6 +10,7 @@ import { Hub as HubInterface, Integration, IntegrationClass, + SampleContext, Severity, Span, SpanContext, @@ -369,8 +370,8 @@ export class Hub implements HubInterface { /** * @inheritDoc */ - public startTransaction(context: TransactionContext): Transaction { - return this._callExtensionMethod('startTransaction', context); + public startTransaction(context: TransactionContext, sampleContext?: SampleContext): Transaction { + return this._callExtensionMethod('startTransaction', context, sampleContext); } /** diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 56cfdda9ff11..acceb3e5a377 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -5,6 +5,7 @@ import { Event, Extra, Extras, + SampleContext, Severity, Transaction, TransactionContext, @@ -205,7 +206,8 @@ export function _callOnClient(method: string, ...args: any[]): void { * Sentry. * * @param context Properties of the new `Transaction`. + * @param sampleContext Information given to the transaction sampling function */ -export function startTransaction(context: TransactionContext): Transaction { - return callOnHub('startTransaction', { ...context }); +export function startTransaction(context: TransactionContext, sampleContext?: SampleContext): Transaction { + return callOnHub('startTransaction', { ...context }, sampleContext); } diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index e7d6889408a8..831eb63b31e2 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -32,7 +32,7 @@ function traceHeaders(this: Hub): { [key: string]: string } { * * Mutates the given Transaction object and then returns the mutated object. */ -function sample(hub: Hub, transaction: T): T { +function sample(hub: Hub, transaction: T, sampleContext: SampleContext = {}): T { const client = hub.getClient(); const options = (client && client.getOptions()) || {}; @@ -47,8 +47,6 @@ function sample(hub: Hub, transaction: T): T { // we have to test for a pre-existsing sampling decision, in case this transaction is a child transaction and has // inherited its parent's decision if (transaction.sampled === undefined) { - const sampleContext: SampleContext = {}; // TODO (kmclb) build context object - // we would have bailed at the beginning if neither `tracesSampler` nor `tracesSampleRate` were defined, so one of // these should work; prefer the hook if so const sampleRate = @@ -82,9 +80,9 @@ function sample(hub: Hub, transaction: T): T { /** * {@see Hub.startTransaction} */ -function startTransaction(this: Hub, context: TransactionContext): Transaction { +function startTransaction(this: Hub, context: TransactionContext, sampleContext?: SampleContext): Transaction { const transaction = new Transaction(context, this); - return sample(this, transaction); + return sample(this, transaction, sampleContext); } /** @@ -95,9 +93,10 @@ export function startIdleTransaction( context: TransactionContext, idleTimeout?: number, onScope?: boolean, + sampleContext?: SampleContext, ): IdleTransaction { const transaction = new IdleTransaction(context, hub, idleTimeout, onScope); - return sample(hub, transaction); + return sample(hub, transaction, sampleContext); } /** diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 6ab719821785..4d023d2cfa8c 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -6,7 +6,7 @@ import { Integration, IntegrationClass } from './integration'; import { Scope } from './scope'; import { Severity } from './severity'; import { Span, SpanContext } from './span'; -import { Transaction, TransactionContext } from './transaction'; +import { SampleContext, Transaction, TransactionContext } from './transaction'; import { User } from './user'; /** @@ -198,6 +198,7 @@ export interface Hub { * Sentry. * * @param context Properties of the new `Transaction`. + * @param sampleContext Information given to the transaction sampling function */ - startTransaction(context: TransactionContext): Transaction; + startTransaction(context: TransactionContext, sampleContext?: SampleContext): Transaction; } diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 6bc2eb0b4f1f..c107f6173400 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -151,6 +151,10 @@ export interface Options { * * Can be defined in place of `tracesSampleRate`. If neither is defined, tracing is disabled. * + * `startTransaction` or `startIdleTransaction` accept a `sampleContext` argument, which is then passed to this + * function, meant to provide information on which to make a sampling decision. Additional information may be included + * automatically, depending on the SDK and any included integrations. + * * @returns A sample rate between 0 and 1 (0 drops the trace, 1 guarantees it will be sent). */ tracesSampler?(traceContext: SampleContext): number; From 4f1b48490582c110e3879d90f2974367ab6ac15b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 24 Aug 2020 06:14:12 -0700 Subject: [PATCH 08/40] factor out getActiveDomain as new function --- packages/hub/src/hub.ts | 23 +++++++++++++---------- packages/hub/src/index.ts | 10 +++++++++- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index b054621ed48b..b0ae7a69cee5 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -457,22 +457,25 @@ export function getCurrentHub(): Hub { return getHubFromCarrier(registry); } +/** + * Returns the active domain, if one exists + * + * @returns The domain, or undefined if there is no active domain + */ +export function getActiveDomain(): any | undefined { + const sentry = getMainCarrier().__SENTRY__; + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any + return sentry && sentry.extensions && sentry.extensions.domain && (sentry.extensions.domain as any).active; +} + /** * Try to read the hub from an active domain, and fallback to the registry if one doesn't exist * @returns discovered hub */ function getHubFromActiveDomain(registry: Carrier): Hub { try { - const property = 'domain'; - const carrier = getMainCarrier(); - const sentry = carrier.__SENTRY__; - if (!sentry || !sentry.extensions || !sentry.extensions[property]) { - return getHubFromCarrier(registry); - } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const domain = sentry.extensions[property] as any; - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const activeDomain = domain.active; + const activeDomain = getActiveDomain(); // If there's no active domain, just return global hub if (!activeDomain) { diff --git a/packages/hub/src/index.ts b/packages/hub/src/index.ts index 6656b1b24df9..56f4be9d1292 100644 --- a/packages/hub/src/index.ts +++ b/packages/hub/src/index.ts @@ -1,3 +1,11 @@ export { Carrier, Layer } from './interfaces'; export { addGlobalEventProcessor, Scope } from './scope'; -export { getCurrentHub, getHubFromCarrier, getMainCarrier, Hub, makeMain, setHubOnCarrier } from './hub'; +export { + getActiveDomain, + getCurrentHub, + getHubFromCarrier, + getMainCarrier, + Hub, + makeMain, + setHubOnCarrier, +} from './hub'; From 382320f25462c867bf4aa6f4126d3ff79486062f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 24 Aug 2020 06:30:14 -0700 Subject: [PATCH 09/40] add getDefaultSampleContext method --- packages/tracing/package.json | 1 + packages/tracing/src/hubextensions.ts | 52 ++++++++++++++++++++++++--- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/packages/tracing/package.json b/packages/tracing/package.json index 2376bf177705..3709acd1e8a5 100644 --- a/packages/tracing/package.json +++ b/packages/tracing/package.json @@ -25,6 +25,7 @@ "devDependencies": { "@sentry-internal/eslint-config-sdk": "5.23.0", "@sentry/browser": "5.23.0", + "@sentry/node": "5.22.3", "@types/express": "^4.17.1", "@types/jsdom": "^16.2.3", "eslint": "7.6.0", diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 831eb63b31e2..de321f693942 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -1,6 +1,8 @@ -import { getMainCarrier, Hub } from '@sentry/hub'; +import { getActiveDomain, getMainCarrier, Hub } from '@sentry/hub'; +import { Handlers } from '@sentry/node'; import { SampleContext, TransactionContext } from '@sentry/types'; -import { hasTracingEnabled, logger } from '@sentry/utils'; +import { hasTracingEnabled, isInstanceOf, isNodeEnv, logger } from '@sentry/utils'; +import * as http from 'http'; // TODO - is this okay, or do we need to export the type we need from our node SDK? import { registerErrorInstrumentation } from './errors'; import { IdleTransaction } from './idletransaction'; @@ -76,13 +78,53 @@ function sample(hub: Hub, transaction: T, sampleContext: return transaction; } +/** + * Gets the correct context to pass to the tracesSampler, based on the environment (i.e., which SDK is being used) + * + * @returns A SampleContext object + */ +function getDefaultSampleContext(): SampleContext { + const defaultSampleContext: SampleContext = {}; + + if (isNodeEnv()) { + // look at koa TODO + const domain = getActiveDomain(); + if (domain) { + // TODO is the below true? + // for all node servers that we currently support, we store the incoming request object (which is an instance of + // http.IncomingMessage) on the domain the domain is stored as an array, so our only way to find the request is to + // iterate through the array and compare types + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any + const request = domain.members.find((member: any) => isInstanceOf(member, http.IncomingMessage)); + if (request) { + defaultSampleContext.request = Handlers.extractRequestData(request); + } + } + } + + // we must be in browser-js (or some derivative thereof) + else { + // we take a copy of the location object rather than just a reference to it in case there's a navigation in the + // instant between when the transaction starts and when the sampler is called + defaultSampleContext.location = { ...window.location }; + } + + return defaultSampleContext; +} /** - * {@see Hub.startTransaction} + * Creates a new transaction and adds a sampling decision if it doesn't yet have one. + * + * The Hub.startTransaction method delegates to this method to do its work, passing the Hub instance in as `this`. + * Exists as a separate function so that it can be injected into the class as an "extension method." + * + * @returns The new transaction + * + * @see {@link Hub.startTransaction} */ function startTransaction(this: Hub, context: TransactionContext, sampleContext?: SampleContext): Transaction { const transaction = new Transaction(context, this); - return sample(this, transaction, sampleContext); + return sample(this, transaction, { ...getDefaultSampleContext(), ...sampleContext }); } /** @@ -96,7 +138,7 @@ export function startIdleTransaction( sampleContext?: SampleContext, ): IdleTransaction { const transaction = new IdleTransaction(context, hub, idleTimeout, onScope); - return sample(hub, transaction, sampleContext); + return sample(hub, transaction, { ...getDefaultSampleContext(), ...sampleContext }); } /** From 3587b6425dabb3c8378ab5b861dea3a25bc30de0 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 24 Aug 2020 06:30:44 -0700 Subject: [PATCH 10/40] rename internal startTransaction with leading underscore --- packages/tracing/src/hubextensions.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index de321f693942..65a96fdb7cdf 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -122,7 +122,7 @@ function getDefaultSampleContext(): SampleContext { * * @see {@link Hub.startTransaction} */ -function startTransaction(this: Hub, context: TransactionContext, sampleContext?: SampleContext): Transaction { +function _startTransaction(this: Hub, context: TransactionContext, sampleContext?: SampleContext): Transaction { const transaction = new Transaction(context, this); return sample(this, transaction, { ...getDefaultSampleContext(), ...sampleContext }); } @@ -149,7 +149,7 @@ export function _addTracingExtensions(): void { if (carrier.__SENTRY__) { carrier.__SENTRY__.extensions = carrier.__SENTRY__.extensions || {}; if (!carrier.__SENTRY__.extensions.startTransaction) { - carrier.__SENTRY__.extensions.startTransaction = startTransaction; + carrier.__SENTRY__.extensions.startTransaction = _startTransaction; } if (!carrier.__SENTRY__.extensions.traceHeaders) { carrier.__SENTRY__.extensions.traceHeaders = traceHeaders; From a7a48b5c3539855abfc507fbb9c71135354746c4 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 24 Aug 2020 10:55:42 -0700 Subject: [PATCH 11/40] move extractRequestData to @sentry/utils --- packages/node/src/handlers.ts | 100 +++----------------------- packages/tracing/src/hubextensions.ts | 18 +++-- packages/utils/src/misc.ts | 96 +++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 96 deletions(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index b8b6ca1578ac..59ce66e34f24 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -3,8 +3,14 @@ import { captureException, getCurrentHub, startTransaction, withScope } from '@sentry/core'; import { Span } from '@sentry/tracing'; import { Event } from '@sentry/types'; -import { forget, isPlainObject, isString, logger, normalize, stripUrlQueryAndFragment } from '@sentry/utils'; -import * as cookie from 'cookie'; +import { + extractNodeRequestData, + forget, + isPlainObject, + isString, + logger, + stripUrlQueryAndFragment, +} from '@sentry/utils'; import * as domain from 'domain'; import * as http from 'http'; import * as os from 'os'; @@ -120,94 +126,6 @@ function extractTransaction(req: { [key: string]: any }, type: boolean | Transac } } -/** Default request keys that'll be used to extract data from the request */ -const DEFAULT_REQUEST_KEYS = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; - -/** - * Normalizes data from the request object, accounting for framework differences. - * - * @param req The request object from which to extract data - * @param keys An optional array of keys to include in the normalized data. Defaults to DEFAULT_REQUEST_KEYS if not - * provided. - * @returns An object containing normalized request data - */ -export function extractRequestData( - req: { [key: string]: any }, - keys: string[] = DEFAULT_REQUEST_KEYS, -): { [key: string]: string } { - const requestData: { [key: string]: any } = {}; - - // headers: - // node, express: req.headers - // koa: req.header - const headers = (req.headers || req.header || {}) as { - host?: string; - cookie?: string; - }; - // method: - // node, express, koa: req.method - const method = req.method; - // host: - // express: req.hostname in > 4 and req.host in < 4 - // koa: req.host - // node: req.headers.host - const host = req.hostname || req.host || headers.host || ''; - // protocol: - // node: - // express, koa: req.protocol - const protocol = - req.protocol === 'https' || req.secure || ((req.socket || {}) as { encrypted?: boolean }).encrypted - ? 'https' - : 'http'; - // url (including path and query string): - // node, express: req.originalUrl - // koa: req.url - const originalUrl = (req.originalUrl || req.url) as string; - // absolute url - const absoluteUrl = `${protocol}://${host}${originalUrl}`; - - keys.forEach(key => { - switch (key) { - case 'headers': - requestData.headers = headers; - break; - case 'method': - requestData.method = method; - break; - case 'url': - requestData.url = absoluteUrl; - break; - case 'cookies': - // cookies: - // node, express, koa: req.headers.cookie - requestData.cookies = cookie.parse(headers.cookie || ''); - break; - case 'query_string': - // query string: - // node: req.url (raw) - // express, koa: req.query - requestData.query_string = url.parse(originalUrl || '', false).query; - break; - case 'data': - if (method === 'GET' || method === 'HEAD') { - break; - } - // body data: - // node, express, koa: req.body - if (req.body !== undefined) { - requestData.data = isString(req.body) ? req.body : JSON.stringify(normalize(req.body)); - } - break; - default: - if ({}.hasOwnProperty.call(req, key)) { - requestData[key] = (req as { [key: string]: any })[key]; - } - } - }); - - return requestData; -} - /** Default user keys that'll be used to extract data from the request */ const DEFAULT_USER_KEYS = ['id', 'username', 'email']; @@ -288,7 +206,7 @@ export function parseRequest( if (options.request) { event.request = { ...event.request, - ...extractRequestData(req), // extract default keys given in DEFAULT_REQUEST_KEYS + ...extractNodeRequestData(req), }; } diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 65a96fdb7cdf..aa6f0df936c3 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -1,8 +1,13 @@ import { getActiveDomain, getMainCarrier, Hub } from '@sentry/hub'; -import { Handlers } from '@sentry/node'; import { SampleContext, TransactionContext } from '@sentry/types'; -import { hasTracingEnabled, isInstanceOf, isNodeEnv, logger } from '@sentry/utils'; -import * as http from 'http'; // TODO - is this okay, or do we need to export the type we need from our node SDK? +import { + dynamicRequire, + extractNodeRequestData, + hasTracingEnabled, + isInstanceOf, + isNodeEnv, + logger, +} from '@sentry/utils'; import { registerErrorInstrumentation } from './errors'; import { IdleTransaction } from './idletransaction'; @@ -89,15 +94,18 @@ function getDefaultSampleContext(): SampleContext { if (isNodeEnv()) { // look at koa TODO const domain = getActiveDomain(); + const nodeHttpModule = dynamicRequire(module, 'http'); + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const requestType = nodeHttpModule.IncomingMessage; if (domain) { // TODO is the below true? // for all node servers that we currently support, we store the incoming request object (which is an instance of // http.IncomingMessage) on the domain the domain is stored as an array, so our only way to find the request is to // iterate through the array and compare types // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any - const request = domain.members.find((member: any) => isInstanceOf(member, http.IncomingMessage)); + const request = domain.members.find((member: any) => isInstanceOf(member, requestType)); if (request) { - defaultSampleContext.request = Handlers.extractRequestData(request); + defaultSampleContext.request = extractNodeRequestData(request); } } } diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 32f766789f30..29142029a465 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -2,6 +2,7 @@ import { Event, Integration, Options, StackFrame, WrappedFunction } from '@sentry/types'; import { isString } from './is'; +import { normalize } from './object'; import { snipLine } from './string'; /** Internal */ @@ -529,3 +530,98 @@ export function stripUrlQueryAndFragment(urlPath: string): string { export function hasTracingEnabled(options: Options): boolean { return !!options.tracesSampleRate || !!options.tracesSampler; } + +/** Default request keys that'll be used to extract data from the request */ +const DEFAULT_REQUEST_KEYS = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; + +/** + * Normalizes data from the request object, accounting for framework differences. + * + * @param req The request object from which to extract data + * @param keys An optional array of keys to include in the normalized data. Defaults to DEFAULT_REQUEST_KEYS if not + * provided. + * @returns An object containing normalized request data + */ +export function extractNodeRequestData( + req: { [key: string]: any }, + keys: string[] = DEFAULT_REQUEST_KEYS, +): { [key: string]: string } { + // make sure we can safely use dynamicRequire below + if (!isNodeEnv()) { + throw new Error("Can't get node request data outside of a node environment"); + } + + const requestData: { [key: string]: any } = {}; + + // headers: + // node, express: req.headers + // koa: req.header + const headers = (req.headers || req.header || {}) as { + host?: string; + cookie?: string; + }; + // method: + // node, express, koa: req.method + const method = req.method; + // host: + // express: req.hostname in > 4 and req.host in < 4 + // koa: req.host + // node: req.headers.host + const host = req.hostname || req.host || headers.host || ''; + // protocol: + // node: + // express, koa: req.protocol + const protocol = + req.protocol === 'https' || req.secure || ((req.socket || {}) as { encrypted?: boolean }).encrypted + ? 'https' + : 'http'; + // url (including path and query string): + // node, express: req.originalUrl + // koa: req.url + const originalUrl = (req.originalUrl || req.url) as string; + // absolute url + const absoluteUrl = `${protocol}://${host}${originalUrl}`; + + keys.forEach(key => { + switch (key) { + case 'headers': + requestData.headers = headers; + break; + case 'method': + requestData.method = method; + break; + case 'url': + requestData.url = absoluteUrl; + break; + case 'cookies': + // cookies: + // node, express, koa: req.headers.cookie + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + requestData.cookies = dynamicRequire(module, 'cookie').parse(headers.cookie || ''); + break; + case 'query_string': + // query string: + // node: req.url (raw) + // express, koa: req.query + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + requestData.query_string = dynamicRequire(module, 'url').parse(originalUrl || '', false).query; + break; + case 'data': + if (method === 'GET' || method === 'HEAD') { + break; + } + // body data: + // node, express, koa: req.body + if (req.body !== undefined) { + requestData.data = isString(req.body) ? req.body : JSON.stringify(normalize(req.body)); + } + break; + default: + if ({}.hasOwnProperty.call(req, key)) { + requestData[key] = (req as { [key: string]: any })[key]; + } + } + }); + + return requestData; +} From 3855006bc4ea6ec39917d73ec945d76a2c8b7080 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 24 Aug 2020 11:24:50 -0700 Subject: [PATCH 12/40] validate sample rate and clean up logging --- packages/tracing/src/hubextensions.ts | 38 ++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index aa6f0df936c3..bd29f60a4633 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -49,8 +49,6 @@ function sample(hub: Hub, transaction: T, sampleContext: return transaction; } - logger.log('Tracing enabled'); - // we have to test for a pre-existsing sampling decision, in case this transaction is a child transaction and has // inherited its parent's decision if (transaction.sampled === undefined) { @@ -59,9 +57,20 @@ function sample(hub: Hub, transaction: T, sampleContext: const sampleRate = typeof options.tracesSampler === 'function' ? options.tracesSampler(sampleContext) : options.tracesSampleRate; + // since this is coming from the user, who knows what we might get + if (!isValidSampleRate(sampleRate)) { + logger.warn(`[Tracing] Discarding trace because of invalid sample rate.`); + transaction.sampled = false; + return transaction; + } + // if the function returned 0, or if the sample rate is set to 0, it's a sign the transaction should be dropped if (!sampleRate) { - logger.log('Discarding trace because tracesSampler returned 0 or tracesSampleRate is set to 0'); + logger.log( + `[Tracing] Discarding trace because ${ + typeof options.tracesSampler === 'function' ? 'tracesSampler returned 0' : 'tracesSampleRate is set to 0' + }`, + ); transaction.sampled = false; return transaction; } @@ -71,7 +80,9 @@ function sample(hub: Hub, transaction: T, sampleContext: // if we're not going to keep it, we're done if (!transaction.sampled) { - logger.log(`Discarding trace because it's not included in the random sample (sampling rate = ${sampleRate})`); + logger.log( + `[Tracing] Discarding trace because it's not included in the random sample (sampling rate = ${sampleRate})`, + ); return transaction; } } @@ -120,6 +131,25 @@ function getDefaultSampleContext(): SampleContext { return defaultSampleContext; } +/** + * Checks the given sample rate to make sure it is valid (a number between 0 and 1). + */ +function isValidSampleRate(rate: unknown): boolean { + if (!(typeof rate === 'number')) { + logger.warn( + `[Tracing] Given sample rate is invalid. Sample rate must be a number between 0 and 1. Got ${JSON.stringify( + rate, + )} of type ${JSON.stringify(typeof rate)}.`, + ); + return false; + } + if (rate < 0 || rate > 1) { + logger.warn(`[Tracing] Given sample rate is invalid. Sample rate must be between 0 and 1. Got ${rate}.`); + return false; + } + return true; +} + /** * Creates a new transaction and adds a sampling decision if it doesn't yet have one. * From 2a1d731b3cc71a9dbba73c45fa76320e5bb46115 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 Aug 2020 10:36:34 -0700 Subject: [PATCH 13/40] get location data from global object instead of window to account for service workers --- packages/tracing/src/hubextensions.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index bd29f60a4633..e303823bdb4d 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -3,6 +3,7 @@ import { SampleContext, TransactionContext } from '@sentry/types'; import { dynamicRequire, extractNodeRequestData, + getGlobalObject, hasTracingEnabled, isInstanceOf, isNodeEnv, @@ -123,9 +124,14 @@ function getDefaultSampleContext(): SampleContext { // we must be in browser-js (or some derivative thereof) else { - // we take a copy of the location object rather than just a reference to it in case there's a navigation in the - // instant between when the transaction starts and when the sampler is called - defaultSampleContext.location = { ...window.location }; + // we use `getGlobalObject()` rather than `window` since service workers also have a `location` property on `self` + const globalObject = getGlobalObject(); + + if ('location' in globalObject) { + // we take a copy of the location object rather than just a reference to it in case there's a navigation or + // redirect in the instant between when the transaction starts and when the sampler is called + defaultSampleContext.location = { ...globalObject.location }; + } } return defaultSampleContext; From 551afc4399a6cf2e912d3dfe0edbd242d824573e Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 Aug 2020 14:37:53 -0700 Subject: [PATCH 14/40] create hybrid type for domains acting as carriers --- packages/hub/src/hub.ts | 4 ++-- packages/hub/src/index.ts | 2 +- packages/hub/src/interfaces.ts | 3 +++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index b0ae7a69cee5..f42b214fe9a2 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -20,7 +20,7 @@ import { } from '@sentry/types'; import { consoleSandbox, getGlobalObject, isNodeEnv, logger, timestampWithMs, uuid4 } from '@sentry/utils'; -import { Carrier, Layer } from './interfaces'; +import { Carrier, DomainAsCarrier, Layer } from './interfaces'; import { Scope } from './scope'; /** @@ -462,7 +462,7 @@ export function getCurrentHub(): Hub { * * @returns The domain, or undefined if there is no active domain */ -export function getActiveDomain(): any | undefined { +export function getActiveDomain(): DomainAsCarrier | undefined { const sentry = getMainCarrier().__SENTRY__; // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any diff --git a/packages/hub/src/index.ts b/packages/hub/src/index.ts index 56f4be9d1292..04e8be2aee4f 100644 --- a/packages/hub/src/index.ts +++ b/packages/hub/src/index.ts @@ -1,4 +1,4 @@ -export { Carrier, Layer } from './interfaces'; +export { Carrier, DomainAsCarrier, Layer } from './interfaces'; export { addGlobalEventProcessor, Scope } from './scope'; export { getActiveDomain, diff --git a/packages/hub/src/interfaces.ts b/packages/hub/src/interfaces.ts index c1585bbe1916..7647e4278454 100644 --- a/packages/hub/src/interfaces.ts +++ b/packages/hub/src/interfaces.ts @@ -1,4 +1,5 @@ import { Client } from '@sentry/types'; +import * as domain from 'domain'; import { Hub } from './hub'; import { Scope } from './scope'; @@ -26,3 +27,5 @@ export interface Carrier { extensions?: { [key: string]: Function }; }; } + +export interface DomainAsCarrier extends domain.Domain, Carrier {} From 145309249639c5ae78d178ca2b92b7505b1e5f12 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 Aug 2020 14:38:46 -0700 Subject: [PATCH 15/40] fix bug in parseRequest --- packages/node/src/handlers.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 59ce66e34f24..dcd6c567f0a8 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -204,9 +204,13 @@ export function parseRequest( } if (options.request) { + // if the option value is `true`, use the default set of keys by not passing anything to `extractNodeRequestData()` + const extractedRequestData = Array.isArray(options.request) + ? extractNodeRequestData(req, options.request) + : extractNodeRequestData(req); event.request = { ...event.request, - ...extractNodeRequestData(req), + ...extractedRequestData, }; } From 17512a0be45b7249bbdc3b18e5ea8c7fb757336f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 Aug 2020 17:14:30 -0700 Subject: [PATCH 16/40] break up utils.misc to fix circular imports --- packages/utils/src/browser.ts | 98 +++++++++++++ packages/utils/src/index.ts | 5 +- packages/utils/src/instrument.ts | 3 +- packages/utils/src/misc.ts | 233 +------------------------------ packages/utils/src/node.ts | 118 ++++++++++++++++ packages/utils/src/object.ts | 3 +- packages/utils/src/stacktrace.ts | 17 +++ 7 files changed, 242 insertions(+), 235 deletions(-) create mode 100644 packages/utils/src/browser.ts create mode 100644 packages/utils/src/node.ts create mode 100644 packages/utils/src/stacktrace.ts diff --git a/packages/utils/src/browser.ts b/packages/utils/src/browser.ts new file mode 100644 index 000000000000..e81402501740 --- /dev/null +++ b/packages/utils/src/browser.ts @@ -0,0 +1,98 @@ +import { isString } from './is'; + +/** + * Given a child DOM element, returns a query-selector statement describing that + * and its ancestors + * e.g. [HTMLElement] => body > div > input#foo.btn[name=baz] + * @returns generated DOM path + */ +export function htmlTreeAsString(elem: unknown): string { + type SimpleNode = { + parentNode: SimpleNode; + } | null; + + // try/catch both: + // - accessing event.target (see getsentry/raven-js#838, #768) + // - `htmlTreeAsString` because it's complex, and just accessing the DOM incorrectly + // - can throw an exception in some circumstances. + try { + let currentElem = elem as SimpleNode; + const MAX_TRAVERSE_HEIGHT = 5; + const MAX_OUTPUT_LEN = 80; + const out = []; + let height = 0; + let len = 0; + const separator = ' > '; + const sepLength = separator.length; + let nextStr; + + // eslint-disable-next-line no-plusplus + while (currentElem && height++ < MAX_TRAVERSE_HEIGHT) { + nextStr = _htmlElementAsString(currentElem); + // bail out if + // - nextStr is the 'html' element + // - the length of the string that would be created exceeds MAX_OUTPUT_LEN + // (ignore this limit if we are on the first iteration) + if (nextStr === 'html' || (height > 1 && len + out.length * sepLength + nextStr.length >= MAX_OUTPUT_LEN)) { + break; + } + + out.push(nextStr); + + len += nextStr.length; + currentElem = currentElem.parentNode; + } + + return out.reverse().join(separator); + } catch (_oO) { + return ''; + } +} + +/** + * Returns a simple, query-selector representation of a DOM element + * e.g. [HTMLElement] => input#foo.btn[name=baz] + * @returns generated DOM path + */ +function _htmlElementAsString(el: unknown): string { + const elem = el as { + tagName?: string; + id?: string; + className?: string; + getAttribute(key: string): string; + }; + + const out = []; + let className; + let classes; + let key; + let attr; + let i; + + if (!elem || !elem.tagName) { + return ''; + } + + out.push(elem.tagName.toLowerCase()); + if (elem.id) { + out.push(`#${elem.id}`); + } + + // eslint-disable-next-line prefer-const + className = elem.className; + if (className && isString(className)) { + classes = className.split(/\s+/); + for (i = 0; i < classes.length; i++) { + out.push(`.${classes[i]}`); + } + } + const allowedAttrs = ['type', 'name', 'title', 'alt']; + for (i = 0; i < allowedAttrs.length; i++) { + key = allowedAttrs[i]; + attr = elem.getAttribute(key); + if (attr) { + out.push(`[${key}="${attr}"]`); + } + } + return out.join(''); +} diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 6457c22d419b..472d54dd542a 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -1,14 +1,17 @@ export * from './async'; +export * from './browser'; +export * from './dsn'; export * from './error'; export * from './is'; export * from './logger'; export * from './memo'; export * from './misc'; +export * from './node'; export * from './object'; export * from './path'; export * from './promisebuffer'; +export * from './stacktrace'; export * from './string'; export * from './supports'; export * from './syncpromise'; export * from './instrument'; -export * from './dsn'; diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index 7308d9efd892..1921d11a84e1 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -4,8 +4,9 @@ import { WrappedFunction } from '@sentry/types'; import { isInstanceOf, isString } from './is'; import { logger } from './logger'; -import { getFunctionName, getGlobalObject } from './misc'; +import { getGlobalObject } from './misc'; import { fill } from './object'; +import { getFunctionName } from './stacktrace'; import { supportsHistory, supportsNativeFetch } from './supports'; const global = getGlobalObject(); diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 29142029a465..8aa933fb52ba 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -1,8 +1,7 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import { Event, Integration, Options, StackFrame, WrappedFunction } from '@sentry/types'; -import { isString } from './is'; -import { normalize } from './object'; +import { dynamicRequire, isNodeEnv } from './node'; import { snipLine } from './string'; /** Internal */ @@ -22,26 +21,6 @@ interface SentryGlobal { }; } -/** - * Requires a module which is protected against bundler minification. - * - * @param request The module path to resolve - */ -// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types -export function dynamicRequire(mod: any, request: string): any { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - return mod.require(request); -} - -/** - * Checks whether we're in the Node.js or Browser environment - * - * @returns Answer to given question - */ -export function isNodeEnv(): boolean { - return Object.prototype.toString.call(typeof process !== 'undefined' ? process : 0) === '[object process]'; -} - const fallbackGlobalObject = {}; /** @@ -253,103 +232,6 @@ export function getLocationHref(): string { } } -/** - * Given a child DOM element, returns a query-selector statement describing that - * and its ancestors - * e.g. [HTMLElement] => body > div > input#foo.btn[name=baz] - * @returns generated DOM path - */ -export function htmlTreeAsString(elem: unknown): string { - type SimpleNode = { - parentNode: SimpleNode; - } | null; - - // try/catch both: - // - accessing event.target (see getsentry/raven-js#838, #768) - // - `htmlTreeAsString` because it's complex, and just accessing the DOM incorrectly - // - can throw an exception in some circumstances. - try { - let currentElem = elem as SimpleNode; - const MAX_TRAVERSE_HEIGHT = 5; - const MAX_OUTPUT_LEN = 80; - const out = []; - let height = 0; - let len = 0; - const separator = ' > '; - const sepLength = separator.length; - let nextStr; - - // eslint-disable-next-line no-plusplus - while (currentElem && height++ < MAX_TRAVERSE_HEIGHT) { - nextStr = _htmlElementAsString(currentElem); - // bail out if - // - nextStr is the 'html' element - // - the length of the string that would be created exceeds MAX_OUTPUT_LEN - // (ignore this limit if we are on the first iteration) - if (nextStr === 'html' || (height > 1 && len + out.length * sepLength + nextStr.length >= MAX_OUTPUT_LEN)) { - break; - } - - out.push(nextStr); - - len += nextStr.length; - currentElem = currentElem.parentNode; - } - - return out.reverse().join(separator); - } catch (_oO) { - return ''; - } -} - -/** - * Returns a simple, query-selector representation of a DOM element - * e.g. [HTMLElement] => input#foo.btn[name=baz] - * @returns generated DOM path - */ -function _htmlElementAsString(el: unknown): string { - const elem = el as { - tagName?: string; - id?: string; - className?: string; - getAttribute(key: string): string; - }; - - const out = []; - let className; - let classes; - let key; - let attr; - let i; - - if (!elem || !elem.tagName) { - return ''; - } - - out.push(elem.tagName.toLowerCase()); - if (elem.id) { - out.push(`#${elem.id}`); - } - - // eslint-disable-next-line prefer-const - className = elem.className; - if (className && isString(className)) { - classes = className.split(/\s+/); - for (i = 0; i < classes.length; i++) { - out.push(`.${classes[i]}`); - } - } - const allowedAttrs = ['type', 'name', 'title', 'alt']; - for (i = 0; i < allowedAttrs.length; i++) { - key = allowedAttrs[i]; - attr = elem.getAttribute(key); - if (attr) { - out.push(`[${key}="${attr}"]`); - } - } - return out.join(''); -} - const INITIAL_TIME = Date.now(); let prevNow = 0; @@ -471,24 +353,6 @@ export function parseRetryAfterHeader(now: number, header?: string | number | nu return defaultRetryAfter; } -const defaultFunctionName = ''; - -/** - * Safely extract function name from itself - */ -export function getFunctionName(fn: unknown): string { - try { - if (!fn || typeof fn !== 'function') { - return defaultFunctionName; - } - return fn.name || defaultFunctionName; - } catch (e) { - // Just accessing custom props in some Selenium environments - // can cause a "Permission denied" exception (see raven-js#495). - return defaultFunctionName; - } -} - /** * This function adds context (pre/post/line) lines to the provided frame * @@ -530,98 +394,3 @@ export function stripUrlQueryAndFragment(urlPath: string): string { export function hasTracingEnabled(options: Options): boolean { return !!options.tracesSampleRate || !!options.tracesSampler; } - -/** Default request keys that'll be used to extract data from the request */ -const DEFAULT_REQUEST_KEYS = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; - -/** - * Normalizes data from the request object, accounting for framework differences. - * - * @param req The request object from which to extract data - * @param keys An optional array of keys to include in the normalized data. Defaults to DEFAULT_REQUEST_KEYS if not - * provided. - * @returns An object containing normalized request data - */ -export function extractNodeRequestData( - req: { [key: string]: any }, - keys: string[] = DEFAULT_REQUEST_KEYS, -): { [key: string]: string } { - // make sure we can safely use dynamicRequire below - if (!isNodeEnv()) { - throw new Error("Can't get node request data outside of a node environment"); - } - - const requestData: { [key: string]: any } = {}; - - // headers: - // node, express: req.headers - // koa: req.header - const headers = (req.headers || req.header || {}) as { - host?: string; - cookie?: string; - }; - // method: - // node, express, koa: req.method - const method = req.method; - // host: - // express: req.hostname in > 4 and req.host in < 4 - // koa: req.host - // node: req.headers.host - const host = req.hostname || req.host || headers.host || ''; - // protocol: - // node: - // express, koa: req.protocol - const protocol = - req.protocol === 'https' || req.secure || ((req.socket || {}) as { encrypted?: boolean }).encrypted - ? 'https' - : 'http'; - // url (including path and query string): - // node, express: req.originalUrl - // koa: req.url - const originalUrl = (req.originalUrl || req.url) as string; - // absolute url - const absoluteUrl = `${protocol}://${host}${originalUrl}`; - - keys.forEach(key => { - switch (key) { - case 'headers': - requestData.headers = headers; - break; - case 'method': - requestData.method = method; - break; - case 'url': - requestData.url = absoluteUrl; - break; - case 'cookies': - // cookies: - // node, express, koa: req.headers.cookie - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - requestData.cookies = dynamicRequire(module, 'cookie').parse(headers.cookie || ''); - break; - case 'query_string': - // query string: - // node: req.url (raw) - // express, koa: req.query - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - requestData.query_string = dynamicRequire(module, 'url').parse(originalUrl || '', false).query; - break; - case 'data': - if (method === 'GET' || method === 'HEAD') { - break; - } - // body data: - // node, express, koa: req.body - if (req.body !== undefined) { - requestData.data = isString(req.body) ? req.body : JSON.stringify(normalize(req.body)); - } - break; - default: - if ({}.hasOwnProperty.call(req, key)) { - requestData[key] = (req as { [key: string]: any })[key]; - } - } - }); - - return requestData; -} diff --git a/packages/utils/src/node.ts b/packages/utils/src/node.ts new file mode 100644 index 000000000000..d720dcaa14e2 --- /dev/null +++ b/packages/utils/src/node.ts @@ -0,0 +1,118 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { isString } from './is'; +import { normalize } from './object'; + +/** + * Checks whether we're in the Node.js or Browser environment + * + * @returns Answer to given question + */ +export function isNodeEnv(): boolean { + return Object.prototype.toString.call(typeof process !== 'undefined' ? process : 0) === '[object process]'; +} + +/** + * Requires a module which is protected against bundler minification. + * + * @param request The module path to resolve + */ +// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types +export function dynamicRequire(mod: any, request: string): any { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + return mod.require(request); +} + +/** Default request keys that'll be used to extract data from the request */ +const DEFAULT_REQUEST_KEYS = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; + +/** + * Normalizes data from the request object, accounting for framework differences. + * + * @param req The request object from which to extract data + * @param keys An optional array of keys to include in the normalized data. Defaults to DEFAULT_REQUEST_KEYS if not + * provided. + * @returns An object containing normalized request data + */ +export function extractNodeRequestData( + req: { [key: string]: any }, + keys: string[] = DEFAULT_REQUEST_KEYS, +): { [key: string]: string } { + // make sure we can safely use dynamicRequire below + if (!isNodeEnv()) { + throw new Error("Can't get node request data outside of a node environment"); + } + + const requestData: { [key: string]: any } = {}; + + // headers: + // node, express: req.headers + // koa: req.header + const headers = (req.headers || req.header || {}) as { + host?: string; + cookie?: string; + }; + // method: + // node, express, koa: req.method + const method = req.method; + // host: + // express: req.hostname in > 4 and req.host in < 4 + // koa: req.host + // node: req.headers.host + const host = req.hostname || req.host || headers.host || ''; + // protocol: + // node: + // express, koa: req.protocol + const protocol = + req.protocol === 'https' || req.secure || ((req.socket || {}) as { encrypted?: boolean }).encrypted + ? 'https' + : 'http'; + // url (including path and query string): + // node, express: req.originalUrl + // koa: req.url + const originalUrl = (req.originalUrl || req.url) as string; + // absolute url + const absoluteUrl = `${protocol}://${host}${originalUrl}`; + + keys.forEach(key => { + switch (key) { + case 'headers': + requestData.headers = headers; + break; + case 'method': + requestData.method = method; + break; + case 'url': + requestData.url = absoluteUrl; + break; + case 'cookies': + // cookies: + // node, express, koa: req.headers.cookie + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + requestData.cookies = dynamicRequire(module, 'cookie').parse(headers.cookie || ''); + break; + case 'query_string': + // query string: + // node: req.url (raw) + // express, koa: req.query + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + requestData.query_string = dynamicRequire(module, 'url').parse(originalUrl || '', false).query; + break; + case 'data': + if (method === 'GET' || method === 'HEAD') { + break; + } + // body data: + // node, express, koa: req.body + if (req.body !== undefined) { + requestData.data = isString(req.body) ? req.body : JSON.stringify(normalize(req.body)); + } + break; + default: + if ({}.hasOwnProperty.call(req, key)) { + requestData[key] = (req as { [key: string]: any })[key]; + } + } + }); + + return requestData; +} diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts index 02e5ec7bb1e4..f1b6cc88dca1 100644 --- a/packages/utils/src/object.ts +++ b/packages/utils/src/object.ts @@ -1,9 +1,10 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import { ExtendedError, WrappedFunction } from '@sentry/types'; +import { htmlTreeAsString } from './browser'; import { isElement, isError, isEvent, isInstanceOf, isPlainObject, isPrimitive, isSyntheticEvent } from './is'; import { Memo } from './memo'; -import { getFunctionName, htmlTreeAsString } from './misc'; +import { getFunctionName } from './stacktrace'; import { truncate } from './string'; /** diff --git a/packages/utils/src/stacktrace.ts b/packages/utils/src/stacktrace.ts new file mode 100644 index 000000000000..3ee2344d2dd4 --- /dev/null +++ b/packages/utils/src/stacktrace.ts @@ -0,0 +1,17 @@ +const defaultFunctionName = ''; + +/** + * Safely extract function name from itself + */ +export function getFunctionName(fn: unknown): string { + try { + if (!fn || typeof fn !== 'function') { + return defaultFunctionName; + } + return fn.name || defaultFunctionName; + } catch (e) { + // Just accessing custom props in some Selenium environments + // can cause a "Permission denied" exception (see raven-js#495). + return defaultFunctionName; + } +} From 8252e36a367507bd3a6490245c2eb2017d0a9a75 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 Aug 2020 17:15:15 -0700 Subject: [PATCH 17/40] random cleanup --- packages/tracing/src/hubextensions.ts | 20 ++++++++++++-------- packages/tracing/src/transaction.ts | 1 + packages/types/src/options.ts | 2 +- packages/types/src/transaction.ts | 4 ++-- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index e303823bdb4d..da85ce77539f 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -104,18 +104,22 @@ function getDefaultSampleContext(): SampleContext { const defaultSampleContext: SampleContext = {}; if (isNodeEnv()) { - // look at koa TODO const domain = getActiveDomain(); - const nodeHttpModule = dynamicRequire(module, 'http'); - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const requestType = nodeHttpModule.IncomingMessage; + if (domain) { - // TODO is the below true? // for all node servers that we currently support, we store the incoming request object (which is an instance of - // http.IncomingMessage) on the domain the domain is stored as an array, so our only way to find the request is to - // iterate through the array and compare types - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any + // http.IncomingMessage) on the domain + + // the domain members are stored as an array, so our only way to find the request is to iterate through the array + // and compare types + + const nodeHttpModule = dynamicRequire(module, 'http'); + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const requestType = nodeHttpModule.IncomingMessage; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any const request = domain.members.find((member: any) => isInstanceOf(member, requestType)); + if (request) { defaultSampleContext.request = extractNodeRequestData(request); } diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index 61c5033094e6..997adb3b1ccf 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -68,6 +68,7 @@ export class Transaction extends SpanClass { this.name = ''; } + // just sets the end timestamp super.finish(endTimestamp); if (this.sampled !== true) { diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index c107f6173400..807ac95da5ca 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -157,5 +157,5 @@ export interface Options { * * @returns A sample rate between 0 and 1 (0 drops the trace, 1 guarantees it will be sent). */ - tracesSampler?(traceContext: SampleContext): number; + tracesSampler?(sampleContext: SampleContext): number; } diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index d5d830eebcff..6c376fd0cfbb 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -51,8 +51,8 @@ export interface Transaction extends TransactionContext, Span { /** * The data passed to the `tracesSampler` function, which forms the basis for whatever decisions it might make. + * Combination of default values (which differ per SDK/integration) and data passed to `startTransaction`. */ export interface SampleContext { - [key: string]: string; - // TODO (kmclb) fill this out + [key: string]: any; } From c3e4b6051e4140f1ec901b2fc3032124a1324053 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 Aug 2020 17:15:42 -0700 Subject: [PATCH 18/40] add tests for sampling --- packages/tracing/test/hub.test.ts | 249 ++++++++++++++++++++++++++---- 1 file changed, 219 insertions(+), 30 deletions(-) diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 017be5a5b898..34980e83d64d 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -1,56 +1,245 @@ import { BrowserClient } from '@sentry/browser'; -import { Hub } from '@sentry/hub'; +import * as hubModuleRaw from '@sentry/hub'; // for mocking +import { getMainCarrier, Hub } from '@sentry/hub'; +import { NodeClient } from '@sentry/node'; +import * as utilsModule from '@sentry/utils'; // for mocking +import { getGlobalObject, isNodeEnv, logger } from '@sentry/utils'; +import * as nodeHttpModule from 'http'; import { addExtensionMethods } from '../src/hubextensions'; +// Do this once so that we'll be able to spy on hub methods later. If this isn't done, it results in "TypeError: Cannot +// set property of # which has only a getter." This just converts the module object +// (which has no setters) to a regular object (with regular properties which can be gotten or set). See +// https://stackoverflow.com/a/53307822/. + +// (This doesn't affect the utils module because it uses `export * from './myModule' syntax rather than `export +// {} from './myModule'` syntax in its index.ts. Only *named* exports seem to trigger the +// problem.) +const hubModule = { ...hubModuleRaw} + addExtensionMethods(); describe('Hub', () => { + beforeEach(() => { + jest.spyOn(logger, 'warn'); + jest.spyOn(logger, 'log'); + jest.spyOn(utilsModule, 'isNodeEnv'); + + // NB: Upon refactoring, this spy was no longer needed. Leaving it in as an excuse to leave in the note above, so + // that it can save future folks the headache. + jest.spyOn(hubModule, 'getActiveDomain'); + }); + afterEach(() => { - jest.resetAllMocks(); + jest.restoreAllMocks(); jest.useRealTimers(); }); - describe('getTransaction', () => { - test('simple invoke', () => { + describe('getTransaction()', () => { + + it('should find a transaction which has been set on the scope', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); - const transaction = hub.startTransaction({ name: 'foo' }); + const transaction = hub.startTransaction({ name: 'dogpark' }); hub.configureScope(scope => { scope.setSpan(transaction); }); - hub.configureScope(s => { - expect(s.getTransaction()).toBe(transaction); - }); + + expect(hub.getScope()?.getTransaction()).toBe(transaction) + }); - test('not invoke', () => { + it("should not find an open transaction if it's not on the scope", () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); - const transaction = hub.startTransaction({ name: 'foo' }); - hub.configureScope(s => { - expect(s.getTransaction()).toBeUndefined(); - }); - transaction.finish(); + hub.startTransaction({ name: 'dogpark' }); + + expect(hub.getScope()?.getTransaction()).toBeUndefined() }); - }); + }); // end describe('getTransaction()') + + describe('transaction sampling', () => { + describe('options', () => { + + it("should call tracesSampler if it's defined", () => { + const tracesSampler = jest.fn(); + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); + + expect(tracesSampler).toHaveBeenCalled(); + }); + + it('should prefer tracesSampler to tracesSampleRate', () => { + const tracesSampler = jest.fn(); + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1, tracesSampler: tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); + + expect(tracesSampler).toHaveBeenCalled(); + }); + + }); // end describe('options') + + describe('default sample context', () => { + + it('should extract request data for default sampling context when in node', () => { + // make sure we look like we're in node + (isNodeEnv as jest.Mock).mockReturnValue(true); + + // pre-normalization request object + const mockRequestObject = ({ + headers: { ears: 'furry', nose: 'wet', tongue: 'panting', cookie: 'favorite=zukes' }, + method: 'wagging', + protocol: 'mutualsniffing', + hostname: 'the.dog.park', + originalUrl: '/by/the/trees/?chase=me&please=thankyou', + } as unknown) as nodeHttpModule.IncomingMessage; + + // The "as unknown as nodeHttpModule.IncomingMessage" casting above keeps TS happy, but doesn't actually mean that + // mockRequestObject IS an instance of our desired class. Fix that so that when we search for it by type, we + // actually find it. + Object.setPrototypeOf(mockRequestObject, nodeHttpModule.IncomingMessage.prototype); + + // in production, the domain will have at minimum the request and the response, so make a response object to prove + // that our code identifying the request in domain.members works + const mockResponseObject = new nodeHttpModule.ServerResponse(mockRequestObject); + + // normally the node request handler does this, but that's not part of this test + (getMainCarrier().__SENTRY__!.extensions as any).domain = { + active: { members: [mockRequestObject, mockResponseObject] }, + }; + + const tracesSampler = jest.fn(); + const hub = new Hub(new NodeClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); + + // post-normalization request object + expect(tracesSampler).toHaveBeenCalledWith(expect.objectContaining({ + request: { + headers: { ears: 'furry', nose: 'wet', tongue: 'panting', cookie: 'favorite=zukes' }, + method: 'wagging', + url: 'http://the.dog.park/by/the/trees/?chase=me&please=thankyou', + cookies: { favorite: 'zukes' }, + query_string: 'chase=me&please=thankyou', + }, + })); + }); + + it('should extract window.location/self.location for default sampling context when in browser/service worker', () => { + // make sure we look like we're in the browser + (isNodeEnv as jest.Mock).mockReturnValue(false); + + const dogParkLocation = { + hash: '#next-to-the-fountain', + host: 'the.dog.park', + hostname: 'the.dog.park', + href: 'mutualsniffing://the.dog.park/by/the/trees/?chase=me&please=thankyou#next-to-the-fountain', + origin: "'mutualsniffing://the.dog.park", + pathname: '/by/the/trees/', + port: '', + protocol: 'mutualsniffing:', + search: '?chase=me&please=thankyou', + }; + + getGlobalObject().location = dogParkLocation as any; + + const tracesSampler = jest.fn(); + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); + + expect(tracesSampler).toHaveBeenCalledWith(expect.objectContaining({ location: dogParkLocation })); + }); + }); // end describe('defaultSampleContext') + + describe('while sampling', () => { - describe('spans', () => { - describe('sampling', () => { - test('set tracesSampleRate 0 on transaction', () => { + it('should not sample transactions when tracing is disabled', () => { + // neither tracesSampleRate nor tracesSampler is defined -> tracing disabled + const hub = new Hub(new BrowserClient({})); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + expect(transaction.sampled).toBe(false); + }); + + it('should not sample transactions when tracesSampleRate is 0', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); - const transaction = hub.startTransaction({ name: 'foo' }); + const transaction = hub.startTransaction({ name: 'dogpark' }); + expect(transaction.sampled).toBe(false); }); - test('set tracesSampleRate 1 on transaction', () => { + + it('should sample transactions when tracesSampleRate is 1', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); - const transaction = hub.startTransaction({ name: 'foo' }); - expect(transaction.sampled).toBeTruthy(); - }); - test('set tracesSampleRate should be propergated to children', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); - const transaction = hub.startTransaction({ name: 'foo' }); - const child = transaction.startChild({ op: 'test' }); - expect(child.sampled).toBeFalsy(); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + expect(transaction.sampled).toBe(true); }); + + it("should reject tracesSampleRates which aren't numbers", () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 'dogs!' as any })); + hub.startTransaction({ name: 'dogpark' }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a number')); }); - }); -}); + + it('should reject tracesSampleRates less than 0', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: -26 })); + hub.startTransaction({ name: 'dogpark' }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); + }); + + it('should reject tracesSampleRates greater than 1', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 26 })); + hub.startTransaction({ name: 'dogpark' }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); + }); + + it("should reject tracesSampler return values which aren't numbers", () => { + const tracesSampler = jest.fn().mockReturnValue("dogs!") + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a number')); + }); + + it('should reject tracesSampler return values less than 0', () => { + const tracesSampler = jest.fn().mockReturnValue(-12) + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); + }); + + it('should reject tracesSampler return values greater than 1', () => { + const tracesSampler = jest.fn().mockReturnValue(31) + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); + }); + }); // end describe('while sampling') + + it('should propagate sampling decision to child spans', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); + const transaction = hub.startTransaction({ name: 'dogpark' }); + const child = transaction.startChild({ op: 'test' }); + + expect(child.sampled).toBe(false); + }); + + it('should drop transactions with sampled = false', () => { + const client = new BrowserClient({ tracesSampleRate: 0 }) + jest.spyOn(client, 'captureEvent') + + const hub = new Hub(client); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + jest.spyOn(transaction, 'finish') + transaction.finish() + + expect(transaction.sampled).toBe(false); + expect(transaction.finish).toReturnWith(undefined); + expect(client.captureEvent).not.toBeCalled() + }); + }); // end describe('transaction sampling') +}); // end describe('Hub') From 45823542d81b2de5ef31630850433d6bf12390c6 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 Aug 2020 21:52:23 -0700 Subject: [PATCH 19/40] fix circular package dependency --- packages/tracing/package.json | 1 - packages/tracing/test/hub.test.ts | 7 +++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/tracing/package.json b/packages/tracing/package.json index 3709acd1e8a5..2376bf177705 100644 --- a/packages/tracing/package.json +++ b/packages/tracing/package.json @@ -25,7 +25,6 @@ "devDependencies": { "@sentry-internal/eslint-config-sdk": "5.23.0", "@sentry/browser": "5.23.0", - "@sentry/node": "5.22.3", "@types/express": "^4.17.1", "@types/jsdom": "^16.2.3", "eslint": "7.6.0", diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 34980e83d64d..f09b6ec6a590 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -1,7 +1,6 @@ import { BrowserClient } from '@sentry/browser'; import * as hubModuleRaw from '@sentry/hub'; // for mocking import { getMainCarrier, Hub } from '@sentry/hub'; -import { NodeClient } from '@sentry/node'; import * as utilsModule from '@sentry/utils'; // for mocking import { getGlobalObject, isNodeEnv, logger } from '@sentry/utils'; import * as nodeHttpModule from 'http'; @@ -107,8 +106,12 @@ describe('Hub', () => { active: { members: [mockRequestObject, mockResponseObject] }, }; + // Ideally we'd use a NodeClient here, but @sentry/tracing can't depend on @sentry/node since the reverse is + // already true (node's request handlers start their own transactions) - even as a dev dependency. Fortunately, + // we're not relying on anything other than the client having a captureEvent method, which all clients do (it's + // in the abstract base class), so a BrowserClient will do. const tracesSampler = jest.fn(); - const hub = new Hub(new NodeClient({ tracesSampler })); + const hub = new Hub(new BrowserClient({ tracesSampler })); hub.startTransaction({ name: 'dogpark' }); // post-normalization request object From c9d77ec81634c089235220b48852e244a4eb3a2f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 25 Aug 2020 22:22:40 -0700 Subject: [PATCH 20/40] fix gatsby test --- packages/gatsby/test/gatsby-browser.test.ts | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/packages/gatsby/test/gatsby-browser.test.ts b/packages/gatsby/test/gatsby-browser.test.ts index bef0b169652d..ba83b407002f 100644 --- a/packages/gatsby/test/gatsby-browser.test.ts +++ b/packages/gatsby/test/gatsby-browser.test.ts @@ -53,7 +53,6 @@ describe('onClientEntry', () => { environment: process.env.NODE_ENV, integrations: [], release: (global as any).__SENTRY_RELEASE__, - tracesSampleRate: 0, }); }); @@ -100,6 +99,16 @@ describe('onClientEntry', () => { ); }); + it('sets a tracesSampler if defined as option', () => { + const tracesSampler = jest.fn(); + onClientEntry(undefined, { tracesSampler }); + expect(sentryInit).toHaveBeenLastCalledWith( + expect.objectContaining({ + tracesSampler, + }), + ); + }); + it('adds `BrowserTracing` integration if tracesSampleRate is defined', () => { onClientEntry(undefined, { tracesSampleRate: 0.5 }); expect(sentryInit).toHaveBeenLastCalledWith( @@ -109,6 +118,16 @@ describe('onClientEntry', () => { ); }); + it('adds `BrowserTracing` integration if tracesSampler is defined', () => { + const tracesSampler = jest.fn(); + onClientEntry(undefined, { tracesSampler }); + expect(sentryInit).toHaveBeenLastCalledWith( + expect.objectContaining({ + integrations: [expect.objectContaining({ name: 'BrowserTracing' })], + }), + ); + }); + it('only defines a single `BrowserTracing` integration', () => { const Tracing = jest.requireActual('@sentry/tracing'); const integrations = [new Tracing.Integrations.BrowserTracing()]; From e784f4b8d5db4f949841bf800b4d610890c2b9e0 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 10:57:56 -0700 Subject: [PATCH 21/40] fix linting errors --- packages/tracing/test/hub.test.ts | 111 +++++++++++++++--------------- 1 file changed, 54 insertions(+), 57 deletions(-) diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index f09b6ec6a590..efc4d6356ea8 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable @typescript-eslint/unbound-method */ import { BrowserClient } from '@sentry/browser'; import * as hubModuleRaw from '@sentry/hub'; // for mocking import { getMainCarrier, Hub } from '@sentry/hub'; @@ -15,7 +16,7 @@ import { addExtensionMethods } from '../src/hubextensions'; // (This doesn't affect the utils module because it uses `export * from './myModule' syntax rather than `export // {} from './myModule'` syntax in its index.ts. Only *named* exports seem to trigger the // problem.) -const hubModule = { ...hubModuleRaw} +const hubModule = { ...hubModuleRaw }; addExtensionMethods(); @@ -36,7 +37,6 @@ describe('Hub', () => { }); describe('getTransaction()', () => { - it('should find a transaction which has been set on the scope', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -44,21 +44,19 @@ describe('Hub', () => { scope.setSpan(transaction); }); - expect(hub.getScope()?.getTransaction()).toBe(transaction) - + expect(hub.getScope()?.getTransaction()).toBe(transaction); }); it("should not find an open transaction if it's not on the scope", () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); hub.startTransaction({ name: 'dogpark' }); - expect(hub.getScope()?.getTransaction()).toBeUndefined() + expect(hub.getScope()?.getTransaction()).toBeUndefined(); }); }); // end describe('getTransaction()') describe('transaction sampling', () => { describe('options', () => { - it("should call tracesSampler if it's defined", () => { const tracesSampler = jest.fn(); const hub = new Hub(new BrowserClient({ tracesSampler })); @@ -74,11 +72,9 @@ describe('Hub', () => { expect(tracesSampler).toHaveBeenCalled(); }); - }); // end describe('options') describe('default sample context', () => { - it('should extract request data for default sampling context when in node', () => { // make sure we look like we're in node (isNodeEnv as jest.Mock).mockReturnValue(true); @@ -115,15 +111,17 @@ describe('Hub', () => { hub.startTransaction({ name: 'dogpark' }); // post-normalization request object - expect(tracesSampler).toHaveBeenCalledWith(expect.objectContaining({ - request: { - headers: { ears: 'furry', nose: 'wet', tongue: 'panting', cookie: 'favorite=zukes' }, - method: 'wagging', - url: 'http://the.dog.park/by/the/trees/?chase=me&please=thankyou', - cookies: { favorite: 'zukes' }, - query_string: 'chase=me&please=thankyou', - }, - })); + expect(tracesSampler).toHaveBeenCalledWith( + expect.objectContaining({ + request: { + headers: { ears: 'furry', nose: 'wet', tongue: 'panting', cookie: 'favorite=zukes' }, + method: 'wagging', + url: 'http://the.dog.park/by/the/trees/?chase=me&please=thankyou', + cookies: { favorite: 'zukes' }, + query_string: 'chase=me&please=thankyou', + }, + }), + ); }); it('should extract window.location/self.location for default sampling context when in browser/service worker', () => { @@ -153,7 +151,6 @@ describe('Hub', () => { }); // end describe('defaultSampleContext') describe('while sampling', () => { - it('should not sample transactions when tracing is disabled', () => { // neither tracesSampleRate nor tracesSampler is defined -> tracing disabled const hub = new Hub(new BrowserClient({})); @@ -176,51 +173,51 @@ describe('Hub', () => { expect(transaction.sampled).toBe(true); }); - it("should reject tracesSampleRates which aren't numbers", () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 'dogs!' as any })); - hub.startTransaction({ name: 'dogpark' }); + it("should reject tracesSampleRates which aren't numbers", () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 'dogs!' as any })); + hub.startTransaction({ name: 'dogpark' }); - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a number')); - }); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a number')); + }); - it('should reject tracesSampleRates less than 0', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: -26 })); - hub.startTransaction({ name: 'dogpark' }); + it('should reject tracesSampleRates less than 0', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: -26 })); + hub.startTransaction({ name: 'dogpark' }); - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); - }); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); + }); - it('should reject tracesSampleRates greater than 1', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 26 })); - hub.startTransaction({ name: 'dogpark' }); + it('should reject tracesSampleRates greater than 1', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 26 })); + hub.startTransaction({ name: 'dogpark' }); - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); - }); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); + }); - it("should reject tracesSampler return values which aren't numbers", () => { - const tracesSampler = jest.fn().mockReturnValue("dogs!") - const hub = new Hub(new BrowserClient({ tracesSampler })); - hub.startTransaction({ name: 'dogpark' }); + it("should reject tracesSampler return values which aren't numbers", () => { + const tracesSampler = jest.fn().mockReturnValue('dogs!'); + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a number')); - }); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a number')); + }); - it('should reject tracesSampler return values less than 0', () => { - const tracesSampler = jest.fn().mockReturnValue(-12) - const hub = new Hub(new BrowserClient({ tracesSampler })); - hub.startTransaction({ name: 'dogpark' }); + it('should reject tracesSampler return values less than 0', () => { + const tracesSampler = jest.fn().mockReturnValue(-12); + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); - }); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); + }); - it('should reject tracesSampler return values greater than 1', () => { - const tracesSampler = jest.fn().mockReturnValue(31) - const hub = new Hub(new BrowserClient({ tracesSampler })); - hub.startTransaction({ name: 'dogpark' }); + it('should reject tracesSampler return values greater than 1', () => { + const tracesSampler = jest.fn().mockReturnValue(31); + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); - }); - }); // end describe('while sampling') + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); + }); + }); // end describe('while sampling') it('should propagate sampling decision to child spans', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); @@ -231,18 +228,18 @@ describe('Hub', () => { }); it('should drop transactions with sampled = false', () => { - const client = new BrowserClient({ tracesSampleRate: 0 }) - jest.spyOn(client, 'captureEvent') + const client = new BrowserClient({ tracesSampleRate: 0 }); + jest.spyOn(client, 'captureEvent'); const hub = new Hub(client); const transaction = hub.startTransaction({ name: 'dogpark' }); - jest.spyOn(transaction, 'finish') - transaction.finish() + jest.spyOn(transaction, 'finish'); + transaction.finish(); expect(transaction.sampled).toBe(false); expect(transaction.finish).toReturnWith(undefined); - expect(client.captureEvent).not.toBeCalled() + expect(client.captureEvent).not.toBeCalled(); }); }); // end describe('transaction sampling') }); // end describe('Hub') From 669428cc61bfe1921cfc4434855cbba17b457ac8 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 12:25:44 -0700 Subject: [PATCH 22/40] fix type issues on global object --- packages/tracing/src/hubextensions.ts | 5 +++-- packages/tracing/test/hub.test.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index da85ce77539f..ac9a3f03b642 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -129,12 +129,13 @@ function getDefaultSampleContext(): SampleContext { // we must be in browser-js (or some derivative thereof) else { // we use `getGlobalObject()` rather than `window` since service workers also have a `location` property on `self` - const globalObject = getGlobalObject(); + const globalObject = getGlobalObject(); if ('location' in globalObject) { // we take a copy of the location object rather than just a reference to it in case there's a navigation or // redirect in the instant between when the transaction starts and when the sampler is called - defaultSampleContext.location = { ...globalObject.location }; + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any + defaultSampleContext.location = { ...(globalObject as any).location }; } } diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index efc4d6356ea8..a60e928507c0 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -140,7 +140,7 @@ describe('Hub', () => { search: '?chase=me&please=thankyou', }; - getGlobalObject().location = dogParkLocation as any; + getGlobalObject().location = dogParkLocation as any; const tracesSampler = jest.fn(); const hub = new Hub(new BrowserClient({ tracesSampler })); From 6fb10c377ec78db57030a8fc6700a48a6eed674d Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 12:28:36 -0700 Subject: [PATCH 23/40] add domain to extensions interface and active to domain interface --- packages/hub/src/hub.ts | 3 +-- packages/hub/src/interfaces.ts | 18 +++++++++++++++--- packages/node/src/index.ts | 9 +++------ 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index f42b214fe9a2..02fe999d60e9 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -465,8 +465,7 @@ export function getCurrentHub(): Hub { export function getActiveDomain(): DomainAsCarrier | undefined { const sentry = getMainCarrier().__SENTRY__; - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any - return sentry && sentry.extensions && sentry.extensions.domain && (sentry.extensions.domain as any).active; + return sentry && sentry.extensions && sentry.extensions.domain && sentry.extensions.domain.active; } /** diff --git a/packages/hub/src/interfaces.ts b/packages/hub/src/interfaces.ts index 7647e4278454..ddaf71c80c06 100644 --- a/packages/hub/src/interfaces.ts +++ b/packages/hub/src/interfaces.ts @@ -21,10 +21,22 @@ export interface Carrier { __SENTRY__?: { hub?: Hub; /** - * These are extension methods for the hub, the current instance of the hub will be bound to it + * Extra Hub properties injected by various SDKs */ - // eslint-disable-next-line @typescript-eslint/ban-types - extensions?: { [key: string]: Function }; + extensions?: { + /** Hack to prevent bundlers from breaking our usage of the domain package in the cross-platform Hub package */ + domain?: typeof domain & { + /** + * The currently active domain. This is part of the domain package, but for some reason not declared in the + * package's typedef. + */ + active?: domain.Domain; + }; + } & { + /** Extension methods for the hub, which are bound to the current Hub instance */ + // eslint-disable-next-line @typescript-eslint/ban-types + [key: string]: Function; + }; }; } diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index b8678705fb66..6429ce3a75c9 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -57,13 +57,10 @@ const INTEGRATIONS = { export { INTEGRATIONS as Integrations, Transports, Handlers }; -// We need to patch domain on the global __SENTRY__ object to make it work for node -// if we don't do this, browser bundlers will have troubles resolving require('domain') +// We need to patch domain on the global __SENTRY__ object to make it work for node in cross-platform packages like +// @sentry/hub. If we don't do this, browser bundlers will have troubles resolving `require('domain')`. const carrier = getMainCarrier(); if (carrier.__SENTRY__) { carrier.__SENTRY__.extensions = carrier.__SENTRY__.extensions || {}; - if (!carrier.__SENTRY__.extensions.domain) { - // @ts-ignore domain is missing from extensions Type - carrier.__SENTRY__.extensions.domain = domain; - } + carrier.__SENTRY__.extensions.domain = carrier.__SENTRY__.extensions.domain || domain; } From 2853c8f7f513aeb6e659b4cfe033d5cabe0e37f9 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 14:15:17 -0700 Subject: [PATCH 24/40] move tracing enablement check to tracing package --- packages/gatsby/gatsby-browser.js | 3 +-- packages/gatsby/package.json | 3 +-- packages/tracing/src/hubextensions.ts | 2 +- packages/tracing/src/index.ts | 2 ++ packages/tracing/src/utils.ts | 10 ++++++++++ packages/utils/src/misc.ts | 10 +--------- 6 files changed, 16 insertions(+), 14 deletions(-) create mode 100644 packages/tracing/src/utils.ts diff --git a/packages/gatsby/gatsby-browser.js b/packages/gatsby/gatsby-browser.js index 79b0bc4bd2c1..42d5f606f2c8 100644 --- a/packages/gatsby/gatsby-browser.js +++ b/packages/gatsby/gatsby-browser.js @@ -1,6 +1,5 @@ const Sentry = require('@sentry/react'); const Tracing = require('@sentry/tracing'); -const Utils = require('@sentry/utils') exports.onClientEntry = function(_, pluginParams) { if (pluginParams === undefined) { @@ -9,7 +8,7 @@ exports.onClientEntry = function(_, pluginParams) { const integrations = [...(pluginParams.integrations || [])]; - if (Utils.hasTracingEnabled(pluginParams) && !integrations.some(ele => ele.name === 'BrowserTracing')) { + if (Tracing.hasTracingEnabled(pluginParams) && !integrations.some(ele => ele.name === 'BrowserTracing')) { integrations.push(new Tracing.Integrations.BrowserTracing(pluginParams.browserTracingOptions)); } diff --git a/packages/gatsby/package.json b/packages/gatsby/package.json index 8312bc06354d..8976dba30718 100644 --- a/packages/gatsby/package.json +++ b/packages/gatsby/package.json @@ -27,8 +27,7 @@ }, "dependencies": { "@sentry/react": "5.23.0", - "@sentry/tracing": "5.23.0", - "@sentry/utils": "5.23.0" + "@sentry/tracing": "5.23.0" }, "peerDependencies": { "gatsby": "*" diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index ac9a3f03b642..835bae90d7bd 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -4,7 +4,6 @@ import { dynamicRequire, extractNodeRequestData, getGlobalObject, - hasTracingEnabled, isInstanceOf, isNodeEnv, logger, @@ -13,6 +12,7 @@ import { import { registerErrorInstrumentation } from './errors'; import { IdleTransaction } from './idletransaction'; import { Transaction } from './transaction'; +import { hasTracingEnabled } from './utils'; /** Returns all trace headers that are currently on the top scope. */ function traceHeaders(this: Hub): { [key: string]: string } { diff --git a/packages/tracing/src/index.ts b/packages/tracing/src/index.ts index 457d94df8135..73981afceac5 100644 --- a/packages/tracing/src/index.ts +++ b/packages/tracing/src/index.ts @@ -14,3 +14,5 @@ export { SpanStatus } from './spanstatus'; addExtensionMethods(); export { addExtensionMethods }; + +export * from './utils'; diff --git a/packages/tracing/src/utils.ts b/packages/tracing/src/utils.ts new file mode 100644 index 000000000000..f8ed980b2643 --- /dev/null +++ b/packages/tracing/src/utils.ts @@ -0,0 +1,10 @@ +import { Options } from '@sentry/types'; + +/** + * Determines if tracing is currently enabled. + * + * Tracing is enabled when at least one of `tracesSampleRate` and `tracesSampler` is defined in the SDK config. + */ +export function hasTracingEnabled(options: Options): boolean { + return 'tracesSampleRate' in options || 'tracesSampler' in options; +} diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 8aa933fb52ba..090577781a85 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -1,5 +1,5 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { Event, Integration, Options, StackFrame, WrappedFunction } from '@sentry/types'; +import { Event, Integration, StackFrame, WrappedFunction } from '@sentry/types'; import { dynamicRequire, isNodeEnv } from './node'; import { snipLine } from './string'; @@ -385,12 +385,4 @@ export function addContextToFrame(lines: string[], frame: StackFrame, linesOfCon export function stripUrlQueryAndFragment(urlPath: string): string { // eslint-disable-next-line no-useless-escape return urlPath.split(/[\?#]/, 1)[0]; - -/** -* Determines if tracing is currently enabled. - * - * Tracing is enabled when at least one of `tracesSampleRate` and `tracesSampler` is defined in the SDK config. - */ -export function hasTracingEnabled(options: Options): boolean { - return !!options.tracesSampleRate || !!options.tracesSampler; } From 36b8b4ffbb2ca714434cf8649625f08a5317867e Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 14:15:42 -0700 Subject: [PATCH 25/40] remove end markers on describe blocks --- packages/tracing/test/hub.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index a60e928507c0..fa0f590a3716 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -53,7 +53,7 @@ describe('Hub', () => { expect(hub.getScope()?.getTransaction()).toBeUndefined(); }); - }); // end describe('getTransaction()') + }); describe('transaction sampling', () => { describe('options', () => { @@ -72,7 +72,7 @@ describe('Hub', () => { expect(tracesSampler).toHaveBeenCalled(); }); - }); // end describe('options') + }); describe('default sample context', () => { it('should extract request data for default sampling context when in node', () => { @@ -148,7 +148,7 @@ describe('Hub', () => { expect(tracesSampler).toHaveBeenCalledWith(expect.objectContaining({ location: dogParkLocation })); }); - }); // end describe('defaultSampleContext') + }); describe('while sampling', () => { it('should not sample transactions when tracing is disabled', () => { @@ -217,7 +217,7 @@ describe('Hub', () => { expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); }); - }); // end describe('while sampling') + }); it('should propagate sampling decision to child spans', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); @@ -241,5 +241,5 @@ describe('Hub', () => { expect(transaction.finish).toReturnWith(undefined); expect(client.captureEvent).not.toBeCalled(); }); - }); // end describe('transaction sampling') -}); // end describe('Hub') + }); +}); From 101f9d09076fd73c146b6094133abc31a825a621 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 14:31:20 -0700 Subject: [PATCH 26/40] remove note about weird mocking hack --- packages/tracing/test/hub.test.ts | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index fa0f590a3716..369bab21beea 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -1,6 +1,5 @@ /* eslint-disable @typescript-eslint/unbound-method */ import { BrowserClient } from '@sentry/browser'; -import * as hubModuleRaw from '@sentry/hub'; // for mocking import { getMainCarrier, Hub } from '@sentry/hub'; import * as utilsModule from '@sentry/utils'; // for mocking import { getGlobalObject, isNodeEnv, logger } from '@sentry/utils'; @@ -8,16 +7,6 @@ import * as nodeHttpModule from 'http'; import { addExtensionMethods } from '../src/hubextensions'; -// Do this once so that we'll be able to spy on hub methods later. If this isn't done, it results in "TypeError: Cannot -// set property of # which has only a getter." This just converts the module object -// (which has no setters) to a regular object (with regular properties which can be gotten or set). See -// https://stackoverflow.com/a/53307822/. - -// (This doesn't affect the utils module because it uses `export * from './myModule' syntax rather than `export -// {} from './myModule'` syntax in its index.ts. Only *named* exports seem to trigger the -// problem.) -const hubModule = { ...hubModuleRaw }; - addExtensionMethods(); describe('Hub', () => { @@ -25,10 +14,6 @@ describe('Hub', () => { jest.spyOn(logger, 'warn'); jest.spyOn(logger, 'log'); jest.spyOn(utilsModule, 'isNodeEnv'); - - // NB: Upon refactoring, this spy was no longer needed. Leaving it in as an excuse to leave in the note above, so - // that it can save future folks the headache. - jest.spyOn(hubModule, 'getActiveDomain'); }); afterEach(() => { From 85deeb69845fb7fab70d939e31cb2705b3a243ea Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 18:32:27 -0700 Subject: [PATCH 27/40] add specific types for default and custom sample context data --- packages/hub/src/hub.ts | 6 +-- packages/minimal/src/index.ts | 9 ++-- packages/tracing/src/hubextensions.ts | 16 ++++--- packages/types/src/hub.ts | 7 +-- packages/types/src/index.ts | 9 +++- packages/types/src/misc.ts | 61 +++++++++++++++++++++++++++ packages/types/src/transaction.ts | 23 ++++++++-- packages/utils/src/node.ts | 4 +- 8 files changed, 114 insertions(+), 21 deletions(-) create mode 100644 packages/types/src/misc.ts diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index 02fe999d60e9..f2ac47e2c4b6 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -3,6 +3,7 @@ import { Breadcrumb, BreadcrumbHint, Client, + CustomSampleContext, Event, EventHint, Extra, @@ -10,7 +11,6 @@ import { Hub as HubInterface, Integration, IntegrationClass, - SampleContext, Severity, Span, SpanContext, @@ -370,8 +370,8 @@ export class Hub implements HubInterface { /** * @inheritDoc */ - public startTransaction(context: TransactionContext, sampleContext?: SampleContext): Transaction { - return this._callExtensionMethod('startTransaction', context, sampleContext); + public startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction { + return this._callExtensionMethod('startTransaction', context, customSampleContext); } /** diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index acceb3e5a377..78f69d8dd5f0 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -2,10 +2,10 @@ import { getCurrentHub, Hub, Scope } from '@sentry/hub'; import { Breadcrumb, CaptureContext, + CustomSampleContext, Event, Extra, Extras, - SampleContext, Severity, Transaction, TransactionContext, @@ -206,8 +206,9 @@ export function _callOnClient(method: string, ...args: any[]): void { * Sentry. * * @param context Properties of the new `Transaction`. - * @param sampleContext Information given to the transaction sampling function + * @param customSampleContext Information given to the transaction sampling function (along with context-dependent + * default values) */ -export function startTransaction(context: TransactionContext, sampleContext?: SampleContext): Transaction { - return callOnHub('startTransaction', { ...context }, sampleContext); +export function startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction { + return callOnHub('startTransaction', { ...context }, customSampleContext); } diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 835bae90d7bd..d9f9287bc2b3 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -1,5 +1,5 @@ import { getActiveDomain, getMainCarrier, Hub } from '@sentry/hub'; -import { SampleContext, TransactionContext } from '@sentry/types'; +import { CustomSampleContext, DefaultSampleContext, SampleContext, TransactionContext } from '@sentry/types'; import { dynamicRequire, extractNodeRequestData, @@ -98,10 +98,10 @@ function sample(hub: Hub, transaction: T, sampleContext: /** * Gets the correct context to pass to the tracesSampler, based on the environment (i.e., which SDK is being used) * - * @returns A SampleContext object + * @returns The default sample context */ -function getDefaultSampleContext(): SampleContext { - const defaultSampleContext: SampleContext = {}; +function getDefaultSampleContext(): DefaultSampleContext { + const defaultSampleContext: DefaultSampleContext = {}; if (isNodeEnv()) { const domain = getActiveDomain(); @@ -171,9 +171,13 @@ function isValidSampleRate(rate: unknown): boolean { * * @see {@link Hub.startTransaction} */ -function _startTransaction(this: Hub, context: TransactionContext, sampleContext?: SampleContext): Transaction { +function _startTransaction( + this: Hub, + context: TransactionContext, + customSampleContext?: CustomSampleContext, +): Transaction { const transaction = new Transaction(context, this); - return sample(this, transaction, { ...getDefaultSampleContext(), ...sampleContext }); + return sample(this, transaction, { ...getDefaultSampleContext(), ...customSampleContext }); } /** diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 4d023d2cfa8c..4ff75eb4344d 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -6,7 +6,7 @@ import { Integration, IntegrationClass } from './integration'; import { Scope } from './scope'; import { Severity } from './severity'; import { Span, SpanContext } from './span'; -import { SampleContext, Transaction, TransactionContext } from './transaction'; +import { CustomSampleContext, Transaction, TransactionContext } from './transaction'; import { User } from './user'; /** @@ -198,7 +198,8 @@ export interface Hub { * Sentry. * * @param context Properties of the new `Transaction`. - * @param sampleContext Information given to the transaction sampling function + * @param customSampleContext Information given to the transaction sampling function (along with context-dependent + * default values) */ - startTransaction(context: TransactionContext, sampleContext?: SampleContext): Transaction; + startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction; } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 9f284415a1d9..6d7677fe02d8 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -10,6 +10,7 @@ export { Hub } from './hub'; export { Integration, IntegrationClass } from './integration'; export { LogLevel } from './loglevel'; export { Mechanism } from './mechanism'; +export { ExtractedNodeRequestData, WorkerLocation } from './misc'; export { Options } from './options'; export { Package } from './package'; export { Request } from './request'; @@ -22,7 +23,13 @@ export { Span, SpanContext } from './span'; export { StackFrame } from './stackframe'; export { Stacktrace } from './stacktrace'; export { Status } from './status'; -export { SampleContext, Transaction, TransactionContext } from './transaction'; +export { + CustomSampleContext, + DefaultSampleContext, + SampleContext, + Transaction, + TransactionContext, +} from './transaction'; export { Thread } from './thread'; export { Transport, TransportOptions, TransportClass } from './transport'; export { User } from './user'; diff --git a/packages/types/src/misc.ts b/packages/types/src/misc.ts new file mode 100644 index 000000000000..9e52b5aa6e22 --- /dev/null +++ b/packages/types/src/misc.ts @@ -0,0 +1,61 @@ +/** + * Data extracted from an incoming request to a node server + */ +export interface ExtractedNodeRequestData { + [key: string]: any; + + /** Specific headers from the request */ + headers?: { [key: string]: string }; + + /** The request's method */ + method?: string; + + /** The request's URL, including query string */ + url?: string; + + /** String representing the cookies sent along with the request */ + cookies?: { [key: string]: string }; + + /** The request's query string, without the leading '?' */ + query_string?: string; + + /** Any data sent in the request's body, as a JSON string */ + data?: string; +} + +/** + * Location object on a service worker's `self` object. + * + * See https://developer.mozilla.org/en-US/docs/Web/API/WorkerLocation. + */ +export interface WorkerLocation { + /** The protocol scheme of the URL of the script executed in the Worker, including the final ':'. */ + readonly protocol: string; + + /** The host, that is the hostname, a ':', and the port of the URL of the script executed in the Worker. */ + readonly host: string; + + /** The domain of the URL of the script executed in the Worker. */ + readonly hostname: string; + + /** The canonical form of the origin of the specific location. */ + readonly origin: string; + + /** The port number of the URL of the script executed in the Worker. */ + readonly port: string; + + /** The path of the URL of the script executed in the Worker, beginning with a '/'. */ + readonly pathname: string; + + /** The parameters (query string) of the URL of the script executed in the Worker, beginning with a '?'. */ + readonly search: string; + + /** The fragment identifier of the URL of the script executed in the Worker, beginning with a '#'. */ + readonly hash: string; + + /** Stringifier that returns the whole URL of the script executed in the Worker. */ + readonly href: string; + + /** Synonym for `href` attribute */ + toString(): string; +} diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 6c376fd0cfbb..6410a5145935 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -1,3 +1,4 @@ +import { ExtractedNodeRequestData, WorkerLocation } from './misc'; import { Span, SpanContext } from './span'; /** @@ -50,9 +51,25 @@ export interface Transaction extends TransactionContext, Span { } /** - * The data passed to the `tracesSampler` function, which forms the basis for whatever decisions it might make. - * Combination of default values (which differ per SDK/integration) and data passed to `startTransaction`. + * The context passed to the tracesSampler function by default. + */ +export interface DefaultSampleContext { + /** Object representing the URL of the current page or woker script. Passed in a browser or service worker context */ + location?: Location | WorkerLocation; + + /** Object representing the incoming request to a node server. Passed when in a node server context. */ + request?: ExtractedNodeRequestData; +} + +/** + * Context data passed by the user when starting a transaction, to be used by the tracesSampler method. */ -export interface SampleContext { +export interface CustomSampleContext { [key: string]: any; } + +/** + * The data passed to the `tracesSampler` function, which forms the basis for whatever decisions it might make. + * Combination of default values (which differ per SDK/integration) and data passed to `startTransaction`. + */ +export type SampleContext = DefaultSampleContext & CustomSampleContext; diff --git a/packages/utils/src/node.ts b/packages/utils/src/node.ts index d720dcaa14e2..1154568e45a7 100644 --- a/packages/utils/src/node.ts +++ b/packages/utils/src/node.ts @@ -1,4 +1,6 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ +import { ExtractedNodeRequestData } from '@sentry/types'; + import { isString } from './is'; import { normalize } from './object'; @@ -36,7 +38,7 @@ const DEFAULT_REQUEST_KEYS = ['cookies', 'data', 'headers', 'method', 'query_str export function extractNodeRequestData( req: { [key: string]: any }, keys: string[] = DEFAULT_REQUEST_KEYS, -): { [key: string]: string } { +): ExtractedNodeRequestData { // make sure we can safely use dynamicRequire below if (!isNodeEnv()) { throw new Error("Can't get node request data outside of a node environment"); From 64fde64cf122127542285d0ac9229d705777c5b2 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 18:33:05 -0700 Subject: [PATCH 28/40] don't pass custom context to startIdleTransaction since it's an internal method --- packages/tracing/src/hubextensions.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index d9f9287bc2b3..f8a88c608b28 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -188,10 +188,9 @@ export function startIdleTransaction( context: TransactionContext, idleTimeout?: number, onScope?: boolean, - sampleContext?: SampleContext, ): IdleTransaction { const transaction = new IdleTransaction(context, hub, idleTimeout, onScope); - return sample(hub, transaction, { ...getDefaultSampleContext(), ...sampleContext }); + return sample(hub, transaction, getDefaultSampleContext()); } /** From 573fe73a59a508bde70cf8a440f5c6af4a582474 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 19:13:55 -0700 Subject: [PATCH 29/40] move tracesSampleRate and tracesSampler to be closer in the Options typedef --- packages/types/src/options.ts | 46 +++++++++++++++++------------------ 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 807ac95da5ca..75425470af20 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -79,16 +79,6 @@ export interface Options { /** A global sample rate to apply to all events (0 - 1). */ sampleRate?: number; - /** - * Sample rate to determine trace sampling. - * - * 0.0 = 0% chance of a given trace being sent (send no traces) - * 1.0 = 100% chance of a given trace being sent (send all traces) - * - * Can be omitted without disabling tracing if `tracesSampler` is defined. If neither is defined, tracing is disabled. - */ - tracesSampleRate?: number; - /** Attaches stacktraces to pure capture message / log integrations */ attachStacktrace?: boolean; @@ -119,6 +109,29 @@ export interface Options { [key: string]: any; }; + /** + * Sample rate to determine trace sampling. + * + * 0.0 = 0% chance of a given trace being sent (send no traces) 1.0 = 100% chance of a given trace being sent (send + * all traces) + * + * Tracing is enabled if either this or `tracesSampler` is defined. If both are defined, `tracesSampleRate` is + * ignored. + */ + tracesSampleRate?: number; + + /** + * Function to compute tracing sample rate dynamically and filter unwanted traces. + * + * Tracing is enabled if either this or `tracesSampleRate` is defined. If both are defined, `tracesSampleRate` is + * ignored. + * + * Will automatically be passed a context object of default and optional custom data. See {@link Hub.startTransaction}. + * + * @returns A sample rate between 0 and 1 (0 drops the trace, 1 guarantees it will be sent). + */ + tracesSampler?(sampleContext: SampleContext): number; + /** * A callback invoked during event submission, allowing to optionally modify * the event before it is sent to Sentry. @@ -145,17 +158,4 @@ export interface Options { * @returns The breadcrumb that will be added | null. */ beforeBreadcrumb?(breadcrumb: Breadcrumb, hint?: BreadcrumbHint): Breadcrumb | null; - - /** - * Function to compute tracing sample rate dynamically and filter unwanted traces. - * - * Can be defined in place of `tracesSampleRate`. If neither is defined, tracing is disabled. - * - * `startTransaction` or `startIdleTransaction` accept a `sampleContext` argument, which is then passed to this - * function, meant to provide information on which to make a sampling decision. Additional information may be included - * automatically, depending on the SDK and any included integrations. - * - * @returns A sample rate between 0 and 1 (0 drops the trace, 1 guarantees it will be sent). - */ - tracesSampler?(sampleContext: SampleContext): number; } From b98bded090946dfe03943d55c61ea14c411cd704 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 1 Sep 2020 19:37:12 -0700 Subject: [PATCH 30/40] meld default data into main SampleContext type so it appears in type hints --- packages/minimal/src/index.ts | 2 +- packages/tracing/src/hubextensions.ts | 6 +++--- packages/types/src/hub.ts | 2 +- packages/types/src/index.ts | 8 +------- packages/types/src/options.ts | 3 ++- packages/types/src/transaction.ts | 29 ++++++++++++++------------- 6 files changed, 23 insertions(+), 27 deletions(-) diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 78f69d8dd5f0..cc41eca45e20 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -207,7 +207,7 @@ export function _callOnClient(method: string, ...args: any[]): void { * * @param context Properties of the new `Transaction`. * @param customSampleContext Information given to the transaction sampling function (along with context-dependent - * default values) + * default values). See {@link Options.tracesSampler}. */ export function startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction { return callOnHub('startTransaction', { ...context }, customSampleContext); diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index f8a88c608b28..c96fe60971f4 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -1,5 +1,5 @@ import { getActiveDomain, getMainCarrier, Hub } from '@sentry/hub'; -import { CustomSampleContext, DefaultSampleContext, SampleContext, TransactionContext } from '@sentry/types'; +import { CustomSampleContext, SampleContext, TransactionContext } from '@sentry/types'; import { dynamicRequire, extractNodeRequestData, @@ -100,8 +100,8 @@ function sample(hub: Hub, transaction: T, sampleContext: * * @returns The default sample context */ -function getDefaultSampleContext(): DefaultSampleContext { - const defaultSampleContext: DefaultSampleContext = {}; +function getDefaultSampleContext(): SampleContext { + const defaultSampleContext: SampleContext = {}; if (isNodeEnv()) { const domain = getActiveDomain(); diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index 4ff75eb4344d..c0dc4402bb95 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -199,7 +199,7 @@ export interface Hub { * * @param context Properties of the new `Transaction`. * @param customSampleContext Information given to the transaction sampling function (along with context-dependent - * default values) + * default values). See {@link Options.tracesSampler}. */ startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction; } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 6d7677fe02d8..cbb2e6559888 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -23,13 +23,7 @@ export { Span, SpanContext } from './span'; export { StackFrame } from './stackframe'; export { Stacktrace } from './stacktrace'; export { Status } from './status'; -export { - CustomSampleContext, - DefaultSampleContext, - SampleContext, - Transaction, - TransactionContext, -} from './transaction'; +export { CustomSampleContext, SampleContext, Transaction, TransactionContext } from './transaction'; export { Thread } from './thread'; export { Transport, TransportOptions, TransportClass } from './transport'; export { User } from './user'; diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 75425470af20..ff2e1e3baa61 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -126,7 +126,8 @@ export interface Options { * Tracing is enabled if either this or `tracesSampleRate` is defined. If both are defined, `tracesSampleRate` is * ignored. * - * Will automatically be passed a context object of default and optional custom data. See {@link Hub.startTransaction}. + * Will automatically be passed a context object of default and optional custom data. See + * {@link Transaction.sampleContext} and {@link Hub.startTransaction}. * * @returns A sample rate between 0 and 1 (0 drops the trace, 1 guarantees it will be sent). */ diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 6410a5145935..96b0d3a28640 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -50,17 +50,6 @@ export interface Transaction extends TransactionContext, Span { setName(name: string): void; } -/** - * The context passed to the tracesSampler function by default. - */ -export interface DefaultSampleContext { - /** Object representing the URL of the current page or woker script. Passed in a browser or service worker context */ - location?: Location | WorkerLocation; - - /** Object representing the incoming request to a node server. Passed when in a node server context. */ - request?: ExtractedNodeRequestData; -} - /** * Context data passed by the user when starting a transaction, to be used by the tracesSampler method. */ @@ -69,7 +58,19 @@ export interface CustomSampleContext { } /** - * The data passed to the `tracesSampler` function, which forms the basis for whatever decisions it might make. - * Combination of default values (which differ per SDK/integration) and data passed to `startTransaction`. + * Data passed to the `tracesSampler` function, which forms the basis for whatever decisions it might make. + * + * Adds default data to data provided by the user. See {@link Hub.startTransaction} */ -export type SampleContext = DefaultSampleContext & CustomSampleContext; +export interface SampleContext extends CustomSampleContext { + /** + * Object representing the URL of the current page or worker script. Passed by default in a browser or service worker + * context + */ + location?: Location | WorkerLocation; + + /** + * Object representing the incoming request to a node server. Passed by default in a node server context. + */ + request?: ExtractedNodeRequestData; +} From 9c6f0cbac810c571bbb7c0ba1894c816f4449723 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 2 Sep 2020 09:12:49 -0700 Subject: [PATCH 31/40] add tracesSampler example to gatsby readme --- packages/gatsby/README.md | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/packages/gatsby/README.md b/packages/gatsby/README.md index 4743e583f964..b430dfc6c2be 100644 --- a/packages/gatsby/README.md +++ b/packages/gatsby/README.md @@ -40,7 +40,7 @@ To automatically capture the `release` value on Vercel you will need to register ## Sentry Performance -To enable Tracing support, supply either `tracesSampleRate` or `tracesSampler` to the options and make sure you have installed the `@sentry/tracing` package. This will also turn on the `BrowserTracing` integration for automatic instrumentation of the browser. +To enable tracing, supply either `tracesSampleRate` or `tracesSampler` to the options and make sure you have installed the `@sentry/tracing` package. This will also turn on the `BrowserTracing` integration for automatic instrumentation of pageloads and navigations. ```javascript { @@ -49,11 +49,31 @@ To enable Tracing support, supply either `tracesSampleRate` or `tracesSampler` t { resolve: "@sentry/gatsby", options: { - dsn: process.env.SENTRY_DSN, // this is the default + dsn: process.env.SENTRY_DSN, // this is the default + + // A rate of 1 means all traces will be sent, so it's good for testing. + // In production, you'll likely want to either choose a lower rate or use `tracesSampler` instead (see below). + tracesSampleRate: 1, - // A rate of 1 means all traces will be sent, so it's good for testing. - // In production, you'll likely want to either choose a lower rate or use `tracesSampler` instead. - tracesSampleRate: 1, + // Alternatively: + tracesSampler: sampleContext => { + // Examine provided context data (along with anything in the global namespace) to decide the sample rate + // for this transaction. + // Can return 0 to drop the transaction entirely. + + if ("...") { + return 0.5 // These are important - take a big sample + } + else if ("...") { + return 0.01 // These are less important or happen much more frequently - only take 1% of them + } + else if ("...") { + return 0 // These aren't something worth tracking - drop all transactions like this + } + else { + return 0.1 // Default sample rate + } + } } }, // ... @@ -71,7 +91,7 @@ If you want to supply options to the `BrowserTracing` integration, use the `brow resolve: "@sentry/gatsby", options: { dsn: process.env.SENTRY_DSN, // this is the default - tracesSampleRate: 1, // this is just to test, you should lower this in production + tracesSampleRate: 1, // or tracesSampler (see above) browserTracingOptions: { // disable creating spans for XHR requests traceXHR: false, From fd4d48c182d6e378644388d0b4819db6ce34a539 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 2 Sep 2020 09:13:39 -0700 Subject: [PATCH 32/40] random cleanup --- packages/minimal/src/index.ts | 18 ++++++++---------- packages/types/src/hub.ts | 18 ++++++++---------- packages/types/src/options.ts | 3 +++ packages/types/src/request.ts | 4 +++- packages/types/src/transaction.ts | 4 ++-- packages/utils/src/misc.ts | 6 ++++-- 6 files changed, 28 insertions(+), 25 deletions(-) diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index cc41eca45e20..81eda6b198d0 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -191,23 +191,21 @@ export function _callOnClient(method: string, ...args: any[]): void { } /** - * Starts a new `Transaction` and returns it. This is the entry point to manual - * tracing instrumentation. + * Starts a new `Transaction` and returns it. This is the entry point to manual tracing instrumentation. * - * A tree structure can be built by adding child spans to the transaction, and - * child spans to other spans. To start a new child span within the transaction - * or any span, call the respective `.startChild()` method. + * A tree structure can be built by adding child spans to the transaction, and child spans to other spans. To start a + * new child span within the transaction or any span, call the respective `.startChild()` method. * - * Every child span must be finished before the transaction is finished, - * otherwise the unfinished spans are discarded. + * Every child span must be finished before the transaction is finished, otherwise the unfinished spans are discarded. * - * The transaction must be finished with a call to its `.finish()` method, at - * which point the transaction with all its finished child spans will be sent to - * Sentry. + * The transaction must be finished with a call to its `.finish()` method, at which point the transaction with all its + * finished child spans will be sent to Sentry. * * @param context Properties of the new `Transaction`. * @param customSampleContext Information given to the transaction sampling function (along with context-dependent * default values). See {@link Options.tracesSampler}. + * + * @returns The transaction which was just started */ export function startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction { return callOnHub('startTransaction', { ...context }, customSampleContext); diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index c0dc4402bb95..ca0ab9c37b2f 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -183,23 +183,21 @@ export interface Hub { startSpan(context: SpanContext): Span; /** - * Starts a new `Transaction` and returns it. This is the entry point to manual - * tracing instrumentation. + * Starts a new `Transaction` and returns it. This is the entry point to manual tracing instrumentation. * - * A tree structure can be built by adding child spans to the transaction, and - * child spans to other spans. To start a new child span within the transaction - * or any span, call the respective `.startChild()` method. + * A tree structure can be built by adding child spans to the transaction, and child spans to other spans. To start a + * new child span within the transaction or any span, call the respective `.startChild()` method. * - * Every child span must be finished before the transaction is finished, - * otherwise the unfinished spans are discarded. + * Every child span must be finished before the transaction is finished, otherwise the unfinished spans are discarded. * - * The transaction must be finished with a call to its `.finish()` method, at - * which point the transaction with all its finished child spans will be sent to - * Sentry. + * The transaction must be finished with a call to its `.finish()` method, at which point the transaction with all its + * finished child spans will be sent to Sentry. * * @param context Properties of the new `Transaction`. * @param customSampleContext Information given to the transaction sampling function (along with context-dependent * default values). See {@link Options.tracesSampler}. + * + * @returns The transaction which was just started */ startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction; } diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index ff2e1e3baa61..e8f088121244 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -105,6 +105,9 @@ export interface Options { */ shutdownTimeout?: number; + /** + * Options which are in beta, or otherwise not guaranteed to be stable. + */ _experiments?: { [key: string]: any; }; diff --git a/packages/types/src/request.ts b/packages/types/src/request.ts index 980dd498aa73..0e0e8e172117 100644 --- a/packages/types/src/request.ts +++ b/packages/types/src/request.ts @@ -1,4 +1,6 @@ -/** JSDoc */ +/** + * Request data included in an event as sent to Sentry + */ export interface Request { url?: string; method?: string; diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 96b0d3a28640..45c531db0325 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -65,12 +65,12 @@ export interface CustomSampleContext { export interface SampleContext extends CustomSampleContext { /** * Object representing the URL of the current page or worker script. Passed by default in a browser or service worker - * context + * context. */ location?: Location | WorkerLocation; /** - * Object representing the incoming request to a node server. Passed by default in a node server context. + * Object representing the incoming request to a node server. Passed by default when using the TracingHandler. */ request?: ExtractedNodeRequestData; } diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 090577781a85..ecd753794263 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -157,12 +157,14 @@ export function consoleSandbox(callback: () => any): any { return callback(); } - const originalConsole = global.console as ExtensibleConsole; + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const originalConsole = (global as any).console as ExtensibleConsole; const wrappedLevels: { [key: string]: any } = {}; // Restore all wrapped console methods levels.forEach(level => { - if (level in global.console && (originalConsole[level] as WrappedFunction).__sentry_original__) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (level in (global as any).console && (originalConsole[level] as WrappedFunction).__sentry_original__) { wrappedLevels[level] = originalConsole[level] as WrappedFunction; originalConsole[level] = (originalConsole[level] as WrappedFunction).__sentry_original__; } From 7b47a06fdf9bacea94a88f486534daf912e46897 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 3 Sep 2020 14:38:07 -0700 Subject: [PATCH 33/40] add parent's sampling decision to defaultSampleContext --- packages/tracing/src/hubextensions.ts | 11 ++++++++--- packages/types/src/transaction.ts | 5 +++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index c96fe60971f4..aa7219786ff8 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -100,9 +100,14 @@ function sample(hub: Hub, transaction: T, sampleContext: * * @returns The default sample context */ -function getDefaultSampleContext(): SampleContext { +function getDefaultSampleContext(transaction: T): SampleContext { const defaultSampleContext: SampleContext = {}; + // include parent's sampling decision, if there is one + if (transaction.parentSpanId && transaction.sampled !== undefined) { + defaultSampleContext.parentSampled = transaction.sampled; + } + if (isNodeEnv()) { const domain = getActiveDomain(); @@ -177,7 +182,7 @@ function _startTransaction( customSampleContext?: CustomSampleContext, ): Transaction { const transaction = new Transaction(context, this); - return sample(this, transaction, { ...getDefaultSampleContext(), ...customSampleContext }); + return sample(this, transaction, { ...getDefaultSampleContext(transaction), ...customSampleContext }); } /** @@ -190,7 +195,7 @@ export function startIdleTransaction( onScope?: boolean, ): IdleTransaction { const transaction = new IdleTransaction(context, hub, idleTimeout, onScope); - return sample(hub, transaction, getDefaultSampleContext()); + return sample(hub, transaction, getDefaultSampleContext(transaction)); } /** diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 45c531db0325..dbba7d013587 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -73,4 +73,9 @@ export interface SampleContext extends CustomSampleContext { * Object representing the incoming request to a node server. Passed by default when using the TracingHandler. */ request?: ExtractedNodeRequestData; + + /** + * Sampling decision from the parent transaction, if any. + */ + parentSampled?: boolean; } From be6abc8bef2514a3545613d35fd3dda9e49ea184 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 3 Sep 2020 15:15:46 -0700 Subject: [PATCH 34/40] make tracesSampler able to return a boolean and make it override sampling inheritance --- packages/tracing/src/hubextensions.ts | 100 +++++++++++++++---------- packages/tracing/test/hub.test.ts | 103 ++++++++++++++++++++++---- packages/types/src/options.ts | 5 +- 3 files changed, 150 insertions(+), 58 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index aa7219786ff8..61707f0f7be2 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -29,16 +29,28 @@ function traceHeaders(this: Hub): { [key: string]: string } { } /** - * Use sample rate along with a random number generator to make a sampling decision, which all child spans and child - * transactions inherit. + * Implements sampling inheritance and falls back to user-provided static rate if no parent decision is available. * - * Sample rate is set in SDK config, either as a constant (`tracesSampleRate`) or a function to compute the rate - * (`tracesSampler`). + * @param parentSampled: The parent transaction's sampling decision, if any. + * @param givenRate: The rate to use if no parental decision is available. + * + * @returns The parent's sampling decision (if one exists), or the provided static rate + */ +function _inheritOrUseGivenRate(parentSampled: boolean | undefined, givenRate: unknown): boolean | unknown { + return parentSampled !== undefined ? parentSampled : givenRate; +} + +/** + * Makes a sampling decision for the given transaction and stores it on the transaction. * * Called every time a transaction is created. Only transactions which emerge with a `sampled` value of `true` will be * sent to Sentry. * - * Mutates the given Transaction object and then returns the mutated object. + * @param hub: The hub off of which to read config options + * @param transaction: The transaction needing a sampling decision + * @param sampleContext: Default and user-provided data which may be used to help make the decision + * + * @returns The given transaction with its `sampled` value set */ function sample(hub: Hub, transaction: T, sampleContext: SampleContext = {}): T { const client = hub.getClient(); @@ -50,42 +62,46 @@ function sample(hub: Hub, transaction: T, sampleContext: return transaction; } - // we have to test for a pre-existsing sampling decision, in case this transaction is a child transaction and has - // inherited its parent's decision - if (transaction.sampled === undefined) { - // we would have bailed at the beginning if neither `tracesSampler` nor `tracesSampleRate` were defined, so one of - // these should work; prefer the hook if so - const sampleRate = - typeof options.tracesSampler === 'function' ? options.tracesSampler(sampleContext) : options.tracesSampleRate; - - // since this is coming from the user, who knows what we might get - if (!isValidSampleRate(sampleRate)) { - logger.warn(`[Tracing] Discarding trace because of invalid sample rate.`); - transaction.sampled = false; - return transaction; - } + // we would have bailed already if neither `tracesSampler` nor `tracesSampleRate` were defined, so one of these should + // work; prefer the hook if so + const sampleRate = + typeof options.tracesSampler === 'function' + ? options.tracesSampler(sampleContext) + : _inheritOrUseGivenRate(sampleContext.parentSampled, options.tracesSampleRate); + + // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The + // only valid values are booleans or numbers between 0 and 1.) + if (!isValidSampleRate(sampleRate)) { + logger.warn(`[Tracing] Discarding transaction because of invalid sample rate.`); + transaction.sampled = false; + return transaction; + } - // if the function returned 0, or if the sample rate is set to 0, it's a sign the transaction should be dropped - if (!sampleRate) { - logger.log( - `[Tracing] Discarding trace because ${ - typeof options.tracesSampler === 'function' ? 'tracesSampler returned 0' : 'tracesSampleRate is set to 0' - }`, - ); - transaction.sampled = false; - return transaction; - } + // if the function returned 0 (or false), or if `tracesSampleRate` is 0, it's a sign the transaction should be dropped + if (!sampleRate) { + logger.log( + `[Tracing] Discarding transaction because ${ + typeof options.tracesSampler === 'function' + ? 'tracesSampler returned 0 or false' + : 'tracesSampleRate is set to 0' + }`, + ); + transaction.sampled = false; + return transaction; + } - // now we roll the dice (Math.random is inclusive of 0, but not of 1, so strict < is safe here) - transaction.sampled = Math.random() < sampleRate; + // Now we roll the dice. Math.random is inclusive of 0, but not of 1, so strict < is safe here. In case sampleRate is + // a boolean, the < comparison will cause it to be automatically cast to 1 if it's true and 0 if it's false. + transaction.sampled = Math.random() < (sampleRate as number | boolean); - // if we're not going to keep it, we're done - if (!transaction.sampled) { - logger.log( - `[Tracing] Discarding trace because it's not included in the random sample (sampling rate = ${sampleRate})`, - ); - return transaction; - } + // if we're not going to keep it, we're done + if (!transaction.sampled) { + logger.log( + `[Tracing] Discarding transaction because it's not included in the random sample (sampling rate = ${Number( + sampleRate, + )})`, + ); + return transaction; } // at this point we know we're keeping the transaction, whether because of an inherited decision or because it got @@ -148,17 +164,19 @@ function getDefaultSampleContext(transaction: T): SampleC } /** - * Checks the given sample rate to make sure it is valid (a number between 0 and 1). + * Checks the given sample rate to make sure it is valid type and value (a boolean, or a number between 0 and 1). */ function isValidSampleRate(rate: unknown): boolean { - if (!(typeof rate === 'number')) { + if (!(typeof rate === 'number' || typeof rate === 'boolean')) { logger.warn( - `[Tracing] Given sample rate is invalid. Sample rate must be a number between 0 and 1. Got ${JSON.stringify( + `[Tracing] Given sample rate is invalid. Sample rate must be a boolean or a number between 0 and 1. Got ${JSON.stringify( rate, )} of type ${JSON.stringify(typeof rate)}.`, ); return false; } + + // in case sampleRate is a boolean, it will get automatically cast to 1 if it's true and 0 if it's false if (rate < 0 || rate > 1) { logger.warn(`[Tracing] Given sample rate is invalid. Sample rate must be between 0 and 1. Got ${rate}.`); return false; diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 369bab21beea..51ae330fe904 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -41,7 +41,7 @@ describe('Hub', () => { }); describe('transaction sampling', () => { - describe('options', () => { + describe('tracesSampleRate and tracesSampler options', () => { it("should call tracesSampler if it's defined", () => { const tracesSampler = jest.fn(); const hub = new Hub(new BrowserClient({ tracesSampler })); @@ -52,11 +52,20 @@ describe('Hub', () => { it('should prefer tracesSampler to tracesSampleRate', () => { const tracesSampler = jest.fn(); - const hub = new Hub(new BrowserClient({ tracesSampleRate: 1, tracesSampler: tracesSampler })); + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1, tracesSampler })); hub.startTransaction({ name: 'dogpark' }); expect(tracesSampler).toHaveBeenCalled(); }); + + it('tolerates tracesSampler returning a boolean', () => { + const tracesSampler = jest.fn().mockReturnValue(true); + const hub = new Hub(new BrowserClient({ tracesSampler })); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + expect(tracesSampler).toHaveBeenCalled(); + expect(transaction.sampled).toBe(true); + }); }); describe('default sample context', () => { @@ -133,9 +142,25 @@ describe('Hub', () => { expect(tracesSampler).toHaveBeenCalledWith(expect.objectContaining({ location: dogParkLocation })); }); + + it("should add parent's sampling decision to default sample context", () => { + const tracesSampler = jest.fn(); + const hub = new Hub(new BrowserClient({ tracesSampler })); + const parentSamplingDecsion = false; + + hub.startTransaction({ + name: 'dogpark', + parentSpanId: '12312012', + sampled: parentSamplingDecsion, + }); + + expect(tracesSampler).toHaveBeenLastCalledWith( + expect.objectContaining({ parentSampled: parentSamplingDecsion }), + ); + }); }); - describe('while sampling', () => { + describe('sample()', () => { it('should not sample transactions when tracing is disabled', () => { // neither tracesSampleRate nor tracesSampler is defined -> tracing disabled const hub = new Hub(new BrowserClient({})); @@ -157,14 +182,17 @@ describe('Hub', () => { expect(transaction.sampled).toBe(true); }); + }); - it("should reject tracesSampleRates which aren't numbers", () => { + describe('isValidSampleRate()', () => { + it("should reject tracesSampleRates which aren't numbers or booleans", () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 'dogs!' as any })); hub.startTransaction({ name: 'dogpark' }); - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a number')); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a boolean or a number')); }); + // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampleRates less than 0', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: -26 })); hub.startTransaction({ name: 'dogpark' }); @@ -172,6 +200,7 @@ describe('Hub', () => { expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); }); + // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampleRates greater than 1', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: 26 })); hub.startTransaction({ name: 'dogpark' }); @@ -179,14 +208,15 @@ describe('Hub', () => { expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); }); - it("should reject tracesSampler return values which aren't numbers", () => { + it("should reject tracesSampler return values which aren't numbers or booleans", () => { const tracesSampler = jest.fn().mockReturnValue('dogs!'); const hub = new Hub(new BrowserClient({ tracesSampler })); hub.startTransaction({ name: 'dogpark' }); - expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a number')); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a boolean or a number')); }); + // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampler return values less than 0', () => { const tracesSampler = jest.fn().mockReturnValue(-12); const hub = new Hub(new BrowserClient({ tracesSampler })); @@ -195,6 +225,7 @@ describe('Hub', () => { expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be between 0 and 1')); }); + // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampler return values greater than 1', () => { const tracesSampler = jest.fn().mockReturnValue(31); const hub = new Hub(new BrowserClient({ tracesSampler })); @@ -204,14 +235,6 @@ describe('Hub', () => { }); }); - it('should propagate sampling decision to child spans', () => { - const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 })); - const transaction = hub.startTransaction({ name: 'dogpark' }); - const child = transaction.startChild({ op: 'test' }); - - expect(child.sampled).toBe(false); - }); - it('should drop transactions with sampled = false', () => { const client = new BrowserClient({ tracesSampleRate: 0 }); jest.spyOn(client, 'captureEvent'); @@ -226,5 +249,55 @@ describe('Hub', () => { expect(transaction.finish).toReturnWith(undefined); expect(client.captureEvent).not.toBeCalled(); }); + + describe('sampling inheritance', () => { + it('should propagate sampling decision to child spans', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: Math.random() })); + const transaction = hub.startTransaction({ name: 'dogpark' }); + const child = transaction.startChild({ op: 'test' }); + + expect(child.sampled).toBe(transaction.sampled); + }); + + it('should propagate sampling decision to child transactions in XHR header', () => { + // TODO this doesn't currently happen, but it should + }); + + it('should propagate sampling decision to child transactions in fetch header', () => { + // TODO this doesn't currently happen, but it should + }); + + it("should inherit parent's sampling decision when creating a new transaction if tracesSampler is undefined", () => { + // tracesSampleRate = 1 means every transaction should end up with sampled = true, so make parent's decision the + // opposite to prove that inheritance takes precedence over tracesSampleRate + const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); + const parentSamplingDecsion = false; + + const transaction = hub.startTransaction({ + name: 'dogpark', + parentSpanId: '12312012', + sampled: parentSamplingDecsion, + }); + + expect(transaction.sampled).toBe(parentSamplingDecsion); + }); + + it("should ignore parent's sampling decision when tracesSampler is defined", () => { + // this tracesSampler causes every transaction to end up with sampled = true, so make parent's decision the + // opposite to prove that tracesSampler takes precedence over inheritance + const tracesSampler = () => true; + const parentSamplingDecsion = false; + + const hub = new Hub(new BrowserClient({ tracesSampler })); + + const transaction = hub.startTransaction({ + name: 'dogpark', + parentSpanId: '12312012', + sampled: parentSamplingDecsion, + }); + + expect(transaction.sampled).not.toBe(parentSamplingDecsion); + }); + }); }); }); diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index e8f088121244..54bfae9b6a6d 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -132,9 +132,10 @@ export interface Options { * Will automatically be passed a context object of default and optional custom data. See * {@link Transaction.sampleContext} and {@link Hub.startTransaction}. * - * @returns A sample rate between 0 and 1 (0 drops the trace, 1 guarantees it will be sent). + * @returns A sample rate between 0 and 1 (0 drops the trace, 1 guarantees it will be sent). Returning `true` is + * equivalent to returning 1 and returning `false` is equivalent to returning 0. */ - tracesSampler?(sampleContext: SampleContext): number; + tracesSampler?(sampleContext: SampleContext): number | boolean; /** * A callback invoked during event submission, allowing to optionally modify From 69e49e3a07de08c9a018160d94a3aca9f8f1aa4f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 3 Sep 2020 15:17:54 -0700 Subject: [PATCH 35/40] protect against NaN values for sample rate --- packages/tracing/src/hubextensions.ts | 4 +++- packages/tracing/test/hub.test.ts | 19 +++++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 61707f0f7be2..8d79bb158dfe 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -167,7 +167,9 @@ function getDefaultSampleContext(transaction: T): SampleC * Checks the given sample rate to make sure it is valid type and value (a boolean, or a number between 0 and 1). */ function isValidSampleRate(rate: unknown): boolean { - if (!(typeof rate === 'number' || typeof rate === 'boolean')) { + // we need to check NaN explicitly because it's of type 'number' and therefore wouldn't get caught by this typecheck + // eslint-disable-next-line @typescript-eslint/no-explicit-any + if (isNaN(rate as any) || !(typeof rate === 'number' || typeof rate === 'boolean')) { logger.warn( `[Tracing] Given sample rate is invalid. Sample rate must be a boolean or a number between 0 and 1. Got ${JSON.stringify( rate, diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 51ae330fe904..7cc675e2659b 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -192,6 +192,13 @@ describe('Hub', () => { expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a boolean or a number')); }); + it('should reject tracesSampleRates which are NaN', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 'dogs!' as any })); + hub.startTransaction({ name: 'dogpark' }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a boolean or a number')); + }); + // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampleRates less than 0', () => { const hub = new Hub(new BrowserClient({ tracesSampleRate: -26 })); @@ -216,6 +223,14 @@ describe('Hub', () => { expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a boolean or a number')); }); + it('should reject tracesSampler return values which are NaN', () => { + const tracesSampler = jest.fn().mockReturnValue(NaN); + const hub = new Hub(new BrowserClient({ tracesSampler })); + hub.startTransaction({ name: 'dogpark' }); + + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Sample rate must be a boolean or a number')); + }); + // the rate might be a boolean, but for our purposes, false is equivalent to 0 and true is equivalent to 1 it('should reject tracesSampler return values less than 0', () => { const tracesSampler = jest.fn().mockReturnValue(-12); @@ -260,11 +275,11 @@ describe('Hub', () => { }); it('should propagate sampling decision to child transactions in XHR header', () => { - // TODO this doesn't currently happen, but it should + // TODO fix this and write the test }); it('should propagate sampling decision to child transactions in fetch header', () => { - // TODO this doesn't currently happen, but it should + // TODO fix this and write the test }); it("should inherit parent's sampling decision when creating a new transaction if tracesSampler is undefined", () => { From cbaecb364aca5b99eff02e747a4d5a2a153d42a3 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 3 Sep 2020 15:23:22 -0700 Subject: [PATCH 36/40] remove claim that we're fixing a race condition by copying location object --- packages/tracing/src/hubextensions.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 8d79bb158dfe..1fd05e297f37 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -153,8 +153,6 @@ function getDefaultSampleContext(transaction: T): SampleC const globalObject = getGlobalObject(); if ('location' in globalObject) { - // we take a copy of the location object rather than just a reference to it in case there's a navigation or - // redirect in the instant between when the transaction starts and when the sampler is called // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any defaultSampleContext.location = { ...(globalObject as any).location }; } From ccba78ae869e5d84a6e298ec707204ce43b43ece Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 3 Sep 2020 15:47:41 -0700 Subject: [PATCH 37/40] s/sampleContext/samplingContext --- packages/gatsby/README.md | 2 +- packages/hub/src/hub.ts | 6 +++--- packages/minimal/src/index.ts | 11 +++++++---- packages/tracing/src/hubextensions.ts | 28 +++++++++++++-------------- packages/types/src/hub.ts | 6 +++--- packages/types/src/index.ts | 2 +- packages/types/src/options.ts | 6 +++--- packages/types/src/transaction.ts | 4 ++-- 8 files changed, 34 insertions(+), 31 deletions(-) diff --git a/packages/gatsby/README.md b/packages/gatsby/README.md index b430dfc6c2be..ae8febaed913 100644 --- a/packages/gatsby/README.md +++ b/packages/gatsby/README.md @@ -56,7 +56,7 @@ To enable tracing, supply either `tracesSampleRate` or `tracesSampler` to the op tracesSampleRate: 1, // Alternatively: - tracesSampler: sampleContext => { + tracesSampler: samplingContext => { // Examine provided context data (along with anything in the global namespace) to decide the sample rate // for this transaction. // Can return 0 to drop the transaction entirely. diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts index f2ac47e2c4b6..61d4609a333e 100644 --- a/packages/hub/src/hub.ts +++ b/packages/hub/src/hub.ts @@ -3,7 +3,7 @@ import { Breadcrumb, BreadcrumbHint, Client, - CustomSampleContext, + CustomSamplingContext, Event, EventHint, Extra, @@ -370,8 +370,8 @@ export class Hub implements HubInterface { /** * @inheritDoc */ - public startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction { - return this._callExtensionMethod('startTransaction', context, customSampleContext); + public startTransaction(context: TransactionContext, customSamplingContext?: CustomSamplingContext): Transaction { + return this._callExtensionMethod('startTransaction', context, customSamplingContext); } /** diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts index 81eda6b198d0..1406a4c408e1 100644 --- a/packages/minimal/src/index.ts +++ b/packages/minimal/src/index.ts @@ -2,7 +2,7 @@ import { getCurrentHub, Hub, Scope } from '@sentry/hub'; import { Breadcrumb, CaptureContext, - CustomSampleContext, + CustomSamplingContext, Event, Extra, Extras, @@ -202,11 +202,14 @@ export function _callOnClient(method: string, ...args: any[]): void { * finished child spans will be sent to Sentry. * * @param context Properties of the new `Transaction`. - * @param customSampleContext Information given to the transaction sampling function (along with context-dependent + * @param customSamplingContext Information given to the transaction sampling function (along with context-dependent * default values). See {@link Options.tracesSampler}. * * @returns The transaction which was just started */ -export function startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction { - return callOnHub('startTransaction', { ...context }, customSampleContext); +export function startTransaction( + context: TransactionContext, + customSamplingContext?: CustomSamplingContext, +): Transaction { + return callOnHub('startTransaction', { ...context }, customSamplingContext); } diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 1fd05e297f37..dc8e13baa3a6 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -1,5 +1,5 @@ import { getActiveDomain, getMainCarrier, Hub } from '@sentry/hub'; -import { CustomSampleContext, SampleContext, TransactionContext } from '@sentry/types'; +import { CustomSamplingContext, SamplingContext, TransactionContext } from '@sentry/types'; import { dynamicRequire, extractNodeRequestData, @@ -48,11 +48,11 @@ function _inheritOrUseGivenRate(parentSampled: boolean | undefined, givenRate: u * * @param hub: The hub off of which to read config options * @param transaction: The transaction needing a sampling decision - * @param sampleContext: Default and user-provided data which may be used to help make the decision + * @param samplingContext: Default and user-provided data which may be used to help make the decision * * @returns The given transaction with its `sampled` value set */ -function sample(hub: Hub, transaction: T, sampleContext: SampleContext = {}): T { +function sample(hub: Hub, transaction: T, samplingContext: SamplingContext = {}): T { const client = hub.getClient(); const options = (client && client.getOptions()) || {}; @@ -66,8 +66,8 @@ function sample(hub: Hub, transaction: T, sampleContext: // work; prefer the hook if so const sampleRate = typeof options.tracesSampler === 'function' - ? options.tracesSampler(sampleContext) - : _inheritOrUseGivenRate(sampleContext.parentSampled, options.tracesSampleRate); + ? options.tracesSampler(samplingContext) + : _inheritOrUseGivenRate(samplingContext.parentSampled, options.tracesSampleRate); // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The // only valid values are booleans or numbers between 0 and 1.) @@ -116,12 +116,12 @@ function sample(hub: Hub, transaction: T, sampleContext: * * @returns The default sample context */ -function getDefaultSampleContext(transaction: T): SampleContext { - const defaultSampleContext: SampleContext = {}; +function getDefaultSamplingContext(transaction: T): SamplingContext { + const defaultSamplingContext: SamplingContext = {}; // include parent's sampling decision, if there is one if (transaction.parentSpanId && transaction.sampled !== undefined) { - defaultSampleContext.parentSampled = transaction.sampled; + defaultSamplingContext.parentSampled = transaction.sampled; } if (isNodeEnv()) { @@ -142,7 +142,7 @@ function getDefaultSampleContext(transaction: T): SampleC const request = domain.members.find((member: any) => isInstanceOf(member, requestType)); if (request) { - defaultSampleContext.request = extractNodeRequestData(request); + defaultSamplingContext.request = extractNodeRequestData(request); } } } @@ -154,11 +154,11 @@ function getDefaultSampleContext(transaction: T): SampleC if ('location' in globalObject) { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any - defaultSampleContext.location = { ...(globalObject as any).location }; + defaultSamplingContext.location = { ...(globalObject as any).location }; } } - return defaultSampleContext; + return defaultSamplingContext; } /** @@ -197,10 +197,10 @@ function isValidSampleRate(rate: unknown): boolean { function _startTransaction( this: Hub, context: TransactionContext, - customSampleContext?: CustomSampleContext, + customSamplingContext?: CustomSamplingContext, ): Transaction { const transaction = new Transaction(context, this); - return sample(this, transaction, { ...getDefaultSampleContext(transaction), ...customSampleContext }); + return sample(this, transaction, { ...getDefaultSamplingContext(transaction), ...customSamplingContext }); } /** @@ -213,7 +213,7 @@ export function startIdleTransaction( onScope?: boolean, ): IdleTransaction { const transaction = new IdleTransaction(context, hub, idleTimeout, onScope); - return sample(hub, transaction, getDefaultSampleContext(transaction)); + return sample(hub, transaction, getDefaultSamplingContext(transaction)); } /** diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts index ca0ab9c37b2f..29f538f5fd94 100644 --- a/packages/types/src/hub.ts +++ b/packages/types/src/hub.ts @@ -6,7 +6,7 @@ import { Integration, IntegrationClass } from './integration'; import { Scope } from './scope'; import { Severity } from './severity'; import { Span, SpanContext } from './span'; -import { CustomSampleContext, Transaction, TransactionContext } from './transaction'; +import { CustomSamplingContext, Transaction, TransactionContext } from './transaction'; import { User } from './user'; /** @@ -194,10 +194,10 @@ export interface Hub { * finished child spans will be sent to Sentry. * * @param context Properties of the new `Transaction`. - * @param customSampleContext Information given to the transaction sampling function (along with context-dependent + * @param customSamplingContext Information given to the transaction sampling function (along with context-dependent * default values). See {@link Options.tracesSampler}. * * @returns The transaction which was just started */ - startTransaction(context: TransactionContext, customSampleContext?: CustomSampleContext): Transaction; + startTransaction(context: TransactionContext, customSamplingContext?: CustomSamplingContext): Transaction; } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index cbb2e6559888..7a321cad1324 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -23,7 +23,7 @@ export { Span, SpanContext } from './span'; export { StackFrame } from './stackframe'; export { Stacktrace } from './stacktrace'; export { Status } from './status'; -export { CustomSampleContext, SampleContext, Transaction, TransactionContext } from './transaction'; +export { CustomSamplingContext, SamplingContext, Transaction, TransactionContext } from './transaction'; export { Thread } from './thread'; export { Transport, TransportOptions, TransportClass } from './transport'; export { User } from './user'; diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 54bfae9b6a6d..1fe5444e953d 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -2,7 +2,7 @@ import { Breadcrumb, BreadcrumbHint } from './breadcrumb'; import { Event, EventHint } from './event'; import { Integration } from './integration'; import { LogLevel } from './loglevel'; -import { SampleContext } from './transaction'; +import { SamplingContext } from './transaction'; import { Transport, TransportClass, TransportOptions } from './transport'; /** Base configuration options for every SDK. */ @@ -130,12 +130,12 @@ export interface Options { * ignored. * * Will automatically be passed a context object of default and optional custom data. See - * {@link Transaction.sampleContext} and {@link Hub.startTransaction}. + * {@link Transaction.samplingContext} and {@link Hub.startTransaction}. * * @returns A sample rate between 0 and 1 (0 drops the trace, 1 guarantees it will be sent). Returning `true` is * equivalent to returning 1 and returning `false` is equivalent to returning 0. */ - tracesSampler?(sampleContext: SampleContext): number | boolean; + tracesSampler?(samplingContext: SamplingContext): number | boolean; /** * A callback invoked during event submission, allowing to optionally modify diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index dbba7d013587..2827f1f870d7 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -53,7 +53,7 @@ export interface Transaction extends TransactionContext, Span { /** * Context data passed by the user when starting a transaction, to be used by the tracesSampler method. */ -export interface CustomSampleContext { +export interface CustomSamplingContext { [key: string]: any; } @@ -62,7 +62,7 @@ export interface CustomSampleContext { * * Adds default data to data provided by the user. See {@link Hub.startTransaction} */ -export interface SampleContext extends CustomSampleContext { +export interface SamplingContext extends CustomSamplingContext { /** * Object representing the URL of the current page or worker script. Passed by default in a browser or service worker * context. From d6fd44ae31102b4468be69daf5d950c4ea9169b0 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 7 Sep 2020 10:40:50 -0700 Subject: [PATCH 38/40] pass transaction context in sampling context --- packages/tracing/src/hubextensions.ts | 38 +++++++++++++-------------- packages/tracing/test/hub.test.ts | 16 +++++------ packages/types/src/transaction.ts | 10 +++---- 3 files changed, 30 insertions(+), 34 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index dc8e13baa3a6..c42a3fd2a65c 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -29,15 +29,15 @@ function traceHeaders(this: Hub): { [key: string]: string } { } /** - * Implements sampling inheritance and falls back to user-provided static rate if no parent decision is available. + * Uses existing sampling decision if available; falls back to user-provided static rate. * - * @param parentSampled: The parent transaction's sampling decision, if any. + * @param existingDecision: The transaction's existing sampling decision, if any (likely inherited). * @param givenRate: The rate to use if no parental decision is available. * - * @returns The parent's sampling decision (if one exists), or the provided static rate + * @returns The existing sampling decision (if one exists), or the provided static rate */ -function _inheritOrUseGivenRate(parentSampled: boolean | undefined, givenRate: unknown): boolean | unknown { - return parentSampled !== undefined ? parentSampled : givenRate; +function _useExistingDecisionOrGivenRate(existingDecision: boolean | undefined, givenRate: unknown): boolean | unknown { + return existingDecision !== undefined ? existingDecision : givenRate; } /** @@ -52,7 +52,7 @@ function _inheritOrUseGivenRate(parentSampled: boolean | undefined, givenRate: u * * @returns The given transaction with its `sampled` value set */ -function sample(hub: Hub, transaction: T, samplingContext: SamplingContext = {}): T { +function sample(hub: Hub, transaction: T, samplingContext: SamplingContext): T { const client = hub.getClient(); const options = (client && client.getOptions()) || {}; @@ -67,7 +67,7 @@ function sample(hub: Hub, transaction: T, samplingContext const sampleRate = typeof options.tracesSampler === 'function' ? options.tracesSampler(samplingContext) - : _inheritOrUseGivenRate(samplingContext.parentSampled, options.tracesSampleRate); + : _useExistingDecisionOrGivenRate(samplingContext.transactionContext.sampled, options.tracesSampleRate); // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The // only valid values are booleans or numbers between 0 and 1.) @@ -116,13 +116,8 @@ function sample(hub: Hub, transaction: T, samplingContext * * @returns The default sample context */ -function getDefaultSamplingContext(transaction: T): SamplingContext { - const defaultSamplingContext: SamplingContext = {}; - - // include parent's sampling decision, if there is one - if (transaction.parentSpanId && transaction.sampled !== undefined) { - defaultSamplingContext.parentSampled = transaction.sampled; - } +function getDefaultSamplingContext(transactionContext: TransactionContext): SamplingContext { + const defaultSamplingContext: SamplingContext = { transactionContext }; if (isNodeEnv()) { const domain = getActiveDomain(); @@ -196,11 +191,14 @@ function isValidSampleRate(rate: unknown): boolean { */ function _startTransaction( this: Hub, - context: TransactionContext, + transactionContext: TransactionContext, customSamplingContext?: CustomSamplingContext, ): Transaction { - const transaction = new Transaction(context, this); - return sample(this, transaction, { ...getDefaultSamplingContext(transaction), ...customSamplingContext }); + const transaction = new Transaction(transactionContext, this); + return sample(this, transaction, { + ...getDefaultSamplingContext(transactionContext), + ...customSamplingContext, + }); } /** @@ -208,12 +206,12 @@ function _startTransaction( */ export function startIdleTransaction( hub: Hub, - context: TransactionContext, + transactionContext: TransactionContext, idleTimeout?: number, onScope?: boolean, ): IdleTransaction { - const transaction = new IdleTransaction(context, hub, idleTimeout, onScope); - return sample(hub, transaction, getDefaultSamplingContext(transaction)); + const transaction = new IdleTransaction(transactionContext, hub, idleTimeout, onScope); + return sample(hub, transaction, getDefaultSamplingContext(transactionContext)); } /** diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 7cc675e2659b..7cf524543f90 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -143,20 +143,18 @@ describe('Hub', () => { expect(tracesSampler).toHaveBeenCalledWith(expect.objectContaining({ location: dogParkLocation })); }); - it("should add parent's sampling decision to default sample context", () => { + it('should add transaction context data to default sample context', () => { const tracesSampler = jest.fn(); const hub = new Hub(new BrowserClient({ tracesSampler })); - const parentSamplingDecsion = false; - - hub.startTransaction({ + const transactionContext = { name: 'dogpark', parentSpanId: '12312012', - sampled: parentSamplingDecsion, - }); + sampled: true, + }; - expect(tracesSampler).toHaveBeenLastCalledWith( - expect.objectContaining({ parentSampled: parentSamplingDecsion }), - ); + hub.startTransaction(transactionContext); + + expect(tracesSampler).toHaveBeenLastCalledWith(expect.objectContaining({ transactionContext })); }); }); diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 2827f1f870d7..86e4792fe860 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -63,6 +63,11 @@ export interface CustomSamplingContext { * Adds default data to data provided by the user. See {@link Hub.startTransaction} */ export interface SamplingContext extends CustomSamplingContext { + /** + * Context data with which transaction being sampled was created + */ + transactionContext: TransactionContext; + /** * Object representing the URL of the current page or worker script. Passed by default in a browser or service worker * context. @@ -73,9 +78,4 @@ export interface SamplingContext extends CustomSamplingContext { * Object representing the incoming request to a node server. Passed by default when using the TracingHandler. */ request?: ExtractedNodeRequestData; - - /** - * Sampling decision from the parent transaction, if any. - */ - parentSampled?: boolean; } From 07108608497a21a15b2fd52ba04cd5832615edb9 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 7 Sep 2020 12:35:35 -0700 Subject: [PATCH 39/40] include parentSampled as separate entry in transaction context --- packages/tracing/src/hubextensions.ts | 16 +++++++++------- packages/tracing/test/hub.test.ts | 22 +++++++++++++++++++--- packages/types/src/transaction.ts | 10 ++++++++++ 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index c42a3fd2a65c..2d0610c24a96 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -29,15 +29,15 @@ function traceHeaders(this: Hub): { [key: string]: string } { } /** - * Uses existing sampling decision if available; falls back to user-provided static rate. + * Implements sampling inheritance and falls back to user-provided static rate if no parent decision is available. * - * @param existingDecision: The transaction's existing sampling decision, if any (likely inherited). + * @param parentSampled: The parent transaction's sampling decision, if any. * @param givenRate: The rate to use if no parental decision is available. * - * @returns The existing sampling decision (if one exists), or the provided static rate + * @returns The parent's sampling decision (if one exists), or the provided static rate */ -function _useExistingDecisionOrGivenRate(existingDecision: boolean | undefined, givenRate: unknown): boolean | unknown { - return existingDecision !== undefined ? existingDecision : givenRate; +function _inheritOrUseGivenRate(parentSampled: boolean | undefined, givenRate: unknown): boolean | unknown { + return parentSampled !== undefined ? parentSampled : givenRate; } /** @@ -67,7 +67,7 @@ function sample(hub: Hub, transaction: T, samplingContext const sampleRate = typeof options.tracesSampler === 'function' ? options.tracesSampler(samplingContext) - : _useExistingDecisionOrGivenRate(samplingContext.transactionContext.sampled, options.tracesSampleRate); + : _inheritOrUseGivenRate(samplingContext.parentSampled, options.tracesSampleRate); // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The // only valid values are booleans or numbers between 0 and 1.) @@ -117,7 +117,9 @@ function sample(hub: Hub, transaction: T, samplingContext * @returns The default sample context */ function getDefaultSamplingContext(transactionContext: TransactionContext): SamplingContext { - const defaultSamplingContext: SamplingContext = { transactionContext }; + // promote parent sampling decision (if any) for easy access + const { parentSampled } = transactionContext; + const defaultSamplingContext: SamplingContext = { transactionContext, parentSampled }; if (isNodeEnv()) { const domain = getActiveDomain(); diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 7cf524543f90..9ae9badf7956 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -149,13 +149,29 @@ describe('Hub', () => { const transactionContext = { name: 'dogpark', parentSpanId: '12312012', - sampled: true, + parentSampled: true, }; hub.startTransaction(transactionContext); expect(tracesSampler).toHaveBeenLastCalledWith(expect.objectContaining({ transactionContext })); }); + + it("should add parent's sampling decision to default sample context", () => { + const tracesSampler = jest.fn(); + const hub = new Hub(new BrowserClient({ tracesSampler })); + const parentSamplingDecsion = false; + + hub.startTransaction({ + name: 'dogpark', + parentSpanId: '12312012', + parentSampled: parentSamplingDecsion, + }); + + expect(tracesSampler).toHaveBeenLastCalledWith( + expect.objectContaining({ parentSampled: parentSamplingDecsion }), + ); + }); }); describe('sample()', () => { @@ -289,7 +305,7 @@ describe('Hub', () => { const transaction = hub.startTransaction({ name: 'dogpark', parentSpanId: '12312012', - sampled: parentSamplingDecsion, + parentSampled: parentSamplingDecsion, }); expect(transaction.sampled).toBe(parentSamplingDecsion); @@ -306,7 +322,7 @@ describe('Hub', () => { const transaction = hub.startTransaction({ name: 'dogpark', parentSpanId: '12312012', - sampled: parentSamplingDecsion, + parentSampled: parentSamplingDecsion, }); expect(transaction.sampled).not.toBe(parentSamplingDecsion); diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 86e4792fe860..80f433c61986 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -13,6 +13,11 @@ export interface TransactionContext extends SpanContext { * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction. */ trimEnd?: boolean; + + /** + * If this transaction has a parent, the parent's sampling decision + */ + parentSampled?: boolean; } /** @@ -68,6 +73,11 @@ export interface SamplingContext extends CustomSamplingContext { */ transactionContext: TransactionContext; + /** + * Sampling decision from the parent transaction, if any. + */ + parentSampled?: boolean; + /** * Object representing the URL of the current page or worker script. Passed by default in a browser or service worker * context. From 8572b085c01c3f546f9a504aae4336fce8cae9d8 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 7 Sep 2020 12:37:42 -0700 Subject: [PATCH 40/40] refactor toTraceparent into extractTraceparentData --- packages/node/src/handlers.ts | 18 ++---- .../tracing/src/browser/browsertracing.ts | 17 ++--- packages/tracing/src/index.bundle.ts | 2 +- packages/tracing/src/index.ts | 2 +- packages/tracing/src/span.ts | 34 ---------- packages/tracing/src/utils.ts | 35 +++++++++- packages/tracing/test/span.test.ts | 36 ----------- packages/tracing/test/utils.test.ts | 64 +++++++++++++++++++ packages/types/src/index.ts | 8 ++- packages/types/src/transaction.ts | 9 +++ 10 files changed, 126 insertions(+), 99 deletions(-) create mode 100644 packages/tracing/test/utils.test.ts diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index dcd6c567f0a8..a8af9001cbcc 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -1,7 +1,7 @@ /* eslint-disable max-lines */ /* eslint-disable @typescript-eslint/no-explicit-any */ import { captureException, getCurrentHub, startTransaction, withScope } from '@sentry/core'; -import { Span } from '@sentry/tracing'; +import { extractTraceparentData, Span } from '@sentry/tracing'; import { Event } from '@sentry/types'; import { extractNodeRequestData, @@ -40,26 +40,16 @@ export function tracingHandler(): ( const reqMethod = (req.method || '').toUpperCase(); const reqUrl = req.url && stripUrlQueryAndFragment(req.url); - let traceId; - let parentSpanId; - let sampled; - // If there is a trace header set, we extract the data from it (parentSpanId, traceId, and sampling decision) + let traceparentData; if (req.headers && isString(req.headers['sentry-trace'])) { - const span = Span.fromTraceparent(req.headers['sentry-trace'] as string); - if (span) { - traceId = span.traceId; - parentSpanId = span.parentSpanId; - sampled = span.sampled; - } + traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string); } const transaction = startTransaction({ name: `${reqMethod} ${reqUrl}`, op: 'http.server', - parentSpanId, - sampled, - traceId, + ...traceparentData, }); // We put the transaction on the scope so users can attach children to it diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 8f3add8bc546..1edb81c98295 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -4,8 +4,8 @@ import { logger } from '@sentry/utils'; import { startIdleTransaction } from '../hubextensions'; import { DEFAULT_IDLE_TIMEOUT, IdleTransaction } from '../idletransaction'; -import { Span } from '../span'; import { SpanStatus } from '../spanstatus'; +import { extractTraceparentData } from '../utils'; import { registerBackgroundTabDetection } from './backgroundtab'; import { MetricsInstrumentation } from './metrics'; import { @@ -213,21 +213,16 @@ export class BrowserTracing implements Integration { /** * Gets transaction context from a sentry-trace meta. + * + * @returns Transaction context data from the header or undefined if there's no header or the header is malformed */ -function getHeaderContext(): Partial { +function getHeaderContext(): Partial | undefined { const header = getMetaContent('sentry-trace'); if (header) { - const span = Span.fromTraceparent(header); - if (span) { - return { - parentSpanId: span.parentSpanId, - sampled: span.sampled, - traceId: span.traceId, - }; - } + return extractTraceparentData(header); } - return {}; + return undefined; } /** Returns the value of a meta tag */ diff --git a/packages/tracing/src/index.bundle.ts b/packages/tracing/src/index.bundle.ts index bbe587d18d47..c7d60c3635a3 100644 --- a/packages/tracing/src/index.bundle.ts +++ b/packages/tracing/src/index.bundle.ts @@ -55,7 +55,7 @@ import { getGlobalObject } from '@sentry/utils'; import { BrowserTracing } from './browser'; import { addExtensionMethods } from './hubextensions'; -export { Span, TRACEPARENT_REGEXP } from './span'; +export { Span } from './span'; let windowIntegrations = {}; diff --git a/packages/tracing/src/index.ts b/packages/tracing/src/index.ts index 73981afceac5..c779d122bf38 100644 --- a/packages/tracing/src/index.ts +++ b/packages/tracing/src/index.ts @@ -5,7 +5,7 @@ import * as ApmIntegrations from './integrations'; const Integrations = { ...ApmIntegrations, BrowserTracing }; export { Integrations }; -export { Span, TRACEPARENT_REGEXP } from './span'; +export { Span } from './span'; export { Transaction } from './transaction'; export { SpanStatus } from './spanstatus'; diff --git a/packages/tracing/src/span.ts b/packages/tracing/src/span.ts index f36dea5a04c9..0118871eabd4 100644 --- a/packages/tracing/src/span.ts +++ b/packages/tracing/src/span.ts @@ -4,14 +4,6 @@ import { dropUndefinedKeys, timestampWithMs, uuid4 } from '@sentry/utils'; import { SpanStatus } from './spanstatus'; -export const TRACEPARENT_REGEXP = new RegExp( - '^[ \\t]*' + // whitespace - '([0-9a-f]{32})?' + // trace_id - '-?([0-9a-f]{16})?' + // span_id - '-?([01])?' + // sampled - '[ \\t]*$', // whitespace -); - /** * Keeps track of finished spans for a given transaction * @internal @@ -153,32 +145,6 @@ export class Span implements SpanInterface, SpanContext { } } - /** - * Continues a trace from a string (usually the header). - * @param traceparent Traceparent string - */ - public static fromTraceparent( - traceparent: string, - spanContext?: Pick>, - ): Span | undefined { - const matches = traceparent.match(TRACEPARENT_REGEXP); - if (matches) { - let sampled: boolean | undefined; - if (matches[3] === '1') { - sampled = true; - } else if (matches[3] === '0') { - sampled = false; - } - return new Span({ - ...spanContext, - parentSpanId: matches[2], - sampled, - traceId: matches[1], - }); - } - return undefined; - } - /** * @inheritDoc * @deprecated diff --git a/packages/tracing/src/utils.ts b/packages/tracing/src/utils.ts index f8ed980b2643..24ac56c764b3 100644 --- a/packages/tracing/src/utils.ts +++ b/packages/tracing/src/utils.ts @@ -1,4 +1,12 @@ -import { Options } from '@sentry/types'; +import { Options, TraceparentData } from '@sentry/types'; + +export const TRACEPARENT_REGEXP = new RegExp( + '^[ \\t]*' + // whitespace + '([0-9a-f]{32})?' + // trace_id + '-?([0-9a-f]{16})?' + // span_id + '-?([01])?' + // sampled + '[ \\t]*$', // whitespace +); /** * Determines if tracing is currently enabled. @@ -8,3 +16,28 @@ import { Options } from '@sentry/types'; export function hasTracingEnabled(options: Options): boolean { return 'tracesSampleRate' in options || 'tracesSampler' in options; } + +/** + * Extract transaction context data from a `sentry-trace` header. + * + * @param traceparent Traceparent string + * + * @returns Object containing data from the header, or undefined if traceparent string is malformed + */ +export function extractTraceparentData(traceparent: string): TraceparentData | undefined { + const matches = traceparent.match(TRACEPARENT_REGEXP); + if (matches) { + let parentSampled: boolean | undefined; + if (matches[3] === '1') { + parentSampled = true; + } else if (matches[3] === '0') { + parentSampled = false; + } + return { + traceId: matches[1], + parentSampled, + parentSpanId: matches[2], + }; + } + return undefined; +} diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts index 5fed6cfbf9d4..c8026974dc9d 100644 --- a/packages/tracing/test/span.test.ts +++ b/packages/tracing/test/span.test.ts @@ -100,42 +100,6 @@ describe('Span', () => { }); }); - describe('fromTraceparent', () => { - test('no sample', () => { - const from = Span.fromTraceparent('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb') as any; - - expect(from.parentSpanId).toEqual('bbbbbbbbbbbbbbbb'); - expect(from.traceId).toEqual('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'); - expect(from.spanId).not.toEqual('bbbbbbbbbbbbbbbb'); - expect(from.sampled).toBeUndefined(); - }); - test('sample true', () => { - const from = Span.fromTraceparent('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-1') as any; - expect(from.sampled).toBeTruthy(); - }); - - test('sample false', () => { - const from = Span.fromTraceparent('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-0') as any; - expect(from.sampled).toBeFalsy(); - }); - - test('just sample rate', () => { - const from = Span.fromTraceparent('0') as any; - expect(from.traceId).toHaveLength(32); - expect(from.spanId).toHaveLength(16); - expect(from.sampled).toBeFalsy(); - - const from2 = Span.fromTraceparent('1') as any; - expect(from2.traceId).toHaveLength(32); - expect(from2.spanId).toHaveLength(16); - expect(from2.sampled).toBeTruthy(); - }); - - test('invalid', () => { - expect(Span.fromTraceparent('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-x')).toBeUndefined(); - }); - }); - describe('toJSON', () => { test('simple', () => { const span = JSON.parse( diff --git a/packages/tracing/test/utils.test.ts b/packages/tracing/test/utils.test.ts new file mode 100644 index 000000000000..cda8fbbb062f --- /dev/null +++ b/packages/tracing/test/utils.test.ts @@ -0,0 +1,64 @@ +import { extractTraceparentData } from '../src/utils'; + +describe('extractTraceparentData', () => { + test('no sample', () => { + const data = extractTraceparentData('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb') as any; + + expect(data).toBeDefined(); + expect(data.parentSpanId).toEqual('bbbbbbbbbbbbbbbb'); + expect(data.traceId).toEqual('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'); + expect(data?.parentSampled).toBeUndefined(); + }); + + test('sample true', () => { + const data = extractTraceparentData('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-1') as any; + + expect(data).toBeDefined(); + expect(data.parentSampled).toBeTruthy(); + }); + + test('sample false', () => { + const data = extractTraceparentData('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-0') as any; + + expect(data).toBeDefined(); + expect(data.parentSampled).toBeFalsy(); + }); + + test('just sample decision - false', () => { + const data = extractTraceparentData('0') as any; + + expect(data).toBeDefined(); + expect(data.traceId).toBeUndefined(); + expect(data.spanId).toBeUndefined(); + expect(data.parentSampled).toBeFalsy(); + }); + + test('just sample decision - true', () => { + const data = extractTraceparentData('1') as any; + + expect(data).toBeDefined(); + expect(data.traceId).toBeUndefined(); + expect(data.spanId).toBeUndefined(); + expect(data.parentSampled).toBeTruthy(); + }); + + test('invalid', () => { + // trace id wrong length + expect(extractTraceparentData('a-bbbbbbbbbbbbbbbb-1')).toBeUndefined(); + + // parent span id wrong length + expect(extractTraceparentData('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-b-1')).toBeUndefined(); + + // parent sampling decision wrong length + expect(extractTraceparentData('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-11')).toBeUndefined(); + + // trace id invalid hex value + expect(extractTraceparentData('someStuffHereWhichIsNotAtAllHexy-bbbbbbbbbbbbbbbb-1')).toBeUndefined(); + + // parent span id invalid hex value + expect(extractTraceparentData('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-alsoNotSuperHexy-1')).toBeUndefined(); + + // bogus sampling decision + expect(extractTraceparentData('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-x')).toBeUndefined(); + }); +}); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 7a321cad1324..da4fa2f6129f 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -23,7 +23,13 @@ export { Span, SpanContext } from './span'; export { StackFrame } from './stackframe'; export { Stacktrace } from './stacktrace'; export { Status } from './status'; -export { CustomSamplingContext, SamplingContext, Transaction, TransactionContext } from './transaction'; +export { + CustomSamplingContext, + SamplingContext, + TraceparentData, + Transaction, + TransactionContext, +} from './transaction'; export { Thread } from './thread'; export { Transport, TransportOptions, TransportClass } from './transport'; export { User } from './user'; diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 80f433c61986..3888bcc890e3 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -5,7 +5,11 @@ import { Span, SpanContext } from './span'; * Interface holding Transaction-specific properties */ export interface TransactionContext extends SpanContext { + /** + * Human-readable identifier for the transaction + */ name: string; + /** * If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming * the duration of the transaction. This is useful to discard extra time in the transaction that is not @@ -20,6 +24,11 @@ export interface TransactionContext extends SpanContext { parentSampled?: boolean; } +/** + * Data pulled from a `sentry-trace` header + */ +export type TraceparentData = Pick; + /** * Transaction "Class", inherits Span only has `setName` */