fixup! ✨ util: Warning handler that discards messages that match a regular expression

Remove deduplication. As a side-effect, simplify the implementation.

Author: dlipovetsky

util/apiwarnings/handler.go

@@ -18,68 +18,34 @@ package apiwarnings
 
 import (
 	"regexp"
-	"sync"
 
 	"github.com/go-logr/logr"
 )
 
-// DiscardMatchingHandlerOptions configures the handler created with
-// NewDiscardMatchingHandler.
-type DiscardMatchingHandlerOptions struct {
-	// Deduplicate indicates a given warning message should only be written once.
-	// Setting this to true in a long-running process handling many warnings can
-	// result in increased memory use.
-	Deduplicate bool
+// DiscardMatchingHandler is a handler that discards API server warnings
+// whose message matches any user-defined regular expressions, but otherwise
+// logs warnings to the user-defined logger.
+type DiscardMatchingHandler struct {
+	// Logger is used to log responses with the warning header.
+	Logger logr.Logger
 
 	// Expressions is a slice of regular expressions used to discard warnings.
 	// If the warning message matches any expression, it is not logged.
 	Expressions []regexp.Regexp
 }
 
-// NewDiscardMatchingHandler initializes and returns a new DiscardMatchingHandler.
-func NewDiscardMatchingHandler(l logr.Logger, opts DiscardMatchingHandlerOptions) *DiscardMatchingHandler {
-	h := &DiscardMatchingHandler{logger: l, opts: opts}
-	if opts.Deduplicate {
-		h.logged = map[string]struct{}{}
-	}
-	return h
-}
-
-// DiscardMatchingHandler is a handler that discards API server warnings
-// whose message matches any user-defined regular expressions.
-type DiscardMatchingHandler struct {
-	// logger is used to log responses with the warning header
-	logger logr.Logger
-	// opts contain options controlling warning output
-	opts DiscardMatchingHandlerOptions
-	// loggedLock guards logged
-	loggedLock sync.Mutex
-	// used to keep track of already logged messages
-	// and help in de-duplication.
-	logged map[string]struct{}
-}
-
 // HandleWarningHeader handles logging for responses from API server that are
 // warnings with code being 299 and uses a logr.Logger for its logging purposes.
 func (h *DiscardMatchingHandler) HandleWarningHeader(code int, _, message string) {
 	if code != 299 || message == "" {
 		return
 	}
 
-	for _, exp := range h.opts.Expressions {
+	for _, exp := range h.Expressions {
 		if exp.MatchString(message) {
 			return
 		}
 	}
 
-	if h.opts.Deduplicate {
-		h.loggedLock.Lock()
-		defer h.loggedLock.Unlock()
-
-		if _, alreadyLogged := h.logged[message]; alreadyLogged {
-			return
-		}
-		h.logged[message] = struct{}{}
-	}
-	h.logger.Info(message)
+	h.Logger.Info(message)
 }

util/apiwarnings/handler_test.go

@@ -26,63 +26,65 @@ import (
 
 func TestDiscardMatchingHandler(t *testing.T) {
 	tests := []struct {
-		name       string
-		opts       DiscardMatchingHandlerOptions
-		code       int
-		message    string
-		wantLogged bool
+		name        string
+		code        int
+		message     string
+		expressions []regexp.Regexp
+		wantLogged  bool
 	}{
 		{
-			name:    "log, if warning does not match any expression",
-			code:    299,
-			message: "non-matching warning",
-			opts: DiscardMatchingHandlerOptions{
-				Expressions: []regexp.Regexp{},
-			},
+			name:       "log, if no expressions are defined",
+			code:       299,
+			message:    "non-matching warning",
 			wantLogged: true,
 		},
+		{
+			name:        "log, if warning does not match any expression",
+			code:        299,
+			message:     "non-matching warning",
+			expressions: []regexp.Regexp{},
+			wantLogged:  true,
+		},
 		{
 			name:    "do not log, if warning matches at least one expression",
 			code:    299,
 			message: "matching warning",
-			opts: DiscardMatchingHandlerOptions{
-				Expressions: []regexp.Regexp{
-					*regexp.MustCompile("^matching.*"),
-				},
+			expressions: []regexp.Regexp{
+				*regexp.MustCompile("^matching.*"),
 			},
 			wantLogged: false,
 		},
 		{
-			name:    "do not log, if code is not 299",
-			code:    0,
-			message: "",
-			opts: DiscardMatchingHandlerOptions{
-				Expressions: []regexp.Regexp{},
-			},
-			wantLogged: false,
+			name:        "do not log, if code is not 299",
+			code:        0,
+			message:     "",
+			expressions: []regexp.Regexp{},
+			wantLogged:  false,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 			logged := false
-			h := NewDiscardMatchingHandler(
-				funcr.New(func(_, _ string) {
+			h := DiscardMatchingHandler{
+				Logger: funcr.New(func(_, _ string) {
 					logged = true
 				},
 					funcr.Options{},
 				),
-				tt.opts,
-			)
+				Expressions: tt.expressions,
+			}
 			h.HandleWarningHeader(tt.code, "", tt.message)
 			g.Expect(logged).To(Equal(tt.wantLogged))
 		})
 	}
 }
 
-func TestDiscardMatchingHandler_uninitialized(t *testing.T) {
+func TestDiscardMatchingHandler_NilLogger(t *testing.T) {
 	g := NewWithT(t)
-	h := DiscardMatchingHandler{}
+	h := DiscardMatchingHandler{
+		// Logger is nil
+	}
 	g.Expect(func() {
 		h.HandleWarningHeader(0, "", "")
 	}).ToNot(Panic())