-
-
Notifications
You must be signed in to change notification settings - Fork 274
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
Add Sentry #1043
Add Sentry #1043
Conversation
Your Render PR Server URL is https://toolpad-pr-1043.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-ccl1ge02i3mqcbhrm740. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Is there a way to test out whether we're receiving the logs?
Also, do we want to see logs in deploy previews?
c232241
to
e3221b9
Compare
Should be working in the preview now, I've set up the environment variables.
I think the plan for now is to enable Sentry in demo only. |
not defining the TOOLPAD_SENTRY_DSN variable means disabling sentry. no need to add more redundant variables. also, I'd keep the convention of prefixing Toolpad variables with TOOLPAD_ |
packages/toolpad-app/src/config.ts
Outdated
| 'TOOLPAD_BUILD', | ||
| 'TOOLPAD_BUILD' | ||
| 'IS_SENTRY_ENABLED' | ||
| 'SENTRY_DSN', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure this is build time only? i.e. we bake it in the image? self hosters can't add their own sentry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not defining the TOOLPAD_SENTRY_DSN variable means disabling sentry. no need to add more redundant variables. also, I'd keep the convention of prefixing Toolpad variables with TOOLPAD_
Makes sense, I was thinking of this env variable as more like an on/off toggle but you're right, it's redundant, better to just use TOOLPAD_SENTRY_DSN
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure this is build time only? i.e. we bake it in the image? self hosters can't add their own sentry?
Ok then we shouldn't have this as a built time variable here (I was planning to ask you how the build-time variables work in specific too as I wasn't sure about them).
How can I make an environment variable readable to the Next.js client-side though, without prefixing it with NEXT_PUBLIC
? I need to do that for the Sentry DSN variable. (edit: I've used a runtime env variable for it now - please let me know if it's correct!)
|
||
import * as Sentry from '@sentry/nextjs'; | ||
|
||
const SENTRY_DSN = process.env.SENTRY_DSN || process.env.NEXT_PUBLIC_SENTRY_DSN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure NEXT_PUBLIC_ variables make sense for us
const SENTRY_DSN = process.env.SENTRY_DSN || process.env.NEXT_PUBLIC_SENTRY_DSN; | |
const SENTRY_DSN = process.env.SENTRY_DSN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the right way to have a client-side readable environment variable in our project, if it's not a build-time variable? Is it using the runtime config, like I've changed it to now?
|
||
silent: true, // Suppresses all logs | ||
|
||
dryRun: !process.env.TOOLPAD_SENTRY_DSN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apedroferreira Hmm, I'm not sure this would work well for us. This only gets executed at build time. Would it still work if we don't upload sourcemaps and instead rely on production sourcemap urls? https://nextjs.org/docs/advanced-features/source-maps#configuration-flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which thing in specific would not work well?
I added this dryRun
condition because if dryRun
has its default value Sentry seems to require an auth token, but an auth token should not be required if someone is not using Sentry.
This logic seems to fix that issue both locally and in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i could add a comment making it more clear though if that's the confusion
edit: it could also be that i set the auth token in production though and the issue is still there, i can try to check again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's about hideSourceMaps
and there's a chance they could still be visible in the browser, we can try not uploading them and setting them manually in Sentry just for safety
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind them being visible in the browser. This is an OS project after all 🙂 .
My point is basically, if we have sourcemaps publicly available, hosted from the toolpad application, do we still need to upload them for Sentry? Will Sentry find the sourcemaps if our js files have //# sourceMappingURL
directives? If yes, then I'd just disable uploading them altogether.
If Sentry can't work like that, then I think we should make this a separate env variable specifically for the build process that explicitly enables uploading sourcemaps (e.g. TOOLPAD_UPLOAD_SOURCEMAPS
or something, not user facing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that we should be fine having soruce-maps hosted with app itself: https://docs.sentry.io/platforms/javascript/sourcemaps/#2-provide-source-maps-to-sentry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish Sentry would more clearly explain why they recommend uploading. Their callout https://docs.sentry.io/platforms/javascript/sourcemaps/uploading/hosting-publicly/ doesn't make sense to me:
Closes #998
Adds Sentry to Toolpad - reporting errors in client and server.
Most of this configuration should be correct, but I cannot get any errors to create issues in the MUI Sentry account...(Nevermind, I found and fixed the problem)Not sure what might be wrong, I've tried with a personal account and error reporting seems to work, so the problem seems to be related with the MUI Sentry account in specific.