Skip to content

Commit fb0577a

Browse files
bharathi-tennetibharathi-tenneti
authored
šŸ› Fix allowed levels for --zap-log-levels flag to align with logr levels (#991)
* Fix zap levels to align with logr * Fix sampling logic for debug levels
1 parent 57e78d0 commit fb0577a

File tree

3 files changed

+187
-19
lines changed

3 files changed

+187
-19
lines changed

ā€Žpkg/log/zap/flags.go

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,16 @@ import (
2929
)
3030

3131
var levelStrings = map[string]zapcore.Level{
32-
"debug": zap.DebugLevel,
33-
"-1": zap.DebugLevel,
34-
"info": zap.InfoLevel,
35-
"0": zap.InfoLevel,
36-
"error": zap.ErrorLevel,
37-
"2": zap.ErrorLevel,
38-
"dpanic": zap.DPanicLevel,
39-
"panic": zap.PanicLevel,
40-
"warn": zap.WarnLevel,
41-
"fatal": zap.FatalLevel,
32+
"debug": zap.DebugLevel,
33+
"info": zap.InfoLevel,
34+
"error": zap.ErrorLevel,
35+
}
36+
37+
// TODO Add level to disable stacktraces.
38+
// https://github.com/kubernetes-sigs/controller-runtime/issues/1035
39+
var stackLevelStrings = map[string]zapcore.Level{
40+
"info": zap.InfoLevel,
41+
"error": zap.ErrorLevel,
4242
}
4343

4444
type encoderFlag struct {
@@ -100,8 +100,9 @@ func (ev *levelFlag) Set(flagValue string) error {
100100
} else {
101101
return fmt.Errorf("invalid log level \"%s\"", flagValue)
102102
}
103+
} else {
104+
ev.setFunc(zap.NewAtomicLevelAt(level))
103105
}
104-
ev.setFunc(zap.NewAtomicLevelAt(level))
105106
ev.value = flagValue
106107
return nil
107108
}
@@ -122,7 +123,7 @@ type stackTraceFlag struct {
122123
var _ pflag.Value = &stackTraceFlag{}
123124

124125
func (ev *stackTraceFlag) Set(flagValue string) error {
125-
level, validLevel := levelStrings[strings.ToLower(flagValue)]
126+
level, validLevel := stackLevelStrings[strings.ToLower(flagValue)]
126127
if !validLevel {
127128
return fmt.Errorf("invalid stacktrace level \"%s\"", flagValue)
128129
}

ā€Žpkg/log/zap/zap.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -181,12 +181,15 @@ func (o *Options) addDefaults() {
181181
lvl := zap.NewAtomicLevelAt(zap.ErrorLevel)
182182
o.StacktraceLevel = &lvl
183183
}
184-
o.ZapOpts = append(o.ZapOpts,
185-
zap.WrapCore(func(core zapcore.Core) zapcore.Core {
186-
return zapcore.NewSampler(core, time.Second, 100, 100)
187-
}))
184+
// Disable sampling for increased Debug levels. Otherwise, this will
185+
// cause index out of bounds errors in the sampling code.
186+
if !o.Level.Enabled(zapcore.Level(-2)) {
187+
o.ZapOpts = append(o.ZapOpts,
188+
zap.WrapCore(func(core zapcore.Core) zapcore.Core {
189+
return zapcore.NewSampler(core, time.Second, 100, 100)
190+
}))
191+
}
188192
}
189-
190193
o.ZapOpts = append(o.ZapOpts, zap.AddStacktrace(o.StacktraceLevel))
191194
}
192195

@@ -215,7 +218,7 @@ func NewRaw(opts ...Opts) *zap.Logger {
215218
// zap-encoder: Zap log encoding ('json' or 'console')
216219
// zap-log-level: Zap Level to configure the verbosity of logging. Can be one of 'debug', 'info', 'error',
217220
// or any integer value > 0 which corresponds to custom debug levels of increasing verbosity")
218-
// zap-stacktrace-level: Zap Level at and above which stacktraces are captured (one of 'warn' or 'error')
221+
// zap-stacktrace-level: Zap Level at and above which stacktraces are captured (one of 'info' or 'error')
219222
func (o *Options) BindFlags(fs *flag.FlagSet) {
220223

221224
// Set Development mode value
@@ -245,7 +248,7 @@ func (o *Options) BindFlags(fs *flag.FlagSet) {
245248
o.StacktraceLevel = fromFlag
246249
}
247250
fs.Var(&stackVal, "zap-stacktrace-level",
248-
"Zap Level at and above which stacktraces are captured (one of 'warn' or 'error')")
251+
"Zap Level at and above which stacktraces are captured (one of 'info', 'error').")
249252
}
250253

