feat(nextjs): Use OpenTelemetry for performance monitoring and tracing by lforst · Pull Request #11016 · getsentry/sentry-javascript

lforst · 2024-03-11T12:30:49Z

This PR moves the Next.js over to OpenTelemetry based performance monitoring.

Resolves #11042

Follow-up items:

Find more robust logic to filter out sentry ingest spans
Investigate noisy data
Check if we can remove the 5 second timeout in node tests again

github-actions · 2024-03-18T12:54:58Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay, Feedback)	80.55 KB (0%)
@sentry/browser (incl. Tracing, Replay)	71.89 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas)	75.7 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	65.44 KB (0%)
@sentry/browser (incl. Tracing)	36.71 KB (0%)
@sentry/browser (incl. browserTracingIntegration)	36.71 KB (0%)
@sentry/browser (incl. feedbackIntegration)	31.35 KB (0%)
@sentry/browser (incl. feedbackModalIntegration)	31.46 KB (0%)
@sentry/browser (incl. feedbackScreenshotIntegration)	31.47 KB (0%)
@sentry/browser (incl. sendFeedback)	27.42 KB (0%)
@sentry/browser	22.58 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback)	74.9 KB (0%)
CDN Bundle (incl. Tracing, Replay)	69.73 KB (0%)
CDN Bundle (incl. Tracing)	36.3 KB (0%)
CDN Bundle	23.94 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed	219.01 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed	109.57 KB (0%)
CDN Bundle - uncompressed	70.92 KB (0%)
@sentry/react (incl. Tracing, Replay)	71.87 KB (0%)
@sentry/react	22.61 KB (0%)

In order to proceed with removing `Sentry.Integrations.X` as per #8844, there's still some places to clean up. This does conflict with #11016, but not sure when that merges in, so opening this in the meantime to unblock the integrations cleanup work. if we think the OTEL nextjs work will merge in sooner then the next 1-2 days, I'm happy to leave this alone for now!

mydea · 2024-03-26T08:52:52Z

