feat(remix): Set sentry-trace <meta> on server-side.#5440
Conversation
There was a problem hiding this comment.
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
baggagefor baggage/dynamic sampling context propagation?
This data should always be sent in combination with thesentry-tracedata 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 onbaggagesee 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.
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.
Yes, that's correct. (docs) Also tested manually. 👍
| if (span) { | ||
| return { | ||
| ...origMetaResult, | ||
| 'sentry-trace': span.toTraceparent(), |
There was a problem hiding this comment.
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
metafunction to set<meta>tags for route modules.This PR adds a meta function to each route to set
sentry-traceusing thehttp.server(root) span.