Please expose body when handling HTTP errors in the v1 package #479
Assignees
Comments
|
@krasi-georgiev this one is for you again... :o) |
|
@porridge I just had a quick look and when there is an error the golang http library doesn't return any Body either. so it is one or the other. Meaning even if we expose it will be empty. Did you try it locally to see if Body does include anything when the Do method returns an error? |
|
Hmm, I'm pretty sure it does return a body if a proxy returns a 500
response.
czw., 11 paź 2018, 18:25 użytkownik Krasi Georgiev <[email protected]>
napisał:
… @porridge <https://github.com/porridge> I just had a quick look and when
there is an error the golang http library doesn't return any Body either.
so it is one or the other. Meaning even if we expose it will be empty.
Did you try it locally to see if Body does include anything when the Do
method returns an error?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#479 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAd3zFRbd8iGBzWTdyfVmjY4FG7vEaxPks5uj3DugaJpZM4XXtCz>
.
|
|
I mean I hacked the code a little and checked that it does.
czw., 11 paź 2018, 18:30 użytkownik Marcin Owsiany <[email protected]>
napisał:
… Hmm, I'm pretty sure it does return a body if a proxy returns a 500
response.
czw., 11 paź 2018, 18:25 użytkownik Krasi Georgiev <
***@***.***> napisał:
> @porridge <https://github.com/porridge> I just had a quick look and when
> there is an error the golang http library doesn't return any Body either.
> so it is one or the other. Meaning even if we expose it will be empty.
>
> Did you try it locally to see if Body does include anything when the Do
> method returns an error?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#479 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAd3zFRbd8iGBzWTdyfVmjY4FG7vEaxPks5uj3DugaJpZM4XXtCz>
> .
>
|
|
Maybe it depends on the golang version, I checked the go 1.11 implementation. |
|
one other option is to try with error assertion.
|
just to confirm when you are testing you should test that error is not nil and at the same time the body does contain what you are after.. |
|
I did use 1.11 :-)
czw., 11 paź 2018, 18:36 użytkownik Krasi Georgiev <[email protected]>
napisał:
… Maybe it depends on the golang version, I checked the go 1.11
implementation.
Could you try go 1.11 as well and let me know.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#479 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAd3zPBw4fuHXNyg-PeTro7G-sjKhicJks5uj3N_gaJpZM4XXtCz>
.
|
|
Let me try and send you a test case tomorrow.
czw., 11 paź 2018, 18:52 użytkownik Marcin Owsiany <[email protected]>
napisał:
… I did use 1.11 :-)
czw., 11 paź 2018, 18:36 użytkownik Krasi Georgiev <
***@***.***> napisał:
> Maybe it depends on the golang version, I checked the go 1.11
> implementation.
> Could you try go 1.11 as well and let me know.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#479 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAd3zPBw4fuHXNyg-PeTro7G-sjKhicJks5uj3N_gaJpZM4XXtCz>
> .
>
|
porridge
added a commit
to porridge/client_golang
that referenced
this issue
Oct 12, 2018
This is to illustrate prometheus#479 For this to run, you need to serve an HTTP 500 error on localhost:80. I did this with the following nginx snippet: ``` error_page 500 /500.html; location /api/v1/query { return 500; } ``` and the following file in `/var/www/html`: ``` $ head /var/www/html/500.html important troubleshooting detail $ ``` Then running the test suite shows: ``` --- FAIL: TestReproIssue479 (0.00s) api_test.go:720: querying failed with [bad_response: bad response code 500]. Details: [important troubleshooting detail] FAIL ```
|
@krasi-georgiev Please take a look at porridge@5b54e79 In that case I'm serving a 500 right at the destination server, while this request is for the case of 500 being returned by a proxy, but the mechanics are exactly the same. |
|
what you are suggesting is reasonable. PR is welcome. |
|
Sometimes the body is long and useless, so I would be reluctant to add it
to msg unconditionally.
But I will try to come up with something.
pt., 12 paź 2018, 10:04 użytkownik Krasi Georgiev <[email protected]>
napisał:
… what you are suggesting is reasonable. PR is welcome.
Maybe even add it to
Msg: fmt.Sprintf("response code:%d, body:%s", resp.StatusCode,body),
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#479 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAd3zMSxOwoDNt4rOy4JlBsosowuwgDEks5ukEz7gaJpZM4XXtCz>
.
|
porridge
added a commit
to porridge/client_golang
that referenced
this issue
Oct 23, 2018
porridge
added a commit
to porridge/client_golang
that referenced
this issue
Oct 23, 2018
Fixes: prometheus#479. Signed-off-by: Marcin Owsiany <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The code here does not expose body on encountering an HTTP error. This makes it hard to troubleshoot issues such as a typo in the prometheus server address when using an HTTP proxy. In that case the proxy returns a helpful message in the HTTP error response body, but this library does not propagate this back.
One way to resolve this would be to add a
Detailfield to the Error type and fill it with the stringified body here.The text was updated successfully, but these errors were encountered: