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

feat(remix): Set sentry-trace <meta> on server-side. #5440

Merged
merged 2 commits into from Jul 25, 2022

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Jul 21, 2022

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 the http.server (root) span.

@onurtemizkan onurtemizkan added this to the Sentry Remix SDK milestone Jul 21, 2022
@onurtemizkan onurtemizkan self-assigned this Jul 21, 2022
@onurtemizkan onurtemizkan mentioned this pull request Jul 21, 2022
26 tasks
Copy link
Member

@Lms24 Lms24 left a 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 the sentry-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 on baggage 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(),
Copy link
Member

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="..." />

Copy link
Collaborator Author

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(),
Copy link
Member

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:

Suggested change
'sentry-trace': span.toTraceparent(),
'sentry-trace': span.toTraceparent(),
baggage: serializeBaggage(transaction.getBaggage()),

@onurtemizkan
Copy link
Collaborator Author

@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 (pageload as the child of http.server)?
Screenshot 2022-07-22 at 11 00 57

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 instrumentServer) with the integration tests which I think is my next chunk of work?

@Lms24
Copy link
Member

Lms24 commented Jul 22, 2022

Is this what we expect (pageload as the child of http.server)?

Chatted with @lobsterkatie about it: This seems about right 👍

Would it be OK if we test this (and whole instrumentServer) with the integration tests

Good point, yes let's wait for integration tests for now!

@Lms24 Lms24 merged commit b37e88e into master Jul 25, 2022
@Lms24 Lms24 deleted the onur/remix-trace-meta branch July 25, 2022 15:20
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.

None yet

2 participants