Skip to content
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

Merged
merged 5 commits into from Aug 3, 2022

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Aug 2, 2022

Fixes: #5490

We are currently wrapping meta functions to create sentry-trace and baggage <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 wrapping createRequestHandler), 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 in meta functions. Furthermore, using a loader data in meta seems to spare it from hydration and let it add <meta> tags whenever the data is available (which is handleDocumentRequest in this case, so just before the start of our pageload transaction).

Also, it turns out we don't need to add <meta> tags to every sub-route. If we add them to the root route, they will be available on the sub-routes.

So, this PR removes the monkey patching logic for meta functions. Instead, introduces a special loader wrapper for root. This will require the users to set sentry-trace and baggage in meta functions in root.tsx.

It will look like:

// root.tsx
export const meta: MetaFunction = ({data}) => {
    return {
        charset: "utf-8",
        title: "New Remix App",
        viewport: "width=device-width,initial-scale=1",
        'sentry-trace': data.sentryTrace,
        baggage: data.sentryBaggage,
    };
};

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.

@onurtemizkan onurtemizkan self-assigned this Aug 2, 2022
@onurtemizkan onurtemizkan added this to the Sentry Remix SDK milestone Aug 2, 2022
@AbhiPrasad
Copy link
Member

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.

@AbhiPrasad
Copy link
Member

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.

packages/remix/src/utils/instrumentServer.ts Outdated Show resolved Hide resolved
Comment on lines 251 to 256
sentryTrace = span.toTraceparent();
sentryBaggage = serializeBaggage(activeTransaction.getBaggage());
Copy link
Member

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`
Copy link
Member

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?

Copy link
Collaborator Author

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.

Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.36 KB (-0.02% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 59.95 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.93 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 52.83 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.7 KB (0%)
@sentry/browser - Webpack (minified) 64.17 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.72 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 44.11 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.77 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.01 KB (-0.01% 🔽)

@AbhiPrasad AbhiPrasad merged commit 31fd072 into master Aug 3, 2022
@AbhiPrasad AbhiPrasad deleted the onur/remix-meta-with-loaders branch August 3, 2022 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remix: Hydration errors using 7.8.0
2 participants