From 844c76195f51735ff7fae5a92789c0a181a1f924 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Thu, 8 Jul 2021 15:58:55 +0100 Subject: [PATCH 1/3] Static Analysis: Fix the static analysis Lint the code to remove any bit rot. --- benchmarks_test.go | 13 +++++++++---- context.go | 2 +- context_test.go | 4 ++-- example/main.go | 6 ++++-- export_test.go | 2 +- global.go | 4 +++- logging_test.go | 3 ++- module.go | 4 +--- 8 files changed, 23 insertions(+), 15 deletions(-) diff --git a/benchmarks_test.go b/benchmarks_test.go index e0b55bd..e3ac851 100644 --- a/benchmarks_test.go +++ b/benchmarks_test.go @@ -28,7 +28,7 @@ func (s *BenchmarksSuite) SetUpTest(c *gc.C) { func (s *BenchmarksSuite) BenchmarkLoggingNoWriters(c *gc.C) { // No writers - loggo.RemoveWriter("test") + _, _ = loggo.RemoveWriter("test") for i := 0; i < c.N; i++ { s.logger.Warningf("just a simple warning for %d", i) } @@ -36,7 +36,7 @@ func (s *BenchmarksSuite) BenchmarkLoggingNoWriters(c *gc.C) { func (s *BenchmarksSuite) BenchmarkLoggingNoWritersNoFormat(c *gc.C) { // No writers - loggo.RemoveWriter("test") + _, _ = loggo.RemoveWriter("test") for i := 0; i < c.N; i++ { s.logger.Warningf("just a simple warning") } @@ -68,7 +68,10 @@ func (s *BenchmarksSuite) BenchmarkLoggingDiskWriterNoMessages(c *gc.C) { // Change the log level writer, err := loggo.RemoveWriter("testfile") c.Assert(err, gc.IsNil) - loggo.RegisterWriter("testfile", loggo.NewMinimumLevelWriter(writer, loggo.WARNING)) + + err = loggo.RegisterWriter("testfile", loggo.NewMinimumLevelWriter(writer, loggo.WARNING)) + c.Assert(err, gc.IsNil) + msg := "just a simple warning for %d" for i := 0; i < c.N; i++ { s.logger.Debugf(msg, i) @@ -84,6 +87,7 @@ func (s *BenchmarksSuite) BenchmarkLoggingDiskWriterNoMessagesLogLevel(c *gc.C) defer logFile.Close() // Change the log level s.logger.SetLogLevel(loggo.WARNING) + msg := "just a simple warning for %d" for i := 0; i < c.N; i++ { s.logger.Debugf(msg, i) @@ -95,9 +99,10 @@ func (s *BenchmarksSuite) BenchmarkLoggingDiskWriterNoMessagesLogLevel(c *gc.C) } func (s *BenchmarksSuite) setupTempFileWriter(c *gc.C) *os.File { - loggo.RemoveWriter("test") + _, _ = loggo.RemoveWriter("test") logFile, err := ioutil.TempFile(c.MkDir(), "loggo-test") c.Assert(err, gc.IsNil) + writer := loggo.NewSimpleWriter(logFile, loggo.DefaultFormatter) err = loggo.RegisterWriter("testfile", writer) c.Assert(err, gc.IsNil) diff --git a/context.go b/context.go index 60b4f7c..c49979b 100644 --- a/context.go +++ b/context.go @@ -86,7 +86,7 @@ func (c *Context) getLoggerModule(name string, labels []string) *module { if found { return impl } - parentName := "" + var parentName string if i := strings.LastIndex(name, "."); i >= 0 { parentName = name[0:i] } diff --git a/context_test.go b/context_test.go index ac19139..0e2afb4 100644 --- a/context_test.go +++ b/context_test.go @@ -104,7 +104,8 @@ func (*ContextSuite) TestNewContextNoWriter(c *gc.C) { func (*ContextSuite) newContextWithTestWriter(c *gc.C, level loggo.Level) (*loggo.Context, *loggo.TestWriter) { writer := &loggo.TestWriter{} context := loggo.NewContext(level) - context.AddWriter("test", writer) + err := context.AddWriter("test", writer) + c.Assert(err, gc.IsNil) return context, writer } @@ -420,7 +421,6 @@ func (*ContextSuite) TestWriter(c *gc.C) { c.Check(context.Writer("second"), gc.Equals, second) c.Check(first, gc.Not(gc.Equals), second) - } type writer struct { diff --git a/example/main.go b/example/main.go index e6a23b0..c10526d 100644 --- a/example/main.go +++ b/example/main.go @@ -2,18 +2,20 @@ package main import ( "fmt" + "log" "os" "github.com/juju/loggo" ) -var logger = loggo.GetLogger("main") var rootLogger = loggo.GetLogger("") func main() { args := os.Args if len(args) > 1 { - loggo.ConfigureLoggers(args[1]) + if err := loggo.ConfigureLoggers(args[1]); err != nil { + log.Fatal(err) + } } else { fmt.Println("Add a parameter to configure the logging:") fmt.Println("E.g. \"=INFO;first=TRACE\"") diff --git a/export_test.go b/export_test.go index 8810064..3bae1a4 100644 --- a/export_test.go +++ b/export_test.go @@ -16,5 +16,5 @@ func (c *Context) WriterNames() []string { func ResetDefaultContext() { ResetLogging() - DefaultContext().AddWriter(DefaultWriterName, defaultWriter()) + _ = DefaultContext().AddWriter(DefaultWriterName, defaultWriter()) } diff --git a/global.go b/global.go index 054ea39..054c60b 100644 --- a/global.go +++ b/global.go @@ -9,7 +9,9 @@ var ( func newDefaultContxt() *Context { ctx := NewContext(WARNING) - ctx.AddWriter(DefaultWriterName, defaultWriter()) + if err := ctx.AddWriter(DefaultWriterName, defaultWriter()); err != nil { + panic(err) + } return ctx } diff --git a/logging_test.go b/logging_test.go index 052b2d2..1e4a51a 100644 --- a/logging_test.go +++ b/logging_test.go @@ -26,7 +26,8 @@ var _ = gc.Suite(&LoggingSuite{Labels: []string{"ONE", "TWO"}}) func (s *LoggingSuite) SetUpTest(c *gc.C) { s.writer = &writer{} s.context = loggo.NewContext(loggo.TRACE) - s.context.AddWriter("test", s.writer) + err := s.context.AddWriter("test", s.writer) + c.Assert(err, gc.IsNil) s.logger = s.context.GetLogger("test", s.Labels...) } diff --git a/module.go b/module.go index d5d05f6..9e49378 100644 --- a/module.go +++ b/module.go @@ -5,9 +5,7 @@ package loggo // Do not change rootName: modules.resolve() will misbehave if it isn't "". const ( - rootString = "" - defaultRootLevel = WARNING - defaultLevel = UNSPECIFIED + rootString = "" ) type module struct { From 2d422324b4de006fc3e63b2d1976c6cb11b58fc6 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Thu, 8 Jul 2021 16:00:06 +0100 Subject: [PATCH 2/3] go mod: Add go mod files To help with tighter integration with juju and the way go.mod is used include the files here. We haven't removed the old files, so dependencies are managed via dependencies.tsv and go.mod. This should be fine as we've done this in other libraries as well. --- go.mod | 11 +++++++++++ go.sum | 8 ++++++++ 2 files changed, 19 insertions(+) create mode 100644 go.mod create mode 100644 go.sum diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..8240395 --- /dev/null +++ b/go.mod @@ -0,0 +1,11 @@ +module github.com/juju/loggo + +go 1.14 + +require ( + github.com/juju/ansiterm v0.0.0-20180109212912-720a0952cc2a + github.com/lunixbochs/vtclean v0.0.0-20160125035106-4fbf7632a2c6 + github.com/mattn/go-colorable v0.0.6 + github.com/mattn/go-isatty v0.0.0-20160806122752-66b8e73f3f5c + gopkg.in/check.v1 v1.0.0-20160105164936-4f90aeace3a2 +) diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..827fe7f --- /dev/null +++ b/go.sum @@ -0,0 +1,8 @@ +github.com/juju/ansiterm v0.0.0-20180109212912-720a0952cc2a h1:FaWFmfWdAUKbSCtOU2QjDaorUexogfaMgbipgYATUMU= +github.com/juju/ansiterm v0.0.0-20180109212912-720a0952cc2a/go.mod h1:UJSiEoRfvx3hP73CvoARgeLjaIOjybY9vj8PUPPFGeU= +github.com/lunixbochs/vtclean v0.0.0-20160125035106-4fbf7632a2c6/go.mod h1:pHhQNgMf3btfWnGBVipUOjRYhoOsdGqdm/+2c2E2WMI= +github.com/mattn/go-colorable v0.0.6 h1:jGqlOoCjqVR4hfTO9H1qrR2xi0xZNYmX2T1xlw7P79c= +github.com/mattn/go-colorable v0.0.6/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU= +github.com/mattn/go-isatty v0.0.0-20160806122752-66b8e73f3f5c h1:3nKFouDdpgGUV/uerJcYWH45ZbJzX0SiVWfTgmUeTzc= +github.com/mattn/go-isatty v0.0.0-20160806122752-66b8e73f3f5c/go.mod h1:M+lRXTBqGeGNdLjl/ufCoiOlB5xdOkqRJdNxMWT7Zi4= +gopkg.in/check.v1 v1.0.0-20160105164936-4f90aeace3a2/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= From cdf1d3f1e41ef2a0a7ea831ba213a54d0e35c3db Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Thu, 8 Jul 2021 16:02:29 +0100 Subject: [PATCH 3/3] Newly added loggers w/ labels use existing config As labels are created with lazy configurations, newly added loggers didn't come up with the correct label set. To fix this, we keep around a labels config map that can ensure that the labels is correctly set with the right level. Adding logging levels test --- context.go | 28 ++++++++++++++---- context_test.go | 78 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/context.go b/context.go index c49979b..b8a5a75 100644 --- a/context.go +++ b/context.go @@ -16,8 +16,10 @@ type Context struct { root *module // Perhaps have one mutex? - modulesMutex sync.Mutex - modules map[string]*module + // All `modules` variables are managed by the one mutex. + modulesMutex sync.Mutex + modules map[string]*module + modulesLabelConfig map[string]Level writersMutex sync.Mutex writers map[string]Writer @@ -33,8 +35,9 @@ func NewContext(rootLevel Level) *Context { rootLevel = WARNING } context := &Context{ - modules: make(map[string]*module), - writers: make(map[string]Writer), + modules: make(map[string]*module), + modulesLabelConfig: make(map[string]Level), + writers: make(map[string]Writer), } context.root = &module{ level: rootLevel, @@ -97,13 +100,21 @@ func (c *Context) getLoggerModule(name string, labels []string) *module { // Ensure that we create a new logger module for the name, that includes the // label. - labelMap := map[string]struct{}{} + level := UNSPECIFIED + labelMap := make(map[string]struct{}) for _, label := range labels { labelMap[label] = struct{}{} + + // First label wins when setting the logger label from the config label + // level cache. If there are no label configs, then fallback to + // UNSPECIFIED and inherit the level correctly. + if configLevel, ok := c.modulesLabelConfig[label]; ok && level == UNSPECIFIED { + level = configLevel + } } impl = &module{ name: name, - level: UNSPECIFIED, + level: level, parent: parent, context: c, labels: labels, @@ -168,6 +179,9 @@ func (c *Context) ApplyConfig(config Config) { continue } + // Ensure that we save the config for lazy loggers to pick up correctly. + c.modulesLabelConfig[label] = level + // Config contains a named label, use that for selecting the loggers. modules := c.getLoggerModulesByLabel(label) for _, module := range modules { @@ -185,6 +199,8 @@ func (c *Context) ResetLoggerLevels() { for _, module := range c.modules { module.setLevel(UNSPECIFIED) } + // We can safely just wipe everything here. + c.modulesLabelConfig = make(map[string]Level) } func (c *Context) write(entry Entry) { diff --git a/context_test.go b/context_test.go index 0e2afb4..c5296b0 100644 --- a/context_test.go +++ b/context_test.go @@ -260,6 +260,84 @@ func (*ContextSuite) TestApplyConfigLabels(c *gc.C) { }) } +func (*ContextSuite) TestApplyConfigLabelsAppliesToNewLoggers(c *gc.C) { + context := loggo.NewContext(loggo.WARNING) + + context.ApplyConfig(loggo.Config{"#one": loggo.TRACE}) + context.ApplyConfig(loggo.Config{"#two": loggo.DEBUG}) + + context.GetLogger("a.b", "one") + context.GetLogger("c.d", "one") + context.GetLogger("e", "two") + + c.Assert(context.Config(), gc.DeepEquals, + loggo.Config{ + "": loggo.WARNING, + "a.b": loggo.TRACE, + "c.d": loggo.TRACE, + "e": loggo.DEBUG, + }) + c.Assert(context.CompleteConfig(), gc.DeepEquals, + loggo.Config{ + "": loggo.WARNING, + "a": loggo.UNSPECIFIED, + "a.b": loggo.TRACE, + "c": loggo.UNSPECIFIED, + "c.d": loggo.TRACE, + "e": loggo.DEBUG, + }) +} + +func (*ContextSuite) TestApplyConfigLabelsAppliesToNewLoggersWithMultipleTags(c *gc.C) { + context := loggo.NewContext(loggo.WARNING) + + // Invert the order here, to ensure that the config order doesn't matter, + // but the way the tags are ordered in `GetLogger`. + context.ApplyConfig(loggo.Config{"#two": loggo.DEBUG}) + context.ApplyConfig(loggo.Config{"#one": loggo.TRACE}) + + context.GetLogger("a.b", "one", "two") + + c.Assert(context.Config(), gc.DeepEquals, + loggo.Config{ + "": loggo.WARNING, + "a.b": loggo.TRACE, + }) + c.Assert(context.CompleteConfig(), gc.DeepEquals, + loggo.Config{ + "": loggo.WARNING, + "a": loggo.UNSPECIFIED, + "a.b": loggo.TRACE, + }) +} + +func (*ContextSuite) TestApplyConfigLabelsResetLoggerLevels(c *gc.C) { + context := loggo.NewContext(loggo.WARNING) + + context.ApplyConfig(loggo.Config{"#one": loggo.TRACE}) + context.ApplyConfig(loggo.Config{"#two": loggo.DEBUG}) + + context.GetLogger("a.b", "one") + context.GetLogger("c.d", "one") + context.GetLogger("e", "two") + + context.ResetLoggerLevels() + + c.Assert(context.Config(), gc.DeepEquals, + loggo.Config{ + "": loggo.WARNING, + }) + c.Assert(context.CompleteConfig(), gc.DeepEquals, + loggo.Config{ + "": loggo.WARNING, + "a": loggo.UNSPECIFIED, + "a.b": loggo.UNSPECIFIED, + "c": loggo.UNSPECIFIED, + "c.d": loggo.UNSPECIFIED, + "e": loggo.UNSPECIFIED, + }) +} + func (*ContextSuite) TestApplyConfigLabelsAddative(c *gc.C) { context := loggo.NewContext(loggo.WARNING) context.ApplyConfig(loggo.Config{"#one": loggo.TRACE})