-
-
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(remix): Set sentry-trace
<meta>
on server-side.
#5440
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.
This is looking good although my knowledge about Remix is still limited. We discussed this briefly at our meeting today and I have two requests:
-
can we add another tag with the name
baggage
for baggage/dynamic sampling context propagation?
This data should always be sent in combination with thesentry-trace
data because only together we can correctly sample traces (for dynamic sampling). I'm realizing that we probably didn't communicate this correctly (apologies for that). I think the change should be fairly small. (I marked the position where I think we need to add a few lines in a comment).
For more information onbaggage
see here (this is how it should look in the meta tags) and here -
can we add a few tests to make sure this change is covered?
if (span) { | ||
return { | ||
...origMetaResult, | ||
'sentry-trace': span.toTraceparent(), |
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 just to confirm, this adds a meta tag to the html that looks as follows?:
<meta name="sentry-trace" content="..." />
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, that's correct. (docs) Also tested manually. 👍
if (span) { | ||
return { | ||
...origMetaResult, | ||
'sentry-trace': span.toTraceparent(), |
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.
RE: baggage
meta tag
Don't take this suggestion literally (e.g. still needs imports and all) but I think this is how it could work.
Slightly above, get the active transaction
(from the scope) and then here:
'sentry-trace': span.toTraceparent(), | |
'sentry-trace': span.toTraceparent(), | |
baggage: serializeBaggage(transaction.getBaggage()), |
@Lms24, thanks a lot for the review! Added the baggage 👍 One question to confirm this update is reflecting correctly on dashboard: Is this what we expect ( About the tests, IMHO it'll be a bit hard to unit test this without importing the actual Remix library (at least lots of types from it). Would it be OK if we test this (and whole |
Chatted with @lobsterkatie about it: This seems about right 👍
Good point, yes let's wait for integration tests for now! |
Ref: #4894
Remix provides a
meta
function to set<meta>
tags for route modules.This PR adds a meta function to each route to set
sentry-trace
using thehttp.server
(root) span.