Adding sentry config to praefectjc-add-sentry-to-praefect

Refactored configs so that both praefect and gitaly can share logging
and sentry config structs

19 files changed, 102 insertions, 29 deletions

diff --git a/changelogs/unreleased/jc-add-sentry-to-praefect.yml b/changelogs/unreleased/jc-add-sentry-to-praefect.yml
new file mode 100644
index 000000000..187007731
--- /dev/null
+++ b/changelogs/unreleased/jc-add-sentry-to-praefect.yml
@@ -0,0 +1,5 @@
+---
+title: Adding sentry config to praefect
+merge_request: 1546
+author:
+type: other
diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go
index 3a506d86e..b8d57adb0 100644
--- a/cmd/gitaly/main.go
+++ b/cmd/gitaly/main.go
@@ -11,6 +11,7 @@ import (
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/bootstrap"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
+	"gitlab.com/gitlab-org/gitaly/internal/config/sentry"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/server"
 	"gitlab.com/gitlab-org/gitaly/internal/storage"
@@ -96,7 +97,7 @@ func main() {
 	}
 
 	config.ConfigureLogging()
-	config.ConfigureSentry(version.GetVersion())
+	sentry.ConfigureSentry(version.GetVersion(), sentry.Config(config.Config.Logging.Sentry))
 	config.ConfigurePrometheus()
 	config.ConfigureConcurrencyLimits()
 	tracing.Initialize(tracing.WithServiceName("gitaly"))
diff --git a/cmd/praefect/main.go b/cmd/praefect/main.go
index 6ffee7756..b45a17c23 100644
--- a/cmd/praefect/main.go
+++ b/cmd/praefect/main.go
@@ -16,11 +16,13 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 
+	"gitlab.com/gitlab-org/gitaly/internal/config/sentry"
 	"gitlab.com/gitlab-org/gitaly/internal/log"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/conn"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/protoregistry"
+	"gitlab.com/gitlab-org/gitaly/internal/version"
 	"gitlab.com/gitlab-org/labkit/tracing"
 )
 
@@ -88,6 +90,8 @@ func configure() (config.Config, error) {
 	registerServerVersionPromGauge()
 	logger.WithField("version", praefect.GetVersionString()).Info("Starting Praefect")
 
+	sentry.ConfigureSentry(version.GetVersion(), conf.Sentry)
+
 	return conf, nil
 }
 
diff --git a/config.praefect.toml.example b/config.praefect.toml.example
index f23ab8472..10a7c9073 100644
--- a/config.praefect.toml.example
+++ b/config.praefect.toml.example
@@ -15,6 +15,9 @@ listen_addr = "127.0.0.1:2305"
 # # One of, in order: debug, info, warn, errror, fatal, panic
 # # Defaults to "info"
 #   level = "warn"
+# [sentry]
+#   sentry_environment = ""
+#   sentry_dsn = ""
 #
 #  Optional: authenticate Gitaly requests using a shared secret. This token works the same way as a gitaly token
 # [auth]
