Pass logger into all middlewares to log warnings on 5xx #328

metalmatze · 2020-04-23T13:12:13Z

Additionally, I've added request IDs to the request context and all middlewares / handlers are going to log with that request ID to correlate log lines for individual requests.

I went through all http.Error calls and made sure to add a log line with it. Everything that's a 4xx response is going to level.Debug and 5xx are going to level.Warn.

/cc @squat @kakkoyun @brancz

Additionally, I've added request IDs to the request context and all middlewares / handlers are going to log with that request ID to correlate log lines for individual requests.

kakkoyun · 2020-04-23T13:42:14Z

pkg/server/forward.go

@@ -134,7 +149,9 @@ func ForwardHandler(logger log.Logger, forwardURL *url.URL) http.HandlerFunc {

 		if resp.StatusCode/100 != 2 {
 			// surfacing upstreams error to our users too
-			http.Error(w, fmt.Errorf("response status code is %s", resp.Status).Error(), resp.StatusCode)
+			msg := fmt.Sprintf("response status code is %s", resp.Status)
+			level.Warn(logger).Log("msg", msg)


This would also log 4xx as well right? Is this what we want?

Good point. I guess for now it's fine. If it becomes too spammy, we might want to add another if in there :)

squat

/lgtm

openshift-bot · 2020-04-23T15:58:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-23T16:11:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-23T17:42:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-23T17:55:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

brancz · 2020-04-23T19:08:45Z

/retest

brancz · 2020-04-23T19:22:52Z

/retest

openshift-bot · 2020-04-23T19:26:16Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-23T20:05:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-23T20:31:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-23T20:44:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-23T22:15:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

kakkoyun · 2020-04-24T06:37:58Z

/retest

metalmatze · 2020-04-24T08:05:12Z

/retest

squat · 2020-04-24T10:16:03Z

/retest

openshift-bot · 2020-04-24T10:22:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-24T10:37:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-24T10:48:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-24T11:01:41Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-24T11:27:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-24T11:41:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-24T11:53:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T06:57:34Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T07:10:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T07:49:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T08:02:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T08:15:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T08:28:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T08:41:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T08:54:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T09:07:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T09:20:34Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T09:33:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T09:46:14Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T09:59:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T10:12:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-25T10:25:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

brancz · 2020-04-25T10:30:48Z

/hold I think something is more fundamentally wrong here :)

kakkoyun · 2020-04-27T16:18:16Z

/retest

metalmatze · 2020-04-27T16:26:49Z

Seems like it was missing a go mod vendor. Thanks for the pointer @kakkoyun.

brancz · 2020-04-27T17:00:58Z

/hold cancel
/lgtm

squat

/lgtm

openshift-ci-robot · 2020-04-27T17:08:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, metalmatze, squat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [brancz,metalmatze,squat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-04-27T18:18:35Z

/retest

Please review the full test history for this PR and help us cut down flakes.

brancz · 2020-04-28T06:10:45Z

This only touches server side telemetry code merging so we can debug our production system :)

Pass logger into all middlewares to log warnings on 5xx

ec58a61

Additionally, I've added request IDs to the request context and all middlewares / handlers are going to log with that request ID to correlate log lines for individual requests.

openshift-ci-robot requested review from brancz, kakkoyun and squat April 23, 2020 13:12

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2020

kakkoyun reviewed Apr 23, 2020

View reviewed changes

squat approved these changes Apr 23, 2020

View reviewed changes

openshift-ci-robot assigned squat Apr 23, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2020

go mod vendor

ccd04fd

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 27, 2020

openshift-ci-robot assigned brancz Apr 27, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2020

squat approved these changes Apr 27, 2020

View reviewed changes

brancz merged commit f2c98f2 into openshift:master Apr 28, 2020

Pass logger into all middlewares to log warnings on 5xx #328

Pass logger into all middlewares to log warnings on 5xx #328

Conversation

metalmatze commented Apr 23, 2020

kakkoyun Apr 23, 2020

Choose a reason for hiding this comment

metalmatze Apr 23, 2020

Choose a reason for hiding this comment

squat left a comment

Choose a reason for hiding this comment

openshift-bot commented Apr 23, 2020

openshift-bot commented Apr 23, 2020

openshift-bot commented Apr 23, 2020

openshift-bot commented Apr 23, 2020

brancz commented Apr 23, 2020

brancz commented Apr 23, 2020

openshift-bot commented Apr 23, 2020

openshift-bot commented Apr 23, 2020

openshift-bot commented Apr 23, 2020

openshift-bot commented Apr 23, 2020

openshift-bot commented Apr 23, 2020

kakkoyun commented Apr 24, 2020

metalmatze commented Apr 24, 2020

squat commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

openshift-bot commented Apr 25, 2020

brancz commented Apr 25, 2020

kakkoyun commented Apr 27, 2020

metalmatze commented Apr 27, 2020

brancz commented Apr 27, 2020

squat left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 27, 2020

openshift-bot commented Apr 27, 2020

brancz commented Apr 28, 2020