Sentry: Add resolved url tag #600

masonmcelvain · 2022-08-08T19:38:52Z

Sometimes it's hard to tell which page a user was on when an error occurred, so adding it in a tag will be useful for debugging.

The resolved url is the url that the user requested (like /Parts/iPhone). This is different than context.req.url, which can be a link like /_next/data..., which is not the helpful for debugging.

QA

Throw an error on any page while running local dev. It should get reported to react-commerce-dev in sentry, and have a resolved_url tag.

Sometimes it's hard to tell which page a user was on when an error occurred, so adding it in a tag will be useful for debugging. The resolved url is the url that the user requested (like /Parts/iPhone). This is different than `context.req.url`, which can be a link like /_next/data..., which is not the helpful for debugging.

vercel · 2022-08-08T19:38:54Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
react-commerce	✅ Ready (Inspect)	Visit Preview	Aug 9, 2022 at 7:43PM (UTC)
react-commerce-prod	✅ Ready (Inspect)	Visit Preview	Aug 9, 2022 at 7:43PM (UTC)

deltuh-vee · 2022-08-08T21:44:52Z

QA 🎬
Resolved URL tag is now present in Sentry.

frontend/pages/Tools/index.tsx

danielbeardsley · 2022-08-09T06:51:45Z

packages/sentry/index.tsx

+};
+
+export const setSentryPageContext = ({ url }: PageContextProps) => {
+   Sentry.setTag('resolved_url', url);


I think this will be inconsistent and flaky. We're in the context of a JS server that does a bunch of async work. This JS server has only one global context yet it may be serving several requests in parallel. I think we can kill two birds though. I'm gonna try something.

I think this will be inconsistent and flaky. We're in the context of a JS server that does a bunch of async work. This JS server has only one global context yet it may be serving several requests in parallel.

I'm not sure I understand. What exactly are you worried about?

Feel free to push to this branch if you come up with something 👍🏻

Wowza I'm seeing a few issues/PR's on sentry-javascript about getting error reporting in getServerSideProps to work.

getsentry/sentry-javascript#5503
getsentry/sentry-javascript#3880

Wow... looks like they already auto-inject code to wrap getServerSideProps() (given that pull request). But it seems (based on experimentation) that they don't capture the relevant url? I wonder if there's a mechanism (via config) to augment or add to the code that auto-wraps getServerSideProps().

But it seems (based on experimentation) that they don't capture the relevant url?

That's what I had seen, but now that I'm testing this branch, I see the correct url tag.

I wonder if there's a mechanism (via config) to augment or add to the code that auto-wraps getServerSideProps().

Maybe via the middleware concept that @jarstelfox was talking about? #600 (comment)

Great thanks for finding all that! Here's an example of a Sentry event from getServerSideProps that didn't get the url tag. But many other events from the server are. I can't explain what's causing the discrepancy, I agree with you that it seems like it should work.

I see that it's setting the url, but it isn't setting resolvedUrl. Is there something in this code that makes you think we should be seeing resolvedUrl already?

Okay, I did some skimming and cross-referenced the example you linked above.

It looks like sentry hooks into the server and wraps a ton of logic. Most notably Next's Request Handler Factory which is what calls down to setting the url.

I still believe it should be working at the time of the error(even though it clearly is not).

Sentry is passing that data down to itself when a request handler is made. i.e. afterRouted

Maybe someone else can spy what's missing, but I think we should provide our own way of getting the data until we can find a reliable way for sentry to hook the url

Just to make sure we're all on the same page, this should only affect page loads where it uses next-link to load json. In those cases, we've seen url set as /_next/.... resolvedUrl will be the url of the page that we were really trying to link to. In the code I've read, I see sentry adding the url field, but I haven't seen them add the resolvedUrl field, so I wouldn't expect it to just work without some changes like in this pull.

Hi, Sentry JS SDK maintainer here. Just stumbled onto this thread.

We're currently working on wrapping Next.js data fetching methods (including getServerSideProps) to improve instrumentation. Currently, the efforts mainly go into properly setting parameterized transaction names (transactions are part of the Sentry performance monitoring product and parameterization means that transactions won't show up as /user/42/post/1337 but rather as /user/[userId]/post/[postId], which is generally more useful).

