From ba7633f2b0914aaae70e835e5da1a535d5c1a7b9 Mon Sep 17 00:00:00 2001 From: Yuri Nikolic Date: Thu, 10 Aug 2023 15:06:28 +0200 Subject: [PATCH] Fixing review findings 2 Signed-off-by: Yuri Nikolic --- CHANGELOG.md | 2 +- log/ratelimit.go | 50 ++++++++++++++++++++++++--------------- log/ratelimit_test.go | 55 ++++++++++++++++++++++++++----------------- 3 files changed, 66 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c787677fd..5ff1a6dd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,7 @@ * [FEATURE] Add support for waiting on the rate limiter using the new `WaitN` method. #279 * [FEATURE] Add `log.BufferedLogger` type. #338 * [FEATURE] Add `flagext.ParseFlagsAndArguments()` and `flagext.ParseFlagsWithoutArguments()` utilities. #341 -* [FEATURE] Add `log.RateLimitedLogger` for limiting the rate of logging. The `rate_limit_logger_discarded_log_lines_total` metrics traces the total number of discarded log lines per level. #352 +* [FEATURE] Add `log.RateLimitedLogger` for limiting the rate of logging. The `logger_rate_limit_discarded_log_lines_total` metrics traces the total number of discarded log lines per level. #352 * [ENHANCEMENT] Add configuration to customize backoff for the gRPC clients. * [ENHANCEMENT] Use `SecretReader` interface to fetch secrets when configuring TLS. #274 * [ENHANCEMENT] Add middleware package. #38 diff --git a/log/ratelimit.go b/log/ratelimit.go index 8d41ba155..9fc592f4a 100644 --- a/log/ratelimit.go +++ b/log/ratelimit.go @@ -17,21 +17,27 @@ type RateLimitedLogger struct { next Interface limiter *rate.Limiter - discardedLogLinesCounter *prometheus.CounterVec + discardedInfoLogLinesCounter prometheus.Counter + discardedDebugLogLinesCounter prometheus.Counter + discardedWarnLogLinesCounter prometheus.Counter + discardedErrorLogLinesCounter prometheus.Counter } // NewRateLimitedLogger returns a logger.Interface that is limited to the given number of logs per second, // with the given burst size. func NewRateLimitedLogger(logger Interface, logsPerSecond rate.Limit, burstSize int, reg prometheus.Registerer) Interface { discardedLogLinesCounter := promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ - Name: "rate_limit_logger_discarded_log_lines_total", + Name: "logger_rate_limit_discarded_log_lines_total", Help: "Total number of discarded log lines per level.", }, []string{"level"}) return &RateLimitedLogger{ - next: logger, - limiter: rate.NewLimiter(logsPerSecond, burstSize), - discardedLogLinesCounter: discardedLogLinesCounter, + next: logger, + limiter: rate.NewLimiter(logsPerSecond, burstSize), + discardedInfoLogLinesCounter: discardedLogLinesCounter.WithLabelValues(infoLevel), + discardedDebugLogLinesCounter: discardedLogLinesCounter.WithLabelValues(debugLevel), + discardedWarnLogLinesCounter: discardedLogLinesCounter.WithLabelValues(warnLevel), + discardedErrorLogLinesCounter: discardedLogLinesCounter.WithLabelValues(errorLevel), } } @@ -39,7 +45,7 @@ func (l *RateLimitedLogger) Debugf(format string, args ...interface{}) { if l.limiter.Allow() { l.next.Debugf(format, args...) } else { - l.discardedLogLinesCounter.WithLabelValues(debugLevel).Inc() + l.discardedDebugLogLinesCounter.Inc() } } @@ -47,7 +53,7 @@ func (l *RateLimitedLogger) Debugln(args ...interface{}) { if l.limiter.Allow() { l.next.Debugln(args...) } else { - l.discardedLogLinesCounter.WithLabelValues(debugLevel).Inc() + l.discardedDebugLogLinesCounter.Inc() } } @@ -55,7 +61,7 @@ func (l *RateLimitedLogger) Infof(format string, args ...interface{}) { if l.limiter.Allow() { l.next.Infof(format, args...) } else { - l.discardedLogLinesCounter.WithLabelValues(infoLevel).Inc() + l.discardedInfoLogLinesCounter.Inc() } } @@ -63,7 +69,7 @@ func (l *RateLimitedLogger) Infoln(args ...interface{}) { if l.limiter.Allow() { l.next.Infoln(args...) } else { - l.discardedLogLinesCounter.WithLabelValues(infoLevel).Inc() + l.discardedInfoLogLinesCounter.Inc() } } @@ -71,7 +77,7 @@ func (l *RateLimitedLogger) Errorf(format string, args ...interface{}) { if l.limiter.Allow() { l.next.Errorf(format, args...) } else { - l.discardedLogLinesCounter.WithLabelValues(errorLevel).Inc() + l.discardedErrorLogLinesCounter.Inc() } } @@ -79,7 +85,7 @@ func (l *RateLimitedLogger) Errorln(args ...interface{}) { if l.limiter.Allow() { l.next.Errorln(args...) } else { - l.discardedLogLinesCounter.WithLabelValues(errorLevel).Inc() + l.discardedErrorLogLinesCounter.Inc() } } @@ -87,7 +93,7 @@ func (l *RateLimitedLogger) Warnf(format string, args ...interface{}) { if l.limiter.Allow() { l.next.Warnf(format, args...) } else { - l.discardedLogLinesCounter.WithLabelValues(warnLevel).Inc() + l.discardedWarnLogLinesCounter.Inc() } } @@ -95,22 +101,28 @@ func (l *RateLimitedLogger) Warnln(args ...interface{}) { if l.limiter.Allow() { l.next.Warnln(args...) } else { - l.discardedLogLinesCounter.WithLabelValues(warnLevel).Inc() + l.discardedWarnLogLinesCounter.Inc() } } func (l *RateLimitedLogger) WithField(key string, value interface{}) Interface { return &RateLimitedLogger{ - next: l.next.WithField(key, value), - limiter: l.limiter, - discardedLogLinesCounter: l.discardedLogLinesCounter, + next: l.next.WithField(key, value), + limiter: l.limiter, + discardedInfoLogLinesCounter: l.discardedInfoLogLinesCounter, + discardedDebugLogLinesCounter: l.discardedDebugLogLinesCounter, + discardedWarnLogLinesCounter: l.discardedWarnLogLinesCounter, + discardedErrorLogLinesCounter: l.discardedErrorLogLinesCounter, } } func (l *RateLimitedLogger) WithFields(f Fields) Interface { return &RateLimitedLogger{ - next: l.next.WithFields(f), - limiter: l.limiter, - discardedLogLinesCounter: l.discardedLogLinesCounter, + next: l.next.WithFields(f), + limiter: l.limiter, + discardedInfoLogLinesCounter: l.discardedInfoLogLinesCounter, + discardedDebugLogLinesCounter: l.discardedDebugLogLinesCounter, + discardedWarnLogLinesCounter: l.discardedWarnLogLinesCounter, + discardedErrorLogLinesCounter: l.discardedErrorLogLinesCounter, } } diff --git a/log/ratelimit_test.go b/log/ratelimit_test.go index 37dec770d..01c3e93ff 100644 --- a/log/ratelimit_test.go +++ b/log/ratelimit_test.go @@ -23,7 +23,7 @@ func TestRateLimitedLoggerLogs(t *testing.T) { assert.Equal(t, 1, c.count) logContains := []string{"error", "Error will be logged"} - c.verify(t, logContains) + c.assertContains(t, logContains) } func TestRateLimitedLoggerLimits(t *testing.T) { @@ -34,47 +34,51 @@ func TestRateLimitedLoggerLimits(t *testing.T) { r.Errorln("error 1 will be logged") assert.Equal(t, 1, c.count) - c.verify(t, []string{"error", "error 1 will be logged"}) + c.assertContains(t, []string{"error", "error 1 will be logged"}) r.Infoln("info 1 will be logged") assert.Equal(t, 2, c.count) - c.verify(t, []string{"info", "info 1 will be logged"}) + c.assertContains(t, []string{"info", "info 1 will be logged"}) r.Debugln("debug 1 will be discarded") assert.Equal(t, 2, c.count) + c.assertNotContains(t, "debug 1 will be discarded") r.Warnln("warning 1 will be discarded") assert.Equal(t, 2, c.count) + c.assertNotContains(t, "warning 1 will be discarded") require.NoError(t, testutil.GatherAndCompare(reg, strings.NewReader(` - # HELP rate_limit_logger_discarded_log_lines_total Total number of discarded log lines per level. - # TYPE rate_limit_logger_discarded_log_lines_total counter - rate_limit_logger_discarded_log_lines_total{level="debug"} 1 - rate_limit_logger_discarded_log_lines_total{level="warning"} 1 + # HELP logger_rate_limit_discarded_log_lines_total Total number of discarded log lines per level. + # TYPE logger_rate_limit_discarded_log_lines_total counter + logger_rate_limit_discarded_log_lines_total{level="debug"} 1 + logger_rate_limit_discarded_log_lines_total{level="warning"} 1 `))) // we wait 1 second, so the next group of lines can be logged time.Sleep(time.Second) r.Debugln("debug 2 will be logged") assert.Equal(t, 3, c.count) - c.verify(t, []string{"debug", "debug 2 will be logged"}) + c.assertContains(t, []string{"debug", "debug 2 will be logged"}) r.Infoln("info 2 will be logged") assert.Equal(t, 4, c.count) - c.verify(t, []string{"info", "info 2 will be logged"}) + c.assertContains(t, []string{"info", "info 2 will be logged"}) r.Errorln("error 2 will be discarded") assert.Equal(t, 4, c.count) + c.assertNotContains(t, "error 2 will be discarded") r.Warnln("warning 2 will be discarded") assert.Equal(t, 4, c.count) + c.assertNotContains(t, "warning 2 will be discarded") require.NoError(t, testutil.GatherAndCompare(reg, strings.NewReader(` - # HELP rate_limit_logger_discarded_log_lines_total Total number of discarded log lines per level. - # TYPE rate_limit_logger_discarded_log_lines_total counter - rate_limit_logger_discarded_log_lines_total{level="debug"} 1 - rate_limit_logger_discarded_log_lines_total{level="error"} 1 - rate_limit_logger_discarded_log_lines_total{level="warning"} 2 + # HELP logger_rate_limit_discarded_log_lines_total Total number of discarded log lines per level. + # TYPE logger_rate_limit_discarded_log_lines_total counter + logger_rate_limit_discarded_log_lines_total{level="debug"} 1 + logger_rate_limit_discarded_log_lines_total{level="error"} 1 + logger_rate_limit_discarded_log_lines_total{level="warning"} 2 `))) } @@ -87,19 +91,24 @@ func TestRateLimitedLoggerWithFields(t *testing.T) { loggerWithFields.Errorln("Error will be logged") assert.Equal(t, 1, c.count) - c.verify(t, []string{"key", "value", "error", "Error will be logged"}) + c.assertContains(t, []string{"key", "value", "error", "Error will be logged"}) logger.Infoln("Info will not be logged") + c.assertNotContains(t, "Info will not be logged") + loggerWithFields.Debugln("Debug will not be logged") + c.assertNotContains(t, "Debug will not be logged") + loggerWithFields.Warnln("Warning will not be logged") + c.assertNotContains(t, "Warning will not be logged") assert.Equal(t, 1, c.count) require.NoError(t, testutil.GatherAndCompare(reg, strings.NewReader(` - # HELP rate_limit_logger_discarded_log_lines_total Total number of discarded log lines per level. - # TYPE rate_limit_logger_discarded_log_lines_total counter - rate_limit_logger_discarded_log_lines_total{level="info"} 1 - rate_limit_logger_discarded_log_lines_total{level="debug"} 1 - rate_limit_logger_discarded_log_lines_total{level="warning"} 1 + # HELP logger_rate_limit_discarded_log_lines_total Total number of discarded log lines per level. + # TYPE logger_rate_limit_discarded_log_lines_total counter + logger_rate_limit_discarded_log_lines_total{level="info"} 1 + logger_rate_limit_discarded_log_lines_total{level="debug"} 1 + logger_rate_limit_discarded_log_lines_total{level="warning"} 1 `))) } @@ -159,12 +168,16 @@ func (c *counterLogger) WithFields(fields Fields) Interface { return c } -func (c *counterLogger) verify(t *testing.T, logContains []string) { +func (c *counterLogger) assertContains(t *testing.T, logContains []string) { for _, content := range logContains { require.True(t, bytes.Contains(c.buf.Bytes(), []byte(content))) } } +func (c *counterLogger) assertNotContains(t *testing.T, content string) { + require.False(t, bytes.Contains(c.buf.Bytes(), []byte(content))) +} + func newCounterLogger(buf *bytes.Buffer) *counterLogger { logrusLogger := logrus.New() logrusLogger.Out = buf