Make logging configurable

Futher a few style improvements were added, and logging is now
configurable in the same way Gitaly is.

6 files changed, 58 insertions, 35 deletions

diff --git a/cmd/praefect/main.go b/cmd/praefect/main.go
index 5e477bda9..6d4189e17 100644
--- a/cmd/praefect/main.go
+++ b/cmd/praefect/main.go
@@ -10,7 +10,7 @@ import (
 	"syscall"
 	"time"
 
-	log "github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/labkit/tracing"
@@ -18,18 +18,12 @@ import (
 
 var (
 	flagConfig = flag.String("config", "", "Location for the config.toml")
-	logger     = log.New()
+	logger     *logrus.Logger
 )
 
-func init() {
-	logger.SetFormatter(&log.JSONFormatter{})
-	logger.SetLevel(log.DebugLevel)
-}
-
 func main() {
 	flag.Parse()
 
-	logger.WithField("config", *flagConfig).Info("reading config file")
 	conf, err := config.FromFile(*flagConfig)
 	if err != nil {
 		logger.Fatalf("%s", err)
@@ -39,6 +33,8 @@ func main() {
 		logger.Fatalf("%s", err)
 	}
 
+	logger := conf.ConfigureLogger()
+
 	tracing.Initialize(tracing.WithServiceName("praefect"))
 
 	l, err := net.Listen("tcp", conf.ListenAddr)
diff --git a/config.praefect.toml.example b/config.praefect.toml.example
index 5b0ba08ed..f21880772 100644
--- a/config.praefect.toml.example
+++ b/config.praefect.toml.example
@@ -4,6 +4,14 @@
 listen_addr = "127.0.0.1:2305"
 # socket_path = "/home/git/gitlab/tmp/sockets/private/praefect.socket"
 
+# # You can optionally configure Praefect to output JSON-formatted log messages to stdout
+# [logging]
+# format = "json"
+# # Optional: Set log level to only log entries with that severity or above
+# # One of, in order: debug, info, warn, errror, fatal, panic
+# # Defaults to "info"
+# level = "warn"
+
 # # One or more Gitaly servers need to be configured to be managed. The names
 # of each server are used to link multiple nodes, or `gitaly_server`s together
 # as shard. listen_addr should be unique for all nodes.
diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go
index 32a11294f..7c58ef277 100644
--- a/internal/praefect/config/config.go
+++ b/internal/praefect/config/config.go
@@ -1,16 +1,18 @@
 package config
 
 import (
-	"fmt"
+	"errors"
 	"os"
 
 	"github.com/BurntSushi/toml"
+	"gitlab.com/gitlab-org/gitaly/internal/config"
 )
 
 // Config is a container for everything found in the TOML config file
 type Config struct {
 	ListenAddr    string          `toml:"listen_addr" split_words:"true"`
 	GitalyServers []*GitalyServer `toml:"gitaly_server", split_words:"true"`
+	Logging       config.Logging  `toml:"logging"`
 }
 
 // GitalyServer allows configuring the servers that RPCs are proxied to
@@ -32,24 +34,31 @@ func FromFile(filePath string) (Config, error) {
 	return *config, err
 }
 
+var (
+	errNoListenAddr        = errors.New("no listen address configured")
+	errNoGitalyServers     = errors.New("no gitaly backends configured")
+	errDuplicateGitalyAddr = errors.New("gitaly listen addresses are not unique")
+	errGitalyWithoutName   = errors.New("all gitaly servers must have a name")
+)
+
 // Validate establishes if the config is valid
 func (c Config) Validate() error {
 	if c.ListenAddr == "" {
-		return fmt.Errorf("no listen address configured")
+		return errNoListenAddr
 	}
 
 	if len(c.GitalyServers) == 0 {
-		return fmt.Errorf("no gitaly backends configured")
+		return errNoGitalyServers
 	}
 
 	listenAddrs := make(map[string]bool, len(c.GitalyServers))
 	for _, gitaly := range c.GitalyServers {
 		if gitaly.Name == "" {
-			return fmt.Errorf("expect %v to have a name", gitaly)
+			return errGitalyWithoutName
 		}
 
 		if _, found := listenAddrs[gitaly.ListenAddr]; found {
-			return fmt.Errorf("gitaly listen_addr: %s is not unique", gitaly.ListenAddr)
+			return errDuplicateGitalyAddr
 		}
 
 		listenAddrs[gitaly.ListenAddr] = true
diff --git a/internal/praefect/config/config_test.go b/internal/praefect/config/config_test.go
index 142af7dd2..d896879b3 100644
--- a/internal/praefect/config/config_test.go
+++ b/internal/praefect/config/config_test.go
@@ -3,47 +3,43 @@ package config
 import (
 	"testing"
 
-	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestConfigValidation(t *testing.T) {
 	gitalySrvs := []*GitalyServer{&GitalyServer{"test", "localhost:23456"}}
 
 	testCases := []struct {
-		desc        string
-		config      *Config
-		expectError bool
+		desc   string
+		config Config
+		err    error
 	}{
 		{
-			desc:        "No ListenAddr",
-			config:      &Config{"", gitalySrvs},
-			expectError: true,
+			desc:   "No ListenAddr",
+			config: Config{ListenAddr: "", GitalyServers: gitalySrvs},
+			err:    errNoListenAddr,
 		},
 		{
-			desc:        "No servers",
-			config:      &Config{"localhost:1234", nil},
-			expectError: true,
+			desc:   "No servers",
+			config: Config{ListenAddr: "localhost:1234", GitalyServers: nil},
+			err:    errNoGitalyServers,
 		},
 		{
-			desc:        "duplicate address",
-			config:      &Config{"localhost:1234", []*GitalyServer{gitalySrvs[0], gitalySrvs[0]}},
-			expectError: true,
+			desc:   "duplicate address",
+			config: Config{ListenAddr: "localhost:1234", GitalyServers: []*GitalyServer{gitalySrvs[0], gitalySrvs[0]}},
+			err:    errDuplicateGitalyAddr,
 		},
 		{
 			desc:   "Valid config",
-			config: &Config{"localhost:1234", gitalySrvs},
+			config: Config{ListenAddr: "localhost:1234", GitalyServers: gitalySrvs},
+			err:    nil,
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t1 *testing.T) {
 			err := tc.config.Validate()
-			if tc.expectError {
-				require.Error(t1, err)
-			} else {
-				require.NoError(t1, err)
-			}
-
+			assert.Equal(t, err, tc.err)
 		})
 	}
 }
diff --git a/internal/praefect/config/log.go b/internal/praefect/config/log.go
new file mode 100644
index 000000000..7d0bb314b
--- /dev/null
+++ b/internal/praefect/config/log.go
@@ -0,0 +1,14 @@
+package config
+
+import (
+	"github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/internal/log"
+)
+
+// ConfigureLogger applies the settings from the configuration file to the
+// logger, setting the log level and format.
+func (c Config) ConfigureLogger() *logrus.Logger {
+	log.Configure(c.Logging.Format, c.Logging.Level)
+
+	return log.Default
+}
diff --git a/internal/praefect/server.go b/internal/praefect/server.go
index 0561a4ec8..04779af2c 100644
--- a/internal/praefect/server.go
+++ b/internal/praefect/server.go
@@ -37,8 +37,8 @@ type Coordinator struct {
 
 	// Nodes will in the first interations have only one key, which limits
 	// the praefect to serve only 1 distinct set of Gitaly nodes.
-	// One limitation this creates; each server needs the same amount of
-	// disk space in case of full replication.
+	// One limitation is that each server needs the same amount of disk
+	// space in case of full replication.
 	nodes map[string]*grpc.ClientConn
 }