Include pushgateway response body in HTTP error messages - #1198
Open
aviseth wants to merge 1 commit into
Open
Conversation
When the pushgateway rejects a push it explains why in the response body, but urllib's HTTPError only renders "HTTP Error 500: Internal Server Error", so the diagnostic was discarded before the caller ever saw it. Read the body in _make_handler and append it to the error message, still raising an HTTPError so existing except clauses keep working, and include the body in the OSError raised for handlers that suppress urllib's default error handling. Fixes prometheus#96 Signed-off-by: Avi Seth <avi.seth8@gmail.com>
This file contains hidden or 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
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.
@csmarchbanks this is #96, which has been open a long time, so tell me if it is no longer wanted and I will close it.
When the pushgateway rejects a push it says why in the response body:
text format parsing error in line 3: expected float as value, a complaint about a duplicate metric name, and so on. urllib raisesHTTPErrorfrom insideopener.open(), andHTTPError.__str__renders onlyHTTP Error 500: Internal Server Error, so the explanation is gone by the time the exception reaches the caller. Most code logs the traceback, and with it the one piece of information that would have said what was wrong with the payload._make_handlernow catchesHTTPError, reads the body, and re-raisesHTTPErrorwith the body appended to the reason. The type is unchanged, soexcept HTTPError,except OSErrorandexcept IOErrorcall sites keep working and only the message text differs. An empty body re-raises the original untouched. Theresp.code >= 400branch below is only reachable with abase_handlerthat suppresses urllib's default error handling, but it had the same problem, so it gets the same treatment.test_push_error_includes_response_bodydrives a 500 with a body through theTestPushGatewaystub handler, which already branches on the request line, and checks that the body text lands instr(exception)and that it is still anHTTPErrorwith.code == 500. Nothing exercised a>= 400response from the gateway before this.Two things I did not want to decide on my own.
The body is not truncated. A reverse proxy in front of the gateway can return a full HTML error page, and all of it would end up in the message. I am glad to cap it if you want one, but the limit is your call.
e.read()consumes the response, so the replacementHTTPErroris constructed withfp=None. Anyone currently writingexcept HTTPError as e: e.read()to get at the body would get an empty read instead. The text is instr(e)now, but it is a behaviour change and you may want it handled another way.Fixes #96