feat(nextjs): Add performance monitoring to server components #7242

lforst · 2023-02-21T15:28:59Z

This PR adds performance monitoring to Next.js 13 server components.

The majority of changes is adding E2E tests for this.

Resolves #7176

…server-components

packages/e2e-tests/test-utils/event-proxy-server.ts

+      const sentryRequest = https.request(
+        sentryIngestUrl,
+        { headers: proxyRequest.headers, method: proxyRequest.method },
+        sentryResponse => {
+          sentryResponse.addListener('data', (chunk: Buffer) => {
+            proxyResponse.write(chunk, 'binary');
+            sentryResponseChunks.push(chunk);
+          });
+
+          sentryResponse.addListener('end', () => {
+            eventCallbackListeners.forEach(listener => {
+              const rawProxyRequestBody = Buffer.concat(proxyRequestChunks).toString();
+              const rawSentryResponseBody = Buffer.concat(sentryResponseChunks).toString();
+
+              const data: SentryRequestCallbackData = {
+                envelope: parseEnvelope(rawProxyRequestBody, new TextEncoder(), new TextDecoder()),
+                rawProxyRequestBody,
+                rawSentryResponseBody,
+                sentryResponseStatusCode: sentryResponse.statusCode,
+              };
+
+              listener(Buffer.from(JSON.stringify(data)).toString('base64'));
+            });
+            proxyResponse.end();
+          });
+
+          sentryResponse.addListener('error', err => {
+            throw err;
+          });
+
+          proxyResponse.writeHead(sentryResponse.statusCode || 500, sentryResponse.headers);
+        },
+      );


Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>

…server-components

lforst · 2023-02-24T15:54:46Z

packages/nextjs/src/config/webpack.ts

+    let appDirPath: string;
+    if (fs.existsSync(path.join(projectDir, 'app')) && fs.lstatSync(path.join(projectDir, 'app')).isDirectory()) {
+      appDirPath = path.join(projectDir, 'app');
+    } else {
      appDirPath = path.join(projectDir, 'src', 'app');
    }


This actually caused a bug and was painful af to find....

Turns out tests help at finding bugs. Who would have thought 🤔

…ment-server-components

AbhiPrasad

Are there links to what this looks like? Does backend <-> frontend tracing work? Screenshots in the PR would also suffice.

AbhiPrasad · 2023-02-24T16:03:59Z

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

