Skip to content

Commit dcac07e

Browse files
kchristidiskchristidis
committed
[FAB-2916] Refactor flogging package
There are several things on this package that could use improvement. Off the top of my head as I'm writing this, I am noting down the following: 1. Test coverage is great but the tests are incredibly verbose and filled with redundant checks (as an example: there is no need to ensure that the log level is the default in `TestSetModuleLevel_moduleWithSubmodule`, this is taken care of in a previous test). 2. Asymmetry on the return types; some functions return the logging level as the string type, others as the `logging.Level` type. 3. Functions whose purpose is unclear (`InitFromViper`), whose name is inaccurate (`SetLoggingFormat` does not just set the logging format), or whose logic is simply flawed (`InitFromViper` - pay attention to how the input argument is used). 4. Outdated and --in some cases-- nonsensical comments on the exported functions. This is my attempt to tidy things up. I've also updated the code that used to reference the now-deprecated methods. Change-Id: I0cc020f20b3e4a4c2a4785f98aee2cb3282758bc Signed-off-by: Kostas Christidis <[email protected]>
1 parent a1dccef commit dcac07e

File tree

8 files changed

+252
-584
lines changed

8 files changed

+252
-584
lines changed

‎common/flogging/logging.go

+115-137
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright IBM Corp. 2016 All Rights Reserved.
2+
Copyright IBM Corp. 2017 All Rights Reserved.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
@@ -24,186 +24,164 @@ import (
2424
"sync"
2525

2626
"github.com/op/go-logging"
27-
"github.com/spf13/viper"
2827
)
2928

30-
// A logger to log logging logs!
31-
var logger = logging.MustGetLogger("logging")
32-
33-
var defaultFormat = "%{color}%{time:2006-01-02 15:04:05.000 MST} [%{module}] %{shortfunc} -> %{level:.4s} %{id:03x}%{color:reset} %{message}"
34-
var defaultOutput = os.Stderr
29+
const (
30+
pkgLogID = "flogging"
31+
defaultFormat = "%{color}%{time:2006-01-02 15:04:05.000 MST} [%{module}] %{shortfunc} -> %{level:.4s} %{id:03x}%{color:reset} %{message}"
32+
defaultLevel = logging.INFO
33+
)
3534

36-
// The default logging level, in force until LoggingInit() is called or in
37-
// case of configuration errors.
38-
var fallbackDefaultLevel = logging.INFO
35+
var (
36+
logger *logging.Logger
3937

40-
var lock = sync.Mutex{}
38+
defaultOutput *os.File
4139

42-
// modules holds the map of all modules and the current log level
43-
var modules map[string]string
40+
modules map[string]string // Holds the map of all modules and their respective log level
41+
lock sync.Mutex
4442

45-
// IsSetLevelByRegExpEnabled disables the code that will allow setting log
46-
// levels using a regular expression instead of one module/submodule at a time.
47-
// TODO - remove once peer code has switched over to using
48-
// flogging.MustGetLogger in place of logging.MustGetLogger
49-
var IsSetLevelByRegExpEnabled = false
43+
// IsSetLevelByRegExpEnabled allows the setting of log levels using a regular
44+
// expression, instead of one module at a time, when set to true.
45+
// TODO Remove once all other packages have switched to
46+
// `flogging.MustGetLogger` from `logging.MustGetLogger`.
47+
IsSetLevelByRegExpEnabled bool
48+
)
5049

5150
func init() {
52-
modules = make(map[string]string)
53-
}
54-
55-
// InitFromViper is a 'hook' called at the beginning of command processing to
56-
// parse logging-related options specified either on the command-line or in
57-
// config files. Command-line options take precedence over config file
58-
// options, and can also be passed as suitably-named environment variables. To
59-
// change module logging levels at runtime call `logging.SetLevel(level,
60-
// module)`. To debug this routine include logging=debug as the first
61-
// term of the logging specification.
62-
// TODO this initialization is specific to the peer config format. The viper
63-
// references should be removed, and this path should be moved into the peer
64-
func InitFromViper(command string) {
65-
spec := viper.GetString("logging_level")
66-
if spec == "" {
67-
spec = viper.GetString("logging." + command)
68-
}
69-
defaultLevel := InitFromSpec(spec)
70-
logger.Debugf("Setting default logging level to %s for command '%s'", defaultLevel, command)
71-
}
72-
73-
// InitFromSpec initializes the logging based on the supplied spec, it is exposed externally
74-
// so that consumers of the flogging package who do not wish to use the default config structure
75-
// may parse their own logging specification
76-
func InitFromSpec(spec string) logging.Level {
77-
// Parse the logging specification in the form
78-
// [<module>[,<module>...]=]<level>[:[<module>[,<module>...]=]<level>...]
79-
defaultLevel := fallbackDefaultLevel
80-
var err error
81-
if spec != "" {
82-
fields := strings.Split(spec, ":")
83-
for _, field := range fields {
84-
split := strings.Split(field, "=")
85-
switch len(split) {
86-
case 1:
87-
// Default level
88-
defaultLevel, err = logging.LogLevel(field)
89-
if err != nil {
90-
logger.Warningf("Logging level '%s' not recognized, defaulting to %s : %s", field, defaultLevel, err)
91-
defaultLevel = fallbackDefaultLevel // NB - 'defaultLevel' was overwritten
92-
}
93-
case 2:
94-
// <module>[,<module>...]=<level>
95-
if level, err := logging.LogLevel(split[1]); err != nil {
96-
logger.Warningf("Invalid logging level in '%s' ignored", field)
97-
} else if split[0] == "" {
98-
logger.Warningf("Invalid logging override specification '%s' ignored - no module specified", field)
99-
} else {
100-
modules := strings.Split(split[0], ",")
101-
for _, module := range modules {
102-
logging.SetLevel(level, module)
103-
logger.Debugf("Setting logging level for module '%s' to %s", module, level)
104-
}
105-
}
106-
default:
107-
logger.Warningf("Invalid logging override '%s' ignored; Missing ':' ?", field)
108-
}
109-
}
110-
}
111-
// Set the default logging level for all modules
112-
logging.SetLevel(defaultLevel, "")
113-
return defaultLevel
51+
logger = logging.MustGetLogger(pkgLogID)
52+
Reset()
11453
}
11554

