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

feat(nuxt): Add connected tracing meta tags #13098

Merged
merged 3 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
8 changes: 7 additions & 1 deletion packages/nuxt/src/module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,13 @@ function findDefaultSdkInitFile(type: 'server' | 'client'): string | undefined {

const cwd = process.cwd();
const filePath = possibleFileExtensions
.map(e => path.resolve(path.join(cwd, `sentry.${type}.config.${e}`)))
.map(e =>
path.resolve(
type === 'server'
? path.join(cwd, 'public', `instrument.${type}.${e}`)
: path.join(cwd, `sentry.${type}.config.${e}`),
),
Comment on lines +65 to +68
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason we're changing this?

Copy link
Member Author

@s1gr1d s1gr1d Jul 30, 2024

Choose a reason for hiding this comment

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

Yes, the file is added with --import (Nuxt 3+ is always ESM) and therefore needs to be added to the build as-is. This is why it is added to the public folder. And I renamed it to instrument as this got kinda the convention for the Sentry server-side setups...or what do you think? Should the file still be called sentry.server.config.ts?

Copy link
Member

Choose a reason for hiding this comment

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

Have you considered how to deploy this on vercel? I'm in a similar boat with the solidstart sdk and have not yet found a good solution for how to actually --import the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

No I haven't but good point! I will also have to check this out.

)
.find(filename => fs.existsSync(filename));

return filePath ? path.basename(filePath) : undefined;
Expand Down
8 changes: 7 additions & 1 deletion packages/nuxt/src/runtime/plugins/sentry.server.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { captureException } from '@sentry/node';
import { H3Error } from 'h3';
import { defineNitroPlugin } from 'nitropack/runtime';
import { extractErrorContext } from '../utils';
import type { NuxtRenderHTMLContext } from 'nuxt/app';
import { addSentryTracingMetaTags, extractErrorContext } from '../utils';

export default defineNitroPlugin(nitroApp => {
nitroApp.hooks.hook('error', (error, errorContext) => {
Expand All @@ -20,4 +21,9 @@ export default defineNitroPlugin(nitroApp => {
mechanism: { handled: false },
});
});

// @ts-expect-error - 'render:html' is a valid hook name in the Nuxt context
nitroApp.hooks.hook('render:html', (html: NuxtRenderHTMLContext) => {
addSentryTracingMetaTags(html.head);
});
Comment on lines +26 to +28
Copy link
Member

Choose a reason for hiding this comment

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

damn, what a nice API!

Comment on lines +26 to +28
Copy link
Member

Choose a reason for hiding this comment

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

q: Does nuxt run nitro in dev mode? For example solidstart (vinxi) doesn't run nitro in dev mode, so just wondering if you're planning to support tracing via meta tags in dev mode. Also applies for the server error handler I guess.

Talked with @Lms24 about this, and in general we want to have it working for production first and foremost but would be nice if we can cover dev mode too.

Nothing to do here for this PR tho.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could not find evidence that nitro does not run in dev mode. I get all nitro-related logs in dev mode as well, so it seems to rn. However, it's working only in the production build right now, as I have other issues in dev mode still.

According to this discussion, nitro runs in a worker thread but I haven't tested that to confirm it.

});
27 changes: 27 additions & 0 deletions packages/nuxt/src/runtime/utils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { getActiveSpan, getRootSpan, spanToTraceHeader } from '@sentry/core';
import { getDynamicSamplingContextFromSpan } from '@sentry/opentelemetry';
import type { Context } from '@sentry/types';
import { dropUndefinedKeys } from '@sentry/utils';
import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils';
import type { CapturedErrorContext } from 'nitropack';
import type { NuxtRenderHTMLContext } from 'nuxt/app';

/**
* Extracts the relevant context information from the error context (H3Event in Nitro Error)
Expand All @@ -26,3 +30,26 @@ export function extractErrorContext(errorContext: CapturedErrorContext): Context

return dropUndefinedKeys(structuredContext);
}

/**
* Adds Sentry tracing <meta> tags to the returned html page.
*
* Exported only for testing
*/
export function addSentryTracingMetaTags(head: NuxtRenderHTMLContext['head']): void {
const activeSpan = getActiveSpan();
const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined;

if (rootSpan) {
Copy link
Member

Choose a reason for hiding this comment

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

cc @Lms24 I remember you wrote this somewhere at some point - should we also inject this for Tracing Without Performance? We do not do this anywhere today I believe, but there was talk about starting to do this?

Copy link
Member

Choose a reason for hiding this comment

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

hmm that's a great point! You're right, we should also inject meta tags for TwP mode. @s1gr1d I propose we merge this as-is and follow up on generally changing the logic here across SDKs. I'll search if we already have an issue for this and otherwise create one to track.

Copy link
Member

Choose a reason for hiding this comment

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

I looked it up: This issue already exists (#8520). I added Nuxt as an item to track but there was a concern raised that I think held us off from implementing

const traceParentData = spanToTraceHeader(rootSpan);
const dynamicSamplingContext = dynamicSamplingContextToSentryBaggageHeader(
getDynamicSamplingContextFromSpan(rootSpan),
);
const contentMeta = [
`<meta name="sentry-trace" content="${traceParentData}"/>`,
`<meta name="baggage" content="${dynamicSamplingContext}"/>`,
];

head.push(...contentMeta);
Copy link
Member

Choose a reason for hiding this comment

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

super-l: I think we can skip this array, we can just head.push('<meta...') twice, safe a tiny bit of processing? 😅

}
}
68 changes: 68 additions & 0 deletions packages/nuxt/test/server/runtime/plugin.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { afterEach, describe, expect, it, vi } from 'vitest';
import { addSentryTracingMetaTags } from '../../../src/runtime/utils';

const mockReturns = vi.hoisted(() => {
return {
traceHeader: 'trace-header',
baggageHeader: 'baggage-header',
};
});

vi.mock('@sentry/core', async () => {
const actual = await vi.importActual('@sentry/core');

return {
...actual,
getActiveSpan: vi.fn().mockReturnValue({ spanId: '123' }),
getRootSpan: vi.fn().mockReturnValue({ spanId: 'root123' }),
spanToTraceHeader: vi.fn(() => mockReturns.traceHeader),
};
});

vi.mock('@sentry/opentelemetry', async () => {
const actual = await vi.importActual('@sentry/opentelemetry');

return {
...actual,
getDynamicSamplingContextFromSpan: vi.fn().mockReturnValue('contextValue'),
};
});

vi.mock('@sentry/utils', async () => {
const actual = await vi.importActual('@sentry/utils');

return {
...actual,
dynamicSamplingContextToSentryBaggageHeader: vi.fn().mockReturnValue(mockReturns.baggageHeader),
};
});

describe('addSentryTracingMetaTags', () => {
afterEach(() => {
vi.resetAllMocks();
});

it('should add meta tags when there is an active root span', () => {
const head: string[] = [];
addSentryTracingMetaTags(head);

expect(head).toContain(`<meta name="sentry-trace" content="${mockReturns.traceHeader}"/>`);
expect(head).toContain(`<meta name="baggage" content="${mockReturns.baggageHeader}"/>`);
});

it('should not add meta tags when there is no active root span', () => {
vi.doMock('@sentry/core', async () => {
const actual = await vi.importActual('@sentry/core');

return {
...actual,
getActiveSpan: vi.fn().mockReturnValue(undefined),
};
});

const head: string[] = [];
addSentryTracingMetaTags(head);

expect(head).toHaveLength(0);
});
});
Loading