-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) => { | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. damn, what a nice API!
Comment on lines
+26
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}); |
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) | ||
|
@@ -26,3 +30,23 @@ 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||
); | ||
|
||
head.push(`<meta name="sentry-trace" content="${traceParentData}"/>`); | ||
head.push(`<meta name="baggage" content="${dynamicSamplingContext}"/>`); | ||
} | ||
} |
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); | ||
}); | ||
}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 thepublic
folder. And I renamed it toinstrument
as this got kinda the convention for the Sentry server-side setups...or what do you think? Should the file still be calledsentry.server.config.ts
?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.