Stop unnecessary config duplication (#431)

* Changes to allow config to follow hierarchy * Follow config hierCarchy * Align yaml * Remove unnecessary check for nil * Fix for arguments in tests. * Clean up based on feedback and fixed lint issue. * Go fmt * Documentation updated and changelog * fmt of Yaml * Make defaultRemoteWrite name consistent * Make defaultRemoteWrite name consistent
grafana · Mar 1, 2021 · f9f9ff1 · f9f9ff1
1 parent 52844a5
commit f9f9ff1
Show file tree

Hide file tree

Showing 12 changed files with 51 additions and 30 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,8 @@ can be found at [#317](https://github.com/grafana/agent/issues/317).
 
 - [ENHANCEMENT] Support compression for trace export. (@mdisibio)
 
+- [ENHANCEMENT] Allow Prometheus URL configuration to propagate to instances and integrations, if not given. (@mattdurham)
+
 - [ENHANCEMENT] Go 1.16 is now used for all builds of the Agent. (@rfratto)
 
 - [BUGFIX] Native Darwin arm64 builds will no longer crash when writing metrics

diff --git a/docs/configuration-reference.md b/docs/configuration-reference.md
@@ -176,6 +176,11 @@ configs:
 # How to spawn instances based on instance configs. Supported values: shared,
 # distinct.
 [instance_mode: <string> | default = "shared"]
+
+# A list of remote_write targets. This is the global remote write list, it will propagate to the other remote writes
+# if they are not defined. If they are defined then they will not be overwritten. 
+remote_write:
+  - [<remote_write>]
 ```
 
 ### server_tls_config

diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -54,8 +54,10 @@ func (c *Config) ApplyDefaults() error {
 		c.Integrations.ListenPort = &c.Server.HTTPListenPort
 		c.Integrations.ListenHost = &c.Server.HTTPListenAddress
 		c.Integrations.ServerUsingTLS = c.Server.HTTPTLSConfig.TLSKeyPath != "" && c.Server.HTTPTLSConfig.TLSCertPath != ""
+		if len(c.Integrations.PrometheusRemoteWrite) == 0 {
+			c.Integrations.PrometheusRemoteWrite = c.Prometheus.RemoteWrite
+		}
 	}
-
 	return nil
 }
 

diff --git a/pkg/integrations/manager_test.go b/pkg/integrations/manager_test.go
@@ -91,7 +91,7 @@ func TestManager_ValidInstanceConfigs(t *testing.T) {
 	integrations := map[Config]Integration{icfg: mock}
 	im := instance.NewBasicManager(instance.DefaultBasicManagerConfig, log.NewNopLogger(), mockInstanceFactory, func(c *instance.Config) error {
 		globalConfig := prom_config.DefaultConfig.GlobalConfig
-		return c.ApplyDefaults(&globalConfig)
+		return c.ApplyDefaults(&globalConfig, nil)
 	})
 	m, err := newManager(mockManagerConfig(), log.NewNopLogger(), im, integrations)
 	require.NoError(t, err)
@@ -109,7 +109,7 @@ func TestManager_instanceConfigForIntegration(t *testing.T) {
 
 	im := instance.NewBasicManager(instance.DefaultBasicManagerConfig, log.NewNopLogger(), mockInstanceFactory, func(c *instance.Config) error {
 		globalConfig := prom_config.DefaultConfig.GlobalConfig
-		return c.ApplyDefaults(&globalConfig)
+		return c.ApplyDefaults(&globalConfig, nil)
 	})
 	m, err := newManager(mockManagerConfig(), log.NewNopLogger(), im, nil)
 	require.NoError(t, err)
@@ -131,7 +131,7 @@ func TestManager_NoIntegrationsScrape(t *testing.T) {
 	integrations := map[Config]Integration{icfg: mock}
 	im := instance.NewBasicManager(instance.DefaultBasicManagerConfig, log.NewNopLogger(), mockInstanceFactory, func(c *instance.Config) error {
 		globalConfig := prom_config.DefaultConfig.GlobalConfig
-		return c.ApplyDefaults(&globalConfig)
+		return c.ApplyDefaults(&globalConfig, nil)
 	})
 
 	cfg := mockManagerConfig()
@@ -160,7 +160,7 @@ func TestManager_NoIntegrationScrape(t *testing.T) {
 	integrations := map[Config]Integration{icfg: mock}
 	im := instance.NewBasicManager(instance.DefaultBasicManagerConfig, log.NewNopLogger(), mockInstanceFactory, func(c *instance.Config) error {
 		globalConfig := prom_config.DefaultConfig.GlobalConfig
-		return c.ApplyDefaults(&globalConfig)
+		return c.ApplyDefaults(&globalConfig, nil)
 	})
 
 	m, err := newManager(mockManagerConfig(), log.NewNopLogger(), im, integrations)

diff --git a/pkg/prom/agent.go b/pkg/prom/agent.go
@@ -37,15 +37,16 @@ type Config struct {
 	// Whether the Prometheus subsystem should be enabled.
 	Enabled bool `yaml:"-"`
 
-	Global                 config.GlobalConfig `yaml:"global"`
-	WALDir                 string              `yaml:"wal_directory"`
-	WALCleanupAge          time.Duration       `yaml:"wal_cleanup_age"`
-	WALCleanupPeriod       time.Duration       `yaml:"wal_cleanup_period"`
-	ServiceConfig          ha.Config           `yaml:"scraping_service"`
-	ServiceClientConfig    client.Config       `yaml:"scraping_service_client"`
-	Configs                []instance.Config   `yaml:"configs,omitempty"`
-	InstanceRestartBackoff time.Duration       `yaml:"instance_restart_backoff,omitempty"`
-	InstanceMode           instance.Mode       `yaml:"instance_mode"`
+	Global                 config.GlobalConfig           `yaml:"global"`
+	WALDir                 string                        `yaml:"wal_directory"`
+	WALCleanupAge          time.Duration                 `yaml:"wal_cleanup_age"`
+	WALCleanupPeriod       time.Duration                 `yaml:"wal_cleanup_period"`
+	ServiceConfig          ha.Config                     `yaml:"scraping_service"`
+	ServiceClientConfig    client.Config                 `yaml:"scraping_service_client"`
+	Configs                []instance.Config             `yaml:"configs,omitempty"`
+	InstanceRestartBackoff time.Duration                 `yaml:"instance_restart_backoff,omitempty"`
+	InstanceMode           instance.Mode                 `yaml:"instance_mode"`
+	RemoteWrite            []*instance.RemoteWriteConfig `yaml:"remote_write,omitempty"`
 }
 
 // UnmarshalYAML implements yaml.Unmarshaler.
@@ -74,7 +75,7 @@ func (c *Config) ApplyDefaults() error {
 
 	for i := range c.Configs {
 		name := c.Configs[i].Name
-		if err := c.Configs[i].ApplyDefaults(&c.Global); err != nil {
+		if err := c.Configs[i].ApplyDefaults(&c.Global, c.RemoteWrite); err != nil {
 			// Try to show a helpful name in the error
 			if name == "" {
 				name = fmt.Sprintf("at index %d", i)
@@ -172,7 +173,7 @@ func newAgent(reg prometheus.Registerer, cfg Config, logger log.Logger, fact ins
 
 	if cfg.ServiceConfig.Enabled {
 		var err error
-		a.ha, err = ha.New(reg, cfg.ServiceConfig, &cfg.Global, cfg.ServiceClientConfig, a.logger, a.mm)
+		a.ha, err = ha.New(reg, cfg.ServiceConfig, &cfg.Global, cfg.ServiceClientConfig, a.logger, a.mm, cfg.RemoteWrite)
 		if err != nil {
 			return nil, err
 		}
@@ -197,7 +198,7 @@ func (a *Agent) newInstance(c instance.Config) (instance.ManagedInstance, error)
 }
 
 func (a *Agent) validateInstance(c *instance.Config) error {
-	return c.ApplyDefaults(&a.cfg.Global)
+	return c.ApplyDefaults(&a.cfg.Global, c.RemoteWrite)
 }
 
 func (a *Agent) WireGRPC(s *grpc.Server) {

diff --git a/pkg/prom/ha/http.go b/pkg/prom/ha/http.go
@@ -160,7 +160,7 @@ func (s *Server) PutConfiguration(r *http.Request) (interface{}, error) {
 	}
 
 	// Validate the incoming config
-	if err := inst.ApplyDefaults(s.globalConfig); err != nil {
+	if err := inst.ApplyDefaults(s.globalConfig, s.defaultRemoteWrite); err != nil {
 		return nil, err
 	}
 

diff --git a/pkg/prom/ha/http_test.go b/pkg/prom/ha/http_test.go
@@ -135,7 +135,7 @@ func TestServer_PutConfiguration(t *testing.T) {
 		MetricsPath: "/metrics",
 		Scheme:      "http",
 	}}
-	_ = cfg.ApplyDefaults(&config.DefaultGlobalConfig)
+	_ = cfg.ApplyDefaults(&config.DefaultGlobalConfig, nil)
 
 	bb, err := yaml.Marshal(cfg)
 	require.NoError(t, err)
@@ -174,7 +174,7 @@ func TestServer_PutConfiguration_URLEncoded(t *testing.T) {
 		MetricsPath: "/metrics",
 		Scheme:      "http",
 	}}
-	_ = cfg.ApplyDefaults(&config.DefaultGlobalConfig)
+	_ = cfg.ApplyDefaults(&config.DefaultGlobalConfig, nil)
 
 	bb, err := yaml.Marshal(cfg)
 	require.NoError(t, err)
@@ -213,7 +213,7 @@ func TestServer_PutConfiguration_NonUnique(t *testing.T) {
 		MetricsPath: "/metrics",
 		Scheme:      "http",
 	}}
-	_ = conflictA.ApplyDefaults(&config.DefaultGlobalConfig)
+	_ = conflictA.ApplyDefaults(&config.DefaultGlobalConfig, nil)
 
 	//
 	// Put conflict A; it should succeed
@@ -371,7 +371,7 @@ func newAPITestEnvironment(t *testing.T) apiTestEnvironment {
 			Prefix: "configs/",
 		},
 		Lifecycler: testLifecyclerConfig(),
-	}, &config.DefaultGlobalConfig, haClient.Config{}, logger, newFakeInstanceManager())
+	}, &config.DefaultGlobalConfig, haClient.Config{}, logger, newFakeInstanceManager(), nil)
 	require.NoError(t, err)
 
 	// Wire the API

diff --git a/pkg/prom/ha/server.go b/pkg/prom/ha/server.go
@@ -101,10 +101,12 @@ type Server struct {
 	exited chan bool
 
 	closeDependencies func() error
+
+	defaultRemoteWrite []*instance.RemoteWriteConfig
 }
 
 // New creates a new HA scraping service instance.
-func New(reg prometheus.Registerer, cfg Config, globalConfig *config.GlobalConfig, clientConfig client.Config, logger log.Logger, im instance.Manager) (*Server, error) {
+func New(reg prometheus.Registerer, cfg Config, globalConfig *config.GlobalConfig, clientConfig client.Config, logger log.Logger, im instance.Manager, defaultRemoteWrite []*instance.RemoteWriteConfig) (*Server, error) {
 	// Force ReplicationFactor to be 1, since replication isn't supported for the
 	// scraping service yet.
 	cfg.Lifecycler.RingConfig.ReplicationFactor = 1
@@ -157,6 +159,8 @@ func New(reg prometheus.Registerer, cfg Config, globalConfig *config.GlobalConfi
 		// The lifecycler must stop first since the shutdown process depends on the
 		// ring still polling.
 		stopServices(lc, r),
+
+		defaultRemoteWrite,
 	)
 
 	lazy.inner = s
@@ -178,7 +182,7 @@ func newRing(cfg ring.Config, name, key string, reg prometheus.Registerer) (*rin
 }
 
 // newServer creates a new Server. Abstracted from New for testing.
-func newServer(cfg Config, globalCfg *config.GlobalConfig, clientCfg client.Config, log log.Logger, im instance.Manager, addr string, r ReadRing, kv kv.Client, stopFunc func() error) *Server {
+func newServer(cfg Config, globalCfg *config.GlobalConfig, clientCfg client.Config, log log.Logger, im instance.Manager, addr string, r ReadRing, kv kv.Client, stopFunc func() error, defaultRemoteWrite []*instance.RemoteWriteConfig) *Server {
 	ctx, cancel := context.WithCancel(context.Background())
 
 	s := &Server{
@@ -197,6 +201,8 @@ func newServer(cfg Config, globalCfg *config.GlobalConfig, clientCfg client.Conf
 		cancel:            cancel,
 		exited:            make(chan bool),
 		closeDependencies: stopFunc,
+
+		defaultRemoteWrite: defaultRemoteWrite,
 	}
 
 	go s.run(ctx)

diff --git a/pkg/prom/ha/server_test.go b/pkg/prom/ha/server_test.go
@@ -215,7 +215,7 @@ func newTestServer(r ReadRing, kv kv.Client, im instance.Manager, reshard time.D
 	logger := log.NewNopLogger()
 	closer := func() error { return nil }
 
-	return newServer(cfg, &config.DefaultGlobalConfig, clientConfig, logger, im, "test", r, kv, closer)
+	return newServer(cfg, &config.DefaultGlobalConfig, clientConfig, logger, im, "test", r, kv, closer, nil)
 }
 
 func getRunningConfigs(im instance.Manager) []string {

diff --git a/pkg/prom/instance/instance.go b/pkg/prom/instance/instance.go
@@ -114,7 +114,7 @@ func (c Config) MarshalYAML() (interface{}, error) {
 // ApplyDefaults applies default configurations to the configuration to all
 // values that have not been changed to their non-zero value. ApplyDefaults
 // also validates the config.
-func (c *Config) ApplyDefaults(global *config.GlobalConfig) error {
+func (c *Config) ApplyDefaults(global *config.GlobalConfig, defaultRemoteWrite []*RemoteWriteConfig) error {
 	switch {
 	case c.Name == "":
 		return errors.New("missing instance name")
@@ -158,6 +158,11 @@ func (c *Config) ApplyDefaults(global *config.GlobalConfig) error {
 	}
 
 	rwNames := map[string]struct{}{}
+
+	// If the instance remote write is not filled in, then apply the prometheus write config
+	if len(c.RemoteWrite) == 0 {
+		c.RemoteWrite = defaultRemoteWrite
+	}
 	for _, cfg := range c.RemoteWrite {
 		if cfg == nil {
 			return fmt.Errorf("empty or null remote write config section")

diff --git a/pkg/prom/instance/instance_integration_test.go b/pkg/prom/instance/instance_integration_test.go
@@ -251,6 +251,6 @@ remote_write: []
 func loadConfig(t *testing.T, s string) Config {
 	cfg, err := UnmarshalConfig(strings.NewReader(s))
 	require.NoError(t, err)
-	require.NoError(t, cfg.ApplyDefaults(&config.DefaultGlobalConfig))
+	require.NoError(t, cfg.ApplyDefaults(&config.DefaultGlobalConfig, nil))
 	return *cfg
 }
diff --git a/pkg/prom/instance/instance_test.go b/pkg/prom/instance/instance_test.go
@@ -39,7 +39,7 @@ remote_write:
 	cfg, err := UnmarshalConfig(strings.NewReader(cfgText))
 	require.NoError(t, err)
 
-	err = cfg.ApplyDefaults(&global)
+	err = cfg.ApplyDefaults(&global, nil)
 	require.NoError(t, err)
 
 	require.Equal(t, DefaultConfig.HostFilter, cfg.HostFilter)
@@ -162,7 +162,7 @@ func TestConfig_ApplyDefaults_Validations(t *testing.T) {
 				tc.mutation(&input)
 			}
 
-			err := input.ApplyDefaults(&global)
+			err := input.ApplyDefaults(&global, nil)
 			if tc.err == nil {
 				require.NoError(t, err)
 			} else {
@@ -185,7 +185,7 @@ remote_write:
 
 	cfg, err := UnmarshalConfig(strings.NewReader(cfgText))
 	require.NoError(t, err)
-	require.NoError(t, cfg.ApplyDefaults(&global))
+	require.NoError(t, cfg.ApplyDefaults(&global, nil))
 	require.NotEmpty(t, cfg.RemoteWrite[0].Base.Name)
 }