-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(nuxt): Always add tracing meta tags #13273
Conversation
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.
lgtm!
packages/nuxt/src/runtime/utils.ts
Outdated
head.push(`<meta name="baggage" content="${dynamicSamplingContext}"/>`); | ||
} | ||
const metaTags = getTraceMetaTags(); | ||
head.push(metaTags); |
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.
l: Not sure if this is problematic but I see we did this differently before: If nuxt expects one tag per array item we should split the return value from getTraceMetaTags()
at \n
since we usually return two tags separated by a \n
.
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.
I checked this and nuxt puts the html together in the correct way so this should be fine. But I would rather add a function parameter { options.asArray }
to return it as array instead of adding \n
and splitting it again afterwards.
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.
I created a PR for this: #13293
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.
As I closed the PR I just use the returned string now. This works with Nuxt as well and we don't have to split the string (slightly better performance).
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.
Thanks for explaining! LGTM!
].join('\n'); | ||
|
||
// return value is mocked here as return values of `getTraceMetaTags` are tested separately (in @sentry/core) | ||
(getTraceMetaTags as Mock).mockReturnValue(mockMetaTags); |
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.
I think that's fine, given the unit under test here is the Nuxt-specific addSentryTracingMetaTags
function.
What we should do in the near future is to make sure our e2e tests cover distributed tracing behaviour. For example, we should check that
- the sever-side request transaction and the client-side pageload transaction is connected by the same trace. This will validate that the trace meta tag injection generally works.
- potential requests made on the client side to the server (thinking of API endpoint or data loading calls, whatever nuxt offers) are connected to the server side. This is probably easiest testable when making these calls during or directly after a navigation so that we can make sure a client-side root span exists.
I'm also working on a SvelteKit e2e test app that's specifically configured for tracing without performance. We could think about doing something similar for Nuxt but I wouldn't require it for the moment.
Making sure tracing without performance works.
ref (SvelteKit): #13231