dev-packages/e2e-tests/test-applications/nextjs-14/package.json

    "wait-port": "1.0.4"
  },
  "devDependencies": {
+    "@sentry-internal/feedback": "latest || *",


Do we need all of these?

I think so, at least I couldn't be bothered to filter them out. These are all deps that nextjs depends on or could ever depend on.

mydea · 2024-03-26T08:53:47Z

dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts

        'http.method': 'GET',
        'sentry.op': 'http.client',
-        'sentry.origin': 'auto.http.node.undici',
+        'next.span_type': 'AppRender.fetch', // This span is created by Next.js own fetch instrumentation


l: We could think about adding sentry.origin: 'auto.http.nextjs.fetch' or something like this if we detect this.

I have started a broader discussion around this topic last week because we need a generic decision in all SDKs for this. The important question being: "What sentry.origin do we give spans that are not generated through Sentry SDK API?" Right now it is manual - which is obv wrong. I'd argue we don't set anything, because sentry.origin doesn't make sense anymore.

mydea

Looking great! Left some small nitty-comments, great work (the outcome always looks so simple...! "it just works, what do you mean this was tricky???")

mydea · 2024-03-26T08:54:37Z

dev-packages/e2e-tests/test-applications/nextjs-app-dir/assert-build.ts

+// if (nextjsVersion !== 'latest' && nextjsVersion !== 'canary') {
+//   assert.doesNotMatch(buildStderr, /Import trace for requested module/); // This is Next.js/Webpack speech for "something is off"
+// }
+// Note(lforst): I disabled this for the time being to figure out OTEL + Next.js - Next.js is currently complaining about a critical import in the @opentelemetry/instrumentation package.


For reference, what exactly is it complaining about?

Added a comment with an example!

mydea · 2024-03-26T08:55:02Z

dev-packages/e2e-tests/test-applications/nextjs-app-dir/package.json

    "@playwright/test": "^1.27.1"
  },
  "devDependencies": {
+    "@sentry-internal/feedback": "latest || *",


Do we need all of these?

Same as #11016 (comment)

mydea · 2024-03-26T08:57:41Z

packages/nextjs/src/server/index.ts


-  const filterTransactions: EventProcessor = event => {
-    return event.type === 'transaction' && event.transaction === '/404' ? null : event;
+  const filterLowQualityTransactions: EventProcessor = event => {


Is it on purpose that we are not filtering /404 transactions anymore here? (or do these not exist anymore?)

Right, I think I accidentally removed this - added it back!

mydea · 2024-03-26T08:59:51Z

packages/nextjs/src/server/index.ts


-  const filterTransactions: EventProcessor = event => {
-    return event.type === 'transaction' && event.transaction === '/404' ? null : event;
+  const filterLowQualityTransactions: EventProcessor = event => {


l: This is fine, but could also possible be simplified to:

addEventListener(Object.assign((event: Event) => { // ... }, { id: 'NextLowQualityTransactionsFilter' }));

Not sure how much better, but avoids some local variables - feel free to ignore or not :D

mydea · 2024-03-26T09:02:39Z

packages/nextjs/src/server/index.ts

+        // Filter out spans for Sentry event sends
+        const httpTargetAttribute: unknown = span.data?.['http.target'];
+        if (typeof httpTargetAttribute === 'string') {
+          // TODO: Find a more robust matching logic


We should use suppressTracing here 🤔 something to look into in the future!

to be clear, with "here" I mean in the transport :D

Yep, I'll add a TODO to potentially revisit this in the future.

AbhiPrasad · 2024-03-26T13:03:31Z

packages/nextjs/src/server/index.ts

-
-  filterTransactions.id = 'NextServer404TransactionFilter';
+  addEventProcessor(
+    Object.assign(


should we add these as integrations?

Maybe in a followup. I want this pr closed asap.

With this change, there are two remaining references to node-experimental. 1. nextjs, which is handled by #11016 2. express integration tests, which I will address after #11285 gets merged Once those two are done, we can remove the old node packages entirely!

Next.js provides their own OTel http integration, which conflicts with ours ref #11016 added commit from this PR: #11319 --------- Co-authored-by: Luca Forstner <luca.forstner@sentry.io> Co-authored-by: Francesco Novy <francesco.novy@sentry.io>

In order to proceed with removing `Sentry.Integrations.X` as per getsentry#8844, there's still some places to clean up. This does conflict with getsentry#11016, but not sure when that merges in, so opening this in the meantime to unblock the integrations cleanup work. if we think the OTEL nextjs work will merge in sooner then the next 1-2 days, I'm happy to leave this alone for now!

getsentry#11016) Co-authored-by: s1gr1d <sigrid.huemer@posteo.at>

With this change, there are two remaining references to node-experimental. 1. nextjs, which is handled by getsentry#11016 2. express integration tests, which I will address after getsentry#11285 gets merged Once those two are done, we can remove the old node packages entirely!

Next.js provides their own OTel http integration, which conflicts with ours ref getsentry#11016 added commit from this PR: getsentry#11319 --------- Co-authored-by: Luca Forstner <luca.forstner@sentry.io> Co-authored-by: Francesco Novy <francesco.novy@sentry.io>

lforst mentioned this pull request Mar 14, 2024

[v8] Port @sentry/nextjs to be powered by OpenTelemetry #11042

Closed

AbhiPrasad mentioned this pull request Mar 18, 2024

feat(v8/nextjs): Remove usage of class integrations #11182

Merged

lforst force-pushed the lforst-nextjs-otel branch 2 times, most recently from 33a4f94 to 815b156 Compare March 19, 2024 15:55

lforst mentioned this pull request Mar 21, 2024

feat(opentelemetry): Do not capture exceptions for timed events #11221

Merged

feat(nextjs): Use OpenTelemetry for performance monitoring

3b5d9ce

lforst force-pushed the lforst-nextjs-otel branch from 3b4fa31 to 3b5d9ce Compare March 21, 2024 10:42

lforst changed the title ~~feat(nextjs): Use OTEL tracing~~ feat(nextjs): Use OpenTelemetry for performance monitoring and tracing Mar 21, 2024

Luca Forstner added 6 commits March 21, 2024 11:12

revert accidental change

c5146f3

Update E2E test to check for Next.js fetch

5d1e52a

Fix integration tests

93bf4f9

Increase nock count timeout

e3ead78

unflake tests

6a5e564

maybe?

cf5fe92

AbhiPrasad mentioned this pull request Mar 22, 2024

[v8] New Performance APIs #9956

Closed

Luca Forstner and others added 11 commits March 22, 2024 08:20

Merge remote-tracking branch 'origin/develop' into lforst-nextjs-otel

15ffe0f

Update dev dependencies in e2e test

d4b0e65

logs

0973ee0

Merge remote-tracking branch 'origin/develop' into lforst-nextjs-otel

e4aecd0

Print build errors

201d459

Merge remote-tracking branch 'origin/develop' into lforst-nextjs-otel

c4c37fd

whoops

55c3bc7

Update replay dev dep

c16a2d9

Build errors

d11620e

filter static assets

7e7824c

Increase buffer size

728c086

lforst mentioned this pull request Mar 25, 2024

fix(opentelemetry): Do not stomp span status when startSpan callback throws #11170

Merged

Merge remote-tracking branch 'origin/develop' into lforst-nextjs-otel

b61b646

mydea reviewed Mar 26, 2024

View reviewed changes

mydea approved these changes Mar 26, 2024

View reviewed changes

Luca Forstner added 6 commits March 26, 2024 11:07

Merge remote-tracking branch 'origin/develop' into lforst-nextjs-otel

f297b78

Add back dev test

3aaeb06

Add comment about build assertion

785f36d

Clean up devDeps resolutions in e2e test

caa9fb0

Undo node integration test timeout

ba3f500

Refactor new filtering logic

4e6097c

AbhiPrasad reviewed Mar 26, 2024

View reviewed changes

lforst force-pushed the lforst-nextjs-otel branch from 740805b to ad74746 Compare March 26, 2024 13:12

AbhiPrasad approved these changes Mar 26, 2024

View reviewed changes

Fix types

47f3b1b

lforst force-pushed the lforst-nextjs-otel branch from ad74746 to 9858846 Compare March 26, 2024 13:32

Try to unflake

65f3e73

lforst force-pushed the lforst-nextjs-otel branch from 9858846 to 65f3e73 Compare March 26, 2024 15:23

Luca Forstner added 2 commits March 26, 2024 15:38

add timeout - last resort

348a656

Add exports

0eb8af0

AbhiPrasad mentioned this pull request Mar 26, 2024

ref: remove node-experimental references #11290

Merged

Declare bancruptcy

c6977a2

lforst enabled auto-merge (squash) March 26, 2024 16:31

lforst merged commit 0e9cdee into develop Mar 27, 2024

lforst deleted the lforst-nextjs-otel branch March 27, 2024 08:04

s1gr1d mentioned this pull request Mar 27, 2024

fix(nextjs): Remove Http integration from Next.js #11304

Merged

cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024

feat(nextjs): Use OpenTelemetry for performance monitoring and tracing (

1fb749f

getsentry#11016) Co-authored-by: s1gr1d <sigrid.huemer@posteo.at>

Uh oh!

Conversation

lforst commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mydea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lforst commented Mar 11, 2024 •

edited

Loading

github-actions bot commented Mar 18, 2024 •

edited

Loading