diff --git a/internal/auth/auth.go b/internal/config/auth/auth.go
index 1bf8de921..1bf8de921 100644
--- a/internal/auth/auth.go
+++ b/internal/config/auth/auth.go
diff --git a/internal/config/config.go b/internal/config/config.go
index 98631a6ac..5fa84002f 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -12,7 +12,9 @@ import (
 	"github.com/BurntSushi/toml"
 	"github.com/kelseyhightower/envconfig"
 	log "github.com/sirupsen/logrus"
-	"gitlab.com/gitlab-org/gitaly/internal/auth"
+	"gitlab.com/gitlab-org/gitaly/internal/config/auth"
+	internallog "gitlab.com/gitlab-org/gitaly/internal/config/log"
+	"gitlab.com/gitlab-org/gitaly/internal/config/sentry"
 )
 
 const (
@@ -84,14 +86,16 @@ type Storage struct {
 	Path string
 }
 
+// Sentry is a sentry.Config. We redefine this type to a different name so
+// we can embed both structs into Logging
+type Sentry sentry.Config
+
 // Logging contains the logging configuration for Gitaly
 type Logging struct {
-	Dir               string `toml:"dir"`
-	Format            string `toml:"format"`
-	SentryDSN         string `toml:"sentry_dsn"`
-	RubySentryDSN     string `toml:"ruby_sentry_dsn"`
-	SentryEnvironment string `toml:"sentry_environment"`
-	Level             string `toml:"level"`
+	internallog.Config
+	Sentry
+
+	RubySentryDSN string `toml:"ruby_sentry_dsn"`
 }
 
 // Prometheus contains additional configuration data for prometheus
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 7785dbcde..9827a2668 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -14,6 +14,8 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitaly/internal/config/sentry"
 )
 
 func configFileReader(content string) io.Reader {
@@ -93,6 +95,29 @@ path="/tmp/repos2"`)
 	}
 }
 
+func TestLoadSentry(t *testing.T) {
+	tmpFile := configFileReader(`[logging]
+sentry_environment = "production"
+sentry_dsn = "abc123"
+ruby_sentry_dsn = "xyz456"`)
+
+	err := Load(tmpFile)
+	assert.NoError(t, err)
+
+	expectedConf := Cfg{
+		Logging: Logging{
+			Sentry: Sentry(sentry.Config{
+				Environment: "production",
+				DSN:         "abc123",
+			}),
+			RubySentryDSN: "xyz456",
+		},
+	}
+	expectedConf.setDefaults()
+
+	assert.Equal(t, expectedConf, Config)
+}
+
 func TestLoadPrometheus(t *testing.T) {
 	tmpFile := configFileReader(`prometheus_listen_addr=":9236"`)
 
diff --git a/internal/config/log/log.go b/internal/config/log/log.go
new file mode 100644
index 000000000..b61c73cfc
--- /dev/null
+++ b/internal/config/log/log.go
@@ -0,0 +1,8 @@
+package log
+
+// Config contains logging configuration values
+type Config struct {
+	Dir    string `toml:"dir"`
+	Format string `toml:"format"`
+	Level  string `toml:"level"`
+}
diff --git a/internal/config/sentry.go b/internal/config/sentry/sentry.go
index 1b8aa7d06..4e43ae484 100644
--- a/internal/config/sentry.go
+++ b/internal/config/sentry/sentry.go
@@ -1,4 +1,4 @@
-package config
+package sentry
 
 import (
 	"fmt"
@@ -8,20 +8,26 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/panichandler"
 )
 
+// Config contains configuration for sentry
+type Config struct {
+	DSN         string `toml:"sentry_dsn"`
+	Environment string `toml:"sentry_environment"`
+}
+
 // ConfigureSentry configures the sentry DSN
-func ConfigureSentry(version string) {
-	if Config.Logging.SentryDSN == "" {
+func ConfigureSentry(version string, sentryConf Config) {
+	if sentryConf.DSN == "" {
 		return
 	}
 
 	log.Debug("Using sentry logging")
-	raven.SetDSN(Config.Logging.SentryDSN)
+	raven.SetDSN(sentryConf.DSN)
 	if version != "" {
 		raven.SetRelease("v" + version)
 	}
 
-	if Config.Logging.SentryEnvironment != "" {
-		raven.SetEnvironment(Config.Logging.SentryEnvironment)
+	if sentryConf.Environment != "" {
+		raven.SetEnvironment(sentryConf.Environment)
 	}
 
 	panichandler.InstallPanicHandler(func(grpcMethod string, _err interface{}) {
diff --git a/internal/praefect/auth_test.go b/internal/praefect/auth_test.go
index 25fe7e4a0..cc3e5c39a 100644
--- a/internal/praefect/auth_test.go
+++ b/internal/praefect/auth_test.go
@@ -9,7 +9,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
-	"gitlab.com/gitlab-org/gitaly/internal/auth"
+	"gitlab.com/gitlab-org/gitaly/internal/config/auth"
 	"gitlab.com/gitlab-org/gitaly/internal/log"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/conn"
diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go
index 89e62a488..e400350fd 100644
--- a/internal/praefect/config/config.go
+++ b/internal/praefect/config/config.go
@@ -6,8 +6,9 @@ import (
 
 	"github.com/BurntSushi/toml"
 
-	"gitlab.com/gitlab-org/gitaly/internal/auth"
-	"gitlab.com/gitlab-org/gitaly/internal/config"
+	"gitlab.com/gitlab-org/gitaly/internal/config/auth"
+	"gitlab.com/gitlab-org/gitaly/internal/config/log"
+	"gitlab.com/gitlab-org/gitaly/internal/config/sentry"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/models"
 )
 
@@ -19,9 +20,10 @@ type Config struct {
 
 	Nodes []*models.Node `toml:"node"`
 
-	Logging              config.Logging `toml:"logging"`
-	PrometheusListenAddr string         `toml:"prometheus_listen_addr"`
-	Auth                 auth.Config    `toml:"auth"`
+	Logging              log.Config    `toml:"logging"`
+	Sentry               sentry.Config `toml:"sentry"`
+	PrometheusListenAddr string        `toml:"prometheus_listen_addr"`
+	Auth                 auth.Config   `toml:"auth"`
 }
 
 // FromFile loads the config for the passed file path
diff --git a/internal/praefect/config/config_test.go b/internal/praefect/config/config_test.go
index 658effa7e..bf707d5d7 100644
--- a/internal/praefect/config/config_test.go
+++ b/internal/praefect/config/config_test.go
@@ -5,6 +5,8 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/config/log"
+	"gitlab.com/gitlab-org/gitaly/internal/config/sentry"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/models"
 )
 
@@ -74,6 +76,14 @@ func TestConfigParsing(t *testing.T) {
 			filePath: "testdata/config.toml",
 			expected: Config{
 				VirtualStorageName: "praefect",
+				Logging: log.Config{
+					Level:  "info",
+					Format: "json",
+				},
+				Sentry: sentry.Config{
+					DSN:         "abcd123",
+					Environment: "production",
+				},
 				Nodes: []*models.Node{
 					&models.Node{
 						Address:        "tcp://gitaly-internal-1.example.com",
diff --git a/internal/praefect/config/testdata/config.toml b/internal/praefect/config/testdata/config.toml
index c7f920e90..1c85c7e47 100644
--- a/internal/praefect/config/testdata/config.toml
+++ b/internal/praefect/config/testdata/config.toml
@@ -4,10 +4,12 @@ socket_path = ""
 prometheus_listen_addr = ""
 
 [logging]
-  format = ""
-  sentry_dsn = ""
-  ruby_sentry_dsn = ""
-  level = ""
+  format = "json"
+  level = "info"
+
+[sentry]
+  sentry_environment = "production"
+  sentry_dsn = "abcd123"
 
 [[node]]
   address = "tcp://gitaly-internal-1.example.com"
diff --git a/internal/praefect/helper_test.go b/internal/praefect/helper_test.go
index ae55ade51..dd8064fd8 100644
--- a/internal/praefect/helper_test.go
+++ b/internal/praefect/helper_test.go
@@ -11,7 +11,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/client"
-	internalauth "gitlab.com/gitlab-org/gitaly/internal/auth"
+	internalauth "gitlab.com/gitlab-org/gitaly/internal/config/auth"
 	"gitlab.com/gitlab-org/gitaly/internal/log"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/conn"
diff --git a/internal/praefect/server.go b/internal/praefect/server.go
index 7a4349260..ada0d05f6 100644
--- a/internal/praefect/server.go
+++ b/internal/praefect/server.go
@@ -13,6 +13,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/cancelhandler"
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/metadatahandler"
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/panichandler"
+	"gitlab.com/gitlab-org/gitaly/internal/middleware/sentryhandler"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/conn"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/grpc-proxy/proxy"
@@ -60,6 +61,7 @@ func NewServer(c *Coordinator, repl ReplMgr, grpcOpts []grpc.ServerOption, l *lo
 			grpccorrelation.StreamServerCorrelationInterceptor(), // Must be above the metadata handler
 			grpc_prometheus.StreamServerInterceptor,
 			grpc_logrus.StreamServerInterceptor(l),
+			sentryhandler.StreamLogHandler,
 			cancelhandler.Stream, // Should be below LogHandler
 			grpctracing.StreamServerTracingInterceptor(),
 			auth.StreamServerInterceptor(conf.Auth),
@@ -72,6 +74,7 @@ func NewServer(c *Coordinator, repl ReplMgr, grpcOpts []grpc.ServerOption, l *lo
 			metadatahandler.UnaryInterceptor,
 			grpc_prometheus.UnaryServerInterceptor,
 			grpc_logrus.UnaryServerInterceptor(l),
+			sentryhandler.UnaryLogHandler,
 			cancelhandler.Unary, // Should be below LogHandler
 			grpctracing.UnaryServerTracingInterceptor(),
 			auth.UnaryServerInterceptor(conf.Auth),
diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go
index b9cc7d19d..21bfbd744 100644
--- a/internal/rubyserver/rubyserver.go
+++ b/internal/rubyserver/rubyserver.go
@@ -129,7 +129,7 @@ func (s *Server) start() error {
 		env = append(env, "SENTRY_DSN="+dsn)
 	}
 
-	if sentryEnvironment := cfg.Logging.SentryEnvironment; sentryEnvironment != "" {
+	if sentryEnvironment := cfg.Logging.Sentry.Environment; sentryEnvironment != "" {
 		env = append(env, "SENTRY_ENVIRONMENT="+sentryEnvironment)
 	}
 
diff --git a/internal/server/auth/auth.go b/internal/server/auth/auth.go
index 8ad2963d8..8e2eec143 100644
--- a/internal/server/auth/auth.go
+++ b/internal/server/auth/auth.go
@@ -7,7 +7,7 @@ import (
 	grpc_auth "github.com/grpc-ecosystem/go-grpc-middleware/auth"
 	"github.com/prometheus/client_golang/prometheus"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
-	internalauth "gitlab.com/gitlab-org/gitaly/internal/auth"
+	internalauth "gitlab.com/gitlab-org/gitaly/internal/config/auth"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
diff --git a/internal/server/auth_test.go b/internal/server/auth_test.go
index 3ee2a340b..9366d2ada 100644
--- a/internal/server/auth_test.go
+++ b/internal/server/auth_test.go
@@ -10,8 +10,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
-	"gitlab.com/gitlab-org/gitaly/internal/auth"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
+	"gitlab.com/gitlab-org/gitaly/internal/config/auth"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
diff --git a/internal/service/server/info_test.go b/internal/service/server/info_test.go
index 106c9f8fa..9a2d636ef 100644
--- a/internal/service/server/info_test.go
+++ b/internal/service/server/info_test.go
@@ -7,8 +7,8 @@ import (
 
 	"github.com/stretchr/testify/require"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
-	internalauth "gitlab.com/gitlab-org/gitaly/internal/auth"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
+	internalauth "gitlab.com/gitlab-org/gitaly/internal/config/auth"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/server/auth"
 	"gitlab.com/gitlab-org/gitaly/internal/storage"