@@ -34,7 +34,7 @@ type LoaderOptions = {
  appDir: string;
  pageExtensionRegex: string;
  excludeServerRoutes: Array<RegExp | string>;
-  wrappingTargetKind: 'page' | 'api-route' | 'middleware' | 'page-server-component';
+  wrappingTargetKind: 'page' | 'api-route' | 'middleware' | 'server-component';


lowest l possible: Would be nice if this was a string enum

packages/nextjs/src/config/webpack.ts

AbhiPrasad · 2023-02-24T16:08:03Z

packages/e2e-tests/test-applications/nextjs-13-app-dir/start-event-proxy.ts

+import { startEventProxyServer, waitForTransaction } from '../../test-utils/event-proxy-server';
+
+startEventProxyServer({
+  port: 27496,


are these ports random? Should they not be?

It's kinda tricky. For Next.js we need to know the port of the proxy server at runtime AND at build time. Playwright (and implicitly the proxy server) are started after the project is built (obviously).

Instead of writing some convoluted logic that picks a port and then passes it everywhere, for now, I just opted to pick some port statically. Of course, this is somewhat error-prone but usually, it is just a "write once and forget" kinda thing.

good enough for me!

AbhiPrasad · 2023-02-24T16:08:28Z

packages/e2e-tests/test-applications/nextjs-13-app-dir/sentry.server.config.ts

+
+Sentry.init({
+  dsn: process.env.NEXT_PUBLIC_E2E_TEST_DSN,
+  tunnel: 'http://localhost:27496/', // proxy server


l: maybe we can inject the tunnel in via a environmental variable?

AbhiPrasad · 2023-02-24T16:09:42Z

packages/e2e-tests/test-applications/nextjs-13-app-dir/tests/transactions.test.ts

+  const transactionEvent = await serverComponentTransactionPromise;
+  const transactionEventId = transactionEvent.event_id;
+
+  console.log(`Polling for server component transaction eventId: ${transactionEventId}`);


m: Could we have waitForTransaction emit these logs? I'd like to keep the test surface as clean as possible.

That in particular won't work since we use waitForTransaction to get the transactionEventId. If it is about the logging statement I can just remove it.

But we can just log it out once the serverComponentTransactionPromise resolves internally?

Ideally the tests themselves have no logs. And if it does, it's optional debug logging that can be turned on.

AbhiPrasad · 2023-02-24T16:18:54Z

packages/nextjs/src/server/wrapServerComponentWithSentry.ts

+
+        const transaction = startTransaction({
+          op: 'function.nextjs',
+          name: `${componentType} Server Component (${componentRoute})`,


I wonder if this is the name we should choose for this 🤔

Maybe we try to have the componentRoute at the start?

I tried to pick something that makes it immediately obvious what the transaction is measuring. Right now it is already super confusing to see API route transactions like GET /api/my/route intertwined with pageload transactions like /my/route.

IMO having the transacitons show up as Page Server Component (/user/[id]) and Layout Server Component (/user/[id]) is very clear.

Yeah that's fair, makes sense to me.

AbhiPrasad · 2023-02-24T16:19:34Z

packages/nextjs/src/server/wrapServerComponentWithSentry.ts

+
+        const currentScope = getCurrentHub().getScope();
+        if (currentScope) {
+          currentScope.setSpan(transaction);


I wonder if you throw some React.withProfiler calls around child components if they'll show up attached as spans!

We could try it out at some point. Right now, Sentry.withProfiler is still littered with browser code which is a big nono in server components.

lforst · 2023-02-24T16:42:57Z

Are there links to what this looks like? Does backend <-> frontend tracing work? Screenshots in the PR would also suffice.

@AbhiPrasad Nope doesn't work, unfortunately. As soon as we use the headers() function inside a server component in order to continue a trace Next.js will make the component dynamic (regenerate on every request) instead of static and we obviously can't in good faith do this to users. Connected traces are gonna be a future "us" problem.

More info on static vs dynamic: https://beta.nextjs.org/docs/rendering/static-and-dynamic-rendering

AbhiPrasad · 2023-02-24T16:46:12Z

Could we inject in a meta tag for backend -> frontend tracing?

As soon as we use the headers() function inside a server component in order to continue a trace Next.js will make the component dynamic (regenerate on every request) instead of static and we obviously can't in good faith do this to users

We can monkey patch this away :)))

AbhiPrasad

Only thing here is if there is a lot of value here is there is not many child spans on the transaction.

lforst · 2023-02-24T17:21:47Z

Only thing here is if there is a lot of value here is there is not many child spans on the transaction.

The interesting aspect when looking at server component traces is gonna be how long the fetches or db queries inside the component take. Right now we're missing the fetch instrumentation but as soon as that is done the transactions will be super useful.

AbhiPrasad · 2023-02-24T18:11:41Z

The interesting aspect when looking at server component traces is gonna be how long the fetches or db queries inside the component take. Right now we're missing the fetch instrumentation but as soon as that is done the transactions will be super useful.

With this in mind IMO instrumenting server-side fetch is should be higher prio if we want this to be adopted/used.

lforst added 12 commits February 21, 2023 15:07

feat(nextjs): Add performance monitoring to server components

948251f

Use original comp

9b96913

Fix tests

eb9b72b

Merge branch 'develop' into lforst-perf-instrument-server-components

3b4496f

Lint

041cb5e

lil test

351f6f4

test: Add test utitilty to intercept sentry requests

4822861

Add some comments

2e832ca

Merge branch 'lforst-event-proxy-server' into lforst-perf-instrument-…

313093e

…server-components

Add tests

d379fc8

cjs

b41ce3b

Merge branch 'develop' into lforst-event-proxy-server

61cad4d

This comment was marked as outdated.

Sign in to view

lforst added 4 commits February 23, 2023 17:55

.

ffcc206

move

94daab4

Merge branch 'lforst-event-proxy-server' into lforst-perf-instrument-…

6fcff66

…server-components

.

24249cf

github-advanced-security bot found potential problems Feb 23, 2023

View reviewed changes

lforst and others added 12 commits February 23, 2023 21:19

.

b505bf2

.

a70e3fa

.

a3eb731

.

cc3905a

.

4170568

.

d9a8153

pls finally pass

130084c

sigh

41f266a

remove console.log

bb16d7d

.

f1b5be4

work pls?

493098b

Update package.json

aadeb29

Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>

lforst added 7 commits February 24, 2023 13:41

debug

6f4aa7b

pls finally work

2493a52

Friggin finally

b5feedc

spare me

ae7ca3d

Extract server port registration into helper function

9090f79

remove redundant stuff

f2bc116

Merge branch 'lforst-event-proxy-server' into lforst-perf-instrument-…

92b093d

…server-components

lforst marked this pull request as ready for review February 24, 2023 15:49

lforst requested review from AbhiPrasad, Lms24 and mydea and removed request for AbhiPrasad February 24, 2023 15:49

lforst commented Feb 24, 2023

View reviewed changes

Merge remote-tracking branch 'origin/develop' into lforst-perf-instru…

e02698e

…ment-server-components

AbhiPrasad reviewed Feb 24, 2023

View reviewed changes

AbhiPrasad approved these changes Feb 24, 2023

View reviewed changes

lforst added 2 commits February 24, 2023 21:47

Remove console.logs from tests

3e328bc

extract var

98499ec

lforst merged commit 4b95c04 into develop Feb 25, 2023

lforst deleted the lforst-perf-instrument-server-components branch February 25, 2023 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nextjs): Add performance monitoring to server components #7242

feat(nextjs): Add performance monitoring to server components #7242

lforst commented Feb 21, 2023 •

edited

This comment was marked as outdated.

lforst Feb 24, 2023

lforst Feb 24, 2023

AbhiPrasad left a comment

AbhiPrasad Feb 24, 2023

AbhiPrasad Feb 24, 2023

lforst Feb 24, 2023

AbhiPrasad Feb 24, 2023

AbhiPrasad Feb 24, 2023

AbhiPrasad Feb 24, 2023

lforst Feb 24, 2023

AbhiPrasad Feb 24, 2023

AbhiPrasad Feb 24, 2023

lforst Feb 24, 2023

AbhiPrasad Feb 24, 2023

AbhiPrasad Feb 24, 2023

lforst Feb 24, 2023

lforst commented Feb 24, 2023

AbhiPrasad commented Feb 24, 2023

AbhiPrasad left a comment

lforst commented Feb 24, 2023

AbhiPrasad commented Feb 24, 2023

feat(nextjs): Add performance monitoring to server components #7242

feat(nextjs): Add performance monitoring to server components #7242

Conversation

lforst commented Feb 21, 2023 • edited

This comment was marked as outdated.

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

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

lforst commented Feb 24, 2023

AbhiPrasad commented Feb 24, 2023

AbhiPrasad left a comment

Choose a reason for hiding this comment

lforst commented Feb 24, 2023

AbhiPrasad commented Feb 24, 2023

lforst commented Feb 21, 2023 •

edited