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

@@ -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
 }

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,82 +11,64 @@ 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
 	}
 
 	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,
+	}
 }

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,46 +28,27 @@ func TestNewMlogLogger(t *testing.T) {
 		cfg := &ConfigPushProxy{
 			EnableFileLog:   true,
 			LogFileLocation: log.Name(),
+			LogFormat:       "json",
 		}
 
 		logger, err := NewMlogLogger(cfg)
 		assert.NoError(t, err)
 		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)