feat(nextjs): Create spans and route parameterization in server-side `getInitialProps` #5587

lforst · 2022-08-16T13:34:44Z

This PR mainly introduces span creation in the server-side getInitialProps wrapping (except for _app, _document, _error pages) - secondly it does a minor refactor on the generic data fetcher wrapper we have, slightly cleaning up types.

github-actions · 2022-08-16T13:43:24Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.38 KB (-0.1% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	59.99 KB (-0.11% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	17.96 KB (-0.08% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	52.88 KB (-0.1% 🔽)
@sentry/browser - Webpack (gzipped + minified)	19.73 KB (-0.2% 🔽)
@sentry/browser - Webpack (minified)	64.21 KB (-0.15% 🔽)
@sentry/react - Webpack (gzipped + minified)	19.75 KB (-0.19% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified)	44.39 KB (-0.7% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.89 KB (-0.03% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.26 KB (-0.05% 🔽)

AbhiPrasad · 2022-08-16T16:15:58Z

packages/nextjs/src/config/wrappers/wrapperUtils.ts

+    },
+    err => {
+      // TODO: Can we somehow associate the error with the span?
+      span.finish();


We can associate the error with the specific span if the span is on the scope.

We should set span status here though.

The span won't be on the scope, as it's one of (eventually) potentially many running simultaneously inside the transaction. But don't we have some function that associates errors to transactions? (I can dig it up later if you don't know what it's called off the top of your head.)

This is how we do error linking in the product:

sentry-javascript/packages/hub/src/scope.ts

Lines 471 to 480 in bdd7fde

// We want to set the trace context for normal events only if there isn't already

// a trace context on the event. There is a product feature in place where we link

// errors with transaction and it relies on that.

if (this._span) {

event.contexts = { trace: this._span.getTraceContext(), ...event.contexts };

const transactionName = this._span.transaction && this._span.transaction.name;

if (transactionName) {

event.tags = { transaction: transactionName, ...event.tags };

}

}

packages/nextjs/src/config/loaders/dataFetchersLoader.ts

lobsterkatie · 2022-08-16T18:03:41Z

packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts

+  origGetServerSideProps: GetServerSideProps,
+  route: string,
+): GetServerSideProps {
+  return function (...getServerSidePropsArguments: Parameters<GetServerSideProps>): ReturnType<GetServerSideProps> {


Next has actual types for these, which is what's embedded in the hacky types objects I was using hoping I could get TS to be smart about the fact that they all went together. Any reason not to import them here, directly from next, and use them?

It's just safer. Assume Next.js released that makes getServerSideProps accept an additional argument. The current implementation of the wrapper... breaks. It doesn't pass the new argument. The Parameters is the most elegant way to type this. I wouldn't change this.

Okay, your call. I do see your point when it comes to the arguments, since you're gathering them into an array, and you're probably right - as stable as this is, who needs another thing to update if nextjs changes their mind and radically rewrites everything (which they have been known to do).

packages/nextjs/src/config/wrappers/wrapperUtils.ts

lobsterkatie · 2022-08-16T18:04:09Z

packages/nextjs/src/config/wrappers/wrapperUtils.ts

+  // We do the following instead of `await`-ing the return value of `origFunction`, because that would require us to
+  // make this function async which might in turn create a mismatch of function signatures between the original


This feels overly complicated to me. Whether the user provides a sync or an async function, all three of the data fetchers are set up to accept a promise:

getInitialProps: https://github.com/vercel/next.js/blob/de7aa2d6e486c40b8be95a1327639cbed75a8782/packages/next/shared/lib/utils.ts#L20

getStaticProps: https://github.com/vercel/next.js/blob/de7aa2d6e486c40b8be95a1327639cbed75a8782/packages/next/types/index.d.ts#L117-L123

getServerSideProps: https://github.com/vercel/next.js/blob/de7aa2d6e486c40b8be95a1327639cbed75a8782/packages/next/types/index.d.ts#L170-L176

So I think it's fine that we're always creating an async function. To handle the case of errors, we can just use a try-catch-finally. (It's a good thought - I'll update my PR to include it also.)

I don't think this is overly complicated. If we have the opportunity not to change the function signature, why would we change it? It's not about what the Framework accepts, it's about not changing what the user provides. I don't really see the value behind adjusting this.

Nextjs is going to have to await it, regardless, just in case it's async, though.

To me, the question isn't why would we change it but why shouldn't we change it, if it makes no difference to nextjs and it allows us to us to simplify our code? Regardless of whether the alternative is "overly" complicated, it is certainly more complicated than simply doing await original(...).

Having the simpler version is good for its own sake, but it will also make error handling simpler, because it'll allow us to just use try-catch, and it brings us in line with what withSentry already does. (Eventually, I'd like to meld that into this mix also.)

Alright. Changed in f45ceb6

AbhiPrasad · 2022-08-16T19:22:57Z

packages/nextjs/src/config/wrappers/wrapperUtils.ts

+export async function callDataFetcherTraced<F extends (...args: any[]) => Promise<any> | any>(
+  origFunction: F,
+  origFunctionArgs: Parameters<F>,
+  options: {


what do you think about making this the SpanContext instead of options? we would make route -> description.

This is blocked on #5564. I have to see what we're gonna do with op and description there before I make any more changes here.

Honestly, I wouldn't read too much into this PR. We're likely gonna scrap this wrapper anyways because it's too generic to be used across different data fetching methods.

Lms24

This LGTM. Let's get this in to unblock the release. As you wrote, this will probably change again in the future, so we can revisit stuff then.

Lms24 · 2022-08-17T08:43:00Z

packages/nextjs/src/config/loaders/dataFetchersLoader.ts

@@ -111,6 +111,21 @@ export default function wrapDataFetchersLoader(this: LoaderThis<LoaderOptions>,
  // We know one or the other will be defined, depending on the version of webpack being used
  const { projectDir, pagesDir } = 'getOptions' in this ? this.getOptions() : this.query;

+  // Get the parameterized route name from this page's filepath
+  const parameterizedRouteName = path


Nice chaining approach! I like it! (I wish it'd work as nicely in all our SDKs)
(EDIT, I'm referring to the lines below 115, I just botched the line selection in the review :D)

Props go to Katie.

Lms24 · 2022-08-17T08:50:04Z

packages/nextjs/src/config/wrappers/wrapperUtils.ts

+  // Logic will be: If there is no active transaction, start one with correct name and source. If there is an active
+  // transaction, create a child span with correct name and source.


Just for me to get this right: If we already have a transaction, this means that we started it beforehand in a previous data fetcher function (iirc you said that getInitialProps can e.g. be executed before getServerSideProps), right?

Right now it's started in instrumentServer.ts - but actually, that's not working so there never is a transaction. Soooo.... this PR currently doesn't do anything lol.

Yeah, something is weird, because there IS in fact a transaction in our integration tests, otherwise they wouldn't pass. And there's a transaction if you run the server inside of the debugger. But seemingly not if you just run the server straight. It's very mysterious.

lforst mentioned this pull request Aug 16, 2022

Improve Nextjs tracing #5505

Closed

43 tasks

lforst changed the base branch from master to kmclb-nextjs-add-spans-and-routes-in-datafetch-wrappers August 16, 2022 15:32

lforst force-pushed the lforst-instrument-get-initial-props branch from 2ab7218 to ba8deb5 Compare August 16, 2022 15:39

lforst requested a review from lobsterkatie August 16, 2022 15:41

lforst marked this pull request as ready for review August 16, 2022 15:41

lforst changed the title ~~feat(nextjs): Create spans in serverside getInitialProps~~ feat(nextjs): Create spans and route parameterization in server-side getInitialProps Aug 16, 2022

AbhiPrasad reviewed Aug 16, 2022

View reviewed changes

AbhiPrasad mentioned this pull request Aug 16, 2022

meta: CHANGELOG for 7.11.0 #5588

Merged

lobsterkatie reviewed Aug 16, 2022

View reviewed changes

lforst requested a review from lobsterkatie August 16, 2022 18:37

AbhiPrasad reviewed Aug 16, 2022

View reviewed changes

lobsterkatie force-pushed the kmclb-nextjs-add-spans-and-routes-in-datafetch-wrappers branch 3 times, most recently from 01300bd to 84cd837 Compare August 16, 2022 22:45

Base automatically changed from kmclb-nextjs-add-spans-and-routes-in-datafetch-wrappers to master August 16, 2022 23:00

lforst added 5 commits August 17, 2022 06:51

feat(nextjs): Create spans in serverside getInitialProps

927746c

Remove unfinished sentence

88b8d18

Remove unnecessary regex

7c93961

Make callDataFetcherTraced async

02c1d43

Rebase onto changes

d996be7

lforst force-pushed the lforst-instrument-get-initial-props branch from f45ceb6 to d996be7 Compare August 17, 2022 07:06

minor jsdoc

515a1b8

lforst requested a review from Lms24 August 17, 2022 07:38

Lms24 approved these changes Aug 17, 2022

View reviewed changes

lforst merged commit 2f6deeb into master Aug 17, 2022

lforst deleted the lforst-instrument-get-initial-props branch August 17, 2022 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nextjs): Create spans and route parameterization in server-side `getInitialProps` #5587

feat(nextjs): Create spans and route parameterization in server-side `getInitialProps` #5587

lforst commented Aug 16, 2022 •

edited

Loading

github-actions bot commented Aug 16, 2022 •

edited

Loading

AbhiPrasad Aug 16, 2022

lobsterkatie Aug 16, 2022

AbhiPrasad Aug 16, 2022

lobsterkatie Aug 16, 2022

lforst Aug 16, 2022

lobsterkatie Aug 16, 2022 •

edited

Loading

lobsterkatie Aug 16, 2022

lforst Aug 16, 2022

lobsterkatie Aug 16, 2022

lforst Aug 16, 2022

AbhiPrasad Aug 16, 2022

lforst Aug 16, 2022

Lms24 left a comment

Lms24 Aug 17, 2022 •

edited

Loading

lforst Aug 17, 2022

Lms24 Aug 17, 2022

lforst Aug 17, 2022

lobsterkatie Aug 17, 2022 •

edited

Loading

	// We want to set the trace context for normal events only if there isn't already
	// a trace context on the event. There is a product feature in place where we link
	// errors with transaction and it relies on that.
	if (this._span) {
	event.contexts = { trace: this._span.getTraceContext(), ...event.contexts };
	const transactionName = this._span.transaction && this._span.transaction.name;
	if (transactionName) {
	event.tags = { transaction: transactionName, ...event.tags };
	}
	}

		// We do the following instead of `await`-ing the return value of `origFunction`, because that would require us to
		// make this function async which might in turn create a mismatch of function signatures between the original

		// Logic will be: If there is no active transaction, start one with correct name and source. If there is an active
		// transaction, create a child span with correct name and source.

feat(nextjs): Create spans and route parameterization in server-side getInitialProps #5587

feat(nextjs): Create spans and route parameterization in server-side getInitialProps #5587

Conversation

lforst commented Aug 16, 2022 • edited Loading

github-actions bot commented Aug 16, 2022 • edited Loading

size-limit report 📦

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

lobsterkatie Aug 16, 2022 • edited Loading

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

Lms24 left a comment

Choose a reason for hiding this comment

Lms24 Aug 17, 2022 • edited Loading

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

lobsterkatie Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

feat(nextjs): Create spans and route parameterization in server-side `getInitialProps` #5587

feat(nextjs): Create spans and route parameterization in server-side `getInitialProps` #5587

lforst commented Aug 16, 2022 •

edited

Loading

github-actions bot commented Aug 16, 2022 •

edited

Loading

lobsterkatie Aug 16, 2022 •

edited

Loading

Lms24 Aug 17, 2022 •

edited

Loading

lobsterkatie Aug 17, 2022 •

edited

Loading