fix(httpclient): Fixed logger transport config (#143)

ankorstore · Mar 14, 2024 · a4438a4 · a4438a4
1 parent 35b3319
commit a4438a4
Show file tree

Hide file tree

Showing 4 changed files with 88 additions and 46 deletions.
diff --git a/httpclient/transport/logger.go b/httpclient/transport/logger.go
@@ -65,19 +65,19 @@ func (t *LoggerTransport) Base() http.RoundTripper {
 func (t *LoggerTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	logger := log.CtxLogger(req.Context())
 
-	reqEvt := logger.WithLevel(t.config.LogRequestLevel)
-
 	if t.config.LogRequest {
+		reqEvt := logger.WithLevel(t.config.LogRequestLevel)
+
 		reqDump, err := httputil.DumpRequestOut(req, t.config.LogRequestBody)
 		if err == nil {
 			reqEvt.Bytes("request", reqDump)
 		}
-	}
 
-	reqEvt.
-		Str("method", req.Method).
-		Str("url", req.URL.String()).
-		Msg("http client request")
+		reqEvt.
+			Str("method", req.Method).
+			Str("url", req.URL.String()).
+			Msg("http client request")
+	}
 
 	start := time.Now()
 	resp, err := t.transport.RoundTrip(req)
@@ -89,34 +89,34 @@ func (t *LoggerTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 		return resp, err
 	}
 
-	var respEvt *zerolog.Event
-
-	if t.config.LogResponseLevelFromResponseCode {
-		switch {
-		case resp.StatusCode >= http.StatusBadRequest && resp.StatusCode < http.StatusInternalServerError:
-			respEvt = logger.Warn()
-		case resp.StatusCode >= http.StatusInternalServerError:
-			respEvt = logger.Error()
-		default:
+	if t.config.LogResponse {
+		var respEvt *zerolog.Event
+
+		if t.config.LogResponseLevelFromResponseCode {
+			switch {
+			case resp.StatusCode >= http.StatusBadRequest && resp.StatusCode < http.StatusInternalServerError:
+				respEvt = logger.Warn()
+			case resp.StatusCode >= http.StatusInternalServerError:
+				respEvt = logger.Error()
+			default:
+				respEvt = logger.WithLevel(t.config.LogResponseLevel)
+			}
+		} else {
 			respEvt = logger.WithLevel(t.config.LogResponseLevel)
 		}
-	} else {
-		respEvt = logger.WithLevel(t.config.LogResponseLevel)
-	}
 
-	if t.config.LogResponse {
 		respDump, err := httputil.DumpResponse(resp, t.config.LogResponseBody)
 		if err == nil {
 			respEvt.Bytes("response", respDump)
 		}
-	}
 
-	respEvt.
-		Str("method", resp.Request.Method).
-		Str("url", resp.Request.URL.String()).
-		Int("code", resp.StatusCode).
-		Str("latency", latency).
-		Msg("http client response")
+		respEvt.
+			Str("method", resp.Request.Method).
+			Str("url", resp.Request.URL.String()).
+			Int("code", resp.StatusCode).
+			Str("latency", latency).
+			Msg("http client response")
+	}
 
 	return resp, err
 }
diff --git a/httpclient/transport/logger_test.go b/httpclient/transport/logger_test.go
@@ -72,14 +72,14 @@ func TestLoggerTransportRoundTrip(t *testing.T) {
 
 	assert.Equal(t, http.StatusNoContent, resp.StatusCode)
 
-	logtest.AssertHasLogRecord(t, logBuffer, map[string]interface{}{
+	logtest.AssertHasNotLogRecord(t, logBuffer, map[string]interface{}{
 		"level":   "info",
 		"method":  "GET",
 		"url":     server.URL,
 		"message": "http client request",
 	})
 
-	logtest.AssertHasLogRecord(t, logBuffer, map[string]interface{}{
+	logtest.AssertHasNotLogRecord(t, logBuffer, map[string]interface{}{
 		"level":   "info",
 		"url":     server.URL,
 		"code":    http.StatusNoContent,
@@ -212,6 +212,48 @@ func TestLoggerTransportRoundTripWithConfig(t *testing.T) {
 		"response": `{"output":"error"}`,
 		"message":  "http client response",
 	})
+
+	// other transport with response level logging
+	trans2 := transport.NewLoggerTransportWithConfig(nil, &transport.LoggerTransportConfig{
+		LogRequest:                       true,
+		LogResponse:                      true,
+		LogRequestBody:                   true,
+		LogResponseBody:                  true,
+		LogRequestLevel:                  zerolog.DebugLevel,
+		LogResponseLevel:                 zerolog.WarnLevel,
+		LogResponseLevelFromResponseCode: false,
+	})
+
+	// 404 response
+	data = []byte(`{"input":"data"}`)
+	req = httptest.NewRequest(http.MethodPost, server.URL, bytes.NewBuffer(data))
+	req.Header.Add("expected-response-code", "404")
+	req.Header.Add("expected-response-body", `{"output":"not found"}`)
+	req = req.WithContext(logger.WithContext(context.Background()))
+
+	resp, err = trans2.RoundTrip(req)
+	assert.NoError(t, err)
+
+	err = resp.Body.Close()
+	assert.NoError(t, err)
+
+	assert.Equal(t, http.StatusNotFound, resp.StatusCode)
+
+	logtest.AssertContainLogRecord(t, logBuffer, map[string]interface{}{
+		"level":   "debug",
+		"method":  "POST",
+		"url":     server.URL,
+		"request": `{"input":"data"}`,
+		"message": "http client request",
+	})
+
+	logtest.AssertContainLogRecord(t, logBuffer, map[string]interface{}{
+		"level":    "warn",
+		"url":      server.URL,
+		"code":     http.StatusNotFound,
+		"response": `{"output":"not found"}`,
+		"message":  "http client response",
+	})
 }
 
 func TestLoggerTransportRoundTripWithFailure(t *testing.T) {
@@ -241,7 +283,7 @@ func TestLoggerTransportRoundTripWithFailure(t *testing.T) {
 	assert.Error(t, err)
 	assert.Equal(t, "custom http error", err.Error())
 
-	logtest.AssertHasLogRecord(t, logBuffer, map[string]interface{}{
+	logtest.AssertHasNotLogRecord(t, logBuffer, map[string]interface{}{
 		"level":   "info",
 		"method":  "GET",
 		"url":     server.URL,

diff --git a/httpclient/transport/metrics.go b/httpclient/transport/metrics.go
@@ -10,8 +10,8 @@ import (
 )
 
 const (
-	HttpClientMetricsRequestsCount    = "client_requests_total"
-	HttpClientMetricsRequestsDuration = "client_requests_duration_seconds"
+	HttpClientMetricsRequestsCount    = "httpclient_requests_total"
+	HttpClientMetricsRequestsDuration = "httpclient_requests_duration_seconds"
 )
 
 // MetricsTransport is a wrapper around [http.RoundTripper] with some [MetricsTransportConfig] configuration.

diff --git a/httpclient/transport/metrics_test.go b/httpclient/transport/metrics_test.go
@@ -39,17 +39,17 @@ func TestMetricsTransportRoundTrip(t *testing.T) {
 	// requests counter assertions
 	expectedCounterMetric := fmt.Sprintf(
 		`
-			# HELP client_requests_total Number of performed HTTP requests
-			# TYPE client_requests_total counter
-			client_requests_total{host="%s",method="GET",path="",status="5xx"} 1
+			# HELP httpclient_requests_total Number of performed HTTP requests
+			# TYPE httpclient_requests_total counter
+			httpclient_requests_total{host="%s",method="GET",path="",status="5xx"} 1
 		`,
 		server.URL,
 	)
 
 	err = testutil.GatherAndCompare(
 		prometheus.DefaultGatherer,
 		strings.NewReader(expectedCounterMetric),
-		"client_requests_total",
+		"httpclient_requests_total",
 	)
 	assert.NoError(t, err)
 }
@@ -107,11 +107,11 @@ func TestMetricsTransportRoundTripWithBaseAndConfig(t *testing.T) {
 	// requests counter assertions
 	expectedCounterMetric := fmt.Sprintf(
 		`
-			# HELP foo_bar_client_requests_total Number of performed HTTP requests
-			# TYPE foo_bar_client_requests_total counter
-    		foo_bar_client_requests_total{host="%s",method="GET",path="",status="204"} 1
-    		foo_bar_client_requests_total{host="%s",method="GET",path="/foo/4/baz",status="204"} 1
-    		foo_bar_client_requests_total{host="%s",method="GET",path="/foo/{fooId}/bar?page={pageId}",status="204"} 3
+			# HELP foo_bar_httpclient_requests_total Number of performed HTTP requests
+			# TYPE foo_bar_httpclient_requests_total counter
+    		foo_bar_httpclient_requests_total{host="%s",method="GET",path="",status="204"} 1
+    		foo_bar_httpclient_requests_total{host="%s",method="GET",path="/foo/4/baz",status="204"} 1
+    		foo_bar_httpclient_requests_total{host="%s",method="GET",path="/foo/{fooId}/bar?page={pageId}",status="204"} 3
 		`,
 		server.URL,
 		server.URL,
@@ -121,7 +121,7 @@ func TestMetricsTransportRoundTripWithBaseAndConfig(t *testing.T) {
 	err := testutil.GatherAndCompare(
 		registry,
 		strings.NewReader(expectedCounterMetric),
-		"foo_bar_client_requests_total",
+		"foo_bar_httpclient_requests_total",
 	)
 	assert.NoError(t, err)
 }
@@ -161,17 +161,17 @@ func TestMetricsTransportRoundTripWithFailure(t *testing.T) {
 	// requests counter assertions
 	expectedCounterMetric := fmt.Sprintf(
 		`
-			# HELP foo_bar_client_requests_total Number of performed HTTP requests
-			# TYPE foo_bar_client_requests_total counter
-			foo_bar_client_requests_total{host="%s",method="GET",path="",status="error"} 1
+			# HELP foo_bar_httpclient_requests_total Number of performed HTTP requests
+			# TYPE foo_bar_httpclient_requests_total counter
+			foo_bar_httpclient_requests_total{host="%s",method="GET",path="",status="error"} 1
 		`,
 		server.URL,
 	)
 
 	err = testutil.GatherAndCompare(
 		registry,
 		strings.NewReader(expectedCounterMetric),
-		"foo_bar_client_requests_total",
+		"foo_bar_httpclient_requests_total",
 	)
 	assert.NoError(t, err)
 }