116-
// DefaultLevel returns the fallback value for loggers to use if parsing fails
117-
func DefaultLevel() logging.Level {
118-
return fallbackDefaultLevel
119-
}
55+
// Reset sets to logging to the defaults defined in this package.
56+
func Reset() {
57+
IsSetLevelByRegExpEnabled = false // redundant since the default for booleans is `false` but added for clarity
58+
modules = make(map[string]string)
59+
lock = sync.Mutex{}
12060

121-
// Initiate 'leveled' logging using the default format and output location
122-
func init() {
123-
SetLoggingFormat(defaultFormat, defaultOutput)
61+
defaultOutput = os.Stderr
62+
InitBackend(SetFormat(defaultFormat), defaultOutput)
63+
InitFromSpec("")
12464
}
12565

126-
// SetLoggingFormat sets the logging format and the location of the log output
127-
func SetLoggingFormat(formatString string, output io.Writer) {
128-
if formatString == "" {
129-
formatString = defaultFormat
66+
// SetFormat sets the logging format.
67+
func SetFormat(formatSpec string) logging.Formatter {
68+
if formatSpec == "" {
69+
formatSpec = defaultFormat
13070
}
131-
format := logging.MustStringFormatter(formatString)
132-
133-
initLoggingBackend(format, output)
71+
return logging.MustStringFormatter(formatSpec)
13472
}
13573

136-
// initialize the logging backend based on the provided logging formatter
137-
// and I/O writer
138-
func initLoggingBackend(logFormatter logging.Formatter, output io.Writer) {
74+
// InitBackend sets up the logging backend based on
75+
// the provided logging formatter and I/O writer.
76+
func InitBackend(formatter logging.Formatter, output io.Writer) {
13977
backend := logging.NewLogBackend(output, "", 0)
140-
backendFormatter := logging.NewBackendFormatter(backend, logFormatter)
141-
logging.SetBackend(backendFormatter).SetLevel(fallbackDefaultLevel, "")
78+
backendFormatter := logging.NewBackendFormatter(backend, formatter)
79+
logging.SetBackend(backendFormatter).SetLevel(defaultLevel, "")
14280
}
14381

144-
// GetModuleLevel gets the current logging level for the specified module
145-
func GetModuleLevel(module string) (string, error) {
82+
// DefaultLevel returns the fallback value for loggers to use if parsing fails.
83+
func DefaultLevel() string {
84+
return defaultLevel.String()
85+
}
86+
87+
// GetModuleLevel gets the current logging level for the specified module.
88+
func GetModuleLevel(module string) string {
14689
// logging.GetLevel() returns the logging level for the module, if defined.
147-
// otherwise, it returns the default logging level, as set by
148-
// flogging/logging.go
90+
// Otherwise, it returns the default logging level, as set by
91+
// `flogging/logging.go`.
14992
level := logging.GetLevel(module).String()
150-
151-
logger.Debugf("Module '%s' logger enabled for log level: %s", module, level)
152-
153-
return level, nil
93+
logger.Debugf("Module '%s' logger set to '%s' log level", module, level)
94+
return level
15495
}
15596

156-
// SetModuleLevel sets the logging level for the modules that match the
157-
// supplied regular expression. This is can be called from anywhere in the
158-
// code on a running peer to dynamically change log levels.
159-
func SetModuleLevel(moduleRegExp string, logLevel string) (string, error) {
160-
level, err := logging.LogLevel(logLevel)
97+
// SetModuleLevel sets the logging level for the modules that match the supplied
98+
// regular expression. Can be used to dynamically change the log level for the
99+
// module.
100+
func SetModuleLevel(moduleRegExp string, level string) (string, error) {
101+
var re *regexp.Regexp
161102

103+
logLevel, err := logging.LogLevel(level)
162104
if err != nil {
163-
logger.Warningf("Invalid logging level: %s - ignored", logLevel)
105+
logger.Warningf("Invalid logging level '%s' - ignored", level)
164106
} else {
165-
// TODO - this check is in here temporarily until all modules have been
166-
// converted to using flogging.MustGetLogger. until that point, this flag
167-
// keeps the previous behavior in place.
107+
// TODO This check is here to preserve the old functionality until all
108+
// other packages switch to `flogging.MustGetLogger` (from
109+
// `logging.MustGetLogger`).
168110
if !IsSetLevelByRegExpEnabled {
169-
logging.SetLevel(logging.Level(level), moduleRegExp)
170-
logger.Infof("Module '%s' logger enabled for log level: %s", moduleRegExp, level)
111+
logging.SetLevel(logging.Level(logLevel), moduleRegExp)
112+
logger.Debugf("Module '%s' logger enabled for log level '%s'", moduleRegExp, logLevel)
171113
} else {
172-
re, err := regexp.Compile(moduleRegExp)
114+
re, err = regexp.Compile(moduleRegExp)
173115
if err != nil {
174-
logger.Warningf("Invalid regular expression for module: %s", moduleRegExp)
116+
logger.Warningf("Invalid regular expression: %s", moduleRegExp)
175117
return "", err
176118
}
177119

178120
lock.Lock()
179121
defer lock.Unlock()
180122
for module := range modules {
181123
if re.MatchString(module) {
182-
logging.SetLevel(logging.Level(level), module)
183-
modules[module] = logLevel
184-
logger.Infof("Module '%s' logger enabled for log level: %s", module, level)
124+
logging.SetLevel(logging.Level(logLevel), module)
125+
modules[module] = logLevel.String()
126+
logger.Infof("Module '%s' logger enabled for log level '%s'", module, logLevel)
185127
}
186128
}
187129
}
188130
}
189-
190-
logLevelString := level.String()
191-
192-
return logLevelString, err
131+
return logLevel.String(), err
193132
}
194133

195-
// MustGetLogger is used in place of logging.MustGetLogger to allow us to store
196-
// a map of all modules and submodules that have loggers in the system
134+
// MustGetLogger is used in place of `logging.MustGetLogger` to allow us to
135+
// store a map of all modules and submodules that have loggers in the system.
197136
func MustGetLogger(module string) *logging.Logger {
198-
logger := logging.MustGetLogger(module)
199-
200-
// retrieve the current log level for the logger
201-
level := logging.GetLevel(module).String()
202-
203-
// store the module's name as well as the current log level for the logger
137+
l := logging.MustGetLogger(module)
204138
lock.Lock()
205139
defer lock.Unlock()
206-
modules[module] = level
140+
modules[module] = GetModuleLevel(module)
141+
return l
142+
}
143+
144+
// InitFromSpec initializes the logging based on the supplied spec. It is
145+
// exposed externally so that consumers of the flogging package may parse their
146+
// own logging specification. The logging specification has the following form:
147+
// [<module>[,<module>...]=]<level>[:[<module>[,<module>...]=]<level>...]
148+
func InitFromSpec(spec string) string {
149+
levelAll := defaultLevel
150+
var err error
151+
152+
if spec != "" {
153+
fields := strings.Split(spec, ":")
154+
for _, field := range fields {
155+
split := strings.Split(field, "=")
156+
switch len(split) {
157+
case 1:
158+
if levelAll, err = logging.LogLevel(field); err != nil {
159+
logger.Warningf("Logging level '%s' not recognized, defaulting to '%s': %s", field, defaultLevel, err)
160+
levelAll = defaultLevel // need to reset cause original value was overwritten
161+
}
162+
case 2:
163+
// <module>[,<module>...]=<level>
164+
levelSingle, err := logging.LogLevel(split[1])
165+
if err != nil {
166+
logger.Warningf("Invalid logging level in '%s' ignored", field)
167+
continue
168+
}
169+
170+
if split[0] == "" {
171+
logger.Warningf("Invalid logging override specification '%s' ignored - no module specified", field)
172+
} else {
173+
modules := strings.Split(split[0], ",")
174+
for _, module := range modules {
175+
logger.Debugf("Setting logging level for module '%s' to '%s'", module, levelSingle)
176+
logging.SetLevel(levelSingle, module)
177+
}
178+
}
179+
default:
180+
logger.Warningf("Invalid logging override '%s' ignored - missing ':'?", field)
181+
}
182+
}
183+
}
207184

208-
return logger
185+
logging.SetLevel(levelAll, "") // set the logging level for all modules
186+
return levelAll.String()
209187
}

0 commit comments

Comments
 (0)