-
Notifications
You must be signed in to change notification settings - Fork 6
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: Added Sentry for error logging purposes #390
Conversation
// For session replay: during development capture all sessions, but sample only a subset in production. | ||
replaysSessionSampleRate: isProd ? 0.05 : 1.0, | ||
// Increase session capture rate specifically for sessions where errors occur. | ||
replaysOnErrorSampleRate: isProd ? 0.5 : 1.0, |
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.
any of these options can be modified as we move to production. If you feel we should be more aggressive im all for it, let me know what i should change these to
Coverage after merging feature/add-sentry-monitoring into master
Coverage Report
|
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #390 +/- ##
==========================================
- Coverage 20.56% 20.51% -0.05%
==========================================
Files 223 224 +1
Lines 3093 3100 +7
Branches 767 771 +4
==========================================
Hits 636 636
- Misses 2388 2395 +7
Partials 69 69
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
It looks like the |
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 will need to run through a bit of a tutorial for Sentry since I've never used it. Otherwise, found three minor things:
- Extra space in production string?
- When running
npm install
sentry-cli inpackage-lock
is removed and is showing as a file change, not sure if this was missed as part of a commit. Might also be part of what was mentioned by Trevor. - Not seen in current Master branch but when loading this branch the text area input boxes on
Upload A Song
andProfile
aren't styled correctly... I'm not sure how the changes you made affect this, but wanted to double check if you are seeing it on your side as well.
src/sentryConfig.ts
Outdated
dsn: "https://b98c0562df1b2b5d4c6bf708b91a561f@o1174944.ingest.sentry.io/4505967320956928", | ||
// For testing purposes you can set the environment to "debug" | ||
// to separate development and production errors | ||
environment: isProd ? "production " : "development", |
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.
Extra space 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.
What's the extra space for?
Coverage after merging feature/add-sentry-monitoring into master
Coverage Report
|
Coverage after merging feature/add-sentry-monitoring into master
Coverage Report
|
Coverage after merging feature/add-sentry-monitoring into master
Coverage Report
|
|
Coverage after merging feature/add-sentry-monitoring into master
Coverage Report
|
Visit the preview URL for this PR (updated for commit b362312): https://newm-artist-portal--pr390-feature-add-sentry-m-496w8ap8.web.app (expires Sat, 11 Nov 2023 14:10:16 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: ec3483d4c62309afc398865bfd6a9fc9e03e1d46 |
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.
LGTM, just had one minor comment about the .env.template file.
@@ -1,3 +1,4 @@ | |||
REACT_APP_FACEBOOK_CLIENT_ID= | |||
REACT_APP_GOOGLE_CLIENT_ID= | |||
REACT_APP_LINKEDIN_CLIENT_ID= | |||
SENTRY_AUTH_TOKEN= |
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.
Since this isn't used in the app, it should be fine to not include here. It would only need to be added to the .env
file if the app needed to reference it when running locally.
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 was on the fence with this one, if you wanted to debug by running the prod app locally you would need this variable. I can remove it if you think we shouldnt add it
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.
Makes sense to have it, in that case.
src/sentryConfig.ts
Outdated
dsn: "https://b98c0562df1b2b5d4c6bf708b91a561f@o1174944.ingest.sentry.io/4505967320956928", | ||
// For testing purposes you can set the environment to "debug" | ||
// to separate development and production errors | ||
environment: isProd ? "production " : "development", |
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 extra space for?
Coverage after merging feature/add-sentry-monitoring into master
Coverage Report
|
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.
LGTM
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.
LGTM
Along with this setup, i also added "Sentry" slack app addon, you will see all new issues and resolved issues go through there. Created
#sentry-issues-web
to track production environment issues to reduce the noise. Alerts and notifications can be modified as we see fit.