-
Notifications
You must be signed in to change notification settings - Fork 11
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
make telemetry helper resilient to exceptions #452
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #452 +/- ##
==========================================
- Coverage 97.35% 97.33% -0.02%
==========================================
Files 401 401
Lines 33665 33671 +6
==========================================
+ Hits 32773 32775 +2
- Misses 892 896 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #452 +/- ##
==========================================
- Coverage 97.35% 97.33% -0.02%
==========================================
Files 401 401
Lines 33665 33671 +6
==========================================
+ Hits 32773 32775 +2
- Misses 892 896 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #452 +/- ##
==========================================
- Coverage 97.35% 97.33% -0.02%
==========================================
Files 401 401
Lines 33665 33671 +6
==========================================
+ Hits 32773 32775 +2
- Misses 892 896 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #452 +/- ##
==========================================
- Coverage 97.37% 97.36% -0.02%
==========================================
Files 432 432
Lines 34355 34361 +6
==========================================
+ Hits 33454 33456 +2
- Misses 901 905 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Are we fine with having the catch-all exceptions?
Out of curiosity, does having log.exception
creates a sentry issue?
i think for this component it makes sense. if telemetry fails, there isn't really anything we want the caller to do to retry or recover, and we certainly don't want to fail with an uncaught exception. should just log and continue
don't actually know, but the errors i was seeing were "DB transaction was already rolled back because of another exception: {other exception}" and the other exception has errors in sentry so they at least will not be lost |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
https://github.com/codecov/internal-issues/issues/461