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

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

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)

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
 	}

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() {}

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
 	}

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()
 

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 = `