Skip to content

Commit

Permalink
RSDK-5481: Reduce surface area of log creation APIs. (viamrobotics#3136)
Browse files Browse the repository at this point in the history
Zap has a very rich set of customization that would be expensive for a new implementation to keep parity with. Thankfully we only use a small subset of zap's configuration. This patch/refactor removes unused (or lightly used) code and consolidates the configuration down to (nearly) a single spot.

A description of the existing logger configurations. There were four public `golog.New*Logger` API methods, mapping to three distinct configurations:
* NewLogger -> NewProductionLogger
* NewProductionLogger
* NewDevelopmentLogger
* NewDebugLogger

There are now two logger creation APIs: `NewLogger` and `NewDebugLogger`. `NewLogger` maps to existing `NewDevelopmentLogger`. `NewDebugLogger` remained the same.

The (prior) Development and Debug logger were exactly the same except for log level (info vs debug). That distinction is the same post-PR.

The "ProductionLogger" logged at info and had a few differences:
* Used lower-case uncolored text for error levels in logs. Development/debug used capitals + ascii color codes
* Encoded time as "seconds since epoch" vs ISO 8601 dates
* And `time.Duration`s were encoded as floats in seconds versus a string encoding.

The main viam server only used the `NewDevelopmentLogger` and `NewDebugLogger`. There were only a handful of "production logger" instances. Relegated to some CLI programs and a few log lines in other code calling `NewLogger`. There wasn't a lot of meaningful log content in those usages, so I've deemed the usages incidental to using a "production logger" and not intentional.

Other miscellaneous changes
* `ReplaceGoabl` -> `ReplaceGlobal`
* Remove GPC (google cloud provider) loggers. Those were from golog and only used from `viam-robotics/app`.
* Removed Panic/DPanic APIs from Logger.
  * There was one usage of `logger.Panic` and it was confusing to say the least. The behavior was largely retained by separating logger calls + panic call.
  • Loading branch information
dgottlieb authored Oct 26, 2023
1 parent d5d608b commit 7ea5dc1
Show file tree
Hide file tree
Showing 14 changed files with 165 additions and 320 deletions.
2 changes: 1 addition & 1 deletion cli/module_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ func sameModels(a, b []ModuleComponent) bool {

// UpdateModelsAction figures out the models that a module supports and updates it's metadata file.
func UpdateModelsAction(c *cli.Context) error {
logger := logging.NewDevelopmentLogger("x")
logger := logging.NewLogger("x")
newModels, err := readModels(c.String("binary"), logger)
if err != nil {
return err
Expand Down
6 changes: 3 additions & 3 deletions components/camera/videosource/logging/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func NewLogger() (*Logger, error) {
}
}

cfg := logging.NewDevelopmentLoggerConfig()
cfg := logging.NewLoggerConfig()
cfg.OutputPaths = []string{filePath}

// only keep message
Expand Down Expand Up @@ -311,8 +311,8 @@ func (l *Logger) write(title string, m InfoMap) {
}

t.AppendFooter(table.Row{time.Now().UTC().Format(time.RFC3339)})
l.logger.Infoln(t.Render())
l.logger.Infoln(fmt.Sprintln())
l.logger.Info(t.Render())
l.logger.Info()
}

func (l *Logger) logError(err error, msg string) {
Expand Down
2 changes: 1 addition & 1 deletion config/logging_level_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
)

func TestConfigDebugFlag(t *testing.T) {
logConfig := logging.NewDevelopmentLoggerConfig()
logConfig := logging.NewLoggerConfig()
logger := logging.FromZapCompatible(zap.Must(logConfig.Build()).Sugar())

for _, cmdLineValue := range []bool{true, false} {
Expand Down
2 changes: 1 addition & 1 deletion data/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func (c *collector) getAndPushNextReading() {
timeReceived := timestamppb.New(c.clock.Now().UTC())
if err != nil {
if errors.Is(err, ErrNoCaptureToStore) {
c.logger.Debugln("capture filtered out by modular resource")
c.logger.Debug("capture filtered out by modular resource")
return
}
c.captureErrors <- errors.Wrap(err, "error while capturing data")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
)

func main() {
logger := logging.NewDevelopmentLogger("client")
logger := logging.NewLogger("client")
robot, err := client.New(
context.Background(),
"localhost:8080",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
)

func main() {
logger := logging.NewDevelopmentLogger("client")
logger := logging.NewLogger("client")

// Connect to the default localhost port for viam-server.
robot, err := client.New(
Expand Down
2 changes: 1 addition & 1 deletion examples/customresources/demos/simplemodule/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
var myModel = resource.NewModel("acme", "demo", "mycounter")

func main() {
utils.ContextualMain(mainWithArgs, logging.NewDevelopmentLogger("SimpleModule"))
utils.ContextualMain(mainWithArgs, logging.NewLogger("SimpleModule"))
}

func mainWithArgs(ctx context.Context, args []string, logger logging.Logger) error {
Expand Down
76 changes: 76 additions & 0 deletions logging/level.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package logging

import (
"encoding/json"
"fmt"
"strings"
)

// Level is an enum of log levels. Its value can be `DEBUG`, `INFO`, `WARN` or `ERROR`.
type Level int

const (
// This numbering scheme serves two purposes:
// - A statement is logged if its log level matches or exceeds the configured level. I.e:
// Statement(WARN) >= LogConfig(INFO) would be logged because "1" > "0".
// - INFO is the default level. So we start counting at DEBUG=-1 such that INFO is given Go's
// zero-value.

// DEBUG log level.
DEBUG Level = iota - 1
// INFO log level.
INFO
// WARN log level.
WARN
// ERROR log level.
ERROR
)

func (level Level) String() string {
switch level {
case DEBUG:
return "Debug"
case INFO:
return "Info"
case WARN:
return "Warn"
case ERROR:
return "Error"
}

panic(fmt.Sprintf("unreachable: %d", level))
}

// LevelFromString parses an input string to a log level. The string must be one of `debug`, `info`,
// `warn` or `error`. The parsing is case-insensitive. An error is returned if the input does not
// match one of labeled cases.
func LevelFromString(inp string) (Level, error) {
switch strings.ToLower(inp) {
case "debug":
return DEBUG, nil
case "info":
return INFO, nil
case "warn":
return WARN, nil
case "error":
return ERROR, nil
}

return DEBUG, fmt.Errorf("unknown log level: %q", inp)
}

// MarshalJSON converts a log level to a json string.
func (level Level) MarshalJSON() ([]byte, error) {
return json.Marshal(level.String())
}

// UnmarshalJSON converts a json string to a log level.
func (level *Level) UnmarshalJSON(data []byte) (err error) {
var levelStr string
if err := json.Unmarshal(data, &levelStr); err != nil {
return err
}

*level, err = LevelFromString(levelStr)
return
}
Loading

0 comments on commit 7ea5dc1

Please sign in to comment.