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

Is it possible to move sentry.client.config.js, sentry.server.config.js to a custom folder? #4249

Closed
thiagobraga opened this issue Dec 7, 2021 · 19 comments
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK

Comments

@thiagobraga
Copy link

Hi friends.

The title says it all. Is it possible to move the above files to a config folder for example?

  • sentry.client.config.js
  • sentry.server.config.js
  • sentry.properties
  • .sentryclirc

Our project is new, hasn't been released yet, and we already have several files in the root.

Just FYI, we're using:

"@sentry/nextjs": "^6.15.0",
"next": "10.1.3",
"webpack": "5.36.0"

Thanks anyway. :)

@AbhiPrasad
Copy link
Member

Hey thanks for writing in.

There isn’t a first class option to do this at the moment, but there is a PR open to add this that we are evaluating: #4187

@AbhiPrasad AbhiPrasad added Package: nextjs Issues related to the Sentry Nextjs SDK Status: In Progress and removed Status: Untriaged labels Dec 8, 2021
@lobsterkatie
Copy link
Member

lobsterkatie commented Dec 8, 2021

Hi, @thiagobraga.

We discussed it as a team and have decided not to incorporate the PR Abhi linked, but that doesn't mean you can't get things a little more streamlined. Looking at the four files you've named:

  • sentry.server.config.js and sentry.client.config.js need to remain at the root level of your project.
  • sentry.properties and .sentryclirc, on the other hand, can be eliminated entirely. As the name of the latter implies, these aren't actually SDK config files at all, but rather config files for sentry-cli (which our webpack plugin uses under the hood to upload sourcemaps to Sentry), and everything in them can be set in other ways.
    • sentryclirc: This holds your auth token, and is a separate file from sentry.properties simply so that it can be kept out of git while still allowing you to commit the rest of your sentry-cli config. In other words, it only exists locally. If you'd rather use a different way to set that token on your machine, there are a number of other options for doing so in our sentry-cli docs.

      (In prod, the token needs to be set as an environment secret, SENTRY_AUTH_TOKEN.)

    • sentry.properties: The default URL is only relevant if you're hosting sentry yourself; if you're a SAAS customer, it doesn't need to be set at all. The path to the sentry-cli executable can similarly be eliminated unless you've purposefully moved said executable out of node_modules for some reason.

      All that remains at that point is setting your org and project name. You can use the environment variables SENTRY_ORG and SENTRY_PROJECT, or you can set the org and project keys in your SentryWebpackPluginOptions in next.config.js.

I've put a ticket into our backlog to think about incorporating some of this streamlining into our default setup, also.

