-
-
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
fix(remix): Provide sentry-trace
and baggage
via root loader.
#5509
Conversation
8066250
to
1846cef
Compare
Is it possible to check if the root meta was defined? If so, can we import and monkey patch it? If not, can we inject it ourselves? If there's no other way around this we can make this part of set up, but I'd rather it be as out of the box as possible. |
Update. We decided after chatting on discord to stay with the current method - and just update the docs for those who want to enable backend -> frontend tracing. |
sentryTrace = span.toTraceparent(); | ||
sentryBaggage = serializeBaggage(activeTransaction.getBaggage()); |
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.
nit: Extract this into a function that returns { sentryTrace, sentryBaggage }
and spread that object into the return value below.
@@ -313,6 +311,16 @@ function makeWrappedCreateRequestHandler( | |||
fill(wrappedRoute.module, 'loader', makeWrappedLoader); | |||
} | |||
|
|||
// Entry module should have a loader function to provide `sentry-trace` and `baggage` | |||
// They will be available for the root `meta` function as `data.sentryTrace` and `data.sentryBaggage` |
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.
What do we do here for TypeScript users? Won't they get type errors if they attempt to access data.sentryTrace
? Should we also export a type they can use?
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.
Just checked, Remix declares the data
property as any
. So it's not currently breaking TS.
fe9f8cb
to
f62e2c3
Compare
Co-authored-by: Abhijeet Prasad <[email protected]>
size-limit report 📦
|
Fixes: #5490
We are currently wrapping
meta
functions to createsentry-trace
andbaggage
<meta>
tags in the server-side rendered HTML.It all seems convenient to use that facility (not bothering users to configure anything through the whole process), but turns out it's breaking the hydration. While on React 17, it's just a warning (and
<meta>
tags are still available at the end). On React 18, hydration logic fails and falls back to client-side rendering.The problem is that the HTML template for hydration is generated on build time, and uses the
meta
functions before we wrap them. And when we finally wrap it on initial runtime (we are wrappingcreateRequestHandler
), the updated template doesn't match.But we also have
loader
functions available for us that can pass data from server to client-side, and their return values are also available inmeta
functions. Furthermore, using aloader
data inmeta
seems to spare it from hydration and let it add<meta>
tags whenever the data is available (which ishandleDocumentRequest
in this case, so just before the start of ourpageload
transaction).Also, it turns out we don't need to add
<meta>
tags to every sub-route. If we add them to theroot
route, they will be available on the sub-routes.So, this PR removes the monkey patching logic for
meta
functions. Instead, introduces a specialloader
wrapper forroot
. This will require the users to setsentry-trace
andbaggage
inmeta
functions inroot.tsx
.It will look like:
Will need to update the docs if this approach sounds good to the team.
Update:
Depending on the resolutions of: remix-run/remix#2947 and facebook/react#24430, we may be able to switch back to the current approach in the future.