From 7bba4caad8659b19978b931088300f05684f9bdb Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 8 Sep 2022 13:55:20 +0000 Subject: [PATCH 01/32] ref: Make dynamic sampling context mutable --- packages/core/src/envelope.ts | 9 +- .../wrappers/withSentryGetServerSideProps.ts | 5 +- .../withSentryServerSideAppGetInitialProps.ts | 6 +- ...ithSentryServerSideErrorGetInitialProps.ts | 5 +- .../withSentryServerSideGetInitialProps.ts | 5 +- .../src/config/wrappers/wrapperUtils.ts | 8 +- packages/nextjs/src/performance/client.ts | 5 +- packages/nextjs/src/utils/instrumentServer.ts | 8 +- packages/nextjs/src/utils/withSentry.ts | 8 +- packages/node/src/handlers.ts | 9 +- packages/node/src/integrations/http.ts | 48 ++-- packages/remix/src/utils/instrumentServer.ts | 13 +- packages/serverless/src/awslambda.ts | 9 +- packages/serverless/src/gcpfunction/http.ts | 9 +- .../tracing/src/browser/browsertracing.ts | 47 ++-- packages/tracing/src/browser/request.ts | 75 ++++-- packages/tracing/src/transaction.ts | 96 +++---- packages/types/src/baggage.ts | 22 -- packages/types/src/index.ts | 1 - packages/types/src/transaction.ts | 8 +- packages/utils/src/baggage.ts | 244 +++++++----------- 21 files changed, 280 insertions(+), 360 deletions(-) delete mode 100644 packages/types/src/baggage.ts diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index a6f04defc86d..130dd6fafb23 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -1,7 +1,5 @@ import { - Baggage, DsnComponents, - DynamicSamplingContext, Event, EventEnvelope, EventEnvelopeHeaders, @@ -13,7 +11,7 @@ import { SessionEnvelope, SessionItem, } from '@sentry/types'; -import { createEnvelope, dropUndefinedKeys, dsnToString, getSentryBaggageItems } from '@sentry/utils'; +import { createEnvelope, dropUndefinedKeys, dsnToString } from '@sentry/utils'; /** Extract sdk info from from the API metadata */ function getSdkMetadataForEnvelopeHeader(metadata?: SdkMetadata): SdkInfo | undefined { @@ -101,8 +99,7 @@ function createEventEnvelopeHeaders( tunnel: string | undefined, dsn: DsnComponents, ): EventEnvelopeHeaders { - const baggage: Baggage | undefined = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage; - const dynamicSamplingContext = baggage && getSentryBaggageItems(baggage); + const dynamicSamplingContext = event.sdkProcessingMetadata && event.sdkProcessingMetadata.dynamicSamplingContext; return { event_id: event.event_id as string, @@ -111,7 +108,7 @@ function createEventEnvelopeHeaders( ...(!!tunnel && { dsn: dsnToString(dsn) }), ...(event.type === 'transaction' && dynamicSamplingContext && { - trace: dropUndefinedKeys({ ...dynamicSamplingContext }) as DynamicSamplingContext, + trace: dropUndefinedKeys({ ...dynamicSamplingContext }), }), }; } diff --git a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts index dee5ce918c45..baad11081193 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts @@ -1,5 +1,5 @@ import { hasTracingEnabled } from '@sentry/tracing'; -import { serializeBaggage } from '@sentry/utils'; +import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils'; import { GetServerSideProps } from 'next'; import { isBuild } from '../../utils/isBuild'; @@ -44,8 +44,9 @@ export function withSentryGetServerSideProps( if ('props' in serverSideProps) { const requestTransaction = getTransactionFromRequest(req); if (requestTransaction) { + const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); serverSideProps.props._sentryTraceData = requestTransaction.toTraceparent(); - serverSideProps.props._sentryBaggage = serializeBaggage(requestTransaction.getBaggage()); + serverSideProps.props._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); } } diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts index 86cf92fb544e..5c099fd59f03 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts @@ -1,5 +1,5 @@ import { hasTracingEnabled } from '@sentry/tracing'; -import { serializeBaggage } from '@sentry/utils'; +import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils'; import App from 'next/app'; import { isBuild } from '../../utils/isBuild'; @@ -50,8 +50,10 @@ export function withSentryServerSideAppGetInitialProps(origAppGetInitialProps: A const requestTransaction = getTransactionFromRequest(req!); if (requestTransaction) { + const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); appGetInitialProps.pageProps._sentryTraceData = requestTransaction.toTraceparent(); - appGetInitialProps.pageProps._sentryBaggage = serializeBaggage(requestTransaction.getBaggage()); + appGetInitialProps.pageProps._sentryBaggage = + dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); } return appGetInitialProps; diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts index e7d4c7c5b46c..0c0af3715903 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts @@ -1,5 +1,5 @@ import { hasTracingEnabled } from '@sentry/tracing'; -import { serializeBaggage } from '@sentry/utils'; +import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils'; import { NextPageContext } from 'next'; import { ErrorProps } from 'next/error'; @@ -51,8 +51,9 @@ export function withSentryServerSideErrorGetInitialProps( const requestTransaction = getTransactionFromRequest(req!); if (requestTransaction) { + const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); errorGetInitialProps._sentryTraceData = requestTransaction.toTraceparent(); - errorGetInitialProps._sentryBaggage = serializeBaggage(requestTransaction.getBaggage()); + errorGetInitialProps._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); } return errorGetInitialProps; diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts index 8837ae121cf5..d90055ad76f5 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts @@ -1,5 +1,5 @@ import { hasTracingEnabled } from '@sentry/tracing'; -import { serializeBaggage } from '@sentry/utils'; +import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils'; import { NextPage } from 'next'; import { isBuild } from '../../utils/isBuild'; @@ -41,8 +41,9 @@ export function withSentryServerSideGetInitialProps(origGetInitialProps: GetInit const requestTransaction = getTransactionFromRequest(req!); if (requestTransaction) { + const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); initialProps._sentryTraceData = requestTransaction.toTraceparent(); - initialProps._sentryBaggage = serializeBaggage(requestTransaction.getBaggage()); + initialProps._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); } return initialProps; diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index a7be6784dbf6..70411ec1e238 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -2,7 +2,7 @@ import { captureException, getCurrentHub, startTransaction } from '@sentry/core' import { addRequestDataToEvent } from '@sentry/node'; import { getActiveTransaction } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; -import { extractTraceparentData, fill, isString, parseBaggageSetMutability } from '@sentry/utils'; +import { baggageHeaderToDynamicSamplingContext, extractTraceparentData, fill } from '@sentry/utils'; import * as domain from 'domain'; import { IncomingMessage, ServerResponse } from 'http'; @@ -88,11 +88,11 @@ export function callTracedServerSideDataFetcher Pr if (requestTransaction === undefined) { const sentryTraceHeader = req.headers['sentry-trace']; - const rawBaggageString = req.headers && isString(req.headers.baggage) && req.headers.baggage; + const rawBaggageString = req.headers && req.headers.baggage; const traceparentData = typeof sentryTraceHeader === 'string' ? extractTraceparentData(sentryTraceHeader) : undefined; - const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData); + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(rawBaggageString); const newTransaction = startTransaction({ op: 'nextjs.data.server', @@ -100,7 +100,7 @@ export function callTracedServerSideDataFetcher Pr ...traceparentData, metadata: { source: 'route', - baggage, + dynamicSamplingContext, }, }); diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index 764d11e27fe8..6cbbdba61bb7 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -1,10 +1,10 @@ import { getCurrentHub } from '@sentry/hub'; import { Primitive, TraceparentData, Transaction, TransactionContext, TransactionSource } from '@sentry/types'; import { + baggageHeaderToDynamicSamplingContext, extractTraceparentData, getGlobalObject, logger, - parseBaggageHeader, stripUrlQueryAndFragment, } from '@sentry/utils'; import type { NEXT_DATA as NextData } from 'next/dist/next-server/lib/utils'; @@ -128,6 +128,7 @@ export function nextRouterInstrumentation( if (startTransactionOnPageLoad) { const source = route ? 'route' : 'url'; + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggage); activeTransaction = startTransactionCb({ name: prevLocationName, @@ -137,7 +138,7 @@ export function nextRouterInstrumentation( ...traceParentData, metadata: { source, - ...(baggage && { baggage: parseBaggageHeader(baggage) }), + ...(dynamicSamplingContext && { dynamicSamplingContext }), }, }); } diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 2d5bcba3161b..88b0a4537a4d 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -10,10 +10,10 @@ import { import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; import { addExceptionMechanism, + baggageHeaderToDynamicSamplingContext, fill, isString, logger, - parseBaggageSetMutability, stripUrlQueryAndFragment, } from '@sentry/utils'; import * as domain from 'domain'; @@ -255,8 +255,8 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { __DEBUG_BUILD__ && logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`); } - const rawBaggageString = nextReq.headers && isString(nextReq.headers.baggage) && nextReq.headers.baggage; - const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData); + const baggageHeader = nextReq.headers && nextReq.headers.baggage; + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); // pull off query string, if any const reqPath = stripUrlQueryAndFragment(nextReq.url); @@ -270,7 +270,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { name: `${namePrefix}${reqPath}`, op: 'http.server', metadata: { - baggage, + dynamicSamplingContext, requestPath: reqPath, // TODO: Investigate if there's a way to tell if this is a dynamic route, so that we can make this more // like `source: isDynamicRoute? 'url' : 'route'` diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index d1b249bf87a3..3d459f9f20a1 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -3,10 +3,10 @@ import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; import { addExceptionMechanism, + baggageHeaderToDynamicSamplingContext, isString, logger, objectify, - parseBaggageSetMutability, stripUrlQueryAndFragment, } from '@sentry/utils'; import * as domain from 'domain'; @@ -51,8 +51,8 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = __DEBUG_BUILD__ && logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`); } - const rawBaggageString = req.headers && isString(req.headers.baggage) && req.headers.baggage; - const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData); + const baggageHeader = req.headers && req.headers.baggage; + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); const url = `${req.url}`; // pull off query string, if any @@ -72,7 +72,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = name: `${reqMethod}${reqPath}`, op: 'http.server', ...traceparentData, - metadata: { baggage, source: 'route' }, + metadata: { dynamicSamplingContext, source: 'route' }, }, // extra context passed to the `tracesSampler` { request: req }, diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index df67d7fe3755..798f84b5a767 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -4,11 +4,11 @@ import { Event, Span } from '@sentry/types'; import { AddRequestDataToEventOptions, addRequestDataToTransaction, + baggageHeaderToDynamicSamplingContext, extractPathForTransaction, extractTraceparentData, isString, logger, - parseBaggageSetMutability, } from '@sentry/utils'; import * as domain from 'domain'; import * as http from 'http'; @@ -54,9 +54,8 @@ export function tracingHandler(): ( // If there is a trace header set, we extract the data from it (parentSpanId, traceId, and sampling decision) const traceparentData = req.headers && isString(req.headers['sentry-trace']) && extractTraceparentData(req.headers['sentry-trace']); - const rawBaggageString = req.headers && isString(req.headers.baggage) && req.headers.baggage; - - const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData); + const incomingBaggageHeaders = req.headers?.baggage; + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(incomingBaggageHeaders); const [name, source] = extractPathForTransaction(req, { path: true, method: true }); const transaction = startTransaction( @@ -64,7 +63,7 @@ export function tracingHandler(): ( name, op: 'http.server', ...traceparentData, - metadata: { baggage, source }, + metadata: { dynamicSamplingContext, source }, }, // extra context passed to the tracesSampler { request: extractRequestData(req) }, diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 9edd5d8c3d34..fd3afb229397 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,6 +1,12 @@ import { getCurrentHub, Hub } from '@sentry/core'; import { EventProcessor, Integration, Span, TracePropagationTargets } from '@sentry/types'; -import { fill, isMatchingPattern, logger, mergeAndSerializeBaggage, parseSemver } from '@sentry/utils'; +import { + dynamicSamplingContextToSentryBaggageHeader, + fill, + isMatchingPattern, + logger, + parseSemver, +} from '@sentry/utils'; import * as http from 'http'; import * as https from 'https'; @@ -137,7 +143,7 @@ function _createWrappedRequestMethodFactory( return originalRequestMethod.apply(httpModule, requestArgs); } - let span: Span | undefined; + let requestSpan: Span | undefined; let parentSpan: Span | undefined; const scope = getCurrentHub().getScope(); @@ -146,26 +152,36 @@ function _createWrappedRequestMethodFactory( parentSpan = scope.getSpan(); if (parentSpan) { - span = parentSpan.startChild({ + requestSpan = parentSpan.startChild({ description: `${requestOptions.method || 'GET'} ${requestUrl}`, op: 'http.client', }); if (shouldAttachTraceData(requestUrl)) { - const sentryTraceHeader = span.toTraceparent(); + const sentryTraceHeader = requestSpan.toTraceparent(); __DEBUG_BUILD__ && logger.log( `[Tracing] Adding sentry-trace header ${sentryTraceHeader} to outgoing request to "${requestUrl}": `, ); - const baggage = parentSpan.transaction && parentSpan.transaction.getBaggage(); - const headerBaggageString = requestOptions.headers && requestOptions.headers.baggage; - requestOptions.headers = { ...requestOptions.headers, 'sentry-trace': sentryTraceHeader, - baggage: mergeAndSerializeBaggage(baggage, headerBaggageString), }; + + if (parentSpan.transaction) { + const dynamicSamplingContext = parentSpan.transaction.getDynamicSamplingContext(); + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); + const newBaggageHeaderField = + sentryBaggageHeader && requestOptions.headers && Array.isArray(requestOptions.headers.baggage) + ? requestOptions.headers.baggage.concat(sentryBaggageHeader) + : sentryBaggageHeader; + + requestOptions.headers = { + ...requestOptions.headers, + baggage: newBaggageHeaderField, + }; + } } else { __DEBUG_BUILD__ && logger.log( @@ -184,12 +200,12 @@ function _createWrappedRequestMethodFactory( if (breadcrumbsEnabled) { addRequestBreadcrumb('response', requestUrl, req, res); } - if (tracingEnabled && span) { + if (tracingEnabled && requestSpan) { if (res.statusCode) { - span.setHttpStatus(res.statusCode); + requestSpan.setHttpStatus(res.statusCode); } - span.description = cleanSpanDescription(span.description, requestOptions, req); - span.finish(); + requestSpan.description = cleanSpanDescription(requestSpan.description, requestOptions, req); + requestSpan.finish(); } }) .once('error', function (this: http.ClientRequest): void { @@ -199,10 +215,10 @@ function _createWrappedRequestMethodFactory( if (breadcrumbsEnabled) { addRequestBreadcrumb('error', requestUrl, req); } - if (tracingEnabled && span) { - span.setHttpStatus(500); - span.description = cleanSpanDescription(span.description, requestOptions, req); - span.finish(); + if (tracingEnabled && requestSpan) { + requestSpan.setHttpStatus(500); + requestSpan.description = cleanSpanDescription(requestSpan.description, requestOptions, req); + requestSpan.finish(); } }); }; diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 36a62a3749c0..e5f56c11bdf1 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -4,14 +4,13 @@ import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; import { Transaction, TransactionSource, WrappedFunction } from '@sentry/types'; import { addExceptionMechanism, + baggageHeaderToDynamicSamplingContext, + dynamicSamplingContextToSentryBaggageHeader, extractTraceparentData, fill, isNodeEnv, - isSentryBaggageEmpty, loadModule, logger, - parseBaggageSetMutability, - serializeBaggage, } from '@sentry/utils'; import * as domain from 'domain'; @@ -194,9 +193,11 @@ function getTraceAndBaggage(): { sentryTrace?: string; sentryBaggage?: string } const span = currentScope.getSpan(); if (span && transaction) { + const dynamicSamplingContext = transaction.getDynamicSamplingContext(); + return { sentryTrace: span.toTraceparent(), - sentryBaggage: serializeBaggage(transaction.getBaggage()), + sentryBaggage: dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext), }; } } @@ -312,7 +313,7 @@ export function startRequestHandlerTransaction( ): Transaction { // If there is a trace header set, we extract the data from it (parentSpanId, traceId, and sampling decision) const traceparentData = extractTraceparentData(request.headers['sentry-trace']); - const baggage = parseBaggageSetMutability(request.headers.baggage, traceparentData); + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(request.headers.baggage); const transaction = hub.startTransaction({ name, @@ -324,7 +325,7 @@ export function startRequestHandlerTransaction( metadata: { source, // Only attach baggage if it's defined - ...(!isSentryBaggageEmpty(baggage) && { baggage }), + ...(dynamicSamplingContext && { dynamicSamplingContext }), }, }); diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts index 6cd916270cb5..1692ffa6b44e 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -3,7 +3,7 @@ import * as Sentry from '@sentry/node'; import { captureException, captureMessage, flush, getCurrentHub, Scope, withScope } from '@sentry/node'; import { extractTraceparentData } from '@sentry/tracing'; import { Integration } from '@sentry/types'; -import { dsnFromString, dsnToString, isString, logger, parseBaggageSetMutability } from '@sentry/utils'; +import { dsnFromString, dsnToString, isString, logger, baggageHeaderToDynamicSamplingContext } from '@sentry/utils'; // NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil // eslint-disable-next-line import/no-unresolved import { Context, Handler } from 'aws-lambda'; @@ -308,9 +308,8 @@ export function wrapHandler( traceparentData = extractTraceparentData(eventWithHeaders.headers['sentry-trace']); } - const rawBaggageString = - eventWithHeaders.headers && isString(eventWithHeaders.headers.baggage) && eventWithHeaders.headers.baggage; - const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData); + const baggageHeader = eventWithHeaders.headers && eventWithHeaders.headers.baggage; + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); const hub = getCurrentHub(); @@ -318,7 +317,7 @@ export function wrapHandler( name: context.functionName, op: 'awslambda.handler', ...traceparentData, - metadata: { baggage, source: 'component' }, + metadata: { dynamicSamplingContext, source: 'component' }, }); const scope = hub.pushScope(); diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index 99cec2811fae..102550678649 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -6,7 +6,7 @@ import { getCurrentHub, } from '@sentry/node'; import { extractTraceparentData } from '@sentry/tracing'; -import { isString, logger, parseBaggageSetMutability, stripUrlQueryAndFragment } from '@sentry/utils'; +import { baggageHeaderToDynamicSamplingContext, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils'; import { domainify, getActiveDomain, proxyFunction } from './../utils'; import { HttpFunction, WrapperOptions } from './general'; @@ -77,10 +77,9 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial tags. - * @returns Transaction context data or undefined neither tag exists or has valid data - */ -export function extractTraceDataFromMetaTags(): Partial | undefined { - const sentrytraceValue = getMetaContent('sentry-trace'); - const baggageValue = getMetaContent('baggage'); - - const sentrytraceData = sentrytraceValue ? extractTraceparentData(sentrytraceValue) : undefined; - const baggage = parseBaggageSetMutability(baggageValue, sentrytraceValue); - - // TODO more extensive checks for baggage validity/emptyness? - if (sentrytraceData || baggage) { - return { - ...(sentrytraceData && sentrytraceData), - ...(baggage && { metadata: { baggage } }), - }; - } - - return undefined; -} - /** Returns the value of a meta tag */ export function getMetaContent(metaName: string): string | null { // Can't specify generic to `getDomElement` because tracing can be used diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index ae67d5b6ae19..db7fbb401cff 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -1,11 +1,11 @@ /* eslint-disable max-lines */ -import type { Baggage, Span } from '@sentry/types'; +import type { DynamicSamplingContext, Span } from '@sentry/types'; import { addInstrumentationHandler, BAGGAGE_HEADER_NAME, + dynamicSamplingContextToSentryBaggageHeader, isInstanceOf, isMatchingPattern, - mergeAndSerializeBaggage, } from '@sentry/utils'; import { getActiveTransaction, hasTracingEnabled } from '../utils'; @@ -195,16 +195,21 @@ export function fetchCallback( handlerData.fetchData.__span = span.spanId; spans[span.spanId] = span; - const request = (handlerData.args[0] = handlerData.args[0] as string | Request); + const request = handlerData.args[0] as string | Request; // eslint-disable-next-line @typescript-eslint/no-explicit-any - const options = (handlerData.args[1] = (handlerData.args[1] as { [key: string]: any }) || {}); - options.headers = addTracingHeaders(request, activeTransaction.getBaggage(), span, options); + const options = (handlerData.args[1] as { [key: string]: any }) || {}; + options.headers = addTracingHeadersToFetchRequest( + request, + activeTransaction.getDynamicSamplingContext(), + span, + options, + ); } } -function addTracingHeaders( +function addTracingHeadersToFetchRequest( request: string | Request, - incomingBaggage: Baggage | undefined, + dynamicSamplingContext: Partial | undefined, span: Span, options: { [key: string]: any }, ): PolymorphicRequestHeaders { @@ -214,30 +219,48 @@ function addTracingHeaders( headers = (request as Request).headers; } + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); + const sentryTraceHeader = span.toTraceparent(); + if (headers) { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access if (typeof headers.append === 'function') { + // If the same header is appended miultiple times the browser will merge the values into a single request header. + // Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header. + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - headers.append('sentry-trace', span.toTraceparent()); + headers.append('sentry-trace', sentryTraceHeader); // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - headers.append(BAGGAGE_HEADER_NAME, mergeAndSerializeBaggage(incomingBaggage, headers.get(BAGGAGE_HEADER_NAME))); + headers.append(BAGGAGE_HEADER_NAME, sentryBaggageHeader); } else if (Array.isArray(headers)) { - const [, headerBaggageString] = headers.find(([key, _]) => key === BAGGAGE_HEADER_NAME); - headers = [ - ...headers, - ['sentry-trace', span.toTraceparent()], - [BAGGAGE_HEADER_NAME, mergeAndSerializeBaggage(incomingBaggage, headerBaggageString)], - ]; + headers.push(['sentry-trace', sentryTraceHeader]); + + if (sentryBaggageHeader) { + // If there are multiple entries with the same key, the browser will merge the values into a single request header. + // Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header. + headers.push([BAGGAGE_HEADER_NAME, sentryBaggageHeader]); + } } else { + const baggageHeaders: string[] = []; + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (headers.baggage) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + baggageHeaders.push(headers.baggage); + } + + if (sentryBaggageHeader) { + baggageHeaders.push(sentryBaggageHeader); + } + headers = { ...headers, - 'sentry-trace': span.toTraceparent(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - baggage: mergeAndSerializeBaggage(incomingBaggage, headers.baggage), + 'sentry-trace': sentryTraceHeader, + baggage: baggageHeaders.join(', '), }; } } else { - headers = { 'sentry-trace': span.toTraceparent(), baggage: mergeAndSerializeBaggage(incomingBaggage) }; + headers = { 'sentry-trace': sentryTraceHeader, baggage: sentryBaggageHeader }; } return headers; } @@ -297,13 +320,15 @@ export function xhrCallback( try { handlerData.xhr.setRequestHeader('sentry-trace', span.toTraceparent()); - const headerBaggageString = - handlerData.xhr.getRequestHeader && handlerData.xhr.getRequestHeader(BAGGAGE_HEADER_NAME); + const dynamicSamplingContext = activeTransaction.getDynamicSamplingContext(); + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - handlerData.xhr.setRequestHeader( - BAGGAGE_HEADER_NAME, - mergeAndSerializeBaggage(activeTransaction.getBaggage(), headerBaggageString), - ); + if (sentryBaggageHeader) { + // From MDN: "If this method is called several times with the same header, the values are merged into one single request header." + // We can therefore simply set a baggage header without checking what was there before + // https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/setRequestHeader + handlerData.xhr.setRequestHeader(BAGGAGE_HEADER_NAME, sentryBaggageHeader); + } } catch (_) { // Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED. } diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index 2b509763743d..3046d96612b5 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -1,7 +1,6 @@ import { getCurrentHub, Hub } from '@sentry/hub'; import { - Baggage, - BaggageObj, + DynamicSamplingContext, Event, Measurements, MeasurementUnit, @@ -9,7 +8,7 @@ import { TransactionContext, TransactionMetadata, } from '@sentry/types'; -import { createBaggage, dropUndefinedKeys, getSentryBaggageItems, isBaggageMutable, logger } from '@sentry/utils'; +import { dropUndefinedKeys, logger } from '@sentry/utils'; import { Span as SpanClass, SpanRecorder } from './span'; @@ -28,6 +27,10 @@ export class Transaction extends SpanClass implements TransactionInterface { private _trimEnd?: boolean; + // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) + // This value is null + // private _frozenDynamicSamplingContext: Partial | undefined = undefined; + /** * This constructor should never be called manually. Those instrumenting tracing should use * `Sentry.startTransaction()`, and internal methods should use `hub.startTransaction()`. @@ -51,6 +54,11 @@ export class Transaction extends SpanClass implements TransactionInterface { // this is because transactions are also spans, and spans have a transaction pointer this.transaction = this; + + // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) + // if ((transactionContext.metadata || {}).dynamicSamplingContext) { + // this.freezeDynamicSamplingContext(); + // } } /** Getter for `name` property */ @@ -137,6 +145,11 @@ export class Transaction extends SpanClass implements TransactionInterface { }).endTimestamp; } + // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) + // if ((transactionContext.metadata || {}).dynamicSamplingContext) { + // this.freezeDynamicSamplingContext(); + // } + const metadata = this.metadata; const transaction: Event = { @@ -151,7 +164,7 @@ export class Transaction extends SpanClass implements TransactionInterface { type: 'transaction', sdkProcessingMetadata: { ...metadata, - baggage: this.getBaggage(), + dynamicSamplingContext: this.getDynamicSamplingContext(), }, ...(metadata.source && { transaction_info: { @@ -203,73 +216,46 @@ export class Transaction extends SpanClass implements TransactionInterface { } /** - * @inheritdoc * - * @experimental */ - public getBaggage(): Baggage { - const existingBaggage = this.metadata.baggage; - - // Only add Sentry baggage items to baggage, if baggage does not exist yet or it is still - // empty and mutable - const finalBaggage = - !existingBaggage || isBaggageMutable(existingBaggage) - ? this._populateBaggageWithSentryValues(existingBaggage) - : existingBaggage; - - // Update the baggage stored on the transaction. - this.metadata.baggage = finalBaggage; + public getDynamicSamplingContext(): Partial | undefined { + // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) + // if (this._frozenDynamicSamplingContext) { + // return this._frozenDynamicSamplingContext; + // } - return finalBaggage; - } - - /** - * Collects and adds data to the passed baggage object. - * - * Note: This function does not explicitly check if the passed baggage object is allowed - * to be modified. Implicitly, `setBaggageValue` will not make modification to the object - * if it was already set immutable. - * - * After adding the data, the baggage object is set immutable to prevent further modifications. - * - * @param baggage - * - * @returns modified and immutable baggage object - */ - private _populateBaggageWithSentryValues(baggage: Baggage = createBaggage({})): Baggage { const hub: Hub = this._hub || getCurrentHub(); const client = hub && hub.getClient(); - if (!client) return baggage; + if (!client) return undefined; const { environment, release } = client.getOptions() || {}; const { publicKey: public_key } = client.getDsn() || {}; - const sample_rate = - this.metadata && - this.metadata.transactionSampling && - this.metadata.transactionSampling.rate && - this.metadata.transactionSampling.rate.toString(); + const maybeSampleRate = (this.metadata.transactionSampling || {}).rate; + const sample_rate = maybeSampleRate !== undefined ? maybeSampleRate.toString() : undefined; const scope = hub.getScope(); const { segment: user_segment } = (scope && scope.getUser()) || {}; const source = this.metadata.source; + + // We don't want to have a transaction name in the DSC if the source is "url" because URLs might contain PII const transaction = source && source !== 'url' ? this.name : undefined; - return createBaggage( - dropUndefinedKeys({ - environment, - release, - transaction, - user_segment, - public_key, - trace_id: this.traceId, - sample_rate, - ...getSentryBaggageItems(baggage), // keep user-added values - } as BaggageObj), - '', - false, // set baggage immutable - ); + const dsc = dropUndefinedKeys({ + environment, + release, + transaction, + user_segment, + public_key, + trace_id: this.traceId, + sample_rate, + }); + + // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) + // this._frozenDynamicSamplingContext = dsc; + + return dsc; } } diff --git a/packages/types/src/baggage.ts b/packages/types/src/baggage.ts deleted file mode 100644 index d347b2aa9036..000000000000 --- a/packages/types/src/baggage.ts +++ /dev/null @@ -1,22 +0,0 @@ -import { DynamicSamplingContext } from './envelope'; - -export type AllowedBaggageKeys = keyof DynamicSamplingContext; - -export type BaggageObj = Partial & Record>; - -/** - * The baggage data structure represents key,value pairs based on the baggage - * spec: https://www.w3.org/TR/baggage - * - * It is expected that users interact with baggage using the helpers methods: - * `createBaggage`, `getBaggageValue`, and `setBaggageValue`. - * - * Internally, the baggage data structure is a tuple of length 3, separating baggage values - * based on if they are related to Sentry or not. If the baggage values are - * set/used by sentry, they will be stored in an object to be easily accessed. - * If they are not, they are kept as a string to be only accessed when serialized - * at baggage propagation time. - * The third tuple member controls the mutability of the baggage. If it is `false`, - * the baggage can no longer longer be modified (i.e. is immutable). - */ -export type Baggage = [BaggageObj, string, boolean]; diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index c43bb34c8f8e..14745c491f47 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -1,5 +1,4 @@ export type { Attachment } from './attachment'; -export type { AllowedBaggageKeys, Baggage, BaggageObj } from './baggage'; export type { Breadcrumb, BreadcrumbHint } from './breadcrumb'; export type { Client } from './client'; export type { ClientReport, Outcome, EventDropReason } from './clientreport'; diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index d8029c2184cb..9251582d7d18 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -1,4 +1,4 @@ -import { Baggage } from './baggage'; +import { DynamicSamplingContext } from './envelope'; import { MeasurementUnit } from './measurement'; import { ExtractedNodeRequestData, Primitive, WorkerLocation } from './misc'; import { Span, SpanContext } from './span'; @@ -95,8 +95,7 @@ export interface Transaction extends TransactionContext, Span { */ setMetadata(newMetadata: Partial): void; - /** return the baggage for dynamic sampling and trace propagation */ - getBaggage(): Baggage; + getDynamicSamplingContext(): Partial | undefined; } /** @@ -139,8 +138,7 @@ export type TransactionSamplingMethod = 'explicitly_set' | 'client_sampler' | 'c export interface TransactionMetadata { transactionSampling?: { rate?: number; method: TransactionSamplingMethod }; - /** The baggage object of a transaction's baggage header, used for dynamic sampling */ - baggage?: Baggage; + dynamicSamplingContext?: Partial; /** For transactions tracing server-side request handling, the path of the request being tracked. */ requestPath?: string; diff --git a/packages/utils/src/baggage.ts b/packages/utils/src/baggage.ts index 9ca8d34df1ae..8a92ea150064 100644 --- a/packages/utils/src/baggage.ts +++ b/packages/utils/src/baggage.ts @@ -1,4 +1,4 @@ -import { Baggage, BaggageObj, HttpHeaderValue, TraceparentData } from '@sentry/types'; +import { DynamicSamplingContext } from '@sentry/types'; import { isString } from './is'; import { logger } from './logger'; @@ -16,180 +16,114 @@ export const SENTRY_BAGGAGE_KEY_PREFIX_REGEX = /^sentry-/; */ export const MAX_BAGGAGE_STRING_LENGTH = 8192; -/** Create an instance of Baggage */ -export function createBaggage(initItems: BaggageObj, baggageString: string = '', mutable: boolean = true): Baggage { - return [{ ...initItems }, baggageString, mutable]; -} - -/** Get a value from baggage */ -export function getBaggageValue(baggage: Baggage, key: keyof BaggageObj): BaggageObj[keyof BaggageObj] { - return baggage[0][key]; -} - -/** Add a value to baggage */ -export function setBaggageValue(baggage: Baggage, key: keyof BaggageObj, value: BaggageObj[keyof BaggageObj]): void { - if (isBaggageMutable(baggage)) { - baggage[0][key] = value; - } -} - -/** Check if the Sentry part of the passed baggage (i.e. the first element in the tuple) is empty */ -export function isSentryBaggageEmpty(baggage: Baggage): boolean { - return Object.keys(baggage[0]).length === 0; -} - -/** Returns Sentry specific baggage values */ -export function getSentryBaggageItems(baggage: Baggage): BaggageObj { - return baggage[0]; -} - /** - * Returns 3rd party baggage string of @param baggage - * @param baggage + * TODO + * @param baggageHeader */ -export function getThirdPartyBaggage(baggage: Baggage): string { - return baggage[1]; -} - -/** - * Checks if baggage is mutable - * @param baggage - * @returns true if baggage is mutable, else false - */ -export function isBaggageMutable(baggage: Baggage): boolean { - return baggage[2]; -} +export function baggageHeaderToDynamicSamplingContext( + // Very liberal definition of what any incoming header might look like + baggageHeader: string | string[] | number | null | undefined | boolean, +): Partial | undefined { + if (!isString(baggageHeader) && !Array.isArray(baggageHeader)) { + return undefined; + } -/** - * Sets the passed baggage immutable - * @param baggage - */ -export function setBaggageImmutable(baggage: Baggage): void { - baggage[2] = false; -} + // Intermediary object to store baggage key value pairs of incoming baggage headers on. + // It is later used to read Sentry-DSC-values from. + let baggageObject: Readonly> = {}; + + if (Array.isArray(baggageHeader)) { + // Combine all baggage headers into one object containing the baggage values so we can later read the Sentry-DSC-values from it + baggageObject = baggageHeader.reduce>((acc, curr) => { + const currBaggageObject = baggageHeaderToObject(curr); + return { + ...acc, + ...currBaggageObject, + }; + }, {}); + } else { + baggageObject = baggageHeaderToObject(baggageHeader); + } -/** Serialize a baggage object */ -export function serializeBaggage(baggage: Baggage): string { - return Object.keys(baggage[0]).reduce((prev, key: keyof BaggageObj) => { - const val = baggage[0][key] as string; - const baggageEntry = `${SENTRY_BAGGAGE_KEY_PREFIX}${encodeURIComponent(key)}=${encodeURIComponent(val)}`; - const newVal = prev === '' ? baggageEntry : `${prev},${baggageEntry}`; - if (newVal.length > MAX_BAGGAGE_STRING_LENGTH) { - __DEBUG_BUILD__ && - logger.warn(`Not adding key: ${key} with val: ${val} to baggage due to exceeding baggage size limits.`); - return prev; - } else { - return newVal; + // Read all "sentry-" prefixed values out of the baggage object and put it onto a dynamic sampling context object. + const dynamicSamplingContext = Object.entries(baggageObject).reduce>((acc, [key, value]) => { + if (key.match(SENTRY_BAGGAGE_KEY_PREFIX_REGEX)) { + const nonPrefixedKey = key.slice(SENTRY_BAGGAGE_KEY_PREFIX.length); + acc[nonPrefixedKey] = value; } - }, baggage[1]); + return acc; + }, {}); + + // Only return a dynamic sampling context object if there are keys in it. + // A keyless object means there were no sentry values on the header, which means that there is no DSC. + if (Object.keys(dynamicSamplingContext).length > 0) { + return dynamicSamplingContext as Partial; + } else { + return undefined; + } } /** - * Parse a baggage header from a string or a string array and return a Baggage object - * - * If @param includeThirdPartyEntries is set to true, third party baggage entries are added to the Baggage object - * (This is necessary for merging potentially pre-existing baggage headers in outgoing requests with - * our `sentry-` values) + * TODO + * @param dynamicSamplingContext */ -export function parseBaggageHeader( - inputBaggageValue: HttpHeaderValue, - includeThirdPartyEntries: boolean = false, -): Baggage { - // Adding this check here because we got reports of this function failing due to the input value - // not being a string. This debug log might help us determine what's going on here. - if ((!Array.isArray(inputBaggageValue) && !isString(inputBaggageValue)) || typeof inputBaggageValue === 'number') { - __DEBUG_BUILD__ && - logger.warn( - '[parseBaggageHeader] Received input value of incompatible type: ', - typeof inputBaggageValue, - inputBaggageValue, - ); - - // Gonna early-return an empty baggage object so that we don't fail later on - return createBaggage({}, ''); +export function dynamicSamplingContextToSentryBaggageHeader( + // this also takes undefined for convenience and bundle size in other places + dynamicSamplingContext: Partial | undefined, +): string | undefined { + if (!dynamicSamplingContext) { + return undefined; } - const baggageEntries = (isString(inputBaggageValue) ? inputBaggageValue : inputBaggageValue.join(',')) - .split(',') - .map(entry => entry.trim()) - .filter(entry => entry !== '' && (includeThirdPartyEntries || SENTRY_BAGGAGE_KEY_PREFIX_REGEX.test(entry))); - - return baggageEntries.reduce( - ([baggageObj, baggageString], curr) => { - const [key, val] = curr.split('='); - if (SENTRY_BAGGAGE_KEY_PREFIX_REGEX.test(key)) { - const baggageKey = decodeURIComponent(key.split('-')[1]); - return [ - { - ...baggageObj, - [baggageKey]: decodeURIComponent(val), - }, - baggageString, - true, - ]; - } else { - return [baggageObj, baggageString === '' ? curr : `${baggageString},${curr}`, true]; + // Prefix all DSC keys with "sentry-" and put them into a new object + const sentryPrefixedDSC = Object.entries(dynamicSamplingContext).reduce>( + (acc, [dscKey, dscValue]) => { + if (dscValue) { + acc[`${SENTRY_BAGGAGE_KEY_PREFIX}${dscKey}`] = dscValue; } + return acc; }, - [{}, '', true], + {}, ); + + return objectToBaggageHeader(sentryPrefixedDSC); } /** - * Merges the baggage header we saved from the incoming request (or meta tag) with - * a possibly created or modified baggage header by a third party that's been added - * to the outgoing request header. - * - * In case @param headerBaggageString exists, we can safely add the the 3rd party part of @param headerBaggage - * with our @param incomingBaggage. This is possible because if we modified anything beforehand, - * it would only affect parts of the sentry baggage (@see Baggage interface). - * - * @param incomingBaggage the baggage header of the incoming request that might contain sentry entries - * @param thirdPartyBaggageHeader possibly existing baggage header string or string[] added from a third - * party to the request headers - * - * @return a merged and serialized baggage string to be propagated with the outgoing request + * TODO + * @param baggageHeader */ -export function mergeAndSerializeBaggage(incomingBaggage?: Baggage, thirdPartyBaggageHeader?: HttpHeaderValue): string { - if (!incomingBaggage && !thirdPartyBaggageHeader) { - return ''; - } - - const headerBaggage = (thirdPartyBaggageHeader && parseBaggageHeader(thirdPartyBaggageHeader, true)) || undefined; - const thirdPartyHeaderBaggage = headerBaggage && getThirdPartyBaggage(headerBaggage); - - const finalBaggage = createBaggage((incomingBaggage && incomingBaggage[0]) || {}, thirdPartyHeaderBaggage || ''); - return serializeBaggage(finalBaggage); +export function baggageHeaderToObject(baggageHeader: string): Record { + return baggageHeader + .split(',') + .map(baggageEntry => baggageEntry.split('=').map(keyOrValue => decodeURIComponent(keyOrValue.trim()))) + .reduce>((acc, [key, value]) => { + acc[key] = value; + return acc; + }, {}); } /** - * Helper function that takes a raw baggage value (if available) and the processed sentry-trace header - * data (if available), parses the baggage value and creates a Baggage object. If there is no baggage - * value, it will create an empty Baggage object. - * - * In a second step, this functions determines if the created Baggage object should be set immutable - * to prevent mutation of the Sentry data. It does this by looking at the processed sentry-trace header. - * - * @param rawBaggageValue baggage value from header - * @param sentryTraceHeader processed Sentry trace header returned from `extractTraceparentData` + * TODO + * @param object */ -export function parseBaggageSetMutability( - rawBaggageValue: HttpHeaderValue | false | undefined, - sentryTraceHeader: TraceparentData | string | false | undefined | null, -): Baggage { - const baggage = parseBaggageHeader(rawBaggageValue || ''); - - // Because we are always creating a Baggage object by calling `parseBaggageHeader` above - // (either a filled one or an empty one, even if we didn't get a `baggage` header), - // we only need to check if we have a sentry-trace header or not. As soon as we have it, - // we set baggage immutable. In case we don't get a sentry-trace header, we can assume that - // this SDK is the head of the trace and thus we still permit mutation at this time. - // There is one exception though, which is that we get a baggage-header with `sentry-` - // items but NO sentry-trace header. In this case we also set the baggage immutable for now - // but if something like this would ever happen, we should revisit this and determine - // what this would actually mean for the trace (i.e. is this SDK the head?, what happened - // before that we don't have a sentry-trace header?, etc) - (sentryTraceHeader || !isSentryBaggageEmpty(baggage)) && setBaggageImmutable(baggage); - - return baggage; +export function objectToBaggageHeader(object: Record): string | undefined { + if (Object.keys(object).length === 0) { + // An empty baggage header is not spec compliant: We return undefined. + return undefined; + } + + return Object.entries(object).reduce((baggageHeader, [objectKey, objectValue], currentIndex) => { + const baggageEntry = `${encodeURIComponent(objectKey)}=${encodeURIComponent(objectValue)}`; + const newBaggageHeader = currentIndex === 0 ? baggageEntry : `${baggageHeader},${baggageEntry}`; + if (newBaggageHeader.length > MAX_BAGGAGE_STRING_LENGTH) { + __DEBUG_BUILD__ && + logger.warn( + `Not adding key: ${objectKey} with val: ${objectValue} to baggage header due to exceeding baggage size limits.`, + ); + return baggageHeader; + } else { + return newBaggageHeader; + } + }, ''); } From f6a59b1f947ebbb23ad0cd8743ad5203476b1fd6 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 8 Sep 2022 14:25:45 +0000 Subject: [PATCH 02/32] please linter --- packages/serverless/src/awslambda.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts index 1692ffa6b44e..5dc27c8d3dd0 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -3,7 +3,7 @@ import * as Sentry from '@sentry/node'; import { captureException, captureMessage, flush, getCurrentHub, Scope, withScope } from '@sentry/node'; import { extractTraceparentData } from '@sentry/tracing'; import { Integration } from '@sentry/types'; -import { dsnFromString, dsnToString, isString, logger, baggageHeaderToDynamicSamplingContext } from '@sentry/utils'; +import { baggageHeaderToDynamicSamplingContext, dsnFromString, dsnToString, isString, logger } from '@sentry/utils'; // NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil // eslint-disable-next-line import/no-unresolved import { Context, Handler } from 'aws-lambda'; From 7a8608c8a73b399e8128eadded934d023d61c18a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 9 Sep 2022 11:18:12 +0000 Subject: [PATCH 03/32] fix appendage --- packages/node/src/integrations/http.ts | 15 ++++-- packages/tracing/src/browser/request.ts | 66 ++++++++++++++----------- packages/tracing/src/transaction.ts | 28 +++++------ 3 files changed, 58 insertions(+), 51 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index fd3afb229397..5253207a1b2e 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -172,10 +172,17 @@ function _createWrappedRequestMethodFactory( if (parentSpan.transaction) { const dynamicSamplingContext = parentSpan.transaction.getDynamicSamplingContext(); const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - const newBaggageHeaderField = - sentryBaggageHeader && requestOptions.headers && Array.isArray(requestOptions.headers.baggage) - ? requestOptions.headers.baggage.concat(sentryBaggageHeader) - : sentryBaggageHeader; + + let newBaggageHeaderField = undefined; + if (!requestOptions.headers || !requestOptions.headers.baggage) { + newBaggageHeaderField = sentryBaggageHeader; + } else if (!sentryBaggageHeader) { + newBaggageHeaderField = requestOptions.headers.baggage; + } else if (Array.isArray(requestOptions.headers.baggage)) { + newBaggageHeaderField = [...requestOptions.headers.baggage, sentryBaggageHeader]; + } else { + newBaggageHeaderField = [requestOptions.headers.baggage, sentryBaggageHeader] as string[]; + } requestOptions.headers = { ...requestOptions.headers, diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index db7fbb401cff..7e5ea845464a 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -85,7 +85,7 @@ export interface XHRData { } type PolymorphicRequestHeaders = - | Record + | Record | Array<[string, string]> // the below is not preicsely the Header type used in Request, but it'll pass duck-typing | { @@ -211,58 +211,64 @@ function addTracingHeadersToFetchRequest( request: string | Request, dynamicSamplingContext: Partial | undefined, span: Span, - options: { [key: string]: any }, + options: { + headers?: { + [key: string]: string[] | string | undefined; + }; + }, ): PolymorphicRequestHeaders { - let headers = options.headers; - - if (isInstanceOf(request, Request)) { - headers = (request as Request).headers; - } - const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); const sentryTraceHeader = span.toTraceparent(); - if (headers) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - if (typeof headers.append === 'function') { + if (typeof Request !== 'undefined' && isInstanceOf(request, Request)) { + const headers = (request as Request).headers; + const newHeaders = new Headers(headers); + + newHeaders.append('sentry-trace', sentryTraceHeader); + + if (sentryBaggageHeader) { // If the same header is appended miultiple times the browser will merge the values into a single request header. // Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header. + newHeaders.append(BAGGAGE_HEADER_NAME, sentryBaggageHeader); + } + + return newHeaders as PolymorphicRequestHeaders; + } else { + if (!options.headers) { + return { 'sentry-trace': sentryTraceHeader, baggage: sentryBaggageHeader }; + } - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - headers.append('sentry-trace', sentryTraceHeader); - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - headers.append(BAGGAGE_HEADER_NAME, sentryBaggageHeader); - } else if (Array.isArray(headers)) { - headers.push(['sentry-trace', sentryTraceHeader]); + if (Array.isArray(options.headers)) { + const newHeaders = [...options.headers]; + newHeaders.push(['sentry-trace', sentryTraceHeader]); if (sentryBaggageHeader) { // If there are multiple entries with the same key, the browser will merge the values into a single request header. // Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header. - headers.push([BAGGAGE_HEADER_NAME, sentryBaggageHeader]); + newHeaders.push([BAGGAGE_HEADER_NAME, sentryBaggageHeader]); } + return newHeaders; } else { - const baggageHeaders: string[] = []; + const existingBaggageHeader = options.headers.baggage; + const newBaggageHeaders: string[] = []; - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - if (headers.baggage) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - baggageHeaders.push(headers.baggage); + if (Array.isArray(existingBaggageHeader)) { + newBaggageHeaders.push(...existingBaggageHeader); + } else if (existingBaggageHeader) { + newBaggageHeaders.push(existingBaggageHeader); } if (sentryBaggageHeader) { - baggageHeaders.push(sentryBaggageHeader); + newBaggageHeaders.push(sentryBaggageHeader); } - headers = { - ...headers, + return { + ...options.headers, 'sentry-trace': sentryTraceHeader, - baggage: baggageHeaders.join(', '), + baggage: newBaggageHeaders.length > 0 ? newBaggageHeaders.join(', ') : undefined, }; } - } else { - headers = { 'sentry-trace': sentryTraceHeader, baggage: sentryBaggageHeader }; } - return headers; } /** diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index 3046d96612b5..c056180e0546 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -27,9 +27,7 @@ export class Transaction extends SpanClass implements TransactionInterface { private _trimEnd?: boolean; - // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) - // This value is null - // private _frozenDynamicSamplingContext: Partial | undefined = undefined; + private _frozenDynamicSamplingContext: Partial | undefined = undefined; /** * This constructor should never be called manually. Those instrumenting tracing should use @@ -55,10 +53,12 @@ export class Transaction extends SpanClass implements TransactionInterface { // this is because transactions are also spans, and spans have a transaction pointer this.transaction = this; - // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) - // if ((transactionContext.metadata || {}).dynamicSamplingContext) { - // this.freezeDynamicSamplingContext(); - // } + // If Dynamic Sampling Context is provided during the creation of the transaction, we freeze it as it usually means + // there is incoming Dynamic Sampling Context. (Either through an incoming request, a baggage meta-tag, or other means) + const incomingDynamicSamplingContext = (transactionContext.metadata || {}).dynamicSamplingContext; + if (incomingDynamicSamplingContext) { + this._frozenDynamicSamplingContext = incomingDynamicSamplingContext; + } } /** Getter for `name` property */ @@ -145,11 +145,6 @@ export class Transaction extends SpanClass implements TransactionInterface { }).endTimestamp; } - // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) - // if ((transactionContext.metadata || {}).dynamicSamplingContext) { - // this.freezeDynamicSamplingContext(); - // } - const metadata = this.metadata; const transaction: Event = { @@ -219,10 +214,9 @@ export class Transaction extends SpanClass implements TransactionInterface { * */ public getDynamicSamplingContext(): Partial | undefined { - // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) - // if (this._frozenDynamicSamplingContext) { - // return this._frozenDynamicSamplingContext; - // } + if (this._frozenDynamicSamplingContext) { + return this._frozenDynamicSamplingContext; + } const hub: Hub = this._hub || getCurrentHub(); const client = hub && hub.getClient(); @@ -253,7 +247,7 @@ export class Transaction extends SpanClass implements TransactionInterface { sample_rate, }); - // Uncomment if we want to make DSC immutable (Search for "IMMUTABLE DSC" to find all commented out code sections) + // Uncomment if we want to make DSC immutable // this._frozenDynamicSamplingContext = dsc; return dsc; From 4f7d68ac46a1db6fa9f902ba4f03cf944f007585 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 9 Sep 2022 12:01:32 +0000 Subject: [PATCH 04/32] fix(tests): Fix node integration tests sharing SDK initializations --- packages/node-integration-tests/jest.config.js | 1 + packages/node-integration-tests/jest.setup.js | 2 ++ packages/node-integration-tests/package.json | 2 +- packages/node-integration-tests/utils/index.ts | 11 ++++++----- 4 files changed, 10 insertions(+), 6 deletions(-) create mode 100644 packages/node-integration-tests/jest.setup.js diff --git a/packages/node-integration-tests/jest.config.js b/packages/node-integration-tests/jest.config.js index 9a2862dd9b8d..fe377b41b880 100644 --- a/packages/node-integration-tests/jest.config.js +++ b/packages/node-integration-tests/jest.config.js @@ -4,4 +4,5 @@ module.exports = { globalSetup: '/utils/setup-tests.ts', ...baseConfig, testMatch: ['**/test.ts'], + setupFilesAfterEnv: ['./jest.setup.js'], }; diff --git a/packages/node-integration-tests/jest.setup.js b/packages/node-integration-tests/jest.setup.js new file mode 100644 index 000000000000..7c1837cab523 --- /dev/null +++ b/packages/node-integration-tests/jest.setup.js @@ -0,0 +1,2 @@ +// Increases test timeout from 5s to 45s +jest.setTimeout(45000); diff --git a/packages/node-integration-tests/package.json b/packages/node-integration-tests/package.json index 0cc4a697a976..248cad1595ba 100644 --- a/packages/node-integration-tests/package.json +++ b/packages/node-integration-tests/package.json @@ -17,7 +17,7 @@ "fix:prettier": "prettier --write \"{suites,utils}/**/*.ts\"", "type-check": "tsc", "pretest": "run-s --silent prisma:init", - "test": "jest --runInBand --forceExit", + "test": "jest", "test:watch": "yarn test --watch" }, "dependencies": { diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts index 50da10fd7820..ad1bb0d4b188 100644 --- a/packages/node-integration-tests/utils/index.ts +++ b/packages/node-integration-tests/utils/index.ts @@ -137,11 +137,9 @@ export class TestEnv { * @return {*} {Promise} */ public static async init(testDir: string, serverPath?: string, scenarioPath?: string): Promise { - const port = await getPortPromise(); - const url = `http://localhost:${port}/test`; const defaultServerPath = path.resolve(process.cwd(), 'utils', 'defaults', 'server'); - const server = await new Promise(resolve => { + const [server, url] = await new Promise<[http.Server, string]>(resolve => { // eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-unsafe-member-access const app = require(serverPath || defaultServerPath).default as Express; @@ -153,8 +151,11 @@ export class TestEnv { } }); - const server = app.listen(port, () => { - resolve(server); + void getPortPromise().then(port => { + const url = `http://localhost:${port}/test`; + const server = app.listen(port, () => { + resolve([server, url]); + }); }); }); From 365e8a9c652d02646b76cff0f56699dadd1c41a4 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 9 Sep 2022 12:24:59 +0000 Subject: [PATCH 05/32] Try readding forceExit --- packages/node-integration-tests/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node-integration-tests/package.json b/packages/node-integration-tests/package.json index 248cad1595ba..b6fb1dba74d9 100644 --- a/packages/node-integration-tests/package.json +++ b/packages/node-integration-tests/package.json @@ -17,7 +17,7 @@ "fix:prettier": "prettier --write \"{suites,utils}/**/*.ts\"", "type-check": "tsc", "pretest": "run-s --silent prisma:init", - "test": "jest", + "test": "jest --forceExit", "test:watch": "yarn test --watch" }, "dependencies": { From 248a9b6d6a7acbef0d122a232beaa0c728ea6e8c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 09:25:44 +0000 Subject: [PATCH 06/32] Freeze DSC with empty values when there is a sentry-trace header but no sentry baggage --- packages/nextjs/src/config/wrappers/wrapperUtils.ts | 2 +- packages/nextjs/src/performance/client.ts | 2 +- packages/nextjs/src/utils/instrumentServer.ts | 2 +- packages/nextjs/src/utils/withSentry.ts | 5 ++++- packages/node/src/handlers.ts | 5 ++++- packages/node/src/integrations/http.ts | 9 +++++++-- packages/remix/src/utils/instrumentServer.ts | 3 +-- packages/serverless/src/awslambda.ts | 5 ++++- packages/serverless/src/gcpfunction/http.ts | 5 ++++- packages/tracing/src/browser/browsertracing.ts | 2 +- 10 files changed, 28 insertions(+), 12 deletions(-) diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index 70411ec1e238..0b64d17acf9d 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -100,7 +100,7 @@ export function callTracedServerSideDataFetcher Pr ...traceparentData, metadata: { source: 'route', - dynamicSamplingContext, + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, }, }); diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts index 6cbbdba61bb7..fd7d7dadbf2a 100644 --- a/packages/nextjs/src/performance/client.ts +++ b/packages/nextjs/src/performance/client.ts @@ -138,7 +138,7 @@ export function nextRouterInstrumentation( ...traceParentData, metadata: { source, - ...(dynamicSamplingContext && { dynamicSamplingContext }), + dynamicSamplingContext: traceParentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, }, }); } diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 88b0a4537a4d..5fe439024ae8 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -270,7 +270,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { name: `${namePrefix}${reqPath}`, op: 'http.server', metadata: { - dynamicSamplingContext, + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, requestPath: reqPath, // TODO: Investigate if there's a way to tell if this is a dynamic route, so that we can make this more // like `source: isDynamicRoute? 'url' : 'route'` diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 3d459f9f20a1..8e71dcd09119 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -72,7 +72,10 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = name: `${reqMethod}${reqPath}`, op: 'http.server', ...traceparentData, - metadata: { dynamicSamplingContext, source: 'route' }, + metadata: { + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + source: 'route', + }, }, // extra context passed to the `tracesSampler` { request: req }, diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 798f84b5a767..c29d5d836efe 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -63,7 +63,10 @@ export function tracingHandler(): ( name, op: 'http.server', ...traceparentData, - metadata: { dynamicSamplingContext, source }, + metadata: { + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + source, + }, }, // extra context passed to the tracesSampler { request: extractRequestData(req) }, diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 5253207a1b2e..fcccf5dc08e6 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -173,7 +173,8 @@ function _createWrappedRequestMethodFactory( const dynamicSamplingContext = parentSpan.transaction.getDynamicSamplingContext(); const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - let newBaggageHeaderField = undefined; + // Append a new baggage header to + let newBaggageHeaderField; if (!requestOptions.headers || !requestOptions.headers.baggage) { newBaggageHeaderField = sentryBaggageHeader; } else if (!sentryBaggageHeader) { @@ -181,12 +182,16 @@ function _createWrappedRequestMethodFactory( } else if (Array.isArray(requestOptions.headers.baggage)) { newBaggageHeaderField = [...requestOptions.headers.baggage, sentryBaggageHeader]; } else { + // Type-cast explanation: + // Technically this the following could be of type `(number | string)[]` but for the sake of simplicity + // we say this is undefined behaviour, since it would not be baggage spec conform if the user did this. newBaggageHeaderField = [requestOptions.headers.baggage, sentryBaggageHeader] as string[]; } requestOptions.headers = { ...requestOptions.headers, - baggage: newBaggageHeaderField, + // Setting a hader to `undefined` will crash in node so we only set the baggage header when it's defined + ...(newBaggageHeaderField && { baggage: newBaggageHeaderField }), }; } } else { diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index e5f56c11bdf1..bb37dffea8f3 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -324,8 +324,7 @@ export function startRequestHandlerTransaction( ...traceparentData, metadata: { source, - // Only attach baggage if it's defined - ...(dynamicSamplingContext && { dynamicSamplingContext }), + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, }, }); diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts index 5dc27c8d3dd0..ca76cf12382a 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -317,7 +317,10 @@ export function wrapHandler( name: context.functionName, op: 'awslambda.handler', ...traceparentData, - metadata: { dynamicSamplingContext, source: 'component' }, + metadata: { + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + source: 'component', + }, }); const scope = hub.pushScope(); diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index 102550678649..70703be9728a 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -87,7 +87,10 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial Date: Mon, 12 Sep 2022 09:26:34 +0000 Subject: [PATCH 07/32] Close server on test - even on request error --- packages/node-integration-tests/utils/index.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts index ad1bb0d4b188..dafde44e53d1 100644 --- a/packages/node-integration-tests/utils/index.ts +++ b/packages/node-integration-tests/utils/index.ts @@ -202,12 +202,13 @@ export class TestEnv { * @return {*} {Promise} */ public async getAPIResponse(url?: string, headers?: Record): Promise { - const { data } = await axios.get(url || this.url, { headers: headers || {} }); - - await Sentry.flush(); - this.server.close(); - - return data; + try { + const { data } = await axios.get(url || this.url, { headers: headers || {} }); + return data; + } finally { + await Sentry.flush(); + this.server.close(); + } } public async setupNock( From 5da55e65a551d0beec19765c6432b0c1702dddc0 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 09:47:11 +0000 Subject: [PATCH 08/32] Fix some node integration tests --- .../test.ts | 14 ++++++++++---- .../sentry-trace/baggage-other-vendors/test.ts | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts index 71e60bc7c4f3..3de98d14bf06 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts @@ -15,7 +15,10 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', + baggage: [ + 'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item', + 'sentry-release=2.1.0,sentry-environment=myEnv', + ], }, }); }); @@ -29,9 +32,12 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: expect.stringContaining( - 'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public', - ), + baggage: [ + 'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item', + expect.stringMatching( + /sentry-environment=prod,sentry-release=1\.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public,sentry-trace_id=[0-9a-f]{32},sentry-sample_rate=1/, + ), + ], }, }); }); diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts index 3f50b71bc265..2f88610509c5 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts @@ -15,7 +15,7 @@ test('should merge `baggage` header of a third party vendor with the Sentry DSC expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: 'other=vendor,foo=bar,third=party,sentry-release=2.0.0,sentry-environment=myEnv', + baggage: ['other=vendor,foo=bar,third=party', 'sentry-release=2.0.0,sentry-environment=myEnv'], }, }); }); From 7a858892c01d5085c91b2947ea2d8a74fee75c7e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 09:51:31 +0000 Subject: [PATCH 09/32] Adjust tests checking for empty baggage header --- .../express/sentry-trace/baggage-header-assign/test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts index 432790cb3de9..2d51f934b7f4 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts @@ -35,7 +35,7 @@ test('Should propagate sentry trace baggage data from an incoming to an outgoing }); }); -test('Should propagate empty baggage if sentry-trace header is present in incoming request but no baggage header', async () => { +test('Should not propagate baggage if sentry-trace header is present in incoming request but no baggage header', async () => { const env = await TestEnv.init(__dirname, `${path.resolve(__dirname, '..')}/server.ts`); const response = (await env.getAPIResponse(`${env.url}/express`, { @@ -46,12 +46,11 @@ test('Should propagate empty baggage if sentry-trace header is present in incomi expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: '', }, }); }); -test('Should propagate empty sentry and ignore original 3rd party baggage entries if sentry-trace header is present', async () => { +test('Should not propagate baggage and ignore original 3rd party baggage entries if sentry-trace header is present', async () => { const env = await TestEnv.init(__dirname, `${path.resolve(__dirname, '..')}/server.ts`); const response = (await env.getAPIResponse(`${env.url}/express`, { @@ -63,7 +62,6 @@ test('Should propagate empty sentry and ignore original 3rd party baggage entrie expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: '', }, }); }); From b0b65c7c256e97b4453fd5c4ea9f31f255c88594 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 11:38:49 +0000 Subject: [PATCH 10/32] test: Run node integration tests in isolation --- packages/node-integration-tests/package.json | 2 +- .../node-integration-tests/utils/run-tests.ts | 61 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 packages/node-integration-tests/utils/run-tests.ts diff --git a/packages/node-integration-tests/package.json b/packages/node-integration-tests/package.json index b6fb1dba74d9..3c0aa09cfaf0 100644 --- a/packages/node-integration-tests/package.json +++ b/packages/node-integration-tests/package.json @@ -17,7 +17,7 @@ "fix:prettier": "prettier --write \"{suites,utils}/**/*.ts\"", "type-check": "tsc", "pretest": "run-s --silent prisma:init", - "test": "jest --forceExit", + "test": "ts-node ./utils/run-tests.ts", "test:watch": "yarn test --watch" }, "dependencies": { diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts new file mode 100644 index 000000000000..7bd8f290253a --- /dev/null +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -0,0 +1,61 @@ +/* eslint-disable no-console */ +import childProcess from 'child_process'; +import os from 'os'; + +const testPaths = childProcess.execSync('jest --listTests', { encoding: 'utf8' }).trim().split('\n'); + +let testsSucceeded = true; +const testAmount = testPaths.length; +const fails: string[] = []; + +const threads = os.cpus().map(async (_, i) => { + let testPath = testPaths.pop(); + while (testPath !== undefined) { + console.log(`(Worker ${i}) Running test "${testPath}"`); + await new Promise(resolve => { + const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); + + let output = ''; + + p.stdout.on('data', (data: Buffer) => { + output = output + data.toString(); + }); + + p.stderr.on('data', (data: Buffer) => { + output = output + data.toString(); + }); + + p.on('error', error => { + console.log(`(Worker ${i}) Error in test "${testPath}"`, error); + console.log(output); + resolve(); + }); + + p.on('exit', exitcode => { + console.log(`(Worker ${i}) Finished test "${testPath}"`); + console.log(output); + if (exitcode !== 0) { + fails.push(`FAILED: "${testPath}"`); + testsSucceeded = false; + } + resolve(); + }); + }); + testPath = testPaths.pop(); + } +}); + +void Promise.all(threads).then(() => { + console.log('-------------------'); + console.log(`Successfully ran ${testAmount} tests.`); + if (!testsSucceeded) { + console.log('Not all tests succeeded:\n'); + fails.forEach(fail => { + console.log(`● ${fail}`); + }); + process.exit(1); + } else { + console.log('All testcases returned the expected results.'); + process.exit(0); + } +}); From e4b763108b3d1caf9197009c01815fe1802a4a79 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 12:05:44 +0000 Subject: [PATCH 11/32] Avoid port race condition --- packages/node-integration-tests/utils/index.ts | 13 +++++++++++-- packages/node-integration-tests/utils/run-tests.ts | 6 +++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts index ad1bb0d4b188..7baba995b03d 100644 --- a/packages/node-integration-tests/utils/index.ts +++ b/packages/node-integration-tests/utils/index.ts @@ -7,7 +7,7 @@ import { Express } from 'express'; import * as http from 'http'; import nock from 'nock'; import * as path from 'path'; -import { getPortPromise } from 'portfinder'; +import { getPorts } from 'portfinder'; export type TestServerConfig = { url: string; @@ -151,7 +151,16 @@ export class TestEnv { } }); - void getPortPromise().then(port => { + getPorts(50, {}, (err, ports) => { + if (err) { + throw err; + } + + const port = ports.find( + // Only allow ports that do not overlap with other workers - this is done to avoid race-conditions + p => p % Number(process.env.TEST_WORKERS_AMOUNT) === Number(process.env.TEST_PORT_MODULO), + ); + const url = `http://localhost:${port}/test`; const server = app.listen(port, () => { resolve([server, url]); diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index 7bd8f290253a..61c7c82a8c6c 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -13,7 +13,11 @@ const threads = os.cpus().map(async (_, i) => { while (testPath !== undefined) { console.log(`(Worker ${i}) Running test "${testPath}"`); await new Promise(resolve => { - const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); + const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit'], { + // These env vars are used to give workers a limited, non-overlapping set of ports to occupy. + // This is done to avoid race-conditions during port reservation. + env: { ...process.env, TEST_WORKERS_AMOUNT: String(os.cpus().length), TEST_PORT_MODULO: String(i) }, + }); let output = ''; From 97a13b9d5f90bf5eba038b3dbbf93a64f372f678 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 12:08:22 +0000 Subject: [PATCH 12/32] Better log message --- packages/node-integration-tests/utils/run-tests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index 61c7c82a8c6c..aa5eff61ec42 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -59,7 +59,7 @@ void Promise.all(threads).then(() => { }); process.exit(1); } else { - console.log('All testcases returned the expected results.'); + console.log('All tests succeeded.'); process.exit(0); } }); From b86bdb73ab772bbb53e90279999eeed3877c9799 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 12:11:47 +0000 Subject: [PATCH 13/32] Better log message --- packages/node-integration-tests/utils/run-tests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index 61c7c82a8c6c..aa5eff61ec42 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -59,7 +59,7 @@ void Promise.all(threads).then(() => { }); process.exit(1); } else { - console.log('All testcases returned the expected results.'); + console.log('All tests succeeded.'); process.exit(0); } }); From 335a64613537cbace6a8918a553674a54fe99227 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 12:44:27 +0000 Subject: [PATCH 14/32] Fix first round of tests --- packages/core/test/lib/envelope.test.ts | 31 +-- packages/node/test/handlers.test.ts | 26 +- packages/node/test/integrations/http.test.ts | 26 +- packages/utils/test/baggage.test.ts | 254 ------------------- 4 files changed, 31 insertions(+), 306 deletions(-) delete mode 100644 packages/utils/test/baggage.test.ts diff --git a/packages/core/test/lib/envelope.test.ts b/packages/core/test/lib/envelope.test.ts index 8398b4831f53..ecd409b23041 100644 --- a/packages/core/test/lib/envelope.test.ts +++ b/packages/core/test/lib/envelope.test.ts @@ -20,7 +20,7 @@ describe('createEventEnvelope', () => { { type: 'transaction', sdkProcessingMetadata: { - baggage: [{ trace_id: '1234', public_key: 'pubKey123' }, '', false], + dynamicSamplingContext: { trace_id: '1234', public_key: 'pubKey123' }, }, }, { trace_id: '1234', public_key: 'pubKey123' }, @@ -30,7 +30,12 @@ describe('createEventEnvelope', () => { { type: 'transaction', sdkProcessingMetadata: { - baggage: [{ environment: 'prod', release: '1.0.0', public_key: 'pubKey123', trace_id: '1234' }, '', false], + dynamicSamplingContext: { + environment: 'prod', + release: '1.0.0', + public_key: 'pubKey123', + trace_id: '1234', + }, }, }, { release: '1.0.0', environment: 'prod', trace_id: '1234', public_key: 'pubKey123' }, @@ -40,19 +45,15 @@ describe('createEventEnvelope', () => { { type: 'transaction', sdkProcessingMetadata: { - baggage: [ - { - environment: 'prod', - release: '1.0.0', - transaction: 'TX', - user_segment: 'segmentA', - sample_rate: '0.95', - public_key: 'pubKey123', - trace_id: '1234', - }, - '', - false, - ], + dynamicSamplingContext: { + environment: 'prod', + release: '1.0.0', + transaction: 'TX', + user_segment: 'segmentA', + sample_rate: '0.95', + public_key: 'pubKey123', + trace_id: '1234', + }, }, }, { diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 10d2181e3549..a2775e4241c7 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -2,8 +2,8 @@ import * as sentryCore from '@sentry/core'; import * as sentryHub from '@sentry/hub'; import { Hub } from '@sentry/hub'; import { Transaction } from '@sentry/tracing'; -import { Baggage, Event } from '@sentry/types'; -import { getThirdPartyBaggage, isBaggageMutable, isSentryBaggageEmpty, SentryError } from '@sentry/utils'; +import { Event } from '@sentry/types'; +import { SentryError } from '@sentry/utils'; import * as http from 'http'; import { NodeClient } from '../src/client'; @@ -221,10 +221,7 @@ describe('tracingHandler', () => { expect(transaction.traceId).toEqual('12312012123120121231201212312012'); expect(transaction.parentSpanId).toEqual('1121201211212012'); expect(transaction.sampled).toEqual(false); - expect(transaction.metadata?.baggage).toBeDefined(); - expect(isSentryBaggageEmpty(transaction.metadata?.baggage)).toBe(true); - expect(getThirdPartyBaggage(transaction.metadata?.baggage)).toEqual(''); - expect(isBaggageMutable(transaction.metadata?.baggage)).toBe(false); + expect(transaction.metadata?.dynamicSamplingContext).toStrictEqual({}); }); it("pulls parent's data from tracing and baggage headers on the request", () => { @@ -241,15 +238,10 @@ describe('tracingHandler', () => { expect(transaction.traceId).toEqual('12312012123120121231201212312012'); expect(transaction.parentSpanId).toEqual('1121201211212012'); expect(transaction.sampled).toEqual(false); - expect(transaction.metadata?.baggage).toBeDefined(); - expect(transaction.metadata?.baggage).toEqual([ - { version: '1.0', environment: 'production' }, - '', - false, - ] as Baggage); + expect(transaction.metadata?.dynamicSamplingContext).toStrictEqual({ version: '1.0', environment: 'production' }); }); - it('ignores 3rd party entries in incoming baggage header', () => { + it('doesn\t populate dynamic sampling context with 3rd party baggage', () => { req.headers = { 'sentry-trace': '12312012123120121231201212312012-1121201211212012-0', baggage: 'sentry-version=1.0,sentry-environment=production,dogs=great,cats=boring', @@ -258,13 +250,7 @@ describe('tracingHandler', () => { sentryTracingMiddleware(req, res, next); const transaction = (res as any).__sentry_transaction; - - expect(transaction.metadata?.baggage).toBeDefined(); - expect(transaction.metadata?.baggage).toEqual([ - { version: '1.0', environment: 'production' }, - '', - false, - ] as Baggage); + expect(transaction.metadata?.dynamicSamplingContext).toStrictEqual({ version: '1.0', environment: 'production' }); }); it('extracts request data for sampling context', () => { diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts index 72164153bedf..270dc01b9ed8 100644 --- a/packages/node/test/integrations/http.test.ts +++ b/packages/node/test/integrations/http.test.ts @@ -118,8 +118,6 @@ describe('tracing', () => { const request = http.get('http://dogs.are.great/'); const baggageHeader = request.getHeader('baggage') as string; - expect(baggageHeader).toBeDefined(); - expect(typeof baggageHeader).toEqual('string'); expect(baggageHeader).toEqual( 'sentry-environment=production,sentry-release=1.0.0,' + 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + @@ -127,7 +125,7 @@ describe('tracing', () => { ); }); - it('propagates 3rd party baggage header data to outgoing non-sentry requests', async () => { + it('keeps 3rd party baggage header data to outgoing non-sentry requests', async () => { nock('http://dogs.are.great').get('/').reply(200); createTransactionOnScope(); @@ -135,13 +133,10 @@ describe('tracing', () => { const request = http.get({ host: 'http://dogs.are.great/', headers: { baggage: 'dog=great' } }); const baggageHeader = request.getHeader('baggage') as string; - expect(baggageHeader).toBeDefined(); - expect(typeof baggageHeader).toEqual('string'); - expect(baggageHeader).toEqual( - 'dog=great,sentry-environment=production,sentry-release=1.0.0,' + - 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + - 'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1', - ); + expect(baggageHeader).toEqual([ + 'dog=great', + 'sentry-environment=production,sentry-release=1.0.0,sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1', + ]); }); it('adds the transaction name to the the baggage header if a valid transaction source is set', async () => { @@ -152,13 +147,10 @@ describe('tracing', () => { const request = http.get({ host: 'http://dogs.are.great/', headers: { baggage: 'dog=great' } }); const baggageHeader = request.getHeader('baggage') as string; - expect(baggageHeader).toBeDefined(); - expect(typeof baggageHeader).toEqual('string'); - expect(baggageHeader).toEqual( - 'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + - 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + - 'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1', - ); + expect(baggageHeader).toEqual([ + 'dog=great', + 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1', + ]); }); it("doesn't attach the sentry-trace header to outgoing sentry requests", () => { diff --git a/packages/utils/test/baggage.test.ts b/packages/utils/test/baggage.test.ts deleted file mode 100644 index cfd8ebd5f239..000000000000 --- a/packages/utils/test/baggage.test.ts +++ /dev/null @@ -1,254 +0,0 @@ -import { Baggage } from '@sentry/types'; - -import { - createBaggage, - getBaggageValue, - isBaggageMutable, - isSentryBaggageEmpty, - mergeAndSerializeBaggage, - parseBaggageHeader, - parseBaggageSetMutability, - serializeBaggage, - setBaggageImmutable, - setBaggageValue, -} from '../src/baggage'; - -describe('Baggage', () => { - describe('createBaggage', () => { - it.each([ - ['creates an empty baggage instance', {}, [{}, '', true]], - [ - 'creates a baggage instance with initial values', - { environment: 'production', anyKey: 'anyValue' }, - [{ environment: 'production', anyKey: 'anyValue' }, '', true], - ], - ])('%s', (_: string, input, output) => { - expect(createBaggage(input)).toEqual(output); - }); - - it('creates a baggage instance and marks it immutable if explicitly specified', () => { - expect(createBaggage({ environment: 'production', anyKey: 'anyValue' }, '', false)).toEqual([ - { environment: 'production', anyKey: 'anyValue' }, - '', - false, - ]); - }); - }); - - describe('getBaggageValue', () => { - it.each([ - [ - 'gets a baggage item', - createBaggage({ environment: 'production', anyKey: 'anyValue' }), - 'environment', - 'production', - ], - ['finds undefined items', createBaggage({}), 'environment', undefined], - ])('%s', (_: string, baggage, key, value) => { - expect(getBaggageValue(baggage, key)).toEqual(value); - }); - }); - - describe('setBaggageValue', () => { - it.each([ - ['sets a baggage item', createBaggage({}), 'environment', 'production'], - ['overwrites a baggage item', createBaggage({ environment: 'development' }), 'environment', 'production'], - [ - 'does not set a value if the passed baggage item is immutable', - createBaggage({ environment: 'development' }, '', false), - 'environment', - 'development', - ], - ])('%s', (_: string, baggage, key, value) => { - setBaggageValue(baggage, key, value); - expect(getBaggageValue(baggage, key)).toEqual(value); - }); - }); - - describe('serializeBaggage', () => { - it.each([ - ['serializes empty baggage', createBaggage({}), ''], - [ - 'serializes baggage with a single value', - createBaggage({ environment: 'production' }), - 'sentry-environment=production', - ], - [ - 'serializes baggage with multiple values', - createBaggage({ environment: 'production', release: '10.0.2' }), - 'sentry-environment=production,sentry-release=10.0.2', - ], - [ - 'keeps non-sentry prefixed baggage items', - createBaggage( - { environment: 'production', release: '10.0.2' }, - 'userId=alice,serverNode=DF%2028,isProduction=false', - ), - 'userId=alice,serverNode=DF%2028,isProduction=false,sentry-environment=production,sentry-release=10.0.2', - ], - [ - 'can only use non-sentry prefixed baggage items', - createBaggage({}, 'userId=alice,serverNode=DF%2028,isProduction=false'), - 'userId=alice,serverNode=DF%2028,isProduction=false', - ], - ])('%s', (_: string, baggage, serializedBaggage) => { - expect(serializeBaggage(baggage)).toEqual(serializedBaggage); - }); - }); - - describe('parseBaggageHeader', () => { - it.each([ - ['parses an empty string', '', undefined, createBaggage({})], - ['parses a blank string', ' ', undefined, createBaggage({})], - [ - 'parses sentry values into baggage', - 'sentry-environment=production,sentry-release=10.0.2', - undefined, - createBaggage({ environment: 'production', release: '10.0.2' }), - ], - [ - 'ignores 3rd party entries by default', - 'userId=alice,serverNode=DF%2028,isProduction=false,sentry-environment=production,sentry-release=10.0.2', - undefined, - createBaggage({ environment: 'production', release: '10.0.2' }, ''), - ], - [ - 'parses sentry- and arbitrary 3rd party values if the 3rd party entries flag is set to true', - 'userId=alice,serverNode=DF%2028,isProduction=false,sentry-environment=production,sentry-release=10.0.2', - true, - createBaggage( - { environment: 'production', release: '10.0.2' }, - 'userId=alice,serverNode=DF%2028,isProduction=false', - ), - ], - [ - 'parses arbitrary baggage entries from string with empty and blank entries', - 'userId=alice, serverNode=DF%2028 , isProduction=false, ,,sentry-environment=production,,sentry-release=10.0.2', - true, - createBaggage( - { environment: 'production', release: '10.0.2' }, - 'userId=alice,serverNode=DF%2028,isProduction=false', - ), - ], - [ - 'parses a string array', - ['userId=alice', 'sentry-environment=production', 'foo=bar'], - true, - createBaggage({ environment: 'production' }, 'userId=alice,foo=bar'), - ], - [ - 'parses a string array with items containing multiple entries', - ['userId=alice, userName=bob', 'sentry-environment=production,sentry-release=1.0.1', 'foo=bar'], - true, - createBaggage({ environment: 'production', release: '1.0.1' }, 'userId=alice,userName=bob,foo=bar'), - ], - [ - 'parses a string array with empty/blank entries', - ['', 'sentry-environment=production,sentry-release=1.0.1', ' ', 'foo=bar'], - true, - createBaggage({ environment: 'production', release: '1.0.1' }, 'foo=bar'), - ], - ['ignorese other input types than string and string[]', 42, undefined, createBaggage({}, '')], - ])('%s', (_: string, baggageValue, includeThirPartyEntries, expectedBaggage) => { - expect(parseBaggageHeader(baggageValue, includeThirPartyEntries)).toEqual(expectedBaggage); - }); - }); - - describe('isSentryBaggageEmpty', () => { - it.each([ - ['returns true if the Sentry part of baggage is empty', createBaggage({}), true], - ['returns false if the Sentry part of baggage is not empty', createBaggage({ release: '10.0.2' }), false], - ])('%s', (_: string, baggage, outcome) => { - expect(isSentryBaggageEmpty(baggage)).toEqual(outcome); - }); - }); - - describe('mergeAndSerializeBaggage', () => { - it.each([ - [ - 'returns original baggage when there is no additional baggage header', - createBaggage({ release: '1.1.1', user_id: '1234' }), - undefined, - 'sentry-release=1.1.1,sentry-user_id=1234', - ], - [ - 'returns merged baggage when there is a 3rd party header added', - createBaggage({ release: '1.1.1', user_id: '1234' }, 'foo=bar'), - 'bar=baz,key=value', - 'bar=baz,key=value,sentry-release=1.1.1,sentry-user_id=1234', - ], - ['returns merged baggage original baggage is empty', createBaggage({}), 'bar=baz,key=value', 'bar=baz,key=value'], - [ - 'ignores sentry- items in 3rd party baggage header', - createBaggage({}), - 'bar=baz,sentry-user_id=abc,key=value,sentry-sample_rate=0.76', - 'bar=baz,key=value', - ], - ['returns empty string when original and 3rd party baggage are empty', createBaggage({}), '', ''], - ['returns merged baggage original baggage is undefined', undefined, 'bar=baz,key=value', 'bar=baz,key=value'], - ['returns empty string when both params are undefined', undefined, undefined, ''], - ])('%s', (_: string, baggage, headerBaggageString, outcome) => { - expect(mergeAndSerializeBaggage(baggage, headerBaggageString)).toEqual(outcome); - }); - }); - - describe('parseBaggageSetMutability', () => { - it.each([ - [ - 'returns an empty, mutable baggage object if both params are undefined', - undefined, - undefined, - [{}, '', true] as Baggage, - ], - [ - 'returns an empty, immutable baggage object if sentry-trace header data is defined', - undefined, - {}, - [{}, '', false] as Baggage, - ], - [ - 'returns an empty, immutable baggage object if sentry-trace header data is a string', - undefined, - '123', - [{}, '', false] as Baggage, - ], - [ - 'returns a non-empty, mutable baggage object if sentry-trace is not defined and ignores 3rd party baggage items', - 'foo=bar', - undefined, - [{}, '', true] as Baggage, - ], - [ - 'returns a non-empty, immutable baggage object if sentry-trace is defined', - 'foo=bar,sentry-environment=production,sentry-sample_rate=0.96', - {}, - [{ environment: 'production', sample_rate: '0.96' }, '', false] as Baggage, - ], - ])( - '%s', - (_: string, baggageString: string | undefined, sentryTraceData: any | string | undefined, result: Baggage) => { - expect(parseBaggageSetMutability(baggageString, sentryTraceData)).toEqual(result); - }, - ); - }); - - describe('isBaggageMutable', () => { - it.each([ - ['returns false if baggage is set immutable', false], - ['returns true if baggage is set mutable', true], - ])('%s', (_: string, outcome) => { - const baggage: Baggage = [{}, '', outcome]; - expect(isBaggageMutable(baggage)).toEqual(outcome); - }); - }); - - describe('setBaggageImmutable', () => { - it.each([ - ['sets baggage immutable', [{}, '', true] as Baggage], - ['does not do anything when baggage is already immutable', [{}, '', false] as Baggage], - ])('%s', (_: string, baggage: Baggage) => { - setBaggageImmutable(baggage); - expect(baggage[2]).toEqual(false); - }); - }); -}); From dd5fe3fd8e9a817cdf2335b248dd66a73aa10030 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 12:57:11 +0000 Subject: [PATCH 15/32] Avoid port race condition (finally pls) --- packages/node-integration-tests/package.json | 3 +-- .../node-integration-tests/utils/index.ts | 19 ++++--------------- .../node-integration-tests/utils/run-tests.ts | 6 +----- packages/remix/package.json | 3 ++- 4 files changed, 8 insertions(+), 23 deletions(-) diff --git a/packages/node-integration-tests/package.json b/packages/node-integration-tests/package.json index 3c0aa09cfaf0..d966b9cbbdf0 100644 --- a/packages/node-integration-tests/package.json +++ b/packages/node-integration-tests/package.json @@ -34,8 +34,7 @@ "mongodb-memory-server-global": "^7.6.3", "mysql": "^2.18.1", "nock": "^13.1.0", - "pg": "^8.7.3", - "portfinder": "^1.0.28" + "pg": "^8.7.3" }, "config": { "mongodbMemoryServer": { diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts index 7baba995b03d..153cbcb794d2 100644 --- a/packages/node-integration-tests/utils/index.ts +++ b/packages/node-integration-tests/utils/index.ts @@ -5,9 +5,9 @@ import { logger, parseSemver } from '@sentry/utils'; import axios, { AxiosRequestConfig } from 'axios'; import { Express } from 'express'; import * as http from 'http'; +import { AddressInfo } from 'net'; import nock from 'nock'; import * as path from 'path'; -import { getPorts } from 'portfinder'; export type TestServerConfig = { url: string; @@ -151,20 +151,9 @@ export class TestEnv { } }); - getPorts(50, {}, (err, ports) => { - if (err) { - throw err; - } - - const port = ports.find( - // Only allow ports that do not overlap with other workers - this is done to avoid race-conditions - p => p % Number(process.env.TEST_WORKERS_AMOUNT) === Number(process.env.TEST_PORT_MODULO), - ); - - const url = `http://localhost:${port}/test`; - const server = app.listen(port, () => { - resolve([server, url]); - }); + const server = app.listen(0, () => { + const url = `http://localhost:${(server.address() as AddressInfo).port}/test`; + resolve([server, url]); }); }); diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index aa5eff61ec42..047dc59b6080 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -13,11 +13,7 @@ const threads = os.cpus().map(async (_, i) => { while (testPath !== undefined) { console.log(`(Worker ${i}) Running test "${testPath}"`); await new Promise(resolve => { - const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit'], { - // These env vars are used to give workers a limited, non-overlapping set of ports to occupy. - // This is done to avoid race-conditions during port reservation. - env: { ...process.env, TEST_WORKERS_AMOUNT: String(os.cpus().length), TEST_PORT_MODULO: String(i) }, - }); + const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); let output = ''; diff --git a/packages/remix/package.json b/packages/remix/package.json index eb92b97feeb7..85b3e3ed6ab7 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -34,7 +34,8 @@ }, "devDependencies": { "@remix-run/node": "^1.4.3", - "@remix-run/react": "^1.4.3" + "@remix-run/react": "^1.4.3", + "portfinder": "^1.0.28" }, "peerDependencies": { "@remix-run/node": "1.x", From 8028d9e280bcbf07f81d4ef0a5c6a6edcd3407f2 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 13:25:38 +0000 Subject: [PATCH 16/32] Small readability improvements --- .../node-integration-tests/utils/run-tests.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index 047dc59b6080..bb1f596f89b9 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -9,29 +9,29 @@ const testAmount = testPaths.length; const fails: string[] = []; const threads = os.cpus().map(async (_, i) => { - let testPath = testPaths.pop(); - while (testPath !== undefined) { + while (testPaths.length > 0) { + const testPath = testPaths.pop(); console.log(`(Worker ${i}) Running test "${testPath}"`); await new Promise(resolve => { - const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); + const jestProcess = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); let output = ''; - p.stdout.on('data', (data: Buffer) => { + jestProcess.stdout.on('data', (data: Buffer) => { output = output + data.toString(); }); - p.stderr.on('data', (data: Buffer) => { + jestProcess.stderr.on('data', (data: Buffer) => { output = output + data.toString(); }); - p.on('error', error => { + jestProcess.on('error', error => { console.log(`(Worker ${i}) Error in test "${testPath}"`, error); console.log(output); resolve(); }); - p.on('exit', exitcode => { + jestProcess.on('exit', exitcode => { console.log(`(Worker ${i}) Finished test "${testPath}"`); console.log(output); if (exitcode !== 0) { @@ -41,7 +41,6 @@ const threads = os.cpus().map(async (_, i) => { resolve(); }); }); - testPath = testPaths.pop(); } }); From 126e3b14de43ab9a1ba0d176d1671b70581bd596 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 14:02:21 +0000 Subject: [PATCH 17/32] Fix some more tests --- .../test/browser/browsertracing.test.ts | 112 ++---------------- packages/tracing/test/span.test.ts | 28 ++--- 2 files changed, 24 insertions(+), 116 deletions(-) diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 86c437d59c90..57c7f2eaf112 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -1,21 +1,10 @@ import { BrowserClient } from '@sentry/browser'; import { Hub, makeMain } from '@sentry/hub'; -import type { Baggage, BaggageObj, BaseTransportOptions, ClientOptions, DsnComponents } from '@sentry/types'; -import { - getGlobalObject, - getThirdPartyBaggage, - InstrumentHandlerCallback, - InstrumentHandlerType, - isSentryBaggageEmpty, -} from '@sentry/utils'; +import type { BaseTransportOptions, ClientOptions, DsnComponents } from '@sentry/types'; +import { getGlobalObject, InstrumentHandlerCallback, InstrumentHandlerType } from '@sentry/utils'; import { JSDOM } from 'jsdom'; -import { - BrowserTracing, - BrowserTracingOptions, - extractTraceDataFromMetaTags, - getMetaContent, -} from '../../src/browser/browsertracing'; +import { BrowserTracing, BrowserTracingOptions, getMetaContent } from '../../src/browser/browsertracing'; import { defaultRequestInstrumentationOptions } from '../../src/browser/request'; import { instrumentRoutingWithDefaults } from '../../src/browser/router'; import * as hubExtensions from '../../src/hubextensions'; @@ -272,7 +261,7 @@ describe('BrowserTracing', () => { parentSpanId: 'b6e54397b12a2a0f', parentSampled: true, metadata: { - baggage: [{ release: '2.1.14' }, '', false], + dynamicSamplingContext: { release: '2.1.14' }, }, }), expect.any(Number), @@ -400,71 +389,6 @@ describe('BrowserTracing', () => { }); }); - describe('extractTraceDataFromMetaTags()', () => { - it('correctly parses a valid sentry-trace meta header', () => { - document.head.innerHTML = - ''; - - const headerContext = extractTraceDataFromMetaTags(); - - expect(headerContext).toBeDefined(); - expect(headerContext!.traceId).toEqual('12312012123120121231201212312012'); - expect(headerContext!.parentSpanId).toEqual('1121201211212012'); - expect(headerContext!.parentSampled).toEqual(false); - }); - - it('correctly parses a valid baggage meta header and ignored 3rd party entries', () => { - document.head.innerHTML = ''; - - const headerContext = extractTraceDataFromMetaTags(); - - expect(headerContext).toBeDefined(); - expect(headerContext?.metadata?.baggage).toBeDefined(); - const baggage = headerContext?.metadata?.baggage; - expect(baggage && baggage[0]).toBeDefined(); - expect(baggage && baggage[0]).toEqual({ - release: '2.1.12', - } as BaggageObj); - expect(baggage && baggage[1]).toBeDefined(); - expect(baggage && baggage[1]).toEqual(''); - }); - - it('returns undefined if the sentry-trace header is malformed', () => { - document.head.innerHTML = ''; - - const headerContext = extractTraceDataFromMetaTags(); - - expect(headerContext).toBeDefined(); - expect(headerContext?.metadata?.baggage).toBeDefined(); - expect(isSentryBaggageEmpty(headerContext?.metadata?.baggage as Baggage)).toBe(true); - expect(getThirdPartyBaggage(headerContext?.metadata?.baggage as Baggage)).toEqual(''); - }); - - it('does not crash if the baggage header is malformed', () => { - document.head.innerHTML = ''; - - const headerContext = extractTraceDataFromMetaTags(); - - // TODO currently this creates invalid baggage. This must be adressed in a follow-up PR - expect(headerContext).toBeDefined(); - expect(headerContext?.metadata?.baggage).toBeDefined(); - const baggage = headerContext?.metadata?.baggage; - expect(baggage && baggage[0]).toBeDefined(); - expect(baggage && baggage[1]).toBeDefined(); - }); - - it("returns default object if the header isn't there", () => { - document.head.innerHTML = ''; - - const headerContext = extractTraceDataFromMetaTags(); - - expect(headerContext).toBeDefined(); - expect(headerContext?.metadata?.baggage).toBeDefined(); - expect(isSentryBaggageEmpty(headerContext?.metadata?.baggage as Baggage)).toBe(true); - expect(getThirdPartyBaggage(headerContext?.metadata?.baggage as Baggage)).toEqual(''); - }); - }); - describe('using the tag data', () => { beforeEach(() => { hub.getClient()!.getOptions = () => { @@ -490,21 +414,18 @@ describe('BrowserTracing', () => { // pageload transactions are created as part of the BrowserTracing integration's initialization createBrowserTracing(true); const transaction = getActiveTransaction(hub) as IdleTransaction; - const baggage = transaction.getBaggage()!; + const dynamicSamplingContext = transaction.getDynamicSamplingContext()!; expect(transaction).toBeDefined(); expect(transaction.op).toBe('pageload'); expect(transaction.traceId).toEqual('12312012123120121231201212312012'); expect(transaction.parentSpanId).toEqual('1121201211212012'); expect(transaction.sampled).toBe(false); - expect(baggage).toBeDefined(); - expect(baggage[0]).toBeDefined(); - expect(baggage[0]).toEqual({ release: '2.1.14' }); - expect(baggage[1]).toBeDefined(); - expect(baggage[1]).toEqual(''); + expect(dynamicSamplingContext).toBeDefined(); + expect(dynamicSamplingContext).toStrictEqual({ release: '2.1.14' }); }); - it('does not add Sentry baggage data to pageload transactions if sentry-trace data is present but passes on 3rd party baggage', () => { + it('puts frozen Dynamic Sampling Context on pageload transactions if sentry-trace data and only 3rd party baggage is present', () => { // make sampled false here, so we can see that it's being used rather than the tracesSampleRate-dictated one document.head.innerHTML = '' + @@ -513,20 +434,17 @@ describe('BrowserTracing', () => { // pageload transactions are created as part of the BrowserTracing integration's initialization createBrowserTracing(true); const transaction = getActiveTransaction(hub) as IdleTransaction; - const baggage = transaction.getBaggage()!; + const dynamicSamplingContext = transaction.getDynamicSamplingContext()!; expect(transaction).toBeDefined(); expect(transaction.op).toBe('pageload'); expect(transaction.traceId).toEqual('12312012123120121231201212312012'); expect(transaction.parentSpanId).toEqual('1121201211212012'); expect(transaction.sampled).toBe(false); - expect(baggage).toBeDefined(); - expect(isSentryBaggageEmpty(baggage)).toBe(true); - expect(baggage[1]).toBeDefined(); - expect(baggage[1]).toEqual(''); + expect(dynamicSamplingContext).toStrictEqual({}); }); - it('ignores the data for navigation transactions', () => { + it('ignores the meta tag data for navigation transactions', () => { mockChangeHistory = () => undefined; document.head.innerHTML = '' + @@ -536,22 +454,18 @@ describe('BrowserTracing', () => { mockChangeHistory({ to: 'here', from: 'there' }); const transaction = getActiveTransaction(hub) as IdleTransaction; - const baggage = transaction.getBaggage()!; + const dynamicSamplingContext = transaction.getDynamicSamplingContext()!; expect(transaction).toBeDefined(); expect(transaction.op).toBe('navigation'); expect(transaction.traceId).not.toEqual('12312012123120121231201212312012'); expect(transaction.parentSpanId).toBeUndefined(); - expect(baggage).toBeDefined(); - expect(baggage[0]).toBeDefined(); - expect(baggage[0]).toEqual({ + expect(dynamicSamplingContext).toStrictEqual({ release: '1.0.0', environment: 'production', public_key: 'pubKey', trace_id: expect.not.stringMatching('12312012123120121231201212312012'), }); - expect(baggage[1]).toBeDefined(); - expect(baggage[1]).toEqual(''); }); }); }); diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts index 7bb53faca235..a7eba07c3a32 100644 --- a/packages/tracing/test/span.test.ts +++ b/packages/tracing/test/span.test.ts @@ -1,7 +1,6 @@ import { BrowserClient } from '@sentry/browser'; import { Hub, makeMain, Scope } from '@sentry/hub'; import { BaseTransportOptions, ClientOptions, TransactionSource } from '@sentry/types'; -import { createBaggage, getSentryBaggageItems, getThirdPartyBaggage, isSentryBaggageEmpty } from '@sentry/utils'; import { Span, Transaction } from '../src'; import { TRACEPARENT_REGEXP } from '../src/utils'; @@ -393,7 +392,7 @@ describe('Span', () => { }); }); - describe('getBaggage and _populateBaggageWithSentryValues', () => { + describe('getDynamicSamplingContext', () => { beforeEach(() => { hub.getClient()!.getOptions = () => { return { @@ -403,31 +402,28 @@ describe('Span', () => { }; }); - test('leave baggage content untouched and just return baggage if it is immutable', () => { + test('should return DSC that was provided during transaction creation, if it was provided', () => { const transaction = new Transaction( { name: 'tx', - metadata: { baggage: createBaggage({ environment: 'myEnv' }, '', false) }, + metadata: { dynamicSamplingContext: { environment: 'myEnv' } }, }, hub, ); const hubSpy = jest.spyOn(hub.getClient()!, 'getOptions'); - const baggage = transaction.getBaggage(); + const dynamicSamplingContext = transaction.getDynamicSamplingContext(); - expect(hubSpy).toHaveBeenCalledTimes(0); - expect(baggage && isSentryBaggageEmpty(baggage)).toBe(false); - expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({ environment: 'myEnv' }); - expect(baggage && getThirdPartyBaggage(baggage)).toStrictEqual(''); + expect(hubSpy).not.toHaveBeenCalled(); + expect(dynamicSamplingContext).toStrictEqual({ environment: 'myEnv' }); }); - test('add Sentry baggage data to baggage if Sentry content is empty and baggage is mutable', () => { + test('should return new DSC, if no DSC was provided during transaction creation', () => { const transaction = new Transaction( { name: 'tx', metadata: { - baggage: createBaggage({}, '', true), transactionSampling: { rate: 0.56, method: 'client_rate' }, }, }, @@ -436,17 +432,15 @@ describe('Span', () => { const getOptionsSpy = jest.spyOn(hub.getClient()!, 'getOptions'); - const baggage = transaction.getBaggage(); + const dynamicSamplingContext = transaction.getDynamicSamplingContext(); expect(getOptionsSpy).toHaveBeenCalledTimes(1); - expect(baggage && isSentryBaggageEmpty(baggage)).toBe(false); - expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({ + expect(dynamicSamplingContext).toStrictEqual({ release: '1.0.1', environment: 'production', sample_rate: '0.56', trace_id: expect.any(String), }); - expect(baggage && getThirdPartyBaggage(baggage)).toStrictEqual(''); }); describe('Including transaction name in DSC', () => { @@ -464,7 +458,7 @@ describe('Span', () => { hub, ); - const dsc = getSentryBaggageItems(transaction.getBaggage()); + const dsc = transaction.getDynamicSamplingContext()!; expect(dsc.transaction).toBeUndefined(); }); @@ -483,7 +477,7 @@ describe('Span', () => { hub, ); - const dsc = getSentryBaggageItems(transaction.getBaggage()); + const dsc = transaction.getDynamicSamplingContext()!; expect(dsc.transaction).toEqual('tx'); }); From 0e3c3d397f7d19f0b17a847a41d3f1f3ddd7d191 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 15:11:40 +0000 Subject: [PATCH 18/32] Fix serverless tests --- packages/serverless/test/awslambda.test.ts | 24 ++++++++------------ packages/serverless/test/gcpfunction.test.ts | 14 ++++-------- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/packages/serverless/test/awslambda.test.ts b/packages/serverless/test/awslambda.test.ts index 2f2068e1ccba..89ed84183651 100644 --- a/packages/serverless/test/awslambda.test.ts +++ b/packages/serverless/test/awslambda.test.ts @@ -192,7 +192,7 @@ describe('AWSLambda', () => { const fakeTransactionContext = { name: 'functionName', op: 'awslambda.handler', - metadata: { baggage: [{}, '', true], source: 'component' }, + metadata: { source: 'component' }, }; expect(rv).toStrictEqual(42); @@ -219,7 +219,7 @@ describe('AWSLambda', () => { const fakeTransactionContext = { name: 'functionName', op: 'awslambda.handler', - metadata: { baggage: [{}, '', true], source: 'component' }, + metadata: { source: 'component' }, }; // @ts-ignore see "Why @ts-ignore" note @@ -262,13 +262,9 @@ describe('AWSLambda', () => { name: 'functionName', traceId: '12312012123120121231201212312012', metadata: { - baggage: [ - { - release: '2.12.1', - }, - '', - false, - ], + dynamicSamplingContext: { + release: '2.12.1', + }, source: 'component', }, }), @@ -300,7 +296,7 @@ describe('AWSLambda', () => { traceId: '12312012123120121231201212312012', parentSpanId: '1121201211212012', parentSampled: false, - metadata: { baggage: [{}, '', false], source: 'component' }, + metadata: { dynamicSamplingContext: {}, source: 'component' }, }; // @ts-ignore see "Why @ts-ignore" note @@ -327,7 +323,7 @@ describe('AWSLambda', () => { const fakeTransactionContext = { name: 'functionName', op: 'awslambda.handler', - metadata: { baggage: [{}, '', true], source: 'component' }, + metadata: { source: 'component' }, }; expect(rv).toStrictEqual(42); @@ -365,7 +361,7 @@ describe('AWSLambda', () => { const fakeTransactionContext = { name: 'functionName', op: 'awslambda.handler', - metadata: { baggage: [{}, '', true], source: 'component' }, + metadata: { source: 'component' }, }; // @ts-ignore see "Why @ts-ignore" note @@ -407,7 +403,7 @@ describe('AWSLambda', () => { const fakeTransactionContext = { name: 'functionName', op: 'awslambda.handler', - metadata: { baggage: [{}, '', true], source: 'component' }, + metadata: { source: 'component' }, }; expect(rv).toStrictEqual(42); @@ -445,7 +441,7 @@ describe('AWSLambda', () => { const fakeTransactionContext = { name: 'functionName', op: 'awslambda.handler', - metadata: { baggage: [{}, '', true], source: 'component' }, + metadata: { source: 'component' }, }; // @ts-ignore see "Why @ts-ignore" note diff --git a/packages/serverless/test/gcpfunction.test.ts b/packages/serverless/test/gcpfunction.test.ts index 9f1498714d0e..1c587df44634 100644 --- a/packages/serverless/test/gcpfunction.test.ts +++ b/packages/serverless/test/gcpfunction.test.ts @@ -114,7 +114,7 @@ describe('GCPFunction', () => { const fakeTransactionContext = { name: 'POST /path', op: 'gcp.function.http', - metadata: { baggage: [{}, '', true], source: 'route' }, + metadata: { source: 'route' }, }; // @ts-ignore see "Why @ts-ignore" note const fakeTransaction = { ...Sentry.fakeTransaction, ...fakeTransactionContext }; @@ -151,13 +151,9 @@ describe('GCPFunction', () => { parentSpanId: '1121201211212012', parentSampled: false, metadata: { - baggage: [ - { - release: '2.12.1', - }, - '', - false, - ], + dynamicSamplingContext: { + release: '2.12.1', + }, source: 'route', }, }; @@ -190,7 +186,7 @@ describe('GCPFunction', () => { traceId: '12312012123120121231201212312012', parentSpanId: '1121201211212012', parentSampled: false, - metadata: { baggage: [{}, '', false], source: 'route' }, + metadata: { dynamicSamplingContext: {}, source: 'route' }, }; // @ts-ignore see "Why @ts-ignore" note const fakeTransaction = { ...Sentry.fakeTransaction, ...fakeTransactionContext }; From 969567d7c3e80554b70f7011a8b16702d080c358 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 09:06:19 +0000 Subject: [PATCH 19/32] Why did I try to refactor things --- packages/tracing/src/browser/request.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 7e5ea845464a..8df7075cfdc0 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -195,9 +195,10 @@ export function fetchCallback( handlerData.fetchData.__span = span.spanId; spans[span.spanId] = span; - const request = handlerData.args[0] as string | Request; + const request = (handlerData.args[0] = handlerData.args[0] as string | Request); // eslint-disable-next-line @typescript-eslint/no-explicit-any - const options = (handlerData.args[1] as { [key: string]: any }) || {}; + const options = (handlerData.args[1] = (handlerData.args[1] as { [key: string]: any }) || {}); + options.headers = addTracingHeadersToFetchRequest( request, activeTransaction.getDynamicSamplingContext(), From 28b77898449720d26d956f69a0e0e54219e74c02 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 11:12:21 +0000 Subject: [PATCH 20/32] Fix bug --- packages/tracing/src/browser/request.ts | 81 +++++++++++++------------ 1 file changed, 43 insertions(+), 38 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 8df7075cfdc0..74e795144733 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -195,9 +195,13 @@ export function fetchCallback( handlerData.fetchData.__span = span.spanId; spans[span.spanId] = span; - const request = (handlerData.args[0] = handlerData.args[0] as string | Request); + const request: string | Request = handlerData.args[0]; + + // In case the user hasn't set the second argument of a fetch call we default it to `{}`. + handlerData.args[1] = handlerData.args[1] || {}; + // eslint-disable-next-line @typescript-eslint/no-explicit-any - const options = (handlerData.args[1] = (handlerData.args[1] as { [key: string]: any }) || {}); + const options: { [key: string]: any } = handlerData.args[1]; options.headers = addTracingHeadersToFetchRequest( request, @@ -213,17 +217,23 @@ function addTracingHeadersToFetchRequest( dynamicSamplingContext: Partial | undefined, span: Span, options: { - headers?: { - [key: string]: string[] | string | undefined; - }; + headers?: + | { + [key: string]: string[] | string | undefined; + } + | Request['headers']; }, ): PolymorphicRequestHeaders { const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); const sentryTraceHeader = span.toTraceparent(); - if (typeof Request !== 'undefined' && isInstanceOf(request, Request)) { - const headers = (request as Request).headers; - const newHeaders = new Headers(headers); + const headers = + typeof Request !== 'undefined' && isInstanceOf(request, Request) ? (request as Request).headers : options.headers; + + if (!headers) { + return { 'sentry-trace': sentryTraceHeader, baggage: sentryBaggageHeader }; + } else if (typeof Headers !== 'undefined' && isInstanceOf(headers, Headers)) { + const newHeaders = new Headers(headers as Headers); newHeaders.append('sentry-trace', sentryTraceHeader); @@ -234,41 +244,36 @@ function addTracingHeadersToFetchRequest( } return newHeaders as PolymorphicRequestHeaders; - } else { - if (!options.headers) { - return { 'sentry-trace': sentryTraceHeader, baggage: sentryBaggageHeader }; - } + } else if (Array.isArray(headers)) { + const newHeaders = [...headers]; + newHeaders.push(['sentry-trace', sentryTraceHeader]); - if (Array.isArray(options.headers)) { - const newHeaders = [...options.headers]; - newHeaders.push(['sentry-trace', sentryTraceHeader]); + if (sentryBaggageHeader) { + // If there are multiple entries with the same key, the browser will merge the values into a single request header. + // Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header. + newHeaders.push([BAGGAGE_HEADER_NAME, sentryBaggageHeader]); + } - if (sentryBaggageHeader) { - // If there are multiple entries with the same key, the browser will merge the values into a single request header. - // Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header. - newHeaders.push([BAGGAGE_HEADER_NAME, sentryBaggageHeader]); - } - return newHeaders; - } else { - const existingBaggageHeader = options.headers.baggage; - const newBaggageHeaders: string[] = []; - - if (Array.isArray(existingBaggageHeader)) { - newBaggageHeaders.push(...existingBaggageHeader); - } else if (existingBaggageHeader) { - newBaggageHeaders.push(existingBaggageHeader); - } + return newHeaders; + } else { + const existingBaggageHeader = 'baggage' in headers ? headers.baggage : undefined; + const newBaggageHeaders: string[] = []; - if (sentryBaggageHeader) { - newBaggageHeaders.push(sentryBaggageHeader); - } + if (Array.isArray(existingBaggageHeader)) { + newBaggageHeaders.push(...existingBaggageHeader); + } else if (existingBaggageHeader) { + newBaggageHeaders.push(existingBaggageHeader); + } - return { - ...options.headers, - 'sentry-trace': sentryTraceHeader, - baggage: newBaggageHeaders.length > 0 ? newBaggageHeaders.join(', ') : undefined, - }; + if (sentryBaggageHeader) { + newBaggageHeaders.push(sentryBaggageHeader); } + + return { + ...(headers as Exclude), + 'sentry-trace': sentryTraceHeader, + baggage: newBaggageHeaders.length > 0 ? newBaggageHeaders.join(', ') : undefined, + }; } } From 2d339ba70bfbd8ae93937b7636d0c27430705249 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 11:32:05 +0000 Subject: [PATCH 21/32] Fix nextjs unit tests --- packages/nextjs/test/performance/client.test.ts | 4 ++-- packages/nextjs/test/utils/withSentry.test.ts | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts index 01494fd7c519..2e152b6decfd 100644 --- a/packages/nextjs/test/performance/client.test.ts +++ b/packages/nextjs/test/performance/client.test.ts @@ -142,7 +142,7 @@ describe('nextRouterInstrumentation', () => { }, metadata: { source: 'route', - baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true], + dynamicSamplingContext: { environment: 'myEnv', release: '2.1.0' }, }, traceId: 'c82b8554881b4d28ad977de04a4fb40a', parentSpanId: 'a755953cd3394d5f', @@ -168,7 +168,7 @@ describe('nextRouterInstrumentation', () => { }, metadata: { source: 'route', - baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true], + dynamicSamplingContext: { environment: 'myEnv', release: '2.1.0' }, }, traceId: 'c82b8554881b4d28ad977de04a4fb40a', parentSpanId: 'a755953cd3394d5f', diff --git a/packages/nextjs/test/utils/withSentry.test.ts b/packages/nextjs/test/utils/withSentry.test.ts index 4d8709e8e735..105d23735e09 100644 --- a/packages/nextjs/test/utils/withSentry.test.ts +++ b/packages/nextjs/test/utils/withSentry.test.ts @@ -117,7 +117,6 @@ describe('withSentry', () => { op: 'http.server', metadata: { - baggage: expect.any(Array), source: 'route', }, }, From a76c891a645d10d369e361ec1d9f20390725f087 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 13:08:40 +0000 Subject: [PATCH 22/32] Add handling for empty baggage header --- packages/utils/src/baggage.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/utils/src/baggage.ts b/packages/utils/src/baggage.ts index 8a92ea150064..4cf44029f892 100644 --- a/packages/utils/src/baggage.ts +++ b/packages/utils/src/baggage.ts @@ -42,6 +42,12 @@ export function baggageHeaderToDynamicSamplingContext( }; }, {}); } else { + // Return undefined if baggage header is an empty string (technically an empty baggage header is not spec conform but + // this is how we choose to handle it) + if (!baggageHeader) { + return undefined; + } + baggageObject = baggageHeaderToObject(baggageHeader); } From 9cf864496a2195ae237cc72a9a259dae78908b78 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 13:11:51 +0000 Subject: [PATCH 23/32] fix(utils): Make empty traceparent string count as invalid --- packages/tracing/test/utils.test.ts | 3 +++ packages/utils/src/tracing.ts | 30 ++++++++++++++++------------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/packages/tracing/test/utils.test.ts b/packages/tracing/test/utils.test.ts index eae5213758ea..3bd3e9f5f35f 100644 --- a/packages/tracing/test/utils.test.ts +++ b/packages/tracing/test/utils.test.ts @@ -60,6 +60,9 @@ describe('extractTraceparentData', () => { }); test('invalid', () => { + // empty string + expect(extractTraceparentData('')).toBeUndefined(); + // trace id wrong length expect(extractTraceparentData('a-bbbbbbbbbbbbbbbb-1')).toBeUndefined(); diff --git a/packages/utils/src/tracing.ts b/packages/utils/src/tracing.ts index aef92cf5f0ca..cef6e334f7e7 100644 --- a/packages/utils/src/tracing.ts +++ b/packages/utils/src/tracing.ts @@ -17,18 +17,22 @@ export const TRACEPARENT_REGEXP = new RegExp( */ 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], - }; + + if (!traceparent || !matches) { + // empty string or no matches is invalid traceparent data + return undefined; + } + + let parentSampled: boolean | undefined; + if (matches[3] === '1') { + parentSampled = true; + } else if (matches[3] === '0') { + parentSampled = false; } - return undefined; + + return { + traceId: matches[1], + parentSampled, + parentSpanId: matches[2], + }; } From 3d953ad56f5d69bbae55964d40af3b530ede43d3 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 15:20:13 +0000 Subject: [PATCH 24/32] Minor comments --- packages/node/src/integrations/http.ts | 1 - packages/node/test/handlers.test.ts | 2 +- packages/tracing/src/transaction.ts | 2 ++ packages/types/src/transaction.ts | 5 ++++ packages/utils/src/baggage.ts | 33 ++++++++++++++++++-------- 5 files changed, 31 insertions(+), 12 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 520fc2964486..151fa384b9f9 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -173,7 +173,6 @@ function _createWrappedRequestMethodFactory( const dynamicSamplingContext = parentSpan.transaction.getDynamicSamplingContext(); const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - // Append a new baggage header to let newBaggageHeaderField; if (!requestOptions.headers || !requestOptions.headers.baggage) { newBaggageHeaderField = sentryBaggageHeader; diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index a2775e4241c7..9910048cd942 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -241,7 +241,7 @@ describe('tracingHandler', () => { expect(transaction.metadata?.dynamicSamplingContext).toStrictEqual({ version: '1.0', environment: 'production' }); }); - it('doesn\t populate dynamic sampling context with 3rd party baggage', () => { + it("doesn't populate dynamic sampling context with 3rd party baggage", () => { req.headers = { 'sentry-trace': '12312012123120121231201212312012-1121201211212012-0', baggage: 'sentry-version=1.0,sentry-environment=production,dogs=great,cats=boring', diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index 59de9e134a26..acc2f592ccf2 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -224,7 +224,9 @@ export class Transaction extends SpanClass implements TransactionInterface { } /** + * @inheritdoc * + * @experimental */ public getDynamicSamplingContext(): Partial | undefined { if (this._frozenDynamicSamplingContext) { diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 47cc6b0f1ee4..efb9b7158966 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -95,6 +95,7 @@ export interface Transaction extends TransactionContext, Span { */ setMetadata(newMetadata: Partial): void; + /** Return the current Dynamic Sampling Context of this transaction */ getDynamicSamplingContext(): Partial | undefined; } @@ -138,6 +139,10 @@ export type TransactionSamplingMethod = 'explicitly_set' | 'client_sampler' | 'c export interface TransactionMetadata { transactionSampling?: { rate?: number; method: TransactionSamplingMethod }; + /** + * The Dynamic Sampling Context of a transaction. If provided during transaction creation, its Dynamic Sampling + * Context Will be frozen + */ dynamicSamplingContext?: Partial; /** For transactions tracing server-side request handling, the path of the request being tracked. */ diff --git a/packages/utils/src/baggage.ts b/packages/utils/src/baggage.ts index 4cf44029f892..d98ec37f9dc4 100644 --- a/packages/utils/src/baggage.ts +++ b/packages/utils/src/baggage.ts @@ -17,8 +17,11 @@ export const SENTRY_BAGGAGE_KEY_PREFIX_REGEX = /^sentry-/; export const MAX_BAGGAGE_STRING_LENGTH = 8192; /** - * TODO - * @param baggageHeader + * Takes a baggage header and turns it into Dynamic Sampling Context, by extracting all the "sentry-" prefixed values + * from it. + * + * @param baggageHeader A very bread definition of a baggage header as it might appear in various frameworks. + * @returns The Dynamic Sampling Context that was found on `baggageHeader`, if there was any, `undefined` otherwise. */ export function baggageHeaderToDynamicSamplingContext( // Very liberal definition of what any incoming header might look like @@ -70,8 +73,13 @@ export function baggageHeaderToDynamicSamplingContext( } /** - * TODO - * @param dynamicSamplingContext + * Turns a Dynamic Sampling Object into a baggage header by prefixing all the keys on the object with "sentry-". + * + * @param dynamicSamplingContext The Dynamic Sampling Context to turn into a header. For convenience and compatibility + * with the `getDynamicSamplingContext` method on the Transaction class ,this argument can also be `undefined`. If it is + * `undefined` the function will return `undefined`. + * @returns a baggage header, created from `dynamicSamplingContext`, or `undefined` either if `dynamicSamplingContext` + * was `undefined`, or if `dynamicSamplingContext` didn't contain any values. */ export function dynamicSamplingContextToSentryBaggageHeader( // this also takes undefined for convenience and bundle size in other places @@ -96,10 +104,12 @@ export function dynamicSamplingContextToSentryBaggageHeader( } /** - * TODO - * @param baggageHeader + * Will parse a baggage header, which is a simple key-value map, into a flat object. + * + * @param baggageHeader The baggage header to parse. + * @returns a flat object containing all the key-value pairs from `baggageHeader`. */ -export function baggageHeaderToObject(baggageHeader: string): Record { +function baggageHeaderToObject(baggageHeader: string): Record { return baggageHeader .split(',') .map(baggageEntry => baggageEntry.split('=').map(keyOrValue => decodeURIComponent(keyOrValue.trim()))) @@ -110,10 +120,13 @@ export function baggageHeaderToObject(baggageHeader: string): Record): string | undefined { +function objectToBaggageHeader(object: Record): string | undefined { if (Object.keys(object).length === 0) { // An empty baggage header is not spec compliant: We return undefined. return undefined; From 08c708a2aa0048407d4546ea5c3bb23ac1a3f9c6 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 15:20:19 +0000 Subject: [PATCH 25/32] Smol refactor --- packages/tracing/src/browser/browsertracing.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index f96f43827761..8a06f029d53f 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -215,8 +215,10 @@ export class BrowserTracing implements Integration { // eslint-disable-next-line @typescript-eslint/unbound-method const { beforeNavigate, idleTimeout, finalTimeout } = this.options; - const sentryTraceMetaTagValue = context.op === 'pageload' ? getMetaContent('sentry-trace') : null; - const baggageMetaTagValue = context.op === 'pageload' ? getMetaContent('baggage') : null; + const isPageloadTransaction = context.op === 'pageload'; + + const sentryTraceMetaTagValue = isPageloadTransaction ? getMetaContent('sentry-trace') : null; + const baggageMetaTagValue = isPageloadTransaction ? getMetaContent('baggage') : null; const traceParentData = sentryTraceMetaTagValue ? extractTraceparentData(sentryTraceMetaTagValue) : undefined; const dynamicSamplingContext = baggageMetaTagValue From ed682398de9414ec0d0975f2955c5b2c154dd9e4 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 15:20:28 +0000 Subject: [PATCH 26/32] Add tests --- packages/utils/test/baggage.test.ts | 41 +++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 packages/utils/test/baggage.test.ts diff --git a/packages/utils/test/baggage.test.ts b/packages/utils/test/baggage.test.ts new file mode 100644 index 000000000000..539a34e44d9c --- /dev/null +++ b/packages/utils/test/baggage.test.ts @@ -0,0 +1,41 @@ +import { baggageHeaderToDynamicSamplingContext, dynamicSamplingContextToSentryBaggageHeader } from '../src/baggage'; + +test.each([ + ['', undefined], + [' ', undefined], + ['sentry-environment=production,sentry-release=10.0.2', { environment: 'production', release: '10.0.2' }], + [ + 'userId=alice,serverNode=DF%2028,isProduction=false,sentry-environment=production,sentry-release=10.0.2', + { environment: 'production', release: '10.0.2' }, + ], + ['userId=alice,serverNode=DF%2028,isProduction=false', undefined], + [ + 'userId=alice, serverNode=DF%2028 , isProduction=false, ,,sentry-environment=production,,sentry-release=10.0.2', + { environment: 'production', release: '10.0.2' }, + ], + [['userId=alice', 'sentry-environment=production', 'foo=bar'], { environment: 'production' }], + [ + ['userId=alice, userName=bob', 'sentry-environment=production,sentry-release=1.0.1', 'foo=bar'], + { environment: 'production', release: '1.0.1' }, + ], + [ + ['', 'sentry-environment=production,sentry-release=1.0.1', ' ', 'foo=bar'], + { environment: 'production', release: '1.0.1' }, + ], + [42, undefined], +])('baggageHeaderToDynamicSamplingContext(%p) should return %p', (input, expectedOutput) => { + expect(baggageHeaderToDynamicSamplingContext(input)).toStrictEqual(expectedOutput); +}); + +test.each([ + [undefined, undefined], + [{}, undefined], + [{ release: 'abcdf' }, 'sentry-release=abcdf'], + [{ release: 'abcdf', environment: '1234' }, 'sentry-release=abcdf,sentry-environment=1234'], + [ + { release: 'abcdf', environment: '1234', someRandomKey: 'foo' }, + 'sentry-release=abcdf,sentry-environment=1234,sentry-someRandomKey=foo', + ], +])('dynamicSamplingContextToSentryBaggageHeader(%p) should return %p', (input, expectedOutput) => { + expect(dynamicSamplingContextToSentryBaggageHeader(input)).toStrictEqual(expectedOutput); +}); From fafc4afec04750a1392d51157547c97b580b7747 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 14 Sep 2022 09:29:00 +0000 Subject: [PATCH 27/32] Make DSC read from transaction readonly --- packages/tracing/src/transaction.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index acc2f592ccf2..c8402b22b5ee 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -27,7 +27,7 @@ export class Transaction extends SpanClass implements TransactionInterface { private _trimEnd?: boolean; - private _frozenDynamicSamplingContext: Partial | undefined = undefined; + private _frozenDynamicSamplingContext: Readonly> | undefined = undefined; /** * This constructor should never be called manually. Those instrumenting tracing should use @@ -57,9 +57,10 @@ export class Transaction extends SpanClass implements TransactionInterface { // If Dynamic Sampling Context is provided during the creation of the transaction, we freeze it as it usually means // there is incoming Dynamic Sampling Context. (Either through an incoming request, a baggage meta-tag, or other means) - const incomingDynamicSamplingContext = (transactionContext.metadata || {}).dynamicSamplingContext; + const incomingDynamicSamplingContext = this.metadata.dynamicSamplingContext; if (incomingDynamicSamplingContext) { - this._frozenDynamicSamplingContext = incomingDynamicSamplingContext; + // We shallow copy this in case anything writes to the original reference of the passed in `dynamicSamplingContext` + this._frozenDynamicSamplingContext = { ...incomingDynamicSamplingContext }; } } @@ -228,7 +229,7 @@ export class Transaction extends SpanClass implements TransactionInterface { * * @experimental */ - public getDynamicSamplingContext(): Partial | undefined { + public getDynamicSamplingContext(): Readonly> | undefined { if (this._frozenDynamicSamplingContext) { return this._frozenDynamicSamplingContext; } From 5580981949f4cb0254879371fb728a5578842846 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 14 Sep 2022 13:34:08 +0000 Subject: [PATCH 28/32] Move down dynamicSamplingProps --- .../nextjs/src/config/wrappers/withSentryGetServerSideProps.ts | 3 ++- .../config/wrappers/withSentryServerSideAppGetInitialProps.ts | 3 ++- .../wrappers/withSentryServerSideErrorGetInitialProps.ts | 3 ++- .../src/config/wrappers/withSentryServerSideGetInitialProps.ts | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts index baad11081193..700d1899d56b 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts @@ -44,8 +44,9 @@ export function withSentryGetServerSideProps( if ('props' in serverSideProps) { const requestTransaction = getTransactionFromRequest(req); if (requestTransaction) { - const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); serverSideProps.props._sentryTraceData = requestTransaction.toTraceparent(); + + const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); serverSideProps.props._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); } } diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts index 5c099fd59f03..74e811f8f224 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts @@ -50,8 +50,9 @@ export function withSentryServerSideAppGetInitialProps(origAppGetInitialProps: A const requestTransaction = getTransactionFromRequest(req!); if (requestTransaction) { - const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); appGetInitialProps.pageProps._sentryTraceData = requestTransaction.toTraceparent(); + + const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); appGetInitialProps.pageProps._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); } diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts index 0c0af3715903..8950fcf720a0 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts @@ -51,8 +51,9 @@ export function withSentryServerSideErrorGetInitialProps( const requestTransaction = getTransactionFromRequest(req!); if (requestTransaction) { - const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); errorGetInitialProps._sentryTraceData = requestTransaction.toTraceparent(); + + const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); errorGetInitialProps._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); } diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts index d90055ad76f5..f591a4048c85 100644 --- a/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts @@ -41,8 +41,9 @@ export function withSentryServerSideGetInitialProps(origGetInitialProps: GetInit const requestTransaction = getTransactionFromRequest(req!); if (requestTransaction) { - const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); initialProps._sentryTraceData = requestTransaction.toTraceparent(); + + const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); initialProps._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); } From 7fba2fb56d028fbd10a2ca42da093e84a8c48e05 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 14 Sep 2022 13:48:51 +0000 Subject: [PATCH 29/32] Spread instead of push --- packages/tracing/src/browser/request.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 3ab779933f82..a0bdadeb26f7 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -247,8 +247,7 @@ function addTracingHeadersToFetchRequest( return newHeaders as PolymorphicRequestHeaders; } else if (Array.isArray(headers)) { - const newHeaders = [...headers]; - newHeaders.push(['sentry-trace', sentryTraceHeader]); + const newHeaders = [...headers, ['sentry-trace', sentryTraceHeader]]; if (sentryBaggageHeader) { // If there are multiple entries with the same key, the browser will merge the values into a single request header. From 18df790c51aed97aa1b3562c353d7cd16df4b1b0 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 14 Sep 2022 13:49:54 +0000 Subject: [PATCH 30/32] Save a byte --- packages/tracing/src/browser/request.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index a0bdadeb26f7..7649723985e3 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -273,7 +273,7 @@ function addTracingHeadersToFetchRequest( return { ...(headers as Exclude), 'sentry-trace': sentryTraceHeader, - baggage: newBaggageHeaders.length > 0 ? newBaggageHeaders.join(', ') : undefined, + baggage: newBaggageHeaders.length > 0 ? newBaggageHeaders.join(',') : undefined, }; } } From 55aa327a905abbb4e4d8f104de80c337f866d506 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 14 Sep 2022 13:56:19 +0000 Subject: [PATCH 31/32] Always return object in getDynamicSamplingContext --- packages/tracing/src/browser/request.ts | 2 +- packages/tracing/src/transaction.ts | 4 ++-- packages/types/src/transaction.ts | 2 +- packages/utils/src/baggage.ts | 6 +----- 4 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 7649723985e3..4f25c787e158 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -216,7 +216,7 @@ export function fetchCallback( function addTracingHeadersToFetchRequest( request: string | Request, - dynamicSamplingContext: Partial | undefined, + dynamicSamplingContext: Partial, span: Span, options: { headers?: diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index c8402b22b5ee..7f65a935f3d9 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -229,7 +229,7 @@ export class Transaction extends SpanClass implements TransactionInterface { * * @experimental */ - public getDynamicSamplingContext(): Readonly> | undefined { + public getDynamicSamplingContext(): Readonly> { if (this._frozenDynamicSamplingContext) { return this._frozenDynamicSamplingContext; } @@ -237,7 +237,7 @@ export class Transaction extends SpanClass implements TransactionInterface { const hub: Hub = this._hub || getCurrentHub(); const client = hub && hub.getClient(); - if (!client) return undefined; + if (!client) return {}; const { environment, release } = client.getOptions() || {}; const { publicKey: public_key } = client.getDsn() || {}; diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index efb9b7158966..035a6063fbf5 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -96,7 +96,7 @@ export interface Transaction extends TransactionContext, Span { setMetadata(newMetadata: Partial): void; /** Return the current Dynamic Sampling Context of this transaction */ - getDynamicSamplingContext(): Partial | undefined; + getDynamicSamplingContext(): Partial; } /** diff --git a/packages/utils/src/baggage.ts b/packages/utils/src/baggage.ts index d98ec37f9dc4..1a2fa58d52a6 100644 --- a/packages/utils/src/baggage.ts +++ b/packages/utils/src/baggage.ts @@ -83,12 +83,8 @@ export function baggageHeaderToDynamicSamplingContext( */ export function dynamicSamplingContextToSentryBaggageHeader( // this also takes undefined for convenience and bundle size in other places - dynamicSamplingContext: Partial | undefined, + dynamicSamplingContext: Partial, ): string | undefined { - if (!dynamicSamplingContext) { - return undefined; - } - // Prefix all DSC keys with "sentry-" and put them into a new object const sentryPrefixedDSC = Object.entries(dynamicSamplingContext).reduce>( (acc, [dscKey, dscValue]) => { From bf06b7b8eb6aab079d18076a796c464ecf80370b Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 14 Sep 2022 15:35:45 +0000 Subject: [PATCH 32/32] Fix test --- packages/utils/test/baggage.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/utils/test/baggage.test.ts b/packages/utils/test/baggage.test.ts index 539a34e44d9c..8f848badf9de 100644 --- a/packages/utils/test/baggage.test.ts +++ b/packages/utils/test/baggage.test.ts @@ -28,7 +28,6 @@ test.each([ }); test.each([ - [undefined, undefined], [{}, undefined], [{ release: 'abcdf' }, 'sentry-release=abcdf'], [{ release: 'abcdf', environment: '1234' }, 'sentry-release=abcdf,sentry-environment=1234'],