From 299e93b9a1ee0d2916c6ac96c9a75df7c9fb1c07 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 17 Aug 2022 14:32:41 -0400 Subject: [PATCH 1/7] feat(remix): Continue transaction from request headers --- packages/remix/src/utils/instrumentServer.ts | 39 ++++++++++++++++--- .../remix/src/utils/serverAdapters/express.ts | 10 ++++- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 04107fe07939..7afdd3249695 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -2,7 +2,16 @@ import { captureException, getCurrentHub, Hub } from '@sentry/node'; import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; import { Transaction, WrappedFunction } from '@sentry/types'; -import { addExceptionMechanism, fill, isNodeEnv, loadModule, logger, serializeBaggage } from '@sentry/utils'; +import { + addExceptionMechanism, + extractTraceparentData, + fill, + isNodeEnv, + loadModule, + logger, + parseBaggageSetMutability, + serializeBaggage, +} from '@sentry/utils'; import * as domain from 'domain'; import { @@ -289,15 +298,25 @@ function matchServerRoutes( * @param pkg */ export function startRequestHandlerTransaction( + hub: Hub, url: URL, - method: string, routes: ServerRoute[], - hub: Hub, - pkg?: ReactRouterDomPkg, + pkg: ReactRouterDomPkg | undefined, + request: { + headers: { + 'sentry-trace': string; + baggage: string; + }; + method: string; + }, ): Transaction { const currentScope = hub.getScope(); const matches = matchServerRoutes(routes, url.pathname, pkg); + // 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 match = matches && getRequestMatch(url, matches); const name = match === null ? url.pathname : match.route.id; const source = match === null ? 'url' : 'route'; @@ -305,10 +324,12 @@ export function startRequestHandlerTransaction( name, op: 'http.server', tags: { - method: method, + method: request.method, }, + ...traceparentData, metadata: { source, + baggage, }, }); @@ -330,7 +351,13 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui } const url = new URL(request.url); - const transaction = startRequestHandlerTransaction(url, request.method, routes, hub, pkg); + const transaction = startRequestHandlerTransaction(hub, url, routes, pkg, { + headers: { + 'sentry-trace': request.headers.get('sentry-trace') || '', + baggage: request.headers.get('baggage') || '', + }, + method: request.method, + }); const res = (await origRequestHandler.call(this, request, loadContext)) as Response; diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index 71798863430c..f16b0fd5893e 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -2,7 +2,7 @@ import { getCurrentHub } from '@sentry/hub'; import { flush } from '@sentry/node'; import { hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; -import { extractRequestData, loadModule, logger } from '@sentry/utils'; +import { extractRequestData, isString, loadModule, logger } from '@sentry/utils'; import { createRoutes, @@ -51,7 +51,13 @@ function wrapExpressRequestHandler( } const url = new URL(request.url); - const transaction = startRequestHandlerTransaction(url, request.method, routes, hub, pkg); + const transaction = startRequestHandlerTransaction(hub, url, routes, pkg, { + headers: { + 'sentry-trace': (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '', + baggage: (req.headers && isString(req.headers.baggage) && req.headers.baggage) || '', + }, + method: request.method, + }); // save a link to the transaction on the response, so that even if there's an error (landing us outside of // the domain), we can still finish it (albeit possibly missing some scope data) (res as AugmentedExpressResponse).__sentryTransaction = transaction; From 49e907e036fd1406f1e1ca1c7fef018e3cd895c9 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Aug 2022 14:37:56 -0400 Subject: [PATCH 2/7] only add baggage if defined --- packages/remix/src/utils/instrumentServer.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 7afdd3249695..d82d3c45c49b 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -1,12 +1,13 @@ /* eslint-disable max-lines */ import { captureException, getCurrentHub, Hub } from '@sentry/node'; import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; -import { Transaction, WrappedFunction } from '@sentry/types'; +import { Transaction, TransactionSource, WrappedFunction } from '@sentry/types'; import { addExceptionMechanism, extractTraceparentData, fill, isNodeEnv, + isSentryBaggageEmpty, loadModule, logger, parseBaggageSetMutability, @@ -310,16 +311,15 @@ export function startRequestHandlerTransaction( method: string; }, ): Transaction { - const currentScope = hub.getScope(); - const matches = matchServerRoutes(routes, url.pathname, pkg); - // 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 matches = matchServerRoutes(routes, url.pathname, pkg); const match = matches && getRequestMatch(url, matches); - const name = match === null ? url.pathname : match.route.id; - const source = match === null ? 'url' : 'route'; + const [name, source]: [string, TransactionSource] = + match === null ? [url.pathname, 'url'] : [match.route.id, 'route']; + const transaction = hub.startTransaction({ name, op: 'http.server', @@ -329,11 +329,12 @@ export function startRequestHandlerTransaction( ...traceparentData, metadata: { source, - baggage, + // Only attach baggage if it's defined + ...(isSentryBaggageEmpty(baggage) && { baggage }), }, }); - currentScope?.setSpan(transaction); + hub.getScope()?.setSpan(transaction); return transaction; } From 150fa53fd586fb7768214b42a76a123832936657 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Aug 2022 14:40:39 -0400 Subject: [PATCH 3/7] add integration test --- .../node-integration-tests/utils/index.ts | 26 +++++++++++------ .../integration/test/server/loader.test.ts | 28 +++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts index 5af54b065686..87bce682da88 100644 --- a/packages/node-integration-tests/utils/index.ts +++ b/packages/node-integration-tests/utils/index.ts @@ -2,7 +2,7 @@ import * as Sentry from '@sentry/node'; import { EnvelopeItemType } from '@sentry/types'; import { logger, parseSemver } from '@sentry/utils'; -import axios from 'axios'; +import axios, { AxiosRequestConfig } from 'axios'; import { Express } from 'express'; import * as http from 'http'; import nock from 'nock'; @@ -102,12 +102,16 @@ export async function runScenario(url: string): Promise { await Sentry.flush(); } -async function makeRequest(method: 'get' | 'post' = 'get', url: string): Promise { +async function makeRequest( + method: 'get' | 'post' = 'get', + url: string, + axiosConfig?: AxiosRequestConfig, +): Promise { try { if (method === 'get') { - await axios.get(url); + await axios.get(url, axiosConfig); } else { - await axios.post(url); + await axios.post(url, axiosConfig); } } catch (e) { // We sometimes expect the request to fail, but not the test. @@ -161,7 +165,10 @@ export class TestEnv { * @param {DataCollectorOptions} options * @returns The intercepted envelopes. */ - public async getMultipleEnvelopeRequest(options: DataCollectorOptions): Promise[][]> { + public async getMultipleEnvelopeRequest( + options: DataCollectorOptions, + axiosConfig?: AxiosRequestConfig, + ): Promise[][]> { const envelopeTypeArray = typeof options.envelopeType === 'string' ? [options.envelopeType] @@ -173,7 +180,7 @@ export class TestEnv { envelopeTypeArray, ); - void makeRequest(options.method, options.url || this.url); + void makeRequest(options.method, options.url || this.url, axiosConfig); return resProm; } @@ -183,8 +190,11 @@ export class TestEnv { * @param {DataCollectorOptions} options * @returns The extracted envelope. */ - public async getEnvelopeRequest(options?: DataCollectorOptions): Promise>> { - return (await this.getMultipleEnvelopeRequest({ ...options, count: 1 }))[0]; + public async getEnvelopeRequest( + options?: DataCollectorOptions, + axiosConfig?: AxiosRequestConfig, + ): Promise>> { + return (await this.getMultipleEnvelopeRequest({ ...options, count: 1 }, axiosConfig))[0]; } /** diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 8d8b0b93ce00..937228309629 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -157,4 +157,32 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada expect(tags[key]).toEqual(val); }); }); + + it('continues transaction from sentry-trace header and baggage', async () => { + const env = await RemixTestEnv.init(adapter); + const url = `${env.url}/loader-json-response/3`; + + const envelope = await env.getEnvelopeRequest( + { url, envelopeType: 'transaction' }, + { + headers: { + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', + baggage: 'sentry-version=1.0,sentry-environment=production,sentry-trace_id=12312012123120121231201212312012', + }, + }, + ); + + expect(envelope[0].trace).toMatchObject({ + trace_id: '12312012123120121231201212312012', + }); + + assertSentryTransaction(envelope[2], { + contexts: { + trace: { + trace_id: '12312012123120121231201212312012', + parent_span_id: '1121201211212012', + }, + }, + }); + }); }); From f5f3f610a07133e8181ce02b1457ceefe3c3df80 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 30 Aug 2022 15:16:13 -0400 Subject: [PATCH 4/7] use correct conditional --- packages/remix/src/utils/instrumentServer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index d82d3c45c49b..fbf2bad6cae5 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -330,7 +330,7 @@ export function startRequestHandlerTransaction( metadata: { source, // Only attach baggage if it's defined - ...(isSentryBaggageEmpty(baggage) && { baggage }), + ...(!isSentryBaggageEmpty(baggage) && { baggage }), }, }); From adf2ac8403896a8c802836fbb2f20bc6b7c73102 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 31 Aug 2022 13:39:10 -0400 Subject: [PATCH 5/7] make axios config method on testEnv class --- .../node-integration-tests/utils/index.ts | 20 +++++++++---------- .../integration/test/server/loader.test.ts | 15 +++++++------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts index 87bce682da88..b1e01f07a324 100644 --- a/packages/node-integration-tests/utils/index.ts +++ b/packages/node-integration-tests/utils/index.ts @@ -121,6 +121,8 @@ async function makeRequest( } export class TestEnv { + private _axiosConfig: AxiosRequestConfig = undefined; + public constructor(public readonly server: http.Server, public readonly url: string) { this.server = server; this.url = url; @@ -165,10 +167,7 @@ export class TestEnv { * @param {DataCollectorOptions} options * @returns The intercepted envelopes. */ - public async getMultipleEnvelopeRequest( - options: DataCollectorOptions, - axiosConfig?: AxiosRequestConfig, - ): Promise[][]> { + public async getMultipleEnvelopeRequest(options: DataCollectorOptions): Promise[][]> { const envelopeTypeArray = typeof options.envelopeType === 'string' ? [options.envelopeType] @@ -180,7 +179,7 @@ export class TestEnv { envelopeTypeArray, ); - void makeRequest(options.method, options.url || this.url, axiosConfig); + void makeRequest(options.method, options.url || this.url, this._axiosConfig); return resProm; } @@ -190,11 +189,8 @@ export class TestEnv { * @param {DataCollectorOptions} options * @returns The extracted envelope. */ - public async getEnvelopeRequest( - options?: DataCollectorOptions, - axiosConfig?: AxiosRequestConfig, - ): Promise>> { - return (await this.getMultipleEnvelopeRequest({ ...options, count: 1 }, axiosConfig))[0]; + public async getEnvelopeRequest(options?: DataCollectorOptions): Promise>> { + return (await this.getMultipleEnvelopeRequest({ ...options, count: 1 }))[0]; } /** @@ -256,4 +252,8 @@ export class TestEnv { .reply(200); }); } + + public setAxiosConfig(axiosConfig: AxiosRequestConfig): void { + this._axiosConfig = axiosConfig; + } } diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 937228309629..78ecc2a67c01 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -162,15 +162,14 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada const env = await RemixTestEnv.init(adapter); const url = `${env.url}/loader-json-response/3`; - const envelope = await env.getEnvelopeRequest( - { url, envelopeType: 'transaction' }, - { - headers: { - 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', - baggage: 'sentry-version=1.0,sentry-environment=production,sentry-trace_id=12312012123120121231201212312012', - }, + // send sentry-trace and baggage headers to loader + env.setAxiosConfig({ + headers: { + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', + baggage: 'sentry-version=1.0,sentry-environment=production,sentry-trace_id=12312012123120121231201212312012', }, - ); + }); + const envelope = await env.getEnvelopeRequest({ url, envelopeType: 'transaction' }); expect(envelope[0].trace).toMatchObject({ trace_id: '12312012123120121231201212312012', From 472459700ac91fb0fdf7d1e80118e5454758dfad Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 31 Aug 2022 17:19:38 -0400 Subject: [PATCH 6/7] fix axios config type --- packages/node-integration-tests/utils/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts index b1e01f07a324..50da10fd7820 100644 --- a/packages/node-integration-tests/utils/index.ts +++ b/packages/node-integration-tests/utils/index.ts @@ -121,7 +121,7 @@ async function makeRequest( } export class TestEnv { - private _axiosConfig: AxiosRequestConfig = undefined; + private _axiosConfig: AxiosRequestConfig | undefined = undefined; public constructor(public readonly server: http.Server, public readonly url: string) { this.server = server; From fc4f653cdf04ada300e4159e86348b6eb0a1daf1 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 1 Sep 2022 14:04:54 -0400 Subject: [PATCH 7/7] extract transaction name creation logic --- packages/remix/src/utils/instrumentServer.ts | 27 ++++++++++++------- .../remix/src/utils/serverAdapters/express.ts | 4 ++- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index fbf2bad6cae5..36a62a3749c0 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -300,9 +300,8 @@ function matchServerRoutes( */ export function startRequestHandlerTransaction( hub: Hub, - url: URL, - routes: ServerRoute[], - pkg: ReactRouterDomPkg | undefined, + name: string, + source: TransactionSource, request: { headers: { 'sentry-trace': string; @@ -315,11 +314,6 @@ export function startRequestHandlerTransaction( const traceparentData = extractTraceparentData(request.headers['sentry-trace']); const baggage = parseBaggageSetMutability(request.headers.baggage, traceparentData); - const matches = matchServerRoutes(routes, url.pathname, pkg); - const match = matches && getRequestMatch(url, matches); - const [name, source]: [string, TransactionSource] = - match === null ? [url.pathname, 'url'] : [match.route.id, 'route']; - const transaction = hub.startTransaction({ name, op: 'http.server', @@ -338,6 +332,19 @@ export function startRequestHandlerTransaction( return transaction; } +/** + * Get transaction name from routes and url + */ +export function getTransactionName( + routes: ServerRoute[], + url: URL, + pkg?: ReactRouterDomPkg, +): [string, TransactionSource] { + const matches = matchServerRoutes(routes, url.pathname, pkg); + const match = matches && getRequestMatch(url, matches); + return match === null ? [url.pathname, 'url'] : [match.route.id, 'route']; +} + function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler { const routes = createRoutes(build.routes); const pkg = loadModule('react-router-dom'); @@ -352,7 +359,9 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui } const url = new URL(request.url); - const transaction = startRequestHandlerTransaction(hub, url, routes, pkg, { + const [name, source] = getTransactionName(routes, url, pkg); + + const transaction = startRequestHandlerTransaction(hub, name, source, { headers: { 'sentry-trace': request.headers.get('sentry-trace') || '', baggage: request.headers.get('baggage') || '', diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index f16b0fd5893e..f3d3a7218281 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -6,6 +6,7 @@ import { extractRequestData, isString, loadModule, logger } from '@sentry/utils' import { createRoutes, + getTransactionName, instrumentBuild, isRequestHandlerWrapped, startRequestHandlerTransaction, @@ -51,7 +52,8 @@ function wrapExpressRequestHandler( } const url = new URL(request.url); - const transaction = startRequestHandlerTransaction(hub, url, routes, pkg, { + const [name, source] = getTransactionName(routes, url, pkg); + const transaction = startRequestHandlerTransaction(hub, name, source, { headers: { 'sentry-trace': (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '', baggage: (req.headers && isString(req.headers.baggage) && req.headers.baggage) || '',