Uh oh!
There was an error while loading. Please reload this page.
feat(nuxt): Add connected tracing meta tags - #13098
Conversation
| type === 'server' | ||
| ? path.join(cwd, 'public', `instrument.${type}.${e}`) | ||
| : path.join(cwd, `sentry.${type}.config.${e}`), | ||
| ), |
There was a problem hiding this comment.
Is there a particular reason we're changing this?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
No I haven't but good point! I will also have to check this out.
| const activeSpan = getActiveSpan(); | ||
| const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined; | ||
| if (rootSpan) { |
There was a problem hiding this comment.
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.
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.
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
| `<meta name="baggage" content="${dynamicSamplingContext}"/>`, | ||
| ]; | ||
| head.push(...contentMeta); |
There was a problem hiding this comment.
super-l: I think we can skip this array, we can just head.push('<meta...') twice, safe a tiny bit of processing? 😅
Lms24
left a comment
There was a problem hiding this comment.
LGTM! We can merge this now and follow up on the TwP comment later
| nitroApp.hooks.hook('render:html', (html: NuxtRenderHTMLContext) => { | ||
| addSentryTracingMetaTags(html.head); | ||
| }); |
| nitroApp.hooks.hook('render:html', (html: NuxtRenderHTMLContext) => { | ||
| addSentryTracingMetaTags(html.head); | ||
| }); |
There was a problem hiding this comment.
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.
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.
Also changes the
findDefaultSdkInitFilefunction as the backend-related file is located and named differently than the client config file (sentry.client.config.tsandpublic/instrument.server.mjs).closes#13097