From c3bb6b31a9e667363c44af288eb0490a2861c6f7 Mon Sep 17 00:00:00 2001 From: John Cai Date: Wed, 9 Oct 2019 13:42:28 -0700 Subject: Adding sentry config to praefect Refactored configs so that both praefect and gitaly can share logging and sentry config structs --- .../unreleased/jc-add-sentry-to-praefect.yml | 5 +++ cmd/gitaly/main.go | 3 +- cmd/praefect/main.go | 4 ++ config.praefect.toml.example | 3 ++ internal/auth/auth.go | 7 ---- internal/config/auth/auth.go | 7 ++++ internal/config/config.go | 18 +++++---- internal/config/config_test.go | 25 ++++++++++++ internal/config/log/log.go | 8 ++++ internal/config/sentry.go | 38 ------------------- internal/config/sentry/sentry.go | 44 ++++++++++++++++++++++ internal/praefect/auth_test.go | 2 +- internal/praefect/config/config.go | 12 +++--- internal/praefect/config/config_test.go | 10 +++++ internal/praefect/config/testdata/config.toml | 10 +++-- internal/praefect/helper_test.go | 2 +- internal/praefect/server.go | 3 ++ internal/rubyserver/rubyserver.go | 2 +- internal/server/auth/auth.go | 2 +- internal/server/auth_test.go | 2 +- internal/service/server/info_test.go | 2 +- 21 files changed, 141 insertions(+), 68 deletions(-) create mode 100644 changelogs/unreleased/jc-add-sentry-to-praefect.yml delete mode 100644 internal/auth/auth.go create mode 100644 internal/config/auth/auth.go create mode 100644 internal/config/log/log.go delete mode 100644 internal/config/sentry.go create mode 100644 internal/config/sentry/sentry.go 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/auth/auth.go deleted file mode 100644 index 1bf8de921..000000000 --- a/internal/auth/auth.go +++ /dev/null @@ -1,7 +0,0 @@ -package auth - -// Config is a struct for an authentication config -type Config struct { - Transitioning bool `toml:"transitioning"` - Token string `toml:"token"` -} diff --git a/internal/config/auth/auth.go b/internal/config/auth/auth.go new file mode 100644 index 000000000..1bf8de921 --- /dev/null +++ b/internal/config/auth/auth.go @@ -0,0 +1,7 @@ +package auth + +// Config is a struct for an authentication config +type Config struct { + Transitioning bool `toml:"transitioning"` + Token string `toml:"token"` +} 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.go deleted file mode 100644 index 1b8aa7d06..000000000 --- a/internal/config/sentry.go +++ /dev/null @@ -1,38 +0,0 @@ -package config - -import ( - "fmt" - - raven "github.com/getsentry/raven-go" - log "github.com/sirupsen/logrus" - "gitlab.com/gitlab-org/gitaly/internal/middleware/panichandler" -) - -// ConfigureSentry configures the sentry DSN -func ConfigureSentry(version string) { - if Config.Logging.SentryDSN == "" { - return - } - - log.Debug("Using sentry logging") - raven.SetDSN(Config.Logging.SentryDSN) - if version != "" { - raven.SetRelease("v" + version) - } - - if Config.Logging.SentryEnvironment != "" { - raven.SetEnvironment(Config.Logging.SentryEnvironment) - } - - panichandler.InstallPanicHandler(func(grpcMethod string, _err interface{}) { - err, ok := _err.(error) - if !ok { - err = fmt.Errorf("%v", _err) - } - - raven.CaptureError(err, map[string]string{ - "grpcMethod": grpcMethod, - "panic": "1", - }) - }) -} diff --git a/internal/config/sentry/sentry.go b/internal/config/sentry/sentry.go new file mode 100644 index 000000000..4e43ae484 --- /dev/null +++ b/internal/config/sentry/sentry.go @@ -0,0 +1,44 @@ +package sentry + +import ( + "fmt" + + raven "github.com/getsentry/raven-go" + log "github.com/sirupsen/logrus" + "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, sentryConf Config) { + if sentryConf.DSN == "" { + return + } + + log.Debug("Using sentry logging") + raven.SetDSN(sentryConf.DSN) + if version != "" { + raven.SetRelease("v" + version) + } + + if sentryConf.Environment != "" { + raven.SetEnvironment(sentryConf.Environment) + } + + panichandler.InstallPanicHandler(func(grpcMethod string, _err interface{}) { + err, ok := _err.(error) + if !ok { + err = fmt.Errorf("%v", _err) + } + + raven.CaptureError(err, map[string]string{ + "grpcMethod": grpcMethod, + "panic": "1", + }) + }) +} 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" -- cgit v1.2.3