From c5d2c951a7b34859d7f0f9329cea7ca04311c47b Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 2 Aug 2022 09:37:21 +0100 Subject: [PATCH 1/5] fix(remix): Provide `sentry-trace` and `baggage` via root loader. --- packages/remix/src/utils/instrumentServer.ts | 61 +++++++++++--------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 2e6657b6bac9..ddb0eb48748e 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -1,7 +1,7 @@ /* eslint-disable max-lines */ import { captureException, getCurrentHub } from '@sentry/node'; -import { getActiveTransaction } from '@sentry/tracing'; -import { addExceptionMechanism, fill, loadModule, logger, serializeBaggage } from '@sentry/utils'; +import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { addExceptionMechanism, fill, isNodeEnv, loadModule, logger, serializeBaggage } from '@sentry/utils'; // Types vendored from @remix-run/server-runtime@1.6.0: // https://github.com/remix-run/remix/blob/f3691d51027b93caa3fd2cdfe146d7b62a6eb8f2/packages/remix-server-runtime/server.ts @@ -72,6 +72,8 @@ interface HandleDataRequestFunction { interface ServerEntryModule { default: HandleDocumentRequestFunction; + meta: MetaFunction; + loader: DataFunction; handleDataRequest?: HandleDataRequestFunction; } @@ -237,33 +239,30 @@ function makeWrappedLoader(origAction: DataFunction): DataFunction { return makeWrappedDataFunction(origAction, 'loader'); } -function makeWrappedMeta(origMeta: MetaFunction | HtmlMetaDescriptor = {}): MetaFunction { - return function ( - this: unknown, - args: { data: AppData; parentsData: RouteData; params: Params; location: Location }, - ): HtmlMetaDescriptor { - let origMetaResult; - if (origMeta instanceof Function) { - origMetaResult = origMeta.call(this, args); - } else { - origMetaResult = origMeta; - } +function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { + return async function (this: unknown, args: DataFunctionArgs): Promise { + let sentryTrace; + let sentryBaggage; + + const activeTransaction = getActiveTransaction(); + const currentScope = getCurrentHub().getScope(); + + if (isNodeEnv() && hasTracingEnabled()) { + if (currentScope) { + const span = currentScope.getSpan(); - const scope = getCurrentHub().getScope(); - if (scope) { - const span = scope.getSpan(); - const transaction = getActiveTransaction(); - - if (span && transaction) { - return { - ...origMetaResult, - 'sentry-trace': span.toTraceparent(), - baggage: serializeBaggage(transaction.getBaggage()), - }; + if (span && activeTransaction) { + sentryTrace = span.toTraceparent(); + sentryBaggage = serializeBaggage(activeTransaction.getBaggage()); + } } } - return origMetaResult; + const res = await origLoader.call(this, args); + + Object.assign(res, { sentryTrace, sentryBaggage }); + + return res; }; } @@ -378,12 +377,20 @@ function makeWrappedCreateRequestHandler( for (const [id, route] of Object.entries(build.routes)) { const wrappedRoute = { ...route, module: { ...route.module } }; - fill(wrappedRoute.module, 'meta', makeWrappedMeta); - if (wrappedRoute.module.action) { fill(wrappedRoute.module, 'action', makeWrappedAction); } + // Entry module should have a loader function to provide `sentry-trace` and `baggage` + // They will be available for the root `meta` function as `data.sentryTrace` and `data.sentryBaggage` + if (!wrappedRoute.parentId) { + if (!wrappedRoute.module.loader) { + wrappedRoute.module.loader = () => ({}); + } + + fill(wrappedRoute.module, 'loader', makeWrappedRootLoader); + } + if (wrappedRoute.module.loader) { fill(wrappedRoute.module, 'loader', makeWrappedLoader); } From 019d791e0cba3c2c4877e3da3531698b526c68bf Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 2 Aug 2022 12:24:56 +0100 Subject: [PATCH 2/5] Use root wrapper after `loader` wrapper to prevent multiple `loader` spans. --- packages/remix/src/utils/instrumentServer.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index ddb0eb48748e..840154d6da3d 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -381,6 +381,10 @@ function makeWrappedCreateRequestHandler( fill(wrappedRoute.module, 'action', makeWrappedAction); } + if (wrappedRoute.module.loader) { + fill(wrappedRoute.module, 'loader', makeWrappedLoader); + } + // Entry module should have a loader function to provide `sentry-trace` and `baggage` // They will be available for the root `meta` function as `data.sentryTrace` and `data.sentryBaggage` if (!wrappedRoute.parentId) { @@ -391,10 +395,6 @@ function makeWrappedCreateRequestHandler( fill(wrappedRoute.module, 'loader', makeWrappedRootLoader); } - if (wrappedRoute.module.loader) { - fill(wrappedRoute.module, 'loader', makeWrappedLoader); - } - routes[id] = wrappedRoute; } From f62e2c3b1ed28ee9c38d8fb8e4e9a35a8c32e413 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 2 Aug 2022 12:25:08 +0100 Subject: [PATCH 3/5] Update integration tests. --- packages/remix/test/integration/app/root.tsx | 4 ++- .../integration/test/client/meta-tags.test.ts | 34 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/remix/test/integration/app/root.tsx b/packages/remix/test/integration/app/root.tsx index 53e82e0e391e..cbc85172bdfb 100644 --- a/packages/remix/test/integration/app/root.tsx +++ b/packages/remix/test/integration/app/root.tsx @@ -2,10 +2,12 @@ import type { MetaFunction } from '@remix-run/node'; import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration } from '@remix-run/react'; import { withSentry } from '@sentry/remix'; -export const meta: MetaFunction = () => ({ +export const meta: MetaFunction = ({ data }) => ({ charset: 'utf-8', title: 'New Remix App', viewport: 'width=device-width,initial-scale=1', + 'sentry-trace': data.sentryTrace, + baggage: data.sentryBaggage, }); function App() { diff --git a/packages/remix/test/integration/test/client/meta-tags.test.ts b/packages/remix/test/integration/test/client/meta-tags.test.ts index 9ae66122f705..f956787b35c6 100644 --- a/packages/remix/test/integration/test/client/meta-tags.test.ts +++ b/packages/remix/test/integration/test/client/meta-tags.test.ts @@ -1,4 +1,6 @@ import { test, expect } from '@playwright/test'; +import { getFirstSentryEnvelopeRequest } from './utils/helpers'; +import { Event } from '@sentry/types'; test('should inject `sentry-trace` and `baggage` meta tags inside the root page.', async ({ page }) => { await page.goto('/'); @@ -27,3 +29,35 @@ test('should inject `sentry-trace` and `baggage` meta tags inside a parameterize expect(sentryBaggageContent).toEqual(expect.any(String)); }); + +test('should send transactions with corresponding `sentry-trace` and `baggage` inside root page', async ({ page }) => { + const envelope = await getFirstSentryEnvelopeRequest(page, '/'); + + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryTraceContent).toContain( + `${envelope.contexts?.trace.trace_id}-${envelope.contexts?.trace.parent_span_id}-`, + ); + + expect(sentryBaggageContent).toContain(envelope.contexts?.trace.trace_id); +}); + +test('should send transactions with corresponding `sentry-trace` and `baggage` inside a parameterized route', async ({ + page, +}) => { + const envelope = await getFirstSentryEnvelopeRequest(page, '/loader-json-response/0'); + + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryTraceContent).toContain( + `${envelope.contexts?.trace.trace_id}-${envelope.contexts?.trace.parent_span_id}-`, + ); + + expect(sentryBaggageContent).toContain(envelope.contexts?.trace.trace_id); +}); From be37bffd1dbd443864116e186902d1c981f8050f Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 3 Aug 2022 12:01:08 +0100 Subject: [PATCH 4/5] Update packages/remix/src/utils/instrumentServer.ts Co-authored-by: Abhijeet Prasad --- packages/remix/src/utils/instrumentServer.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 840154d6da3d..22797f40b3ec 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -260,9 +260,7 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { const res = await origLoader.call(this, args); - Object.assign(res, { sentryTrace, sentryBaggage }); - - return res; + return { ...res, sentryTrace, sentryBaggage }; }; } From 0290aaa93b11ac3f13953eff7f878d6cabcec95b Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 3 Aug 2022 12:17:30 +0100 Subject: [PATCH 5/5] Address review comments. --- packages/remix/src/utils/instrumentServer.ts | 37 +++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 22797f40b3ec..511ca27b6e01 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -239,28 +239,31 @@ function makeWrappedLoader(origAction: DataFunction): DataFunction { return makeWrappedDataFunction(origAction, 'loader'); } -function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { - return async function (this: unknown, args: DataFunctionArgs): Promise { - let sentryTrace; - let sentryBaggage; - - const activeTransaction = getActiveTransaction(); - const currentScope = getCurrentHub().getScope(); - - if (isNodeEnv() && hasTracingEnabled()) { - if (currentScope) { - const span = currentScope.getSpan(); - - if (span && activeTransaction) { - sentryTrace = span.toTraceparent(); - sentryBaggage = serializeBaggage(activeTransaction.getBaggage()); - } +function getTraceAndBaggage(): { sentryTrace?: string; sentryBaggage?: string } { + const transaction = getActiveTransaction(); + const currentScope = getCurrentHub().getScope(); + + if (isNodeEnv() && hasTracingEnabled()) { + if (currentScope) { + const span = currentScope.getSpan(); + + if (span && transaction) { + return { + sentryTrace: span.toTraceparent(), + sentryBaggage: serializeBaggage(transaction.getBaggage()), + }; } } + } + + return {}; +} +function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { + return async function (this: unknown, args: DataFunctionArgs): Promise { const res = await origLoader.call(this, args); - return { ...res, sentryTrace, sentryBaggage }; + return { ...res, ...getTraceAndBaggage() }; }; }