Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(nextjs): Remove client.(server|client).config.ts functionality in favor of instrumentation.ts #11059

Merged
merged 34 commits into from Mar 18, 2024

Conversation

lforst
Copy link
Member

@lforst lforst commented Mar 12, 2024

Ref #11042
Ref #11046

Because Next.js OTEL implementations require tracers to be set up in instrumentation.ts (not doing so breaks parent-child relationships between spans for some reason) we need to change how users configure the SDK.

With this change we will not pick up client.(server|client).config.(ts|js) files any longer and instead require users to initialize the serverside SDKs inside instrumentation.ts. Because users may miss this critical step, we log a message if we detect the presence of the old config files.

An added benefit of this PR is that it moves us a tiny step closer to not depending on webpack build-time magic for whenever turbopack ships. I personally find it cool that we are moving into a more "native" way of setting up monitoring for Next.js with this PR.

This PR also adjusts the typing for withSentryConfig to be more in line with what it actually does (i.e. we now accept basically any input and just return the same type).

@AbhiPrasad
Copy link
Member

How does edge/client/server specific config work with instrumentation.ts?

@lforst
Copy link
Member Author

lforst commented Mar 12, 2024

How does edge/client/server specific config work with instrumentation.ts?

You do

// instrumentation.ts
import * as Sentry from '@sentry/nextjs';

export function register() {
  if (process.env.NEXT_RUNTIME === 'nodejs') {
    Sentry.init({
      dsn: '...',
    });
  } else if (process.env.NEXT_RUNTIME === 'edge') {
    Sentry.init({
      dsn: '...',
    });
  }
}

And the multiplexer™ handles the rest 😎

@lforst lforst changed the title feat(nextjs): Allow omitting client.(server|client).config.ts and nudge towards using instrumentation.ts feat(nextjs): Remove client.(server|client).config.ts functionality in favor of instrumentation.ts Mar 14, 2024
Copy link
Contributor

github-actions bot commented Mar 15, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) 82.19 KB (added)
@sentry/browser (incl. Tracing, Replay) 73.52 KB (added)
@sentry/browser (incl. Tracing, Replay with Canvas) 77.33 KB (added)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 67.07 KB (added)
@sentry/browser (incl. Tracing) 38.15 KB (added)
@sentry/browser (incl. browserTracingIntegration) 38.15 KB (added)
@sentry/browser (incl. feedbackIntegration) 33.01 KB (added)
@sentry/browser (incl. feedbackModalIntegration) 33.09 KB (added)
@sentry/browser (incl. feedbackScreenshotIntegration) 33.1 KB (added)
@sentry/browser (incl. sendFeedback) 29.08 KB (added)
@sentry/browser 24.25 KB (added)
CDN Bundle (incl. Tracing, Replay, Feedback) 76.57 KB (added)
CDN Bundle (incl. Tracing, Replay) 71.43 KB (added)
CDN Bundle (incl. Tracing) 37.77 KB (added)
CDN Bundle 25.62 KB (added)
CDN Bundle (incl. Tracing, Replay) - uncompressed 223.88 KB (added)
CDN Bundle (incl. Tracing) - uncompressed 113.82 KB (added)
CDN Bundle - uncompressed 75.66 KB (added)
@sentry/react (incl. Tracing, Replay) 73.51 KB (added)
@sentry/react 24.27 KB (added)

@lforst lforst marked this pull request as ready for review March 18, 2024 11:20
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I agree, being more Framework-native is a good change. Can we also update the package README? I think this is safe to update the readme now and docs when we go stable. Also reminder for us to adjust the wizard for v8.

MIGRATION.md Outdated
#### Updated the recommended way of calling `Sentry.init()`

With version 8 of the SDK we will no longer support the use of `sentry.server.config.ts` and `sentry.edge.config.ts`
files (note that `sentry.client.config.ts` is still supported and encouraged). Instead, please initialize the Sentry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: I think the note about the client config should be its own sentence and maybe bold. Users might skip over the sentence and think everything is now initialized in instrument.ts.

import * as Sentry from '@sentry/nextjs';

export function register() {
if (process.env.NEXT_RUNTIME === 'nodejs' || process.env.NEXT_RUNTIME === 'edge') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This begs a question: Suppose I have the same config for node and edge (as in this example). Do I even need the if statement? Asking because I guess this is a common thing for users and if it's indeed necessary, we should point that out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more to be in line with what the Next.js docs say and to be forwards compatible. Let's say Next.js also starts running the register hook on the browser, not having the if statement would horribly fail in some cases.

@lforst lforst merged commit 093531e into develop Mar 18, 2024
62 checks passed
@lforst lforst deleted the lforst-nextjs-instrumentts branch March 18, 2024 12:15
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants