logging: fix the HTTP logger#4041
Conversation
Fix the HTTP logger so that it would take a look at the `url.URL` as well if `r.Host` is empty. Without this, the logging functionality doesn't quite work as Cortex uses `url.URL`: https://github.com/cortexproject/cortex/blob/faf92e8f3dbedfa769fbd6a3ff6e0fbd127694f5/pkg/frontend/downstream_roundtripper.go#L16-L23 Add a test to ensure that this works as expected in both ways. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
yashrsharma44
left a comment
yashrsharma44
left a comment
There was a problem hiding this comment.
There are few questions, feel free to skip if it doesn't make sense
LGTM 💪
| wrapped := httputil.WrapResponseWriterWithStatus(w) | ||
| start := time.Now() | ||
| _, port, err := net.SplitHostPort(r.Host) | ||
| hostPort := r.Host |
There was a problem hiding this comment.
What should we do, if both r.Host and r.URL.Host is set, which one should we display?
There was a problem hiding this comment.
If r.Host is there, it will get priority. We are only using r.URL.Host if r.Host is an empty string.
There was a problem hiding this comment.
I think we can print anyone of them, as long one of them is non-empty.
There was a problem hiding this comment.
From the docs:
// For client requests, the URL's Host specifies the server to
// connect to, while the Request's Host field optionally
// specifies the Host header value to send in the HTTP
// request.
So, we should give priority to r.Host over r.URL.Host because r.Host might be more specific i.e. it's the Host header 😛
There was a problem hiding this comment.
Looks like the current implementation is comprehensive 😄 !
onprem
left a comment
onprem
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the fix, we are seeing these error logs in our deployments as well.
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> Co-authored-by: Yash Sharma <yashrsharma44@gmail.com>
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Thanks! LGTM, just let's fix CHANGELOG but we can fix it anytime during release
| - [#3960](https://github.com/thanos-io/thanos/pull/3960) fix deduplication of equal alerts with different labels | ||
| - [#3937](https://github.com/thanos-io/thanos/pull/3937) Store: Fix race condition in chunk pool. | ||
| - [#4017](https://github.com/thanos-io/thanos/pull/4017) Query Frontend: fix downsampling iterator returning duplicate samples. | ||
| - [#4017](https://github.com/thanos-io/thanos/pull/4041) Logging: fix the HTTP logger |
There was a problem hiding this comment.
Let's mention what is the fix exactly 🤗
| wrapped := httputil.WrapResponseWriterWithStatus(w) | ||
| start := time.Now() | ||
| _, port, err := net.SplitHostPort(r.Host) | ||
| hostPort := r.Host |
4 participants
Fix the HTTP logger so that it would take a look at the
url.URLaswell if
r.Hostis empty. Without this, the logging functionality doesn't quite workas Cortex uses
url.URL:https://github.com/cortexproject/cortex/blob/faf92e8f3dbedfa769fbd6a3ff6e0fbd127694f5/pkg/frontend/downstream_roundtripper.go#L16-L23
Add a test to ensure that this works as expected in both ways.
Without this, the user only sees in the logs:
Signed-off-by: Giedrius Statkevičius giedrius.statkevicius@vinted.com