From 0b850d39b44ad02c0233047e9feaf459a45bf10f Mon Sep 17 00:00:00 2001 From: Domenico Rizzo Date: Tue, 10 Dec 2024 11:42:25 +0100 Subject: [PATCH] Refactor logging configuration The logging configuration has been refactored to simplify the process. Deprecated fields have been removed from the ConfigPushProxy struct and replaced with a single LogFormat field. The NewMlogLogger function now uses this new field to determine the log format, defaulting to "plain" if an invalid value is provided. Additionally, helper functions have been created to build the logger configuration for both console and file outputs based on these changes. Corresponding tests have also been updated accordingly. --- server/config_push_proxy.go | 24 +++---- server/logging.go | 113 +++++++++++++------------------ server/logging_test.go | 128 ++++-------------------------------- 3 files changed, 68 insertions(+), 197 deletions(-) diff --git a/server/config_push_proxy.go b/server/config_push_proxy.go index ec93293..feeea6a 100644 --- a/server/config_push_proxy.go +++ b/server/config_push_proxy.go @@ -12,21 +12,17 @@ import ( ) type ConfigPushProxy struct { - AndroidPushSettings []AndroidPushSettings - ListenAddress string - ThrottleVaryByHeader string - // Deprecated: Use it is maintained for backward compatibility of the Logger struct. Use LoggingCfgFile or LoggingCfgJSON instead. - LogFileLocation string - SendTimeoutSec int - RetryTimeoutSec int - ApplePushSettings []ApplePushSettings - EnableMetrics bool - // Deprecated: Same reason as LogFileLocation. - EnableConsoleLog bool - // Deprecated: Same reason as LogFileLocation. + AndroidPushSettings []AndroidPushSettings + ListenAddress string + ThrottleVaryByHeader string + LogFileLocation string + SendTimeoutSec int + RetryTimeoutSec int + ApplePushSettings []ApplePushSettings + EnableMetrics bool + EnableConsoleLog bool EnableFileLog bool - LoggingCfgFile string - LoggingCfgJSON string + LogFormat string // json or plain ThrottlePerSec int ThrottleMemoryStoreSize int } diff --git a/server/logging.go b/server/logging.go index 69d64a4..aab9f83 100644 --- a/server/logging.go +++ b/server/logging.go @@ -1,9 +1,8 @@ package server import ( - "fmt" + "encoding/json" "github.com/mattermost/mattermost/server/public/shared/mlog" - "strings" ) func NewMlogLogger(cfg *ConfigPushProxy) (*mlog.Logger, error) { @@ -12,12 +11,10 @@ func NewMlogLogger(cfg *ConfigPushProxy) (*mlog.Logger, error) { if err != nil { return nil, err } - cfgJSON := cfg.LoggingCfgJSON - if cfg.LoggingCfgFile == "" && cfgJSON == "" { - // if no logging defined, use default config (console output) - cfgJSON = defaultLoggingConfig(cfg) + if cfg.LogFormat != "plain" && cfg.LogFormat != "json" { + cfg.LogFormat = "plain" } - err = logger.Configure(cfg.LoggingCfgFile, cfgJSON, nil) + err = logger.ConfigureTargets(buildLogConfig(cfg), nil) if err != nil { return logger, err } @@ -25,69 +22,53 @@ func NewMlogLogger(cfg *ConfigPushProxy) (*mlog.Logger, error) { return logger, nil } -func defaultLoggingConfig(cfg *ConfigPushProxy) string { - if cfg.LogFileLocation == "" || !cfg.EnableFileLog { - return defaultLoggingConsoleLogConfig() +func buildLogConfig(cfg *ConfigPushProxy) mlog.LoggerConfiguration { + logConf := make(mlog.LoggerConfiguration) + + if cfg.EnableFileLog && cfg.LogFileLocation != "" { + logConf["file"] = buildLogFileConfig(cfg.LogFileLocation, cfg.LogFormat) + } + + if cfg.EnableConsoleLog || cfg.LogFileLocation == "" || !cfg.EnableFileLog { + logConf["console"] = buildConsoleLogConfig(cfg.LogFormat) } - return defaultLoggingFileLogConfig(cfg.LogFileLocation) -} -func defaultLoggingFileLogConfig(filename string) string { - return fmt.Sprintf(` - { - "def": { - "type": "file", - "options": { - "filename": "%s" - }, - "format": "plain", - "format_options": { - "delim": " ", - "min_level_len": 5, - "min_msg_len": 40, - "enable_color": true, - "enable_caller": true - }, - "levels": [ - {"id": 5, "name": "debug"}, - {"id": 4, "name": "info", "color": 36}, - {"id": 3, "name": "warn"}, - {"id": 2, "name": "error", "color": 31}, - {"id": 1, "name": "fatal", "stacktrace": true}, - {"id": 0, "name": "panic", "stacktrace": true} - ] - } - }`, escapeDoubleQuotes(filename)) + return logConf } -func defaultLoggingConsoleLogConfig() string { - return ` - { - "def": { - "type": "console", - "options": { - "out": "stdout" - }, - "format": "plain", - "format_options": { - "delim": " ", - "min_level_len": 5, - "min_msg_len": 40, - "enable_color": true, - "enable_caller": true - }, - "levels": [ - {"id": 5, "name": "debug"}, - {"id": 4, "name": "info", "color": 36}, - {"id": 3, "name": "warn"}, - {"id": 2, "name": "error", "color": 31}, - {"id": 1, "name": "fatal", "stacktrace": true}, - {"id": 0, "name": "panic", "stacktrace": true} - ] - } - }` +func buildConsoleLogConfig(format string) mlog.TargetCfg { + return mlog.TargetCfg{ + Type: "console", + Levels: mlog.StdAll, + Format: format, + Options: json.RawMessage(`{"out": "stdout"}`), + FormatOptions: json.RawMessage(`{"enable_color": true, "enable_caller": true}`), + MaxQueueSize: 1000, + } } -func escapeDoubleQuotes(input string) string { - return strings.ReplaceAll(input, `"`, `\"`) +func buildLogFileConfig(filename string, format string) mlog.TargetCfg { + opts := struct { + Filename string `json:"filename"` + Max_size int `json:"max_size"` + Max_age int `json:"max_age"` + Max_backups int `json:"max_backups"` + Compress bool `json:"compress"` + }{ + Filename: filename, + Max_size: 100, + Max_age: 0, + Max_backups: 0, + Compress: true, + } + var optsJsonString, _ = json.Marshal(opts) + + return mlog.TargetCfg{ + Type: "file", + Levels: mlog.StdAll, + Format: format, + Options: optsJsonString, + FormatOptions: json.RawMessage(`{"enable_color": true, "enable_caller": true}`), + MaxQueueSize: 1000, + } } diff --git a/server/logging_test.go b/server/logging_test.go index 6a9549d..1b305fa 100644 --- a/server/logging_test.go +++ b/server/logging_test.go @@ -1,101 +1,14 @@ package server import ( - "encoding/json" - "github.com/mattermost/mattermost/server/public/model" - "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "os" "testing" ) -type logOptionsTest map[string]string - -func TestEscapeDoubleQuotes(t *testing.T) { - t.Run("No quotes", func(t *testing.T) { - assert.Equal(t, "test", escapeDoubleQuotes("test")) - }) - t.Run("One quote", func(t *testing.T) { - assert.Equal(t, "\\\"test", escapeDoubleQuotes("\"test")) - }) - t.Run("Two quotes", func(t *testing.T) { - assert.Equal(t, "\\\"test\\\"", escapeDoubleQuotes("\"test\"")) - }) - t.Run("Three quotes", func(t *testing.T) { - assert.Equal(t, "\\\"\\\"test\\\"", escapeDoubleQuotes("\"\"test\"")) - }) - t.Run("Four quotes", func(t *testing.T) { - assert.Equal(t, "\\\"\\\"\\\"\\\"", escapeDoubleQuotes("\"\"\"\"")) - }) -} - -func TestDefaultLoggingConsoleLogConfig(t *testing.T) { - var mapCfgEscaped = new(mlog.LoggerConfiguration) - var logOptionsConsole = new(logOptionsTest) - - var targetCfg mlog.TargetCfg - - t.Run("Unmarshall base correctly", func(t *testing.T) { - err := json.Unmarshal([]byte(defaultLoggingConsoleLogConfig()), mapCfgEscaped) - require.NoError(t, err) - require.NotNil(t, mapCfgEscaped) - targetCfg = (*mapCfgEscaped)["def"] - assert.Equal(t, "console", targetCfg.Type) - assert.Equal(t, "plain", targetCfg.Format) - }) - - t.Run("Unmarshall options correctly", func(t *testing.T) { - err := json.Unmarshal(targetCfg.Options, logOptionsConsole) - require.NoError(t, err) - require.NotNil(t, logOptionsConsole) - assert.Equal(t, "stdout", (*logOptionsConsole)["out"]) - }) -} - -func TestDefaultLoggingFileLogConfig(t *testing.T) { - filename := model.NewId() - var mapCfgEscaped = new(mlog.LoggerConfiguration) - var logOptionsConsole = new(logOptionsTest) - - var targetCfg mlog.TargetCfg - - t.Run("Unmarshall base correctly", func(t *testing.T) { - err := json.Unmarshal([]byte(defaultLoggingFileLogConfig(filename)), mapCfgEscaped) - require.NoError(t, err) - require.NotNil(t, mapCfgEscaped) - targetCfg = (*mapCfgEscaped)["def"] - assert.Equal(t, "file", targetCfg.Type) - assert.Equal(t, "plain", targetCfg.Format) - }) - - t.Run("Unmarshall options correctly", func(t *testing.T) { - err := json.Unmarshal(targetCfg.Options, logOptionsConsole) - require.NoError(t, err) - require.NotNil(t, logOptionsConsole) - assert.Equal(t, filename, (*logOptionsConsole)["filename"]) - }) -} - -func TestDefaultLoggingConfig(t *testing.T) { - t.Run("Console config is get correctly", func(t *testing.T) { - cfg := &ConfigPushProxy{ - EnableFileLog: false, - } - assert.Equal(t, defaultLoggingConsoleLogConfig(), defaultLoggingConfig(cfg)) - }) - - t.Run("File config is get correctly", func(t *testing.T) { - cfg := &ConfigPushProxy{ - EnableFileLog: true, - LogFileLocation: model.NewId(), - } - assert.Equal(t, defaultLoggingFileLogConfig(cfg.LogFileLocation), defaultLoggingConfig(cfg)) - }) -} - func TestNewMlogLogger(t *testing.T) { - t.Run("Instancing logger with console for legacy conf", func(t *testing.T) { + t.Run("Instancing logger with implicit plain console", func(t *testing.T) { cfg := &ConfigPushProxy{ EnableFileLog: false, } @@ -104,7 +17,7 @@ func TestNewMlogLogger(t *testing.T) { assert.NotNil(t, logger) }) - t.Run("Instancing logger with file for legacy conf", func(t *testing.T) { + t.Run("Instancing logger with json file", func(t *testing.T) { log, err := os.CreateTemp("", "log") require.NoError(t, err) @@ -115,6 +28,7 @@ func TestNewMlogLogger(t *testing.T) { cfg := &ConfigPushProxy{ EnableFileLog: true, LogFileLocation: log.Name(), + LogFormat: "json", } logger, err := NewMlogLogger(cfg) @@ -122,39 +36,19 @@ func TestNewMlogLogger(t *testing.T) { assert.NotNil(t, logger) }) - t.Run("Instancing logger with file", func(t *testing.T) { - conf, err := os.CreateTemp("", "logget-cfg-conf.json") - require.NoError(t, err) - - _, err = conf.WriteString(defaultLoggingConsoleLogConfig()) - require.NoError(t, err) - - err = conf.Close() - require.NoError(t, err) - defer os.Remove(conf.Name()) - - cfg := &ConfigPushProxy{ - LoggingCfgFile: conf.Name(), - } - - logger, err := NewMlogLogger(cfg) - assert.NoError(t, err) - assert.NotNil(t, logger) - }) - - t.Run("Instancing logger with json", func(t *testing.T) { - conf, err := os.CreateTemp("", "logget-cfg-conf.json") - require.NoError(t, err) - - _, err = conf.WriteString(defaultLoggingConsoleLogConfig()) + t.Run("Instancing logger with both file and console", func(t *testing.T) { + log, err := os.CreateTemp("", "log") require.NoError(t, err) - err = conf.Close() + err = log.Close() require.NoError(t, err) - defer os.Remove(conf.Name()) + defer os.Remove(log.Name()) cfg := &ConfigPushProxy{ - LoggingCfgJSON: defaultLoggingConsoleLogConfig(), + EnableConsoleLog: true, + EnableFileLog: true, + LogFileLocation: log.Name(), + LogFormat: "json", } logger, err := NewMlogLogger(cfg)