Refactor client configs in Promtail. (#4567)

This refactor a big how we mutate the original config for backward compatibility.
Also add tests on untested code path.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

clients/pkg/promtail/client/logger.go

@@ -13,8 +13,6 @@ import (
 	"gopkg.in/yaml.v2"
 
 	"github.com/grafana/loki/clients/pkg/promtail/api"
-
-	lokiflag "github.com/grafana/loki/pkg/util/flagext"
 )
 
 var (
@@ -38,9 +36,9 @@ type logger struct {
 }
 
 // NewLogger creates a new client logger that logs entries instead of sending them.
-func NewLogger(reg prometheus.Registerer, log log.Logger, externalLabels lokiflag.LabelSet, cfgs ...Config) (Client, error) {
+func NewLogger(reg prometheus.Registerer, log log.Logger, cfgs ...Config) (Client, error) {
 	// make sure the clients config is valid
-	c, err := NewMulti(reg, log, externalLabels, cfgs...)
+	c, err := NewMulti(reg, log, cfgs...)
 	if err != nil {
 		return nil, err
 	}
@@ -82,6 +80,5 @@ func (l *logger) run() {
 		fmt.Fprint(l.Writer, "\n")
 		l.Flush()
 	}
-
 }
 func (l *logger) StopNow() { l.Stop() }

clients/pkg/promtail/client/logger_test.go

@@ -13,14 +13,13 @@ import (
 	"github.com/grafana/loki/clients/pkg/promtail/api"
 
 	"github.com/grafana/loki/pkg/logproto"
-	"github.com/grafana/loki/pkg/util/flagext"
 )
 
 func TestNewLogger(t *testing.T) {
-	_, err := NewLogger(nil, util_log.Logger, flagext.LabelSet{}, []Config{}...)
+	_, err := NewLogger(nil, util_log.Logger, []Config{}...)
 	require.Error(t, err)
 
-	l, err := NewLogger(nil, util_log.Logger, flagext.LabelSet{}, []Config{{URL: cortexflag.URLValue{URL: &url.URL{Host: "string"}}}}...)
+	l, err := NewLogger(nil, util_log.Logger, []Config{{URL: cortexflag.URLValue{URL: &url.URL{Host: "string"}}}}...)
 	require.NoError(t, err)
 	l.Chan() <- api.Entry{Labels: model.LabelSet{"foo": "bar"}, Entry: logproto.Entry{Timestamp: time.Now(), Line: "entry"}}
 	l.Stop()

clients/pkg/promtail/client/multi.go

@@ -8,8 +8,6 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 
 	"github.com/grafana/loki/clients/pkg/promtail/api"
-
-	"github.com/grafana/loki/pkg/util/flagext"
 )
 
 // MultiClient is client pushing to one or more loki instances.
@@ -22,20 +20,13 @@ type MultiClient struct {
 }
 
 // NewMulti creates a new client
-func NewMulti(reg prometheus.Registerer, logger log.Logger, externalLabels flagext.LabelSet, cfgs ...Config) (Client, error) {
+func NewMulti(reg prometheus.Registerer, logger log.Logger, cfgs ...Config) (Client, error) {
 	if len(cfgs) == 0 {
 		return nil, errors.New("at least one client config should be provided")
 	}
 
 	clients := make([]Client, 0, len(cfgs))
 	for _, cfg := range cfgs {
-		// Merge the provided external labels from the single client config/command line with each client config from
-		// `clients`. This is done to allow --client.external-labels=key=value passed at command line to apply to all clients
-		// The order here is specified to allow the yaml to override the command line flag if there are any labels
-		// which exist in both the command line arguments as well as the yaml, and while this is
-		// not typically the order of precedence, the assumption here is someone providing a specific config in
-		// yaml is doing so explicitly to make a key specific to a client.
-		cfg.ExternalLabels = flagext.LabelSet{LabelSet: externalLabels.Merge(cfg.ExternalLabels.LabelSet)}
 		client, err := New(reg, cfg, logger)
 		if err != nil {
 			return nil, err

clients/pkg/promtail/client/multi_test.go

@@ -22,7 +22,7 @@ import (
 )
 
 func TestNewMulti(t *testing.T) {
-	_, err := NewMulti(nil, util_log.Logger, lokiflag.LabelSet{}, []Config{}...)
+	_, err := NewMulti(nil, util_log.Logger, []Config{}...)
 	if err == nil {
 		t.Fatal("expected err but got nil")
 	}
@@ -41,7 +41,7 @@ func TestNewMulti(t *testing.T) {
 		ExternalLabels: lokiflag.LabelSet{LabelSet: model.LabelSet{"hi": "there"}},
 	}
 
-	clients, err := NewMulti(prometheus.DefaultRegisterer, util_log.Logger, lokiflag.LabelSet{LabelSet: model.LabelSet{"order": "command"}}, cc1, cc2)
+	clients, err := NewMulti(prometheus.DefaultRegisterer, util_log.Logger, cc1, cc2)
 	if err != nil {
 		t.Fatalf("expected err: nil got:%v", err)
 	}
@@ -50,7 +50,7 @@ func TestNewMulti(t *testing.T) {
 		t.Fatalf("expected client: 2 got:%d", len(multi.clients))
 	}
 	actualCfg1 := clients.(*MultiClient).clients[0].(*client).cfg
-	// Yaml should overried the command line so 'order: yaml' should be expected
+	// Yaml should overridden the command line so 'order: yaml' should be expected
 	expectedCfg1 := Config{
 		BatchSize:      20,
 		BatchWait:      1 * time.Second,
@@ -61,24 +61,6 @@ func TestNewMulti(t *testing.T) {
 	if !reflect.DeepEqual(actualCfg1, expectedCfg1) {
 		t.Fatalf("expected cfg: %v got:%v", expectedCfg1, actualCfg1)
 	}
-
-	actualCfg2 := clients.(*MultiClient).clients[1].(*client).cfg
-	// No overlapping label keys so both should be in the output
-	expectedCfg2 := Config{
-		BatchSize: 10,
-		BatchWait: 1 * time.Second,
-		URL:       flagext.URLValue{URL: host2},
-		ExternalLabels: lokiflag.LabelSet{
-			LabelSet: model.LabelSet{
-				"order": "command",
-				"hi":    "there",
-			},
-		},
-	}
-
-	if !reflect.DeepEqual(actualCfg2, expectedCfg2) {
-		t.Fatalf("expected cfg: %v got:%v", expectedCfg2, actualCfg2)
-	}
 }
 
 func TestMultiClient_Stop(t *testing.T) {

clients/pkg/promtail/config/config.go

@@ -11,6 +11,8 @@ import (
 	"github.com/grafana/loki/clients/pkg/promtail/scrapeconfig"
 	"github.com/grafana/loki/clients/pkg/promtail/server"
 	"github.com/grafana/loki/clients/pkg/promtail/targets/file"
+
+	"github.com/grafana/loki/pkg/util/flagext"
 )
 
 // Config for promtail, describing what files to watch.
@@ -45,3 +47,31 @@ func (c Config) String() string {
 	}
 	return string(b)
 }
+
+func (c *Config) Setup() {
+	if c.ClientConfig.URL.URL != nil {
+		// if a single client config is used we add it to the multiple client config for backward compatibility
+		c.ClientConfigs = append(c.ClientConfigs, c.ClientConfig)
+	}
+
+	// This is a bit crude but if the Loki Push API target is specified,
+	// force the log level to match the promtail log level
+	for i := range c.ScrapeConfig {
+		if c.ScrapeConfig[i].PushConfig != nil {
+			c.ScrapeConfig[i].PushConfig.Server.LogLevel = c.ServerConfig.LogLevel
+			c.ScrapeConfig[i].PushConfig.Server.LogFormat = c.ServerConfig.LogFormat
+		}
+	}
+
+	// Merge the provided external labels from the single client config/command line with each client config from
+	// `clients`. This is done to allow --client.external-labels=key=value passed at command line to apply to all clients
+	// The order here is specified to allow the yaml to override the command line flag if there are any labels
+	// which exist in both the command line arguments as well as the yaml, and while this is
+	// not typically the order of precedence, the assumption here is someone providing a specific config in
+	// yaml is doing so explicitly to make a key specific to a client.
+	if len(c.ClientConfig.ExternalLabels.LabelSet) > 0 {
+		for i := range c.ClientConfigs {
+			c.ClientConfigs[i].ExternalLabels = flagext.LabelSet{LabelSet: c.ClientConfig.ExternalLabels.LabelSet.Merge(c.ClientConfigs[i].ExternalLabels.LabelSet)}
+		}
+	}
+}

clients/pkg/promtail/config/config_test.go

@@ -1,10 +1,18 @@
 package config
 
 import (
+	"fmt"
+	"net/url"
 	"testing"
 
+	dskitflagext "github.com/grafana/dskit/flagext"
+	"github.com/prometheus/common/model"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/yaml.v2"
+
+	"github.com/grafana/loki/clients/pkg/promtail/client"
+
+	"github.com/grafana/loki/pkg/util/flagext"
 )
 
 const testFile = `
@@ -28,8 +36,91 @@ scrape_configs:
 `
 
 func Test_Load(t *testing.T) {
-
 	var dst Config
 	err := yaml.Unmarshal([]byte(testFile), &dst)
 	require.Nil(t, err)
 }
+
+func TestConfig_Setup(t *testing.T) {
+	for i, tt := range []struct {
+		in       Config
+		expected Config
+	}{
+		{
+			Config{
+				ClientConfig: client.Config{
+					ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"foo": "bar"}},
+				},
+				ClientConfigs: []client.Config{
+					{
+						ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"client1": "1"}},
+					},
+					{
+						ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"client2": "2"}},
+					},
+				},
+			},
+			Config{
+				ClientConfig: client.Config{
+					ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"foo": "bar"}},
+				},
+				ClientConfigs: []client.Config{
+					{
+						ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"client1": "1", "foo": "bar"}},
+					},
+					{
+						ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"client2": "2", "foo": "bar"}},
+					},
+				},
+			},
+		},
+		{
+			Config{
+				ClientConfig: client.Config{
+					ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"foo": "bar"}},
+					URL:            dskitflagext.URLValue{URL: mustURL("http://foo")},
+				},
+				ClientConfigs: []client.Config{
+					{
+						ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"client1": "1"}},
+					},
+					{
+						ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"client2": "2"}},
+					},
+				},
+			},
+			Config{
+				ClientConfig: client.Config{
+					ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"foo": "bar"}},
+					URL:            dskitflagext.URLValue{URL: mustURL("http://foo")},
+				},
+				ClientConfigs: []client.Config{
+					{
+						ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"client1": "1", "foo": "bar"}},
+					},
+					{
+						ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"client2": "2", "foo": "bar"}},
+					},
+					{
+						ExternalLabels: flagext.LabelSet{LabelSet: model.LabelSet{"foo": "bar"}},
+						URL:            dskitflagext.URLValue{URL: mustURL("http://foo")},
+					},
+				},
+			},
+		},
+	} {
+		tt := tt
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			tt.in.Setup()
+			require.Equal(t, tt.expected, tt.in)
+		})
+	}
+}
+
+func mustURL(u string) *url.URL {
+	res, err := url.Parse(u)
+	if err != nil {
+		panic(err)
+	}
+	return res
+}

