-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nuxt): Do not inject trace meta-tags on cached HTML pages #17305
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
Conversation
@@ -19,7 +19,7 @@ export default defineConfig({ | |||
'vite.config.*', | |||
], | |||
}, | |||
reporters: ['default', ...(process.env.CI ? [['junit', { classnameTemplate: '{filepath}' }]] : [])], | |||
reporters: process.env.CI ? ['default', ['junit', { classnameTemplate: '{filepath}' }]] : ['default'], |
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 changed this because I got a TS warning (I think it was not able to detect the correct type with this spreading) and it's also better readable like this.
debug.log( | ||
`Not adding Sentry tracing meta tags to HTML for ${event.path} because ${reason}. This will disable distributed tracing for the page.`, | ||
); |
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.
super-l (feel free to overrule me on this one xD): When I as a user would read this message, I'd wonder if I did anything wrong because apparently something disabled something. I think we can just omit the "This will disable.." part of the log.
debug.log( | |
`Not adding Sentry tracing meta tags to HTML for ${event.path} because ${reason}. This will disable distributed tracing for the page.`, | |
); | |
debug.log( | |
`Not adding Sentry tracing meta tags to HTML for ${event.path} because ${reason}.`, | |
); |
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 thought it would be nice to know the consequence of not adding the trace meta tags. But I can rephrase this a bit :)
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.
Updated it to this: This will disable distributed tracing and prevent connecting multiple client page loads to the same server request.
Don't inject tracing meta tags on cached HTML pages (SWR and pre-rendered pages). This means won't have connected traces for those pages anymore. But before, multiple pageloads were listed in one trace as they were all connected to one backend request. Closes #16045 I spent most of the time creating the tests for this PR and verifying it works - the logic is so small 😅 ### Notes for reviewing - the test setup is the same in nuxt-3-min and nuxt-4.
…7305) (#17319) > Backport of #17305 Don't inject tracing meta tags on cached HTML pages (SWR and pre-rendered pages). This means won't have connected traces for those pages anymore. But before, multiple pageloads were listed in one trace as they were all connected to one backend request. Closes #16045 I spent most of the time creating the tests for this PR and verifying it works - the logic is so small 😅
Don't inject tracing meta tags on cached HTML pages (SWR and pre-rendered pages). This means won't have connected traces for those pages anymore. But before, multiple pageloads were listed in one trace as they were all connected to one backend request.
Closes #16045
I spent most of the time creating the tests for this PR and verifying it works - the logic is so small 😅
Notes for reviewing