251254
// UseFlagOptions configures the logger to use the Options set by parsing zap option flags from the CLI.

ā€Žpkg/log/zap/zap_test.go

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,14 @@ package zap
1919
import (
2020
"bytes"
2121
"encoding/json"
22+
"flag"
2223
"io/ioutil"
24+
"os"
2325

2426
"github.com/go-logr/logr"
2527
. "github.com/onsi/ginkgo"
2628
. "github.com/onsi/gomega"
29+
"go.uber.org/zap/zapcore"
2730
kapi "k8s.io/api/core/v1"
2831
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2932
"k8s.io/apimachinery/pkg/types"
@@ -296,3 +299,164 @@ var _ = Describe("Zap logger setup", func() {
296299
})
297300
})
298301
})
302+
303+
var _ = Describe("Zap log level flag options setup", func() {
304+
var (
305+
fromFlags Options
306+
fs flag.FlagSet
307+
logInfoLevel0 = "info text"
308+
logDebugLevel1 = "debug 1 text"
309+
logDebugLevel2 = "debug 2 text"
310+
logDebugLevel3 = "debug 3 text"
311+
)
312+
313+
BeforeEach(func() {
314+
fromFlags = Options{}
315+
fs = *flag.NewFlagSet(os.Args[0], flag.ExitOnError)
316+
})
317+
318+
Context("with zap-log-level options provided", func() {
319+
It("Should output logs for info and debug zap-log-level.", func() {
320+
args := []string{"--zap-log-level=debug"}
321+
fromFlags.BindFlags(&fs)
322+
err := fs.Parse(args)
323+
Expect(err).ToNot(HaveOccurred())
324+
logOut := new(bytes.Buffer)
325+
326+
logger := New(UseFlagOptions(&fromFlags), WriteTo(logOut))
327+
logger.V(0).Info(logInfoLevel0)
328+
logger.V(1).Info(logDebugLevel1)
329+
330+
outRaw := logOut.Bytes()
331+
332+
Expect(string(outRaw)).Should(ContainSubstring(logInfoLevel0))
333+
Expect(string(outRaw)).Should(ContainSubstring(logDebugLevel1))
334+
335+
})
336+
337+
It("Should output only error logs, otherwise empty logs", func() {
338+
args := []string{"--zap-log-level=error"}
339+
fromFlags.BindFlags(&fs)
340+
err := fs.Parse(args)
341+
Expect(err).ToNot(HaveOccurred())
342+
343+
logOut := new(bytes.Buffer)
344+
345+
logger := New(UseFlagOptions(&fromFlags), WriteTo(logOut))
346+
logger.V(0).Info(logInfoLevel0)
347+
logger.V(1).Info(logDebugLevel1)
348+
349+
outRaw := logOut.Bytes()
350+
351+
Expect(outRaw).To(BeEmpty())
352+
})
353+
354+
})
355+
356+
Context("with zap-log-level with increased verbosity.", func() {
357+
It("Should output debug and info log, with default production mode.", func() {
358+
args := []string{"--zap-log-level=1"}
359+
fromFlags.BindFlags(&fs)
360+
err := fs.Parse(args)
361+
Expect(err).ToNot(HaveOccurred())
362+
logOut := new(bytes.Buffer)
363+
364+
logger := New(UseFlagOptions(&fromFlags), WriteTo(logOut))
365+
logger.V(0).Info(logInfoLevel0)
366+
logger.V(1).Info(logDebugLevel1)
367+
368+
outRaw := logOut.Bytes()
369+
370+
Expect(string(outRaw)).Should(ContainSubstring(logInfoLevel0))
371+
Expect(string(outRaw)).Should(ContainSubstring(logDebugLevel1))
372+
})
373+
374+
It("Should output info and debug logs, with development mode.", func() {
375+
args := []string{"--zap-log-level=1", "--zap-devel=true"}
376+
fromFlags.BindFlags(&fs)
377+
err := fs.Parse(args)
378+
Expect(err).ToNot(HaveOccurred())
379+
logOut := new(bytes.Buffer)
380+
381+
logger := New(UseFlagOptions(&fromFlags), WriteTo(logOut))
382+
logger.V(0).Info(logInfoLevel0)
383+
logger.V(1).Info(logDebugLevel1)
384+
385+
outRaw := logOut.Bytes()
386+
387+
Expect(string(outRaw)).Should(ContainSubstring(logInfoLevel0))
388+
Expect(string(outRaw)).Should(ContainSubstring(logDebugLevel1))
389+
})
390+
391+
It("Should output info, and debug logs with increased verbosity, and with development mode set to true.", func() {
392+
args := []string{"--zap-log-level=3", "--zap-devel=false"}
393+
fromFlags.BindFlags(&fs)
394+
err := fs.Parse(args)
395+
Expect(err).ToNot(HaveOccurred())
396+
logOut := new(bytes.Buffer)
397+
398+
logger := New(UseFlagOptions(&fromFlags), WriteTo(logOut))
399+
logger.V(0).Info(logInfoLevel0)
400+
logger.V(1).Info(logDebugLevel1)
401+
logger.V(2).Info(logDebugLevel2)
402+
logger.V(3).Info(logDebugLevel3)
403+
404+
outRaw := logOut.Bytes()
405+
406+
Expect(string(outRaw)).Should(ContainSubstring(logInfoLevel0))
407+
Expect(string(outRaw)).Should(ContainSubstring(logDebugLevel1))
408+
Expect(string(outRaw)).Should(ContainSubstring(logDebugLevel2))
409+
Expect(string(outRaw)).Should(ContainSubstring(logDebugLevel3))
410+
411+
})
412+
It("Should output info, and debug logs with increased verbosity, and with production mode set to true.", func() {
413+
args := []string{"--zap-log-level=3", "--zap-devel=true"}
414+
fromFlags.BindFlags(&fs)
415+
err := fs.Parse(args)
416+
Expect(err).ToNot(HaveOccurred())
417+
logOut := new(bytes.Buffer)
418+
419+
logger := New(UseFlagOptions(&fromFlags), WriteTo(logOut))
420+
logger.V(0).Info(logInfoLevel0)
421+
logger.V(1).Info(logDebugLevel1)
422+
logger.V(2).Info(logDebugLevel2)
423+
logger.V(3).Info(logDebugLevel3)
424+
425+
outRaw := logOut.Bytes()
426+
427+
Expect(string(outRaw)).Should(ContainSubstring(logInfoLevel0))
428+
Expect(string(outRaw)).Should(ContainSubstring(logDebugLevel1))
429+
Expect(string(outRaw)).Should(ContainSubstring(logDebugLevel2))
430+
Expect(string(outRaw)).Should(ContainSubstring(logDebugLevel3))
431+
432+
})
433+
434+
})
435+
436+
Context("with zap-stacktrace-level options provided", func() {
437+
438+
It("Should output stacktrace at info level, with development mode set to true.", func() {
439+
args := []string{"--zap-stacktrace-level=info", "--zap-devel=true"}
440+
fromFlags.BindFlags(&fs)
441+
err := fs.Parse(args)
442+
Expect(err).ToNot(HaveOccurred())
443+
out := Options{}
444+
UseFlagOptions(&fromFlags)(&out)
445+
446+
Expect(out.StacktraceLevel.Enabled(zapcore.InfoLevel)).To(BeTrue())
447+
})
448+
449+
It("Should output stacktrace at error level, with development mode set to true.", func() {
450+
args := []string{"--zap-stacktrace-level=error", "--zap-devel=true"}
451+
fromFlags.BindFlags(&fs)
452+
err := fs.Parse(args)
453+
Expect(err).ToNot(HaveOccurred())
454+
out := Options{}
455+
UseFlagOptions(&fromFlags)(&out)
456+
457+
Expect(out.StacktraceLevel.Enabled(zapcore.ErrorLevel)).To(BeTrue())
458+
})
459+
460+
})
461+
462+
})

0 commit comments

Comments
Ā (0)