From b5f0c75c037c73eea973560688d94934b27ee4a8 Mon Sep 17 00:00:00 2001 From: Robert Fratto Date: Tue, 28 Jul 2020 16:36:37 -0400 Subject: [PATCH] pkg/promtail: propagate a logger rather than using util.Logger globally (#2438) * pkg/promtail: propagate a logger rather than using util.Logger globally This commit allows for creating promtail with a custom log.Logger instance which will be propagated and used consistently throughout the Promtail package. This allows for clients to provide a Promtail-specific logger. Address a portion of #2405. * remove applyOptions function --- pkg/promtail/client/logger.go | 6 ++-- pkg/promtail/client/logger_test.go | 5 +-- pkg/promtail/promtail.go | 36 +++++++++++++------ pkg/promtail/server/server.go | 18 +++++----- pkg/promtail/targets/manager.go | 7 ++-- .../targets/stdin/stdin_target_manager.go | 15 ++++---- .../stdin/stdin_target_manager_test.go | 9 ++--- 7 files changed, 56 insertions(+), 40 deletions(-) diff --git a/pkg/promtail/client/logger.go b/pkg/promtail/client/logger.go index 5097f21cfbe92..23e978083a8b2 100644 --- a/pkg/promtail/client/logger.go +++ b/pkg/promtail/client/logger.go @@ -8,8 +8,8 @@ import ( "text/tabwriter" "time" - "github.com/cortexproject/cortex/pkg/util" "github.com/fatih/color" + "github.com/go-kit/kit/log" "github.com/prometheus/common/model" "gopkg.in/yaml.v2" ) @@ -32,9 +32,9 @@ type logger struct { } // NewLogger creates a new client logger that logs entries instead of sending them. -func NewLogger(cfgs ...Config) (Client, error) { +func NewLogger(log log.Logger, cfgs ...Config) (Client, error) { // make sure the clients config is valid - c, err := NewMulti(util.Logger, cfgs...) + c, err := NewMulti(log, cfgs...) if err != nil { return nil, err } diff --git a/pkg/promtail/client/logger_test.go b/pkg/promtail/client/logger_test.go index 3ff0e948e66aa..0d86b154180d3 100644 --- a/pkg/promtail/client/logger_test.go +++ b/pkg/promtail/client/logger_test.go @@ -5,16 +5,17 @@ import ( "testing" "time" + "github.com/cortexproject/cortex/pkg/util" "github.com/cortexproject/cortex/pkg/util/flagext" "github.com/prometheus/common/model" "github.com/stretchr/testify/require" ) func TestNewLogger(t *testing.T) { - _, err := NewLogger([]Config{}...) + _, err := NewLogger(util.Logger, []Config{}...) require.Error(t, err) - l, err := NewLogger([]Config{{URL: flagext.URLValue{URL: &url.URL{Host: "string"}}}}...) + l, err := NewLogger(util.Logger, []Config{{URL: flagext.URLValue{URL: &url.URL{Host: "string"}}}}...) require.NoError(t, err) err = l.Handle(model.LabelSet{"foo": "bar"}, time.Now(), "entry") require.NoError(t, err) diff --git a/pkg/promtail/promtail.go b/pkg/promtail/promtail.go index 949b1449d1f7b..259c864b9ae29 100644 --- a/pkg/promtail/promtail.go +++ b/pkg/promtail/promtail.go @@ -4,6 +4,7 @@ import ( "sync" "github.com/cortexproject/cortex/pkg/util" + "github.com/go-kit/kit/log" "github.com/grafana/loki/pkg/promtail/client" "github.com/grafana/loki/pkg/promtail/config" @@ -11,18 +12,38 @@ import ( "github.com/grafana/loki/pkg/promtail/targets" ) +// Option is a function that can be passed to the New method of Promtail and +// customize the Promtail that is created. +type Option func(p *Promtail) + +// WithLogger overrides the default logger for Promtail. +func WithLogger(log log.Logger) Option { + return func(p *Promtail) { + p.logger = log + } +} + // Promtail is the root struct for Promtail... type Promtail struct { client client.Client targetManagers *targets.TargetManagers server server.Server + logger log.Logger stopped bool mtx sync.Mutex } // New makes a new Promtail. -func New(cfg config.Config, dryRun bool) (*Promtail, error) { +func New(cfg config.Config, dryRun bool, opts ...Option) (*Promtail, error) { + // Initialize promtail with some defaults and allow the options to override + // them. + promtail := &Promtail{ + logger: util.Logger, + } + for _, o := range opts { + o(promtail) + } if cfg.ClientConfig.URL.URL != nil { // if a single client config is used we add it to the multiple client config for backward compatibility @@ -30,30 +51,25 @@ func New(cfg config.Config, dryRun bool) (*Promtail, error) { } var err error - var cl client.Client if dryRun { - cl, err = client.NewLogger(cfg.ClientConfigs...) + promtail.client, err = client.NewLogger(promtail.logger, cfg.ClientConfigs...) if err != nil { return nil, err } cfg.PositionsConfig.ReadOnly = true } else { - cl, err = client.NewMulti(util.Logger, cfg.ClientConfigs...) + promtail.client, err = client.NewMulti(promtail.logger, cfg.ClientConfigs...) if err != nil { return nil, err } } - promtail := &Promtail{ - client: cl, - } - - tms, err := targets.NewTargetManagers(promtail, util.Logger, cfg.PositionsConfig, cl, cfg.ScrapeConfig, &cfg.TargetConfig) + tms, err := targets.NewTargetManagers(promtail, promtail.logger, cfg.PositionsConfig, promtail.client, cfg.ScrapeConfig, &cfg.TargetConfig) if err != nil { return nil, err } promtail.targetManagers = tms - server, err := server.New(cfg.ServerConfig, tms) + server, err := server.New(cfg.ServerConfig, promtail.logger, tms) if err != nil { return nil, err } diff --git a/pkg/promtail/server/server.go b/pkg/promtail/server/server.go index 11d18ef7c8947..0c845f7517322 100644 --- a/pkg/promtail/server/server.go +++ b/pkg/promtail/server/server.go @@ -13,7 +13,7 @@ import ( "syscall" "text/template" - logutil "github.com/cortexproject/cortex/pkg/util" + "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" "github.com/pkg/errors" "github.com/prometheus/common/version" @@ -37,6 +37,7 @@ type Server interface { // Server embed weaveworks server with static file and templating capability type server struct { *serverww.Server + log log.Logger tms *targets.TargetManagers externalURL *url.URL healthCheckTarget bool @@ -65,9 +66,9 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { } // New makes a new Server -func New(cfg Config, tms *targets.TargetManagers) (Server, error) { +func New(cfg Config, log log.Logger, tms *targets.TargetManagers) (Server, error) { if cfg.Disable { - return NoopServer, nil + return noopServer{log: log}, nil } wws, err := serverww.New(cfg.Config) if err != nil { @@ -87,6 +88,7 @@ func New(cfg Config, tms *targets.TargetManagers) (Server, error) { serv := &server{ Server: wws, + log: log, tms: tms, externalURL: externalURL, healthCheckTarget: healthCheckTargetFlag, @@ -211,7 +213,7 @@ func (s *server) ready(rw http.ResponseWriter, _ *http.Request) { rw.WriteHeader(http.StatusOK) if _, err := rw.Write(readinessProbeSuccess); err != nil { - level.Error(logutil.Logger).Log("msg", "error writing success message", "error", err) + level.Error(s.log).Log("msg", "error writing success message", "error", err) } } @@ -241,15 +243,13 @@ func computeExternalURL(u string, port int) (*url.URL, error) { return eu, nil } -var NoopServer Server = noopServer{} +type noopServer struct{ log log.Logger } -type noopServer struct{} - -func (noopServer) Run() error { +func (s noopServer) Run() error { sigs := make(chan os.Signal, 1) signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM) sig := <-sigs - level.Info(logutil.Logger).Log("msg", "received shutdown signal", "sig", sig) + level.Info(s.log).Log("msg", "received shutdown signal", "sig", sig) return nil } func (noopServer) Shutdown() {} diff --git a/pkg/promtail/targets/manager.go b/pkg/promtail/targets/manager.go index 7c0d675231d5d..313f94f34f4a1 100644 --- a/pkg/promtail/targets/manager.go +++ b/pkg/promtail/targets/manager.go @@ -1,7 +1,6 @@ package targets import ( - "github.com/cortexproject/cortex/pkg/util" "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" "github.com/pkg/errors" @@ -50,8 +49,8 @@ func NewTargetManagers( targetScrapeConfigs := make(map[string][]scrapeconfig.Config, 4) if targetConfig.Stdin { - level.Debug(util.Logger).Log("msg", "configured to read from stdin") - stdin, err := stdin.NewStdinTargetManager(app, client, scrapeConfigs) + level.Debug(logger).Log("msg", "configured to read from stdin") + stdin, err := stdin.NewStdinTargetManager(logger, app, client, scrapeConfigs) if err != nil { return nil, err } @@ -59,7 +58,7 @@ func NewTargetManagers( return &TargetManagers{targetManagers: targetManagers}, nil } - positions, err := positions.New(util.Logger, positionsConfig) + positions, err := positions.New(logger, positionsConfig) if err != nil { return nil, err } diff --git a/pkg/promtail/targets/stdin/stdin_target_manager.go b/pkg/promtail/targets/stdin/stdin_target_manager.go index 09737157f8ab9..2dd0e1e196802 100644 --- a/pkg/promtail/targets/stdin/stdin_target_manager.go +++ b/pkg/promtail/targets/stdin/stdin_target_manager.go @@ -9,7 +9,6 @@ import ( "strings" "time" - "github.com/cortexproject/cortex/pkg/util" "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" "github.com/prometheus/client_golang/prometheus" @@ -57,8 +56,8 @@ type stdinTargetManager struct { app Shutdownable } -func NewStdinTargetManager(app Shutdownable, client api.EntryHandler, configs []scrapeconfig.Config) (*stdinTargetManager, error) { - reader, err := newReaderTarget(stdIn, client, getStdinConfig(configs)) +func NewStdinTargetManager(log log.Logger, app Shutdownable, client api.EntryHandler, configs []scrapeconfig.Config) (*stdinTargetManager, error) { + reader, err := newReaderTarget(log, stdIn, client, getStdinConfig(log, configs)) if err != nil { return nil, err } @@ -74,12 +73,12 @@ func NewStdinTargetManager(app Shutdownable, client api.EntryHandler, configs [] return stdinManager, nil } -func getStdinConfig(configs []scrapeconfig.Config) scrapeconfig.Config { +func getStdinConfig(log log.Logger, configs []scrapeconfig.Config) scrapeconfig.Config { cfg := defaultStdInCfg // if we receive configs we use the first one. if len(configs) > 0 { if len(configs) > 1 { - level.Warn(util.Logger).Log("msg", fmt.Sprintf("too many scrape configs, skipping %d configs.", len(configs)-1)) + level.Warn(log).Log("msg", fmt.Sprintf("too many scrape configs, skipping %d configs.", len(configs)-1)) } cfg = configs[0] } @@ -103,8 +102,8 @@ type readerTarget struct { ctx context.Context } -func newReaderTarget(in io.Reader, client api.EntryHandler, cfg scrapeconfig.Config) (*readerTarget, error) { - pipeline, err := stages.NewPipeline(log.With(util.Logger, "component", "pipeline"), cfg.PipelineStages, &cfg.JobName, prometheus.DefaultRegisterer) +func newReaderTarget(logger log.Logger, in io.Reader, client api.EntryHandler, cfg scrapeconfig.Config) (*readerTarget, error) { + pipeline, err := stages.NewPipeline(log.With(logger, "component", "pipeline"), cfg.PipelineStages, &cfg.JobName, prometheus.DefaultRegisterer) if err != nil { return nil, err } @@ -121,7 +120,7 @@ func newReaderTarget(in io.Reader, client api.EntryHandler, cfg scrapeconfig.Con cancel: cancel, ctx: ctx, lbs: lbs, - logger: log.With(util.Logger, "component", "reader"), + logger: log.With(logger, "component", "reader"), } go t.read() diff --git a/pkg/promtail/targets/stdin/stdin_target_manager_test.go b/pkg/promtail/targets/stdin/stdin_target_manager_test.go index b282780172f46..20f235ef19f7f 100644 --- a/pkg/promtail/targets/stdin/stdin_target_manager_test.go +++ b/pkg/promtail/targets/stdin/stdin_target_manager_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/cortexproject/cortex/pkg/util" "github.com/prometheus/common/model" "github.com/stretchr/testify/require" "gopkg.in/yaml.v2" @@ -90,7 +91,7 @@ func Test_newReaderTarget(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { recorder := &clientRecorder{} - got, err := newReaderTarget(tt.in, recorder, tt.cfg) + got, err := newReaderTarget(util.Logger, tt.in, recorder, tt.cfg) if (err != nil) != tt.wantErr { t.Errorf("newReaderTarget() error = %v, wantErr %v", err, tt.wantErr) return @@ -129,7 +130,7 @@ func Test_Shutdown(t *testing.T) { stdIn = newFakeStin("line") appMock := &mockShutdownable{called: make(chan bool, 1)} recorder := &clientRecorder{} - manager, err := NewStdinTargetManager(appMock, recorder, []scrapeconfig.Config{{}}) + manager, err := NewStdinTargetManager(util.Logger, appMock, recorder, []scrapeconfig.Config{{}}) require.NoError(t, err) require.NotNil(t, manager) called := <-appMock.called @@ -140,12 +141,12 @@ func Test_Shutdown(t *testing.T) { func Test_StdinConfigs(t *testing.T) { // should take the first config - require.Equal(t, scrapeconfig.DefaultScrapeConfig, getStdinConfig([]scrapeconfig.Config{ + require.Equal(t, scrapeconfig.DefaultScrapeConfig, getStdinConfig(util.Logger, []scrapeconfig.Config{ scrapeconfig.DefaultScrapeConfig, {}, })) // or use the default if none if provided - require.Equal(t, defaultStdInCfg, getStdinConfig([]scrapeconfig.Config{})) + require.Equal(t, defaultStdInCfg, getStdinConfig(util.Logger, []scrapeconfig.Config{})) } var stagesConfig = `