[signalfxexporter] move initialization of the config defaults in Unmarshal. (#18207)

Signed-off-by: Bogdan Drutu <[email protected]>

Author: bogdandrutu

.chloggen/signalfxexporterconfig.yaml

@@ -0,0 +1,11 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
+component: signalfxexporter
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Move initialization of the config defaults in Unmarshal.
+
+# One or more tracking issues related to the change
+issues: [18207]

exporter/signalfxexporter/config.go

@@ -18,13 +18,13 @@ import (
 	"errors"
 	"fmt"
 	"net/url"
-	"time"
 
 	"go.opentelemetry.io/collector/config/confighttp"
 	"go.opentelemetry.io/collector/config/configopaque"
 	"go.opentelemetry.io/collector/config/configtls"
 	"go.opentelemetry.io/collector/confmap"
 	"go.opentelemetry.io/collector/exporter/exporterhelper"
+	"gopkg.in/yaml.v3"
 
 	"github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/internal/correlation"
 	"github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/internal/translation"
@@ -34,6 +34,7 @@ import (
 
 const (
 	translationRulesConfigKey = "translation_rules"
+	excludeMetricsConfigKey   = "exclude_metrics"
 )
 
 var _ confmap.Unmarshaler = (*Config)(nil)
@@ -151,27 +152,26 @@ func (cfg *Config) getAPIURL() (*url.URL, error) {
 	return apiURL, nil
 }
 
-func (cfg *Config) Unmarshal(componentParser *confmap.Conf) (err error) {
+func (cfg *Config) Unmarshal(componentParser *confmap.Conf) error {
 	if componentParser == nil {
 		// Nothing to do if there is no config given.
 		return nil
 	}
 
-	if err = componentParser.Unmarshal(cfg); err != nil {
+	if err := componentParser.Unmarshal(cfg); err != nil {
 		return err
 	}
 
-	// If translations_config is not set in the config, set it to the defaults and return.
+	// If translations_config is not set in the config, set it to the default.
 	if !componentParser.IsSet(translationRulesConfigKey) {
+		var err error
 		cfg.TranslationRules, err = loadDefaultTranslationRules()
-		return err
-	}
-
-	if cfg.HTTPClientSettings.Timeout == 0 {
-		cfg.HTTPClientSettings.Timeout = 5 * time.Second
+		if err != nil {
+			return err
+		}
 	}
 
-	return nil
+	return setDefaultExcludes(cfg)
 }
 
 // Validate checks if the exporter configuration is valid.
@@ -195,3 +195,35 @@ func (cfg *Config) Validate() error {
 
 	return nil
 }
+
+func setDefaultExcludes(cfg *Config) error {
+	exCfg, err := loadConfig([]byte(translation.DefaultExcludeMetricsYaml))
+	if err != nil {
+		return err
+	}
+
+	// If ExcludeMetrics is not set to empty, append defaults.
+	if cfg.ExcludeMetrics == nil || len(cfg.ExcludeMetrics) > 0 {
+		cfg.ExcludeMetrics = append(cfg.ExcludeMetrics, exCfg.ExcludeMetrics...)
+	}
+	return nil
+}
+
+func loadDefaultTranslationRules() ([]translation.Rule, error) {
+	cfg, err := loadConfig([]byte(translation.DefaultTranslationRulesYaml))
+	return cfg.TranslationRules, err
+}
+
+func loadConfig(bytes []byte) (Config, error) {
+	var cfg Config
+	var data map[string]interface{}
+	if err := yaml.Unmarshal(bytes, &data); err != nil {
+		return cfg, err
+	}
+
+	if err := confmap.NewFromStringMap(data).Unmarshal(&cfg, confmap.WithErrorUnused()); err != nil {
+		return cfg, fmt.Errorf("failed to load default exclude metrics: %w", err)
+	}
+
+	return cfg, nil
+}

exporter/signalfxexporter/config_test.go

@@ -26,6 +26,7 @@ import (
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/config/confighttp"
 	"go.opentelemetry.io/collector/config/configopaque"
+	"go.opentelemetry.io/collector/confmap"
 	"go.opentelemetry.io/collector/confmap/confmaptest"
 	"go.opentelemetry.io/collector/exporter/exporterhelper"
 
@@ -53,7 +54,7 @@ func TestLoadConfig(t *testing.T) {
 
 	tests := []struct {
 		id       component.ID
-		expected component.Config
+		expected *Config
 	}{
 		{
 			id:       component.NewIDWithName(typeStr, ""),
@@ -194,6 +195,8 @@ func TestLoadConfig(t *testing.T) {
 			require.NoError(t, component.UnmarshalConfig(sub, cfg))
 
 			assert.NoError(t, component.ValidateConfig(cfg))
+			// We need to add the default exclude rules.
+			assert.NoError(t, setDefaultExcludes(tt.expected))
 			assert.Equal(t, tt.expected, cfg)
 		})
 	}
@@ -404,3 +407,41 @@ func TestConfigValidateErrors(t *testing.T) {
 		})
 	}
 }
+
+func TestUnmarshalExcludeMetrics(t *testing.T) {
+	tests := []struct {
+		name              string
+		cfg               *Config
+		excludeMetricsLen int
+	}{
+		{
+			name:              "empty config",
+			cfg:               &Config{},
+			excludeMetricsLen: 12,
+		},
+		{
+			name: "existing exclude config",
+			cfg: &Config{
+				ExcludeMetrics: []dpfilters.MetricFilter{
+					{
+						MetricNames: []string{"metric1"},
+					},
+				},
+			},
+			excludeMetricsLen: 13,
+		},
+		{
+			name: "existing empty exclude config",
+			cfg: &Config{
+				ExcludeMetrics: []dpfilters.MetricFilter{},
+			},
+			excludeMetricsLen: 0,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require.NoError(t, tt.cfg.Unmarshal(confmap.NewFromStringMap(map[string]interface{}{})))
+			assert.Len(t, tt.cfg.ExcludeMetrics, tt.excludeMetricsLen)
+		})
+	}
+}

exporter/signalfxexporter/factory.go

@@ -22,15 +22,11 @@ import (
 
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/config/confighttp"
-	"go.opentelemetry.io/collector/confmap"
 	"go.opentelemetry.io/collector/exporter"
 	"go.opentelemetry.io/collector/exporter/exporterhelper"
 	"go.uber.org/zap"
-	"gopkg.in/yaml.v2"
 
 	"github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/internal/correlation"
-	"github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/internal/translation"
-	"github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/internal/translation/dpfilters"
 	"github.com/open-telemetry/opentelemetry-collector-contrib/internal/splunk"
 	"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchperresourceattr"
 )
@@ -111,14 +107,8 @@ func createMetricsExporter(
 	set exporter.CreateSettings,
 	config component.Config,
 ) (exporter.Metrics, error) {
-
 	cfg := config.(*Config)
 
-	err := setDefaultExcludes(cfg)
-	if err != nil {
-		return nil, err
-	}
-
 	exp, err := newSignalFxExporter(cfg, set)
 	if err != nil {
 		return nil, err
@@ -155,42 +145,6 @@ func createMetricsExporter(
 	}, nil
 }
 
-func loadDefaultTranslationRules() ([]translation.Rule, error) {
-	cfg, err := loadConfig([]byte(translation.DefaultTranslationRulesYaml))
-	return cfg.TranslationRules, err
-}
-
-// setDefaultExcludes appends default metrics to be excluded to the exclude_metrics option.
-func setDefaultExcludes(cfg *Config) error {
-	defaultExcludeMetrics, err := loadDefaultExcludes()
-	if err != nil {
-		return err
-	}
-	if cfg.ExcludeMetrics == nil || len(cfg.ExcludeMetrics) > 0 {
-		cfg.ExcludeMetrics = append(cfg.ExcludeMetrics, defaultExcludeMetrics...)
-	}
-	return nil
-}
-
-func loadDefaultExcludes() ([]dpfilters.MetricFilter, error) {
-	cfg, err := loadConfig([]byte(translation.DefaultExcludeMetricsYaml))
-	return cfg.ExcludeMetrics, err
-}
-
-func loadConfig(bytes []byte) (Config, error) {
-	var cfg Config
-	var data map[string]interface{}
-	if err := yaml.Unmarshal(bytes, &data); err != nil {
-		return cfg, err
-	}
-
-	if err := confmap.NewFromStringMap(data).Unmarshal(&cfg, confmap.WithErrorUnused()); err != nil {
-		return cfg, fmt.Errorf("failed to load default exclude metrics: %w", err)
-	}
-
-	return cfg, nil
-}
-
 func createLogsExporter(
 	ctx context.Context,
 	set exporter.CreateSettings,

exporter/signalfxexporter/factory_test.go

@@ -35,7 +35,6 @@ import (
 	"go.uber.org/zap"
 
 	"github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/internal/translation"
-	"github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/internal/translation/dpfilters"
 )
 
 func TestCreateDefaultConfig(t *testing.T) {
@@ -222,55 +221,6 @@ func requireDimension(t *testing.T, dims []*sfxpb.Dimension, key, val string) {
 	require.True(t, found, `missing dimension: %s`, key)
 }
 
-func TestCreateMetricsExporterWithDefaultExcludeMetrics(t *testing.T) {
-	config := &Config{
-		AccessToken: "testToken",
-		Realm:       "us1",
-	}
-
-	te, err := createMetricsExporter(context.Background(), exportertest.NewNopCreateSettings(), config)
-	require.NoError(t, err)
-	require.NotNil(t, te)
-
-	// Validate that default excludes are always loaded.
-	assert.Equal(t, 12, len(config.ExcludeMetrics))
-}
-
-func TestCreateMetricsExporterWithExcludeMetrics(t *testing.T) {
-	config := &Config{
-		AccessToken: "testToken",
-		Realm:       "us1",
-		ExcludeMetrics: []dpfilters.MetricFilter{
-			{
-				MetricNames: []string{"metric1"},
-			},
-		},
-	}
-
-	te, err := createMetricsExporter(context.Background(), exportertest.NewNopCreateSettings(), config)
-	require.NoError(t, err)
-	require.NotNil(t, te)
-
-	// Validate that default excludes are always loaded.
-	assert.Equal(t, 13, len(config.ExcludeMetrics))
-}
-
-func TestCreateMetricsExporterWithEmptyExcludeMetrics(t *testing.T) {
-	config := &Config{
-		AccessToken:    "testToken",
-		Realm:          "us1",
-		ExcludeMetrics: []dpfilters.MetricFilter{},
-	}
-
-	te, err := createMetricsExporter(context.Background(), exportertest.NewNopCreateSettings(), config)
-	require.NoError(t, err)
-	require.NotNil(t, te)
-
-	// Validate that default excludes are overridden when exclude metrics
-	// is explicitly set to an empty slice.
-	assert.Equal(t, 0, len(config.ExcludeMetrics))
-}
-
 func testMetricsData() pmetric.Metrics {
 	md := pmetric.NewMetrics()
 
@@ -596,7 +546,7 @@ func TestHostmetricsCPUTranslations(t *testing.T) {
 	require.Equal(t, 590, int(*cpuIdle[0].Value.IntValue))
 }
 
-func TestDefaultExcludes_translated(t *testing.T) {
+func TestDefaultExcludesTranslated(t *testing.T) {
 	f := NewFactory()
 	cfg := f.CreateDefaultConfig().(*Config)
 	require.NoError(t, setDefaultExcludes(cfg))

exporter/signalfxexporter/go.mod

@@ -25,7 +25,7 @@ require (
 	go.uber.org/multierr v1.9.0
 	go.uber.org/zap v1.24.0
 	golang.org/x/sys v0.4.0
-	gopkg.in/yaml.v2 v2.4.0
+	gopkg.in/yaml.v3 v3.0.1
 )
 
 require (
@@ -74,7 +74,6 @@ require (
 	google.golang.org/genproto v0.0.0-20221118155620-16455021b5e6 // indirect
 	google.golang.org/grpc v1.52.3 // indirect
 	google.golang.org/protobuf v1.28.1 // indirect
-	gopkg.in/yaml.v3 v3.0.1 // indirect
 )
 
 replace github.com/open-telemetry/opentelemetry-collector-contrib/internal/common => ../../internal/common

exporter/signalfxexporter/internal/translation/constants.go

@@ -14,11 +14,10 @@
 
 package translation // import "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/internal/translation"
 
-const (
-	// DefaultTranslationRulesYaml defines default translation rules that will be applied to metrics if
-	// config.TranslationRules not specified explicitly.
-	// Keep it in YAML format to be able to easily copy and paste it in config if modifications needed.
-	DefaultTranslationRulesYaml = `
+// DefaultTranslationRulesYaml defines default translation rules that will be applied to metrics if
+// config.TranslationRules not specified explicitly.
+// Keep it in YAML format to be able to easily copy and paste it in config if modifications needed.
+const DefaultTranslationRulesYaml = `
 translation_rules:
 # drops opencensus.resourcetype dimension from metrics generated by receivers written
 # using OC data structures. This rule can be removed once the k8s_cluster and kubeletstats
@@ -455,4 +454,3 @@ translation_rules:
     sf_temp.system.paging.operations.page_in: true
     sf_temp.system.paging.operations.page_out: true
 `
-)

receiver/signalfxreceiver/go.mod

@@ -79,7 +79,6 @@ require (
 	google.golang.org/genproto v0.0.0-20221118155620-16455021b5e6 // indirect
 	google.golang.org/grpc v1.52.3 // indirect
 	google.golang.org/protobuf v1.28.1 // indirect
-	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )