-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(astro): Ensure traces are correctly propagated for static routes #17536
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
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.
today this path is never hit as the http integration does not work, but this will become relevant when we adjust this to not need an otel instrumentation anymore. I opted to still make this forwards-compatible change here, for simplicity...
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.
Sounds good to me! Though see my other comment about sub requests. Again, not sure if this is really a concern so we can also just do this for the moment and adjust if necessary.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 don't know if/where this would happen that the http.server span would be nested, could not find any case of this. Even then, I suppose it is not too bad if the isolation scope would be forked again for the sub-request 🤔
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.
The idea here is that Astro doesn't actually make another network request when you call fetch during SSR and the request targets e.g. an API route on the Astro server. So to avoid the network roundtrip, the framework just programatically re-enters the request lifecycle as a "sub request".
^ now that being said, I don't know if it works like that anymore and the comment suggests that it hasn't been updated in a long time ("domains"). I does work like that still in Sveltekit but obviously that doesn't mean anything 😅
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.
yeah, I think that makes sense/could be, but arguably even then it would be OK to fork the isolation scope I'd say and have a dedicated isolation scope for the "sub-request"? 🤔 e.g. if I had (theoretically):
- GET /page --> isolation scope forked
- GET /sub-request --> fork isolation scope again here, as that could have it's own tags etc. that should not leak to the general /page request, I suppose
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.
yeah sounds good!
yarn.lock
Outdated
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 idea where this changes come from...
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
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! Thanks for refactoring the middleware! Left some remarks but nothing too actionable. RE sub requests: If we don't have tests that cover this scenario I think it's also fine to just live with it right now.
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.
The idea here is that Astro doesn't actually make another network request when you call fetch during SSR and the request targets e.g. an API route on the Astro server. So to avoid the network roundtrip, the framework just programatically re-enters the request lifecycle as a "sub request".
^ now that being said, I don't know if it works like that anymore and the comment suggests that it hasn't been updated in a long time ("domains"). I does work like that still in Sveltekit but obviously that doesn't mean anything 😅
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.
Sounds good to me! Though see my other comment about sub requests. Again, not sure if this is really a concern so we can also just do this for the moment and adjust if necessary.
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: shouldnt this be http.method? (likely was wrong beforehand already)
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.
so http.method will already be there from the http.server span of http. this is added because we used to have this before, for backwards compatibility 😅
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.
side-note (not relevant for this PR ): Should we remove this? I don't think we ever set the user in other SDKs, right? This is something, users could easily achieve with a middleware on their own. It's also kinda weird with sendDefaultPii usually gating IP address storing.
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.
we do set the ip address in the base http requestDataIntegration handling!
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.
right, I blanked and thought we only set event.request but you're right! Though still, since requestDataIntegration does this anyway, having this logic is redundant and still confusing with trackClientIp vs sendDefaultPii, IMHO
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.
it probably makes sense to adjust this in a follow up and look at sendDefaultPii here as well!
3083981 to
93ec015
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This refactors the astro middleware a bit to be more correct & future proof. Right now, http.server spans are not emitted by the node http integration because import-in-the-middle does not work with Astro. So we emit our own. This PR makes astro ready to also handle the base http.server spans and enhance them with routing information. It also handles static routes better: these do not emit spans anymore now, and do not continue traces, but instead only propagate the parametrized route to the client, no trace data.
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
93ec015 to
ccb5060
Compare
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.
Bug: Refactoring Breaks Sentry Data Capture
The refactoring introduced inconsistencies in Sentry data capture. enhanceHttpServerSpan and handleStaticRoute paths are missing client IP and SDK processing metadata (request data) settings. enhanceHttpServerSpan also lacks consistent fallback transaction naming and sentry.source attribute setting when a parametrized route is unavailable, leading to incomplete or inconsistent Sentry events.
This refactors the astro middleware a bit to be more correct & future proof.
Right now, http.server spans are not emitted by the node http integration because import-in-the-middle does not work with Astro. So we emit our own. This PR makes astro ready to also handle the base http.server spans and enhance them with routing information.
It also handles static routes better: these do not emit spans anymore now, and do not continue traces, but instead only propagate the parametrized route to the client, no trace data.
One fundamental problem remains that is not fixed in this PR:
Sentry.init()is only injected viapage-ssrinto SSR pages. This means that only once any SSR page is hit, the server-side part of Sentry is initialized. If you hit a prerendered (static) page first, sentry will not be initialized and thus neither errors are captured nor spans created. For regular prerendered pages this should be fine because we do not need to run anything at runtime there. However, when you hit a prerendered page that has a server-island, the server island (which is dynamic) will not be instrumented either because Sentry is not initialized yet.I don't think we can fix this with the current set of primitives we have, so this is a future problem to look into...