Make DisableRequestSuccessLog configurable. (#284)

- NewLogMiddleware is a public method. Adding a new parameter would make this PR a breaking change one. - However, the behavior is the same: whatever is configured for the existing DisableRequestSuccessLog, it will be used by the log middleware. - Test option to not log successful requests.
grafana · May 22, 2023 · 02fefc0 · 02fefc0
1 parent 145055d
commit 02fefc0
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 15 deletions.
diff --git a/middleware/logging.go b/middleware/logging.go
@@ -14,11 +14,12 @@ import (
 
 // Log middleware logs http requests
 type Log struct {
-	Log                   logging.Interface
-	LogRequestHeaders     bool // LogRequestHeaders true -> dump http headers at debug log level
-	LogRequestAtInfoLevel bool // LogRequestAtInfoLevel true -> log requests at info log level
-	SourceIPs             *SourceIPExtractor
-	HttpHeadersToExclude  map[string]bool
+	Log                      logging.Interface
+	DisableRequestSuccessLog bool
+	LogRequestHeaders        bool // LogRequestHeaders true -> dump http headers at debug log level
+	LogRequestAtInfoLevel    bool // LogRequestAtInfoLevel true -> log requests at info log level
+	SourceIPs                *SourceIPExtractor
+	HttpHeadersToExclude     map[string]bool
 }
 
 var defaultExcludedHeaders = map[string]bool{
@@ -94,20 +95,27 @@ func (l Log) Wrap(next http.Handler) http.Handler {
 
 			return
 		}
-		if 100 <= statusCode && statusCode < 500 || statusCode == http.StatusBadGateway || statusCode == http.StatusServiceUnavailable {
+
+		switch {
+		// success and shouldn't log successful requests.
+		case statusCode >= 200 && statusCode < 300 && l.DisableRequestSuccessLog:
+			return
+
+		case 100 <= statusCode && statusCode < 500 || statusCode == http.StatusBadGateway || statusCode == http.StatusServiceUnavailable:
 			if l.LogRequestAtInfoLevel {
 				requestLog.Infof("%s %s (%d) %s", r.Method, uri, statusCode, time.Since(begin))
-			} else {
-				requestLog.Debugf("%s %s (%d) %s", r.Method, uri, statusCode, time.Since(begin))
-			}
-			if l.LogRequestHeaders && headers != nil {
-				if l.LogRequestAtInfoLevel {
+
+				if l.LogRequestHeaders && headers != nil {
 					requestLog.Infof("ws: %v; %s", IsWSHandshakeRequest(r), string(headers))
-				} else {
-					requestLog.Debugf("ws: %v; %s", IsWSHandshakeRequest(r), string(headers))
 				}
+				return
+			}
+
+			requestLog.Debugf("%s %s (%d) %s", r.Method, uri, statusCode, time.Since(begin))
+			if l.LogRequestHeaders && headers != nil {
+				requestLog.Debugf("ws: %v; %s", IsWSHandshakeRequest(r), string(headers))
 			}
-		} else {
+		default:
 			requestLog.Warnf("%s %s (%d) %s Response: %q ws: %v; %s",
 				r.Method, uri, statusCode, time.Since(begin), buf.Bytes(), IsWSHandshakeRequest(r), headers)
 		}

diff --git a/middleware/logging_test.go b/middleware/logging_test.go
@@ -57,6 +57,55 @@ func TestBadWriteLogging(t *testing.T) {
 	}
 }
 
+func TestDisabledSuccessfulRequestsLogging(t *testing.T) {
+	for _, tc := range []struct {
+		err         error
+		disableLog  bool
+		logContains string
+	}{
+		{
+			err:        nil,
+			disableLog: false,
+		}, {
+			err:         nil,
+			disableLog:  true,
+			logContains: "",
+		},
+	} {
+		buf := bytes.NewBuffer(nil)
+		logrusLogger := logrus.New()
+		logrusLogger.Out = buf
+		logrusLogger.Level = logrus.DebugLevel
+
+		loggingMiddleware := Log{
+			Log:                      logging.Logrus(logrusLogger),
+			DisableRequestSuccessLog: tc.disableLog,
+		}
+
+		handler := func(w http.ResponseWriter, r *http.Request) {
+			io.WriteString(w, "<html><body>Hello World!</body></html>") //nolint:errcheck
+		}
+		loggingHandler := loggingMiddleware.Wrap(http.HandlerFunc(handler))
+
+		req := httptest.NewRequest("GET", "http://example.com/foo", nil)
+		recorder := httptest.NewRecorder()
+
+		w := errorWriter{
+			err: tc.err,
+			w:   recorder,
+		}
+		loggingHandler.ServeHTTP(w, req)
+		content := buf.String()
+
+		if !tc.disableLog {
+			require.Contains(t, content, "GET http://example.com/foo (200)")
+		} else {
+			require.NotContains(t, content, "(200)")
+			require.Empty(t, content)
+		}
+	}
+}
+
 func TestLoggingRequestsAtInfoLevel(t *testing.T) {
 	for _, tc := range []struct {
 		err         error

diff --git a/server/server.go b/server/server.go
@@ -428,12 +428,15 @@ func New(cfg Config) (*Server, error) {
 		}
 	}
 
+	defaultLogMiddleware := middleware.NewLogMiddleware(log, cfg.LogRequestHeaders, cfg.LogRequestAtInfoLevel, sourceIPs, strings.Split(cfg.LogRequestExcludeHeadersList, ","))
+	defaultLogMiddleware.DisableRequestSuccessLog = cfg.DisableRequestSuccessLog
+
 	defaultHTTPMiddleware := []middleware.Interface{
 		middleware.Tracer{
 			RouteMatcher: router,
 			SourceIPs:    sourceIPs,
 		},
-		middleware.NewLogMiddleware(log, cfg.LogRequestHeaders, cfg.LogRequestAtInfoLevel, sourceIPs, strings.Split(cfg.LogRequestExcludeHeadersList, ",")),
+		defaultLogMiddleware,
 		middleware.Instrument{
 			RouteMatcher:     router,
 			Duration:         requestDuration,