We might explore adding the parameterized route + parameters to errors in some way (or at least provide a way for SDK user's to figure out what parameterized route they're in - maybe we can contribute this to the Next.js repo upstream).

We would love for you to try out the changes as soon as they land!

We want to generate a url from the current context in /frontend and in /packages/sentry. This lets us use it in both places.

Accepts the same param that is passed to `getServerSideProps` so that we can easily call it in each file in /frontend/pages. Clears the Sentry scope before setting the tag, because this is going to be called on a JS server that handles many requests, and we don't want to accidentally include context from another request in a Sentry event. Bumps @sentry/nextjs to 7.8.1, which is what we use in /frontend. Uses the react-library tsconfig.json, because TS was complaining that we imported `urlFromContext()` from a .tsx file.

This approach makes us more confident that we won't accidentally include context from the wrong request in a Sentry event sent from the server (because the server can be handling multiple requests at once).

masonmcelvain · 2022-08-09T19:12:02Z

packages/helpers/nextjs.tsx

@@ -0,0 +1,6 @@
+import { GetServerSidePropsContext } from 'next';
+
+export function urlFromContext(context: GetServerSidePropsContext): string {


FYI I didn't include this in @ifixit/helpers/nextjs because doing so would cause a cyclic dependency between @ifixit/sentry and @ifixit/helpers.

jarstelfox · 2022-08-09T19:14:46Z

packages/helpers/nextjs.tsx

+export function urlFromContext(context: GetServerSidePropsContext): string {
+   const protocol = context.req.headers.referer?.split('://')[0] || 'https';
+   return `${protocol}://${context.req.headers.host}${context.resolvedUrl}`;
+}


You could use the actual URL object. I know you may toString it later, but the class may be nice down the road

Suggested change

export function urlFromContext(context: GetServerSidePropsContext): string {

const protocol = context.req.headers.referer?.split('://')[0] || 'https';

return `${protocol}://${context.req.headers.host}${context.resolvedUrl}`;

}

export function urlFromContext(context: GetServerSidePropsContext): URL {

const protocol = context.req.headers.referer?.split('://')[0] || 'https';

return new URL(`${protocol}://${context.req.headers.host}${context.resolvedUrl}`);

}

I agree, but we use string urls throughout most of this app unfortunately, so this would be a more siginificant refactor if we were to do it right. I'd rather leave it to another pull.

packages/sentry/index.tsx

masonmcelvain · 2022-08-09T19:19:03Z

~~Hmm the url is showing up as a sentry tag, but when I add arbitrary tags, they aren't showing up.~~

Nevermind I'm just blind

We were clearing the scope because we were worried that different requests might pollute each other's scopes on the server. Howevever, it looks like Sentry handles this for us, so we don't need to risk blowing away valuable debugging info: getsentry/sentry-javascript#3788

danielbeardsley

CR 👍

Man, crazy that sentry/next does the auto-wrapping of the getServerSideProps() function. Bummer that it doesn't seem to do the thing all the time.

deltuh-vee · 2022-08-11T21:22:25Z

Carry over QA 🎬

lobsterkatie · 2022-08-12T17:22:18Z

Man, crazy that sentry/next does the auto-wrapping of the getServerSideProps() function. Bummer that it doesn't seem to do the thing all the time.

Weighing in as the other half of the team working on this project. So glad y'all are excited for this! It's still very much a WIP, so expect it to get better over the next few weeks.

Also, feel free to follow along on getsentry/sentry-javascript#5505. It's mostly just some notes I wrote up as I was planning the project, but if there are thoughts about it you want to share, that's a great place.

vercel bot deployed to Preview – react-commerce August 8, 2022 19:39 View deployment

vercel bot deployed to Preview – react-commerce-prod August 8, 2022 19:43 View deployment

deltuh-vee self-assigned this Aug 8, 2022

deltuh-vee added the QAing Under QA team review label Aug 8, 2022

deltuh-vee removed the QAing Under QA team review label Aug 8, 2022

sterlinghirsh reviewed Aug 9, 2022

View reviewed changes

frontend/pages/Tools/index.tsx Outdated Show resolved Hide resolved

danielbeardsley reviewed Aug 9, 2022

View reviewed changes

vercel bot deployed to Preview – react-commerce August 9, 2022 18:00 View deployment

vercel bot deployed to Preview – react-commerce-prod August 9, 2022 18:02 View deployment

masonmcelvain added 4 commits August 9, 2022 12:00

urlFromContext: Extract to @ifixit/helpers/nextjs

1d000ad

We want to generate a url from the current context in /frontend and in /packages/sentry. This lets us use it in both places.

sentry: Wrap getServerSideProps

2d0b9a8

This approach makes us more confident that we won't accidentally include context from the wrong request in a Sentry event sent from the server (because the server can be handling multiple requests at once).

pnpm-lock: Run pnpm install:all

7e1519e

masonmcelvain force-pushed the sentry--add-resolved-url-tag branch from 54f373f to 7e1519e Compare August 9, 2022 19:10

masonmcelvain commented Aug 9, 2022

View reviewed changes

vercel bot deployed to Preview – react-commerce August 9, 2022 19:12 View deployment

jarstelfox reviewed Aug 9, 2022

View reviewed changes

vercel bot deployed to Preview – react-commerce-prod August 9, 2022 19:15 View deployment

jarstelfox reviewed Aug 9, 2022

View reviewed changes

packages/sentry/index.tsx Outdated Show resolved Hide resolved

vercel bot deployed to Preview – react-commerce August 9, 2022 19:42 View deployment

vercel bot deployed to Preview – react-commerce-prod August 9, 2022 19:43 View deployment

danielbeardsley approved these changes Aug 9, 2022

View reviewed changes

deltuh-vee added the QAing Under QA team review label Aug 11, 2022

deltuh-vee removed the QAing Under QA team review label Aug 11, 2022

danielbeardsley merged commit c731d99 into main Aug 11, 2022

danielbeardsley deleted the sentry--add-resolved-url-tag branch August 11, 2022 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sentry: Add resolved url tag #600

Sentry: Add resolved url tag #600

masonmcelvain commented Aug 8, 2022

vercel bot commented Aug 8, 2022 •

edited

Loading

deltuh-vee commented Aug 8, 2022

danielbeardsley Aug 9, 2022

masonmcelvain Aug 9, 2022

masonmcelvain Aug 9, 2022

danielbeardsley Aug 9, 2022

masonmcelvain Aug 9, 2022

masonmcelvain Aug 9, 2022

sterlinghirsh Aug 9, 2022

jarstelfox Aug 10, 2022

sterlinghirsh Aug 10, 2022

lforst Aug 12, 2022

masonmcelvain Aug 9, 2022

jarstelfox Aug 9, 2022

masonmcelvain Aug 9, 2022

masonmcelvain commented Aug 9, 2022 •

edited

Loading

danielbeardsley left a comment

deltuh-vee commented Aug 11, 2022

lobsterkatie commented Aug 12, 2022

		@@ -0,0 +1,6 @@
		import { GetServerSidePropsContext } from 'next';

		export function urlFromContext(context: GetServerSidePropsContext): string {

Sentry: Add resolved url tag #600

Sentry: Add resolved url tag #600

Conversation

masonmcelvain commented Aug 8, 2022

QA

vercel bot commented Aug 8, 2022 • edited Loading

deltuh-vee commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masonmcelvain commented Aug 9, 2022 • edited Loading

danielbeardsley left a comment

Choose a reason for hiding this comment

deltuh-vee commented Aug 11, 2022

lobsterkatie commented Aug 12, 2022

vercel bot commented Aug 8, 2022 •

edited

Loading

masonmcelvain commented Aug 9, 2022 •

edited

Loading