feat(nextjs): Improve pageload transaction creation #5574

lforst · 2022-08-12T10:04:48Z

This PR aims to improve Next.js pageload transactions:

We start the pageload transaction, by utilizing data from the __NEXT_DATA__ script tag Next.js provides on each page, instead of having to wait for the next router to initialize. We're pretty much guaranteed to always retrieve a parameterized route from __NEXT_DATA__.
Additionally, this PR lays some groundwork for retrieving trace parent data and baggage from traces that started on the server (i.e., when there are server-side data fetchers like getServerSideProps or getInitialProps in place).

github-actions · 2022-08-12T10:11:40Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.41 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	60.06 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	17.97 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	52.92 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	19.77 KB (0%)
@sentry/browser - Webpack (minified)	64.3 KB (0%)
@sentry/react - Webpack (gzipped + minified)	19.79 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	44.7 KB (+0.62% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.9 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.28 KB (+0.01% 🔺)

AbhiPrasad · 2022-08-12T11:38:26Z

Does this have the chance to change certain transaction names?

lforst · 2022-08-12T12:24:02Z

Does this have the chance to change certain transaction names?

@AbhiPrasad I'm not 100% sure but I think it's unlikely that it does.

AbhiPrasad · 2022-08-12T12:59:10Z

packages/nextjs/src/performance/client.ts

+interface SentryEnhancedNextData extends NextData {
+  // contains props returned by `getInitialProps` - except for `pageProps`, these are the props that got returned by `getServerSideProps` or `getStaticProps`
+  props: {
+    _sentryGetInitialPropsTraceData?: string; // trace id, if injected by server-side `getInitialProps`


it's not just trace id - let's refer to it as trace parent info since it has sampled flag and parent span id as well.

👍 22a0f83

AbhiPrasad · 2022-08-12T13:03:25Z

packages/nextjs/src/performance/client.ts

+  if (startTransactionOnPageLoad) {
+    const { route, traceParentData, baggage, params } = extractNextDataTagInformation();
+
+    prevTransactionName = route || global.document.location.pathname;


why the change to global.document.location? Let's not change for the sake for change.

It was just easier to test. I generally do not change stuff for the sake of changing them. -> 651e8e0

AbhiPrasad · 2022-08-12T14:00:19Z

packages/nextjs/src/performance/client.ts

+  baggage: string | undefined;
+  params: ParsedUrlQuery | undefined;
+} {
+  let nextData: SentryEnhancedNextData | undefined;


low: IMO I would like this function to act as a builder, perhaps something like so:

const nextDataTagInfo = {}; if (nextData) { nextDataTagInfo.route = nextData.page; nextDataTagInfo.params = nextData.query; if (nextData.props) { // ... } }

I just tried this. Trust me, it's uglier than it is right now + you'll get problems with typing. I'm leaving it as is. Mutability is the root of all evil. Feel free to push changes to this if you want though.

Done with 1bde09a, let me know what you think! (feel free to revert if you think it'll cause more bugs than less).

AbhiPrasad · 2022-08-12T15:02:35Z

packages/nextjs/src/performance/client.ts

+/**
+ * Describes data located in the __NEXT_DATA__ script tag. This tag is present on every page of a Next.js app.
+ */
+interface SentryEnhancedNextData extends NextData {


Are there other fields in NextData that we can use? https://github.com/vercel/next.js/blob/0796b6faa991cd58a517b0f160320e9978208066/packages/next/shared/lib/utils.ts#L84-L109 - perhaps create a follow up issue?

Poked around in the type. Nothing really sticks out as very useful. I can create an issue but very low prio imo.

AbhiPrasad · 2022-08-12T15:06:28Z

packages/nextjs/test/performance/client.test.ts

+          {
+            "props": ${JSON.stringify(pageProperties.props)},
+            "page": "${pageProperties.route}",
+            "query": ${JSON.stringify(pageProperties.query)},
+            "buildId": "y76hvndNJBAithejdVGLW",
+            "isFallback": false,
+            "gssp": true,
+            "appGip": true,
+            "scriptLoader": []
+          }


nit: Prefer if we create an object and stringify it for readability + we can type with the type exported from nextjs.

👍 bec58e0

AbhiPrasad · 2022-08-12T15:09:22Z

packages/nextjs/src/performance/client.ts

+      name: prevTransactionName,
+      op: 'pageload',
+      tags: DEFAULT_TAGS,
+      ...(params && { data: params }),


iirc setting data does nothing for transactions - can we throw it into some other part of the event? Like a context?

Does this have PII data concerns? Perhaps we guard this with sendDefaultPII.

Finally found a use for that flag! Yup there are potential pii issues. Added the flag but removed tests around data because that's just not worth the effort of mocking the hub/client/options. 0ba0d6e

Btw, this data shows up in the trace details when you open a transaction:

Btw, this data shows up in the trace details when you open a transaction:

Ah yeah - that means it'll show up on linked errors as well (since we copy over the entire trace context). I guess since we don't index this we can leave it for now.

AbhiPrasad

Such a good change! Good idea on the approach.

lobsterkatie

This looks good!

Small suggestion on your PR description (second bullet point):

Additionally, this PR lays the groundwork for starting pageload traces on the server and continuing them on the client (as opposed to just starting them on the client), by adding a function to retrieve traceparent data and baggage from the __NEXT_DATA__ tag. (Work to set those values will happen in a future PR.)

As per usual, pretty much all of my comments are about explaining stuff. IMHO the code itself is good to go!

lobsterkatie · 2022-08-16T03:32:26Z

packages/nextjs/src/performance/client.ts

+      _sentryGetServerSidePropsTraceData?: string; // trace parent info, if injected by server-side `getServerSideProps`
+      _sentryGetServerSidePropsBaggage?: string; // baggage, if injected by server-side `getServerSideProps`


I know we're leaving getStaticProps somewhat to the side for the moment, but do you think it's worth it to add a TODO about it here?

See #5574 (comment)

lobsterkatie · 2022-08-16T03:32:32Z

packages/nextjs/src/performance/client.ts

+ * Every Next.js page (static and dynamic ones) comes with a script tag with the id "__NEXT_DATA__". This script tag
+ * contains a JSON object with data that was either generated at build time for static pages (`getStaticProps`), or at
+ * runtime with data fetchers like `getServerSideProps.`.


Given that getStaticProps runs an arbitrary amount of time before the page is served (both when it's run at build time and when it's run at runtime as part of revalidation), it doesn't make sense to send trace data from it, only the parameterized route. Do we want to note that here somehow? Could be in the docstring here or could be below where you're dealing with the other two, to explain why it's not all three.

I finally read up on how ISR works and I think I fully understand it now. There is one case where it totally makes sense to trace getStaticProps and I went ahead and added logic and an explanation for it: 652d3eb

lobsterkatie · 2022-08-16T03:32:42Z

packages/nextjs/src/performance/client.ts


+  if (startTransactionOnPageLoad) {
+    const { route, traceParentData, baggage, params } = extractNextDataTagInformation();


Suggested change

const { route, traceParentData, baggage, params } = extractNextDataTagInformation();

// Pull data from the `__NEXT_DATA__` script tag automatically injected serverside by nextjs

const { route, traceParentData, baggage, params } = extractNextDataTagInformation();

lobsterkatie · 2022-08-16T03:32:48Z

packages/nextjs/test/index.client.test.ts

+// Set up JSDom - needed for page load instrumentation
+const dom = new JSDOM(undefined, { url: 'https://example.com/' });
+Object.defineProperty(global, 'document', { value: dom.window.document, writable: true });
+Object.defineProperty(global, 'location', { value: dom.window.document.location, writable: true });
+
+const originalGlobalDocument = getGlobalObject<Window>().document;
+const originalGlobalLocation = getGlobalObject<Window>().location;
+afterAll(() => {
+  // Clean up JSDom
+  Object.defineProperty(global, 'document', { value: originalGlobalDocument });
+  Object.defineProperty(global, 'location', { value: originalGlobalLocation });
+});


Could you add a sentence or two to the comment explaining why we need to do this?

lobsterkatie · 2022-08-16T03:33:09Z

packages/nextjs/test/performance/client.test.ts

+      };
+
+      const dom = new JSDOM(
+        // Just some example what a __NEXT_DATA__ tag might look like


Suggested change

// Just some example what a __NEXT_DATA__ tag might look like

// Just an example of what a __NEXT_DATA__ tag might look like

lobsterkatie · 2022-08-16T03:33:17Z

packages/nextjs/test/performance/client.test.ts

+      ],
+    ])(
+      'creates a pageload transaction (#%#)',
+      (url, route, query, props, hasNextData, expectedStartTransactionCall) => {


Suggested change

(url, route, query, props, hasNextData, expectedStartTransactionCall) => {

(url, route, query, props, hasNextData, expectedStartTransactionArguments) => {

It might be nice to put this list in a comment at the top of the it.each, and/or stick a blank line between test cases - it's pretty hard to tell what you're looking at given that it's half a dozen items per test case, and each test case goes on for a variable number of lines.

lobsterkatie · 2022-08-16T03:33:22Z

packages/types/src/options.ts

@@ -153,16 +153,11 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
  tunnel?: string;

  /**
-   * Important: This option is currently unused and will only work in the next major version of the SDK


Rather than just deleting this line, I think we should say that it works client-side in the nextjs SDK but not elsewhere.

…s-client-transactions

lforst added 2 commits August 12, 2022 09:44

feat(nextjs): Improve pageload transaction creation

f99d89b

Add explanation

936b42e

lforst added 2 commits August 12, 2022 10:16

Traceparent data instead of traceId only

227cd4d

Fix tests by initializing JSDom in the right places

56179ed

lforst marked this pull request as ready for review August 12, 2022 10:44

lforst requested review from lobsterkatie and AbhiPrasad August 12, 2022 10:45

lforst mentioned this pull request Aug 12, 2022

Improve Nextjs tracing #5505

Closed

43 tasks

AbhiPrasad reviewed Aug 12, 2022

View reviewed changes

lforst added 4 commits August 12, 2022 15:44

Clarify comment about trace parent info

22a0f83

Undo change to global.location

651e8e0

Clean up setUpNextPage test helper

bec58e0

Guard Next route data behind default pii flag

0ba0d6e

lforst requested a review from AbhiPrasad August 12, 2022 16:04

Build nextDataTagInfo through mutation

1bde09a

AbhiPrasad approved these changes Aug 12, 2022

View reviewed changes

lobsterkatie approved these changes Aug 16, 2022

View reviewed changes

lforst added 4 commits August 16, 2022 09:23

Add support for getStaticProps tracing

652d3eb

Clarify some stuff in tests

49f51d7

Clarify current state of sendDefaultPii

0a52912

Merge remote-tracking branch 'origin/master' into lforst-better-nextj…

aa4d876

…s-client-transactions

lforst enabled auto-merge (squash) August 16, 2022 09:53

lforst merged commit 3bb8d17 into master Aug 16, 2022

lforst deleted the lforst-better-nextjs-client-transactions branch August 16, 2022 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nextjs): Improve pageload transaction creation #5574

feat(nextjs): Improve pageload transaction creation #5574

lforst commented Aug 12, 2022 •

edited

Loading

github-actions bot commented Aug 12, 2022 •

edited

Loading

AbhiPrasad commented Aug 12, 2022

lforst commented Aug 12, 2022

AbhiPrasad Aug 12, 2022

lforst Aug 12, 2022

AbhiPrasad Aug 12, 2022

lforst Aug 12, 2022

AbhiPrasad Aug 12, 2022

lforst Aug 12, 2022

AbhiPrasad Aug 12, 2022

AbhiPrasad Aug 12, 2022

lforst Aug 12, 2022

AbhiPrasad Aug 12, 2022

lforst Aug 12, 2022 •

edited

Loading

AbhiPrasad Aug 12, 2022

lforst Aug 12, 2022

AbhiPrasad Aug 12, 2022

AbhiPrasad left a comment

lobsterkatie left a comment •

edited

Loading

lobsterkatie Aug 16, 2022

lforst Aug 16, 2022

lobsterkatie Aug 16, 2022

lforst Aug 16, 2022

lobsterkatie Aug 16, 2022

lobsterkatie Aug 16, 2022

lobsterkatie Aug 16, 2022

lobsterkatie Aug 16, 2022

lobsterkatie Aug 16, 2022

		_sentryGetServerSidePropsTraceData?: string; // trace parent info, if injected by server-side `getServerSideProps`
		_sentryGetServerSidePropsBaggage?: string; // baggage, if injected by server-side `getServerSideProps`


		if (startTransactionOnPageLoad) {
		const { route, traceParentData, baggage, params } = extractNextDataTagInformation();

	const { route, traceParentData, baggage, params } = extractNextDataTagInformation();
	// Pull data from the `__NEXT_DATA__` script tag automatically injected serverside by nextjs
	const { route, traceParentData, baggage, params } = extractNextDataTagInformation();

	// Just some example what a __NEXT_DATA__ tag might look like
	// Just an example of what a __NEXT_DATA__ tag might look like

	(url, route, query, props, hasNextData, expectedStartTransactionCall) => {
	(url, route, query, props, hasNextData, expectedStartTransactionArguments) => {

feat(nextjs): Improve pageload transaction creation #5574

feat(nextjs): Improve pageload transaction creation #5574

Conversation

lforst commented Aug 12, 2022 • edited Loading

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

size-limit report 📦

AbhiPrasad commented Aug 12, 2022

lforst commented Aug 12, 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

lforst Aug 12, 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

AbhiPrasad left a comment

Choose a reason for hiding this comment

lobsterkatie left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lforst commented Aug 12, 2022 •

edited

Loading

github-actions bot commented Aug 12, 2022 •

edited

Loading

lforst Aug 12, 2022 •

edited

Loading

lobsterkatie left a comment •

edited

Loading