Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(remix): Provide sentry-trace and baggage via root loader. #5509

Merged
merged 5 commits into from
Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 31 additions & 23 deletions packages/remix/src/utils/instrumentServer.ts
Original file line number Diff line number Diff line change
@@ -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/[email protected]:
// https://github.com/remix-run/remix/blob/f3691d51027b93caa3fd2cdfe146d7b62a6eb8f2/packages/remix-server-runtime/server.ts
Expand Down Expand Up @@ -72,6 +72,8 @@ interface HandleDataRequestFunction {

interface ServerEntryModule {
default: HandleDocumentRequestFunction;
meta: MetaFunction;
loader: DataFunction;
handleDataRequest?: HandleDataRequestFunction;
}

Expand Down Expand Up @@ -237,33 +239,31 @@ 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 getTraceAndBaggage(): { sentryTrace?: string; sentryBaggage?: string } {
const transaction = getActiveTransaction();
const currentScope = getCurrentHub().getScope();

const scope = getCurrentHub().getScope();
if (scope) {
const span = scope.getSpan();
const transaction = getActiveTransaction();
if (isNodeEnv() && hasTracingEnabled()) {
if (currentScope) {
const span = currentScope.getSpan();

if (span && transaction) {
return {
...origMetaResult,
'sentry-trace': span.toTraceparent(),
baggage: serializeBaggage(transaction.getBaggage()),
sentryTrace: span.toTraceparent(),
sentryBaggage: serializeBaggage(transaction.getBaggage()),
};
}
}
}

return origMetaResult;
return {};
}

function makeWrappedRootLoader(origLoader: DataFunction): DataFunction {
return async function (this: unknown, args: DataFunctionArgs): Promise<Response | AppData> {
const res = await origLoader.call(this, args);

return { ...res, ...getTraceAndBaggage() };
};
}

Expand Down Expand Up @@ -378,8 +378,6 @@ 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);
}
Expand All @@ -388,6 +386,16 @@ function makeWrappedCreateRequestHandler(
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`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we do here for TypeScript users? Won't they get type errors if they attempt to access data.sentryTrace? Should we also export a type they can use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked, Remix declares the data property as any. So it's not currently breaking TS.

if (!wrappedRoute.parentId) {
if (!wrappedRoute.module.loader) {
wrappedRoute.module.loader = () => ({});
}

fill(wrappedRoute.module, 'loader', makeWrappedRootLoader);
}

routes[id] = wrappedRoute;
}

Expand Down
4 changes: 3 additions & 1 deletion packages/remix/test/integration/app/root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
34 changes: 34 additions & 0 deletions packages/remix/test/integration/test/client/meta-tags.test.ts
Original file line number Diff line number Diff line change
@@ -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('/');
Expand Down Expand Up @@ -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<Event>(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<Event>(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);
});