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

Improved Sentry server side error reporting #15866

Merged
merged 2 commits into from
Nov 23, 2022

Conversation

ErisDS
Copy link
Member

@ErisDS ErisDS commented Nov 22, 2022

refs: TryGhost/Team#2289
refs: TryGhost/express-hbs#161

  • Themes that resuse layouts as templates trigger horrible errors, which are thrown as 500s
  • But there's nothing the server is doing wrong, it's a theme user, so we downgrade these to 400s
  • There is more to do here to improve the errors shown, but this is just a first step to ensure that theme issues don't look like server failuresGot some code for us? Awesome 🎊!

@ErisDS ErisDS force-pushed the fix-sentry-unexpected branch from 49cea60 to 164c5d7 Compare November 22, 2022 18:27
@ErisDS ErisDS changed the title Downgraded express-hbs errors to 400 Improved Sentry server side error reporting Nov 22, 2022
@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Base: 67.30% // Head: 67.30% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (3534099) compared to base (d15f740).
Patch coverage: 29.62% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15866   +/-   ##
=======================================
  Coverage   67.30%   67.30%           
=======================================
  Files        1625     1625           
  Lines      105999   106022   +23     
  Branches    14975    14981    +6     
=======================================
+ Hits        71338    71357   +19     
- Misses      33378    33384    +6     
+ Partials     1283     1281    -2     
Flag Coverage Δ
e2e-tests 76.59% <12.00%> (-0.03%) ⬇️
unit-tests 53.52% <25.92%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
ghost/core/core/shared/sentry.js 45.31% <24.00%> (+9.59%) ⬆️
ghost/mw-error-handler/lib/mw-error-handler.js 100.00% <100.00%> (ø)
ghost/admin/app/components/gh-file-uploader.js 82.94% <0.00%> (-3.11%) ⬇️
ghost/data-generator/lib/tables/members.js 100.00% <0.00%> (+1.51%) ⬆️
ghost/admin/app/components/gh-dropdown.js 91.83% <0.00%> (+2.04%) ⬆️
...host/data-generator/lib/tables/members-products.js 100.00% <0.00%> (+2.85%) ⬆️
...ata-generator/lib/tables/members-created-events.js 94.28% <0.00%> (+2.85%) ⬆️
...tor/lib/tables/members-paid-subscription-events.js 100.00% <0.00%> (+10.25%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ErisDS ErisDS force-pushed the fix-sentry-unexpected branch from 164c5d7 to 02cf824 Compare November 23, 2022 11:46
- As demonstrated by my comments in the boot file, I thought sentry was already depending on the version package
- IMO it's undesirable to require package.json directly esp when we have a tool setup and ready for tis
- Added a bunch of tests to show that Sentry does roughly what we think
refs: https://github.com/TryGhost/Team/issues/1121
refs: TryGhost@5457402

- The previous change to fall back to a generic error on the server side is resulting in lots of much less useful Sentry reports
- For unexpected errors, change what's sent to Sentry back to context
- This is done by adding a specific code, so we don't have to match on a string that might change
- Also add the error type, id, code & statusCode as tags to the events - these are searchable structured data
- Adding code as a tag also makes it possible to find all errors that showed the generic message
@ErisDS ErisDS force-pushed the fix-sentry-unexpected branch from 02cf824 to 3534099 Compare November 23, 2022 12:19
@ErisDS ErisDS merged commit 62cd52f into TryGhost:main Nov 23, 2022
@ErisDS ErisDS deleted the fix-sentry-unexpected branch November 23, 2022 12:37
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.

1 participant