[UPDATE: I realized that PR had a slightly different motivation than the question addressed here (shared config rather than grouping of config files), though it would have addressed both their issue and yours. I've added a suggestion for their situation to my response there.]

@geoshak
Copy link

geoshak commented Mar 2, 2023

We just implemented sentry on our first NextJS web-app and would really like to have this possibility.
Our code organization is very minimalist, and we try to keep files/folders real tidy and clean.

Adding sentry almost doubled the files on the root folder.
We'll have several more web-apps/server code where we could use sentry, and this would be a drawback against other libs with cleaner setup.

Couldn't we at least consolidate config in one (01) file?

@jamesrweb
Copy link

I agree with @geoshak, it is quite annoying to be forced to have the files in the root when pretty much everything else supports a --config option or some form of custom configuration file to unify settings. It is just a lot of noise for what should, in my opinion, be a single file all-in-one config if it MUST be in the root.

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Apr 11, 2023

We did have a PR for this that we decided to move away from #6957

I think we're always a little hesitant expanding API surface area because of the possibility of introducing bugs - especially since nextjs themselves are pretty liberal between what they change between minors under the hood (the config files are bundled via a webpack plugin we inject, so the logic managing it can be a little delicate).

For now we're not going to re-evaluate this decision, though thank you for the feedback @jamesrweb.

Couldn't we at least consolidate config in one (01) file?

We can't do this because we don't want to run server/client/edge code on the wrong runtimes - we need to make sure that everything is tree-shaken and isolated properly, and separate config files are the best way to accomplish this while minimizing magic.

@jamesrweb
Copy link

I don't want magic either but nextjs does allow you to hook into the Webpack builder itself via the config and the runtime is also passed in there to my understanding. Perhaps exporting a config to use there from the library would be a better option to avoid the file system altogether. Then users could just use the defaults or override as desired in the next config build step. Did you consider that option yet?

@jamesrweb
Copy link

Main point is that the current implementation is yes, stable but also, clutters the root unnecessarily (imo).

@lforst
Copy link
Member

lforst commented Apr 11, 2023

nextjs does allow you to hook into the Webpack builder itself via the config and the runtime is also passed in there to my understanding

Yes, we're already using this.

Perhaps exporting a config to use there from the library would be a better option to avoid the file system altogether.

How would this look like? You generally don't want application code (which sentry.(client|server|edge).config.js are) inside your webpack config.

Main point is that the current implementation is yes, stable but also, clutters the root unnecessarily (imo).

I can see that. This is simply not a priority for us right now.

@ozyman42
Copy link

ozyman42 commented Jun 20, 2023

Typically you'd use environment variables to get custom configuration at runtime, rather than these highly unusual config files. There should be an option to read from specific environment variables whenever these files are missing.

For any customization which isn't a static value (such as integrations or transports), why couldn't someone just call Sentry.init in their own module? Perhaps the implementation for some /health API imports some custom sentry initialization module I create in the src folder of my next app to ensure it's initialized on startup.

In summary the requirement of sentry.*.config.js files in the root of a project is unnecessary given the industry standard alternatives.

@ozyman42
Copy link

For those who want to solve this despite the team maintaining this tool being unwilling to consider reasonable contributions, one solution would be to generate these files on docker build, so they can remain outside of your source code.

@lforst
Copy link
Member

lforst commented Jun 21, 2023

@ozyman42 If you find a relevant technical reason why we should do this we can reevaluate the prio for this. Right now, all of the reasons to do this are purely cosmetic. It's not solving any real problems.

@ozyman42
Copy link

Making an API more ergonomic and more easily abstracted away isn't "purely cosmetic". By that line of reasoning we should all be writing in Assembly. After all higher level languages are purely cosmetic and not solving any real problems.

Also not sure what this has to do w/ priority given someone attempted to make a contribution & the PR was rejected.

Let's say I have a hypothetical monorepo. This monorepo has 1000 microfrontend applications. Each microfrontend application has Sentry enabled. I may, someday, want to migrate my company from Sentry to another observability solution (perhaps because the maintainers of Sentry are unreasonable). When that day comes, I'd prefer to update a small set of files in my observability library which is referenced by all the microfrontend projects rather than making a 3k file PR to remove all the sentry config files. This seems like a real problem being solved, no?

@lforst
Copy link
Member

lforst commented Jun 21, 2023

I may, someday, want to migrate my company from Sentry to another observability solution

I don't think we care a lot about this use case 😆

When that day comes, I'd prefer to update a small set of files in my observability library which is referenced by all the microfrontend projects

You're already able to do that. Nothing prevents you from putting import statements into sentry.(client|server|edge).config.js that point to generic configs.

@ozyman42
Copy link

+------------------------------------------+--------+---------+
|                    _                     | Sentry | Datadog |
+------------------------------------------+--------+---------+
| API attempts to vendor lock-in its users | Yes    | No      |
+------------------------------------------+--------+---------+

good to know Sentry is not suitable for use by large companies 👍

@lforst
Copy link
Member

lforst commented Jun 21, 2023

API attempts to vendor lock-in its users

I sense that you're frustrated. The thing is, there are just a million better things for us to do right now than this.

@AbhiPrasad
Copy link
Member

@ozyman42 I appreciate the feedback - let me try to explain our decision here in more detail, hopefully that helps give some extra context.

Let's say I have a hypothetical monorepo. This monorepo has 1000 microfrontend applications. Each microfrontend application has Sentry enabled. I may, someday, want to migrate my company from Sentry to another observability solution (perhaps because the maintainers of Sentry are unreasonable). When that day comes, I'd prefer to update a small set of files in my observability library which is referenced by all the microfrontend projects rather than making a 3k file PR to remove all the sentry config files. This seems like a real problem being solved, no?

I can understand why this is a problem, but wouldn't symlinking help address some problems here? As Luca said above, we only need the path names to be set, but you can still set a central configuration somewhere and just import it into your sentry config files.

Let's look at another example, which is Next.js's OpenTelemetry integration. OpenTelemetry is a vendor neutral spec for tracing, metrics and logs. Next requires you to create an instrumentation.ts file, which cannot be renamed or moved to a custom folder. As such if you want to use an open standard with OpenTelemetry you'll have to functionally use the same approach that we do with our config files.

For any customization which isn't a static value (such as integrations or transports), why couldn't someone just call Sentry.init in their own module? Perhaps the implementation for some /health API imports some custom sentry initialization module I create in the src folder of my next app to ensure it's initialized on startup.

Part of the reason why we have the specific config files is because the config files are bundled via a webpack plugin we inject, so the logic managing it can be a little delicate. We have to support both the app and pages directory, as well as both the vercel edge and node runtimes. The issue isn't with the initial implementation - it's making sure that our approach will work across versions and paradigms. As per our SDK development philosophy we want to minimize breaking people's apps as much as possible, which we've done before by adjusting our bundling/config path logic.

The specific vendor lock-in here is because of the technical complexity of supporting Next and all it can do. We're actively working with Vercel to expose better hooks so we can get around this, but for now this is what we need to do. If you have any suggestions how we can improve this integration with Next.js we are very open to feedback and PRs.

Here's some examples with issues around bundling we've encountered before:

In regards to how we are spending our time for relative to this issue, we are currently working through our backlog of Next.js bugs and attempting to get the nextjs app directory feature complete. When we are done with that, we can revisit this decision. Hope this explanation was helpful, but happy to answer any follow up questions.

@ozyman42
Copy link

The thing is, there are just a million better things for us to do right now than this.

Nobody's asking your team to implement a new API / extend the existing API. The ask is to allow others to do the work for you and not close their PRs.

Let's look at another example...

Just because one tool commits to following an anti-pattern shouldn't give other projects license to repeat that anti-pattern.

Part of the reason why we have the specific config files is because the config files are bundled via a webpack plugin we inject,

Why magically inject a webpack plugin rather than having users initialize it in their webpack config as is standard for many plugins? Why not allow both for now? It's possible to add features without breaking existing expected behavior. This is often called a minor version bump.

So the logic managing it can be a little delicate

Typically when code has a tendency to break upon changes, the answer is to add extensive unit tests rather than blocking all modifications to the code. Is there a reason the team prefers closing PRs rather than accepting contributions to improve test coverage to bundling/config path logic so that this logic could be upgraded safely?

If you have any suggestions how we can improve this integration with Next.js we are very open to feedback and PRs

The community has attempted contributions for this issue but the PRs were rejected. I would be happy to submit something if there's an actual willingness to accept a fix to the issue rather than using the catch all of "it'll cause bugs" as an excuse to avoid actually collaborating on a fix.

@AbhiPrasad
Copy link
Member

Nobody's asking your team to implement a new API / extend the existing API. The ask is to allow others to do the work for you and not close their PRs.

But then we have the issue of maintenance - which is what our team is going to handle. We'll have to deal with this working with new runtimes, new Next major versions, and new paradigms (in case there is a successor to the app directory). As an example, here is the Next 13 changelog that we had to manually comb through to understand what changes we had to make to our instrumentation. This is necessary again because the hooks are not exposed by Next the framework itself to provide the instrumentation our users expect. We prioritize user experience and instrumentation quality over being correct with the framework.

Just because one tool commits to following an anti-pattern shouldn't give other projects license to repeat that anti-pattern.

It's not an anti-pattern - just a limitation we have at the moment. We recognize this and are not trying to downplay it, just explaining why we aren't going to put effort toward building/maintaining a solution for the problem by also looking at the approach taken by Vercel. I'm taking an educated guess to say that Vercel themselves chose this pattern probably due to the restrictions of the framework itself (app vs. page directory) and figured it was easier to make this decision than have another permutation of possible setups in which to introduce regressions/block future features.

I'd like the emphasize that most of the complexity here comes from the complexity of Next the framework itself, and all of it's different permutations. Maintaining that is the cost we are looking to minimize.

Why magically inject a webpack plugin rather than having users initialize it in their webpack config as is standard for many plugins? Why not allow both for now? It's possible to add features without breaking existing expected behavior

We magically inject because we leave the option open to change the internals/how we inject/how many plugins we inject without exposing this to the user. The average next.js developer does not care at all about this process - they just want stuff to work without thinking about it. In addition, having someone change their webpack config is not friendly toward novice developers or those have no custom webpack config changes (which is what the default next template does). This aligns with our sdk development philosophy, Prioritize Customer Convenience over Correctness.

Is there a reason the team prefers closing PRs rather than accepting contributions to improve test coverage to bundling/config path logic so that this logic could be upgraded safely

The issue isn't at all with the current code - it's anticipating future Next releases and major versions. Every Next major we've had to make quite a bit of adjustments and bug fixes, Next 11, 12, 13 - you can see proof of this by looking up PRs/issues in our repo. Who knows what Next 14 will bring.

We have a significant testing suite already that test against Webpack 4/5 and Next 10, 11, 12, 13 in a matrix for error and performance monitoring. You can see this here: https://github.com/getsentry/sentry-javascript/tree/develop/packages/nextjs/test/integration. We also test against Node 10-20 as an additional parameter to the matrix.

Bugs with configs here would be a large regression since it would mean that sentry doesn't initialize at all or initializes incorrectly. Sentry is mission critical for many devs so we pay extra special attention to things like this.

The community has attempted contributions for this issue but the PRs were rejected. I would be happy to submit something if there's an actual willingness to accept a fix to the issue rather than using the catch all of "it'll cause bugs" as an excuse to avoid actually collaborating on a fix.

My ask was not about this suggestions for this particular change, rather about another approach for instrumentation that would simplify needing to use the configs while supporting Webpack 4,5 as well as Next 10-13 for the edge and node runtimes, and both the page and app directory (plus react server components). If another approach can be found, we are completely ready to change how we generate and use config files for setting up the SDK. With our current approach we are not moving forward with the PR or this change.

using the catch all of "it'll cause bugs"

We know that this is a small change! And that the implementation is relatively trivial! We're rejecting this change because we've been through similar stuff like this before. I know we don't have strict proof or metrics, but I'm asking you to please trust our opinions here regarding this as maintainers who have worked on this SDK for over 2+ years now.

@ozyman42
Copy link

ozyman42 commented Jun 22, 2023

It seems a little odd to cite Prioritize Customer Convenience over Correctness here to justify the decision to close PRs because in this thread you have several customers explicitly telling your team that it would be more convenient for them to have alternative ways to configure the entrypoint for Sentry rather than having sentry.*.config.js files at the root of the project. Essentially what you're saying is you know what Sentry customers want better than what the customers of Sentry say they want. That's a cardinal sin in business which many dev teams often end up committing. The other reason the principle is an odd one to cite is because sentry.*.config.js files are not even the correct approach to API design, it's an antipattern as I've explained above. So essentially you're prioritizing keeping an incorrect API over customer convenience here. Several other Sentry SDK development philosophy principles being ignored here by the team's response include

  • Customer Value Matters as this change benefits customers yet the changes are not considered good to go by the team
  • Configuration Comes at a Cost as the proposal to have Sentry integration work without the presence of sentry.*.config.js files in the root of the project aligns with the idea of having Sentry just working out of the box with reasonable defaults more than the current API does.
  • Enable Customers because the surface of the API would be smaller if customers could keep all their sentry configuration in next config rather than having to also configure 3 files at the root of the project.

Rather than addressing the logical problems in all your responses, I'd like to remain focused on the main topic of the issue, so let's try this. There have been several propositions in this thread to support alternatives to the root config files, and none of these proposals are fundamentally incompatible with Next 10+ or Webpack 4+, and since there is an exhaustive test suite which you have mentioned, the idea that any of these changes would break existing customers causing a large regression is an unreasonable concern, and it's clear what the path is to mitigate the concern (we can add more tests if needed before releasing the change). So what alternative is your team open to so that I or someone else has some guidance on what to work on in a PR which introduces an alternative?

  1. Config option in webpack config in next.config.js to support entry filepath outside of project root
  2. Use environment variables to initialize Sentry whenever the sentry.*.config.js file(s) is missing else use a default for each value
  3. Clients can call Sentry.init from wherever they want to in their application rather than in sentry.*.config.js files

Or do you require an exhaustive design doc weighing the pros and cons of all options above before your team is comfortable reviewing & approving a PR for any of these options?

The company I work for has a rather large contract w/ Sentry so if I need to try a top down approach and go through Sentry customer support or Sentry leadership to get traction on this issue, I'll attempt that route next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK
Projects
None yet
Development

No branches or pull requests

7 participants