-
-
Notifications
You must be signed in to change notification settings - Fork 490
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
Don't report network errors to Sentry #2178
Don't report network errors to Sentry #2178
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2178 +/- ##
==========================================
- Coverage 97.34% 97.31% -0.03%
==========================================
Files 99 99
Lines 3690 3691 +1
==========================================
Hits 3592 3592
- Misses 98 99 +1
|
@st0012 and this one, pending the changelog resolution ;-) |
@@ -14,6 +14,13 @@ class HTTPTransport < Transport | |||
RATE_LIMIT_HEADER = "x-sentry-rate-limits" | |||
USER_AGENT = "sentry-ruby/#{Sentry::VERSION}" | |||
|
|||
NET_HTTP_ERRORS = [ |
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.
should we maybe use this list from bundler? it has a few more types
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.
We can!
Pros:
- Better coverage! (I didn't think about Zlib errors)
Cons:
- Assumes Bundler is there. Or wait.
UPD: I think I misunderstood you. I think it's better to just copy that constant over to our code, than require 'bundler' into sentry-ruby?
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.
yes just copy over
@sl0thentr0py, done — copied the list from Bundler, with two gotchas:
I think in general, having the most common errors is enough. If we miss an error, that's not the end of the world, that's just going to get reported upstream, no big deal. We just need to get 95% right. Right? |
yep all good |
you'll need to move the changelog entry to a new 'Unreleased' section at the top now that i made a release |
5da83ac
to
becc566
Compare
becc566
to
d8a76a8
Compare
Done! AFK for 2 hours. |
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.
ty!
Summary
This pull request makes it so if Sentry encounters one of the network errors when sending data to Sentry, it won't report that error to Sentry and just re-raise it upstream instead.
Closes #1787.
Changes
::Net::HTTP
has quite a few possible errors, but I opted to rescue potential network errors (alongsideSocketError
that was already rescued) instead of rescuingStandardError
, so that if something exotic comes up, the user does get a report in Sentry still.Also added a changelog entry.
/cc @sl0thentr0py does that look like an adequate approach to you?