clients/pkg/promtail/promtail.go

@@ -55,29 +55,17 @@ func New(cfg config.Config, dryRun bool, opts ...Option) (*Promtail, error) {
 		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
-		cfg.ClientConfigs = append(cfg.ClientConfigs, cfg.ClientConfig)
-	}
-
-	// This is a bit crude but if the Loki Push API target is specified,
-	// force the log level to match the promtail log level
-	for i := range cfg.ScrapeConfig {
-		if cfg.ScrapeConfig[i].PushConfig != nil {
-			cfg.ScrapeConfig[i].PushConfig.Server.LogLevel = cfg.ServerConfig.LogLevel
-			cfg.ScrapeConfig[i].PushConfig.Server.LogFormat = cfg.ServerConfig.LogFormat
-		}
-	}
+	cfg.Setup()
 
 	var err error
 	if dryRun {
-		promtail.client, err = client.NewLogger(prometheus.DefaultRegisterer, promtail.logger, cfg.ClientConfig.ExternalLabels, cfg.ClientConfigs...)
+		promtail.client, err = client.NewLogger(prometheus.DefaultRegisterer, promtail.logger, cfg.ClientConfigs...)
 		if err != nil {
 			return nil, err
 		}
 		cfg.PositionsConfig.ReadOnly = true
 	} else {
-		promtail.client, err = client.NewMulti(prometheus.DefaultRegisterer, promtail.logger, cfg.ClientConfig.ExternalLabels, cfg.ClientConfigs...)
+		promtail.client, err = client.NewMulti(prometheus.DefaultRegisterer, promtail.logger, cfg.ClientConfigs...)
 		if err != nil {
 			return nil, err
 		}