Implement non application error codes fix #895 #907
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mstoykov
force-pushed
the
feature/NonApplicationProtocolErrorCodes
branch
from
3e74fb4 to
df4aa05
Compare
na--
reviewed
Jan 30, 2019
Codecov Report
@@ Coverage Diff @@
## master #907 +/- ##
==========================================
+ Coverage 70.35% 70.72% +0.36%
==========================================
Files 118 121 +3
Lines 9111 9188 +77
==========================================
+ Hits 6410 6498 +88
+ Misses 2294 2283 -11
Partials 407 407
Continue to review full report at Codecov.
|
mstoykov
force-pushed
the
feature/NonApplicationProtocolErrorCodes
branch
from
9baec92 to
dc30a2a
Compare
na--
reviewed
Feb 13, 2019
There was a problem hiding this comment.
Mostly LGTM, though I think that we should also add a new error_code metric tag with this PR, that will also be part of the DefaultSystemTagList as well.
I'm not sure if we should remove the error tag from DefaultSystemTagList though. On the one hand, the error_code tag will do the same job, but on the other hand, some people may rely on it currently, and you've fixed its biggest issue (the highly variable string values). So I think we should leave both error and error_code there for now, at least in the next few releases.
mstoykov
force-pushed
the
feature/NonApplicationProtocolErrorCodes
branch
from
b074e8a to
9366d78
Compare
This currently only supports http and the errors currently codified are based on what errors were seen the most. Some errors have new custom messages - this has been done to errors with changing messages with the new custom message primarely removing the changing parts, usually ip/ports and such.
#928) I would've liked if I have moved more or ... less but this is the amount of code that will be useful for future development and is the amount that I could move with some amount of certainty that I am not totally breaking the whole project. There is the very real possibility that some types like URL and Response might need to be copied because of further problems with goja but for the moment this appears to be working.
mstoykov
force-pushed
the
feature/NonApplicationProtocolErrorCodes
branch
from
9366d78 to
8af7f5c
Compare
This also sets the error tag to the now more non dynamic messages. This also changes a little bit of the behaviour specifically in that if there is an error while making the http request that is not from the RoundTripper than we consider this to be exception worthy now and independantly of `throw` it will result in a exception. This should not be ... common at all: the only error that is suppose to be able to happen is if a redirect has an unparsable/missing location.
mstoykov
force-pushed
the
feature/NonApplicationProtocolErrorCodes
branch
from
c77b461 to
9b6f7cc
Compare
Merged
na--
requested changes
Mar 11, 2019
mstoykov
force-pushed
the
feature/NonApplicationProtocolErrorCodes
branch
from
de0b781 to
6e3cf8c
Compare
|
About removing |
This is primary needed because if digest and ntlm authentication as in all other cases we actually make a new transport
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.