exporter/signalfx: Remove send_compatible_metrics option (#2267)

* exporter/signalfx: Remove send_compatible_metrics option

This is a **breaking change**. Remove `send_compatible_metrics` options. Instead
just use the `translation_rules` field to determine whether or not compatible
metrics are to be sent. If `translation_rules` is not specified, default rules
will be applied and compatible metrics will be sent. Compatible metrics can be
turned off by setting it to `[]`.

* Update comment in custom unmarshaler

Author: asuresh4

exporter/signalfxexporter/README.md

@@ -42,9 +42,9 @@ The following configuration options can also be configured:
   receiver](../../receiver/signalfxreceiver/README.md) to preserve datapoint
   origin.
 - `exclude_metrics`: List of metric filters that will determine metrics to be
-  excluded from sending to Signalfx backend. If `send_compatible_metrics`
-  or `translation_rules` options are enabled, the exclusion will be applied
-  on translated metrics. See [here](./testdata/config.yaml) for examples.
+  excluded from sending to Signalfx backend. If `translation_rules` options
+  are enabled, the exclusion will be applied on translated metrics.
+  See [here](./testdata/config.yaml) for examples.
 - `include_metrics`: List of filters to override exclusion of any metrics.
   This option can be used to included metrics that are otherwise dropped by
   default. See [here](./translation/default_metrics.go) for a list of metrics
@@ -62,13 +62,11 @@ The following configuration options can also be configured:
 - `headers` (no default): Headers to pass in the payload.
 - `log_dimension_updates` (default = `false`): Whether or not to log dimension
   updates.
-- `send_compatible_metrics` (default = `false`): Whether metrics must be
-  translated to a format backward-compatible with SignalFx naming conventions.
 - `timeout` (default = 5s): Amount of time to wait for a send operation to
   complete.
 - `translation_rules`: Set of rules on how to translate metrics to a SignalFx
   compatible format. Rules defined in `translation/constants.go` are used by
-  default. Applicable only when `send_compatible_metrics` set to `true`.
+  default. Set this option to `[]` to override the default behavior.
 - `sync_host_metadata`: Defines whether the exporter should scrape host metadata
   and send it as property updates to SignalFx backend. Disabled by default.
   IMPORTANT: Host metadata synchronization relies on `resourcedetection`

exporter/signalfxexporter/config.go

@@ -29,6 +29,10 @@ import (
 	"github.com/open-telemetry/opentelemetry-collector-contrib/internal/splunk"
 )
 
+const (
+	translationRulesConfigKey = "translation_rules"
+)
+
 // Config defines configuration for SignalFx exporter.
 type Config struct {
 	configmodels.ExporterSettings  `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct.
@@ -64,10 +68,6 @@ type Config struct {
 
 	splunk.AccessTokenPassthroughConfig `mapstructure:",squash"`
 
-	// SendCompatibleMetrics specifies if metrics must be sent in a format backward-compatible with
-	// SignalFx naming conventions, "false" by default.
-	SendCompatibleMetrics bool `mapstructure:"send_compatible_metrics"`
-
 	// TranslationRules defines a set of rules how to translate metrics to a SignalFx compatible format
 	// Rules defined in translation/constants.go are used by default.
 	TranslationRules []translation.Rule `mapstructure:"translation_rules"`
@@ -120,12 +120,9 @@ func (cfg *Config) getOptionsFromConfig() (*exporterOptions, error) {
 		cfg.Timeout = 5 * time.Second
 	}
 
-	var metricTranslator *translation.MetricTranslator
-	if cfg.SendCompatibleMetrics {
-		metricTranslator, err = translation.NewMetricTranslator(cfg.TranslationRules, cfg.DeltaTranslationTTL)
-		if err != nil {
-			return nil, fmt.Errorf("invalid \"translation_rules\": %v", err)
-		}
+	metricTranslator, err := translation.NewMetricTranslator(cfg.TranslationRules, cfg.DeltaTranslationTTL)
+	if err != nil {
+		return nil, fmt.Errorf("invalid \"%s\": %v", translationRulesConfigKey, err)
 	}
 
 	return &exporterOptions{

exporter/signalfxexporter/config_test.go

@@ -54,6 +54,9 @@ func TestLoadConfig(t *testing.T) {
 	// Realm doesn't have a default value so set it directly.
 	defaultCfg := factory.CreateDefaultConfig().(*Config)
 	defaultCfg.Realm = "ap0"
+	defaultTranslationRules, err := loadDefaultTranslationRules()
+	require.NoError(t, err)
+	defaultCfg.TranslationRules = defaultTranslationRules
 	assert.Equal(t, defaultCfg, e0)
 
 	expectedName := "signalfx/allsettings"
@@ -86,7 +89,6 @@ func TestLoadConfig(t *testing.T) {
 		}, AccessTokenPassthroughConfig: splunk.AccessTokenPassthroughConfig{
 			AccessTokenPassthrough: false,
 		},
-		SendCompatibleMetrics: true,
 		TranslationRules: []translation.Rule{
 			{
 				Action: translation.ActionRenameDimensionKeys,
@@ -167,17 +169,21 @@ func TestLoadConfig(t *testing.T) {
 }
 
 func TestConfig_getOptionsFromConfig(t *testing.T) {
+	emptyTranslator := func() *translation.MetricTranslator {
+		translator, err := translation.NewMetricTranslator(nil, 3600)
+		require.NoError(t, err)
+		return translator
+	}
 	type fields struct {
-		ExporterSettings      configmodels.ExporterSettings
-		AccessToken           string
-		Realm                 string
-		IngestURL             string
-		APIURL                string
-		Timeout               time.Duration
-		Headers               map[string]string
-		SendCompatibleMetrics bool
-		TranslationRules      []translation.Rule
-		SyncHostMetadata      bool
+		ExporterSettings configmodels.ExporterSettings
+		AccessToken      string
+		Realm            string
+		IngestURL        string
+		APIURL           string
+		Timeout          time.Duration
+		Headers          map[string]string
+		TranslationRules []translation.Rule
+		SyncHostMetadata bool
 	}
 	tests := []struct {
 		name    string
@@ -204,8 +210,9 @@ func TestConfig_getOptionsFromConfig(t *testing.T) {
 					Host:   "api.us1.signalfx.com",
 					Path:   "/",
 				},
-				httpTimeout: 5 * time.Second,
-				token:       "access_token",
+				httpTimeout:      5 * time.Second,
+				token:            "access_token",
+				metricTranslator: emptyTranslator(),
 			},
 			wantErr: false,
 		},
@@ -226,8 +233,9 @@ func TestConfig_getOptionsFromConfig(t *testing.T) {
 					Scheme: "https",
 					Host:   "api.us0.signalfx.com",
 				},
-				httpTimeout: 10 * time.Second,
-				token:       "access_token",
+				httpTimeout:      10 * time.Second,
+				token:            "access_token",
+				metricTranslator: emptyTranslator(),
 			},
 			wantErr: false,
 		},
@@ -270,9 +278,8 @@ func TestConfig_getOptionsFromConfig(t *testing.T) {
 		{
 			name: "Test invalid translation rules",
 			fields: fields{
-				Realm:                 "us0",
-				AccessToken:           "access_token",
-				SendCompatibleMetrics: true,
+				Realm:       "us0",
+				AccessToken: "access_token",
 				TranslationRules: []translation.Rule{
 					{
 						Action: translation.ActionRenameDimensionKeys,
@@ -294,11 +301,12 @@ func TestConfig_getOptionsFromConfig(t *testing.T) {
 				TimeoutSettings: exporterhelper.TimeoutSettings{
 					Timeout: tt.fields.Timeout,
 				},
-				Headers:               tt.fields.Headers,
-				SendCompatibleMetrics: tt.fields.SendCompatibleMetrics,
-				TranslationRules:      tt.fields.TranslationRules,
-				SyncHostMetadata:      tt.fields.SyncHostMetadata,
+				Headers:             tt.fields.Headers,
+				TranslationRules:    tt.fields.TranslationRules,
+				SyncHostMetadata:    tt.fields.SyncHostMetadata,
+				DeltaTranslationTTL: 3600,
 			}
+
 			got, err := cfg.getOptionsFromConfig()
 			if (err != nil) != tt.wantErr {
 				t.Errorf("getOptionsFromConfig() error = %v, wantErr %v", err, tt.wantErr)

exporter/signalfxexporter/factory.go

@@ -21,6 +21,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/spf13/viper"
 	"go.opentelemetry.io/collector/component"
 	otelconfig "go.opentelemetry.io/collector/config"
 	"go.opentelemetry.io/collector/config/configmodels"
@@ -49,6 +50,7 @@ func NewFactory() component.ExporterFactory {
 		exporterhelper.WithMetrics(createMetricsExporter),
 		exporterhelper.WithLogs(createLogsExporter),
 		exporterhelper.WithTraces(createTraceExporter),
+		exporterhelper.WithCustomUnmarshaler(customUnmarshaler),
 	)
 }
 
@@ -64,13 +66,32 @@ func createDefaultConfig() configmodels.Exporter {
 		AccessTokenPassthroughConfig: splunk.AccessTokenPassthroughConfig{
 			AccessTokenPassthrough: true,
 		},
-		SendCompatibleMetrics: false,
-		TranslationRules:      nil,
-		DeltaTranslationTTL:   3600,
-		Correlation:           correlation.DefaultConfig(),
+		DeltaTranslationTTL: 3600,
+		Correlation:         correlation.DefaultConfig(),
 	}
 }
 
+func customUnmarshaler(componentViperSection *viper.Viper, intoCfg interface{}) (err error) {
+	if componentViperSection == nil {
+		// Nothing to do if there is no config given.
+		return nil
+	}
+
+	if err = componentViperSection.Unmarshal(intoCfg); err != nil {
+		return err
+	}
+
+	config := intoCfg.(*Config)
+
+	// If translations_config is not set in the config, set it to the defaults and return.
+	if !componentViperSection.IsSet(translationRulesConfigKey) {
+		config.TranslationRules, err = loadDefaultTranslationRules()
+		return err
+	}
+
+	return nil
+}
+
 func createTraceExporter(
 	_ context.Context,
 	params component.ExporterCreateParams,
@@ -106,12 +127,8 @@ func createMetricsExporter(
 ) (component.MetricsExporter, error) {
 
 	expCfg := config.(*Config)
-	err := setTranslationRules(expCfg)
-	if err != nil {
-		return nil, err
-	}
 
-	err = setDefaultExcludes(expCfg)
+	err := setDefaultExcludes(expCfg)
 	if err != nil {
 		return nil, err
 	}
@@ -149,17 +166,6 @@ func createMetricsExporter(
 	}, nil
 }
 
-func setTranslationRules(cfg *Config) error {
-	if cfg.SendCompatibleMetrics && cfg.TranslationRules == nil {
-		defaultRules, err := loadDefaultTranslationRules()
-		if err != nil {
-			return err
-		}
-		cfg.TranslationRules = defaultRules
-	}
-	return nil
-}
-
 func loadDefaultTranslationRules() ([]translation.Rule, error) {
 	config := Config{}
 

exporter/signalfxexporter/factory_test.go

@@ -185,58 +185,6 @@ func TestFactory_CreateMetricsExporterFails(t *testing.T) {
 	}
 }
 
-func TestCreateMetricsExporterWithDefaultTranslationRules(t *testing.T) {
-	config := &Config{
-		ExporterSettings: configmodels.ExporterSettings{
-			TypeVal: configmodels.Type(typeStr),
-			NameVal: typeStr,
-		},
-		AccessToken:           "testToken",
-		Realm:                 "us1",
-		SendCompatibleMetrics: true,
-	}
-
-	te, err := createMetricsExporter(context.Background(), component.ExporterCreateParams{Logger: zap.NewNop()}, config)
-	assert.NoError(t, err)
-	assert.NotNil(t, te)
-
-	// Validate that default translation rules are loaded
-	// Expected values has to be updated once default config changed
-	assert.Equal(t, 64, len(config.TranslationRules))
-	assert.Equal(t, translation.ActionRenameDimensionKeys, config.TranslationRules[0].Action)
-	assert.Equal(t, 33, len(config.TranslationRules[0].Mapping))
-}
-
-func TestCreateMetricsExporterWithSpecifiedTranslaitonRules(t *testing.T) {
-	config := &Config{
-		ExporterSettings: configmodels.ExporterSettings{
-			TypeVal: configmodels.Type(typeStr),
-			NameVal: typeStr,
-		},
-		AccessToken:           "testToken",
-		Realm:                 "us1",
-		SendCompatibleMetrics: true,
-		TranslationRules: []translation.Rule{
-			{
-				Action: translation.ActionRenameDimensionKeys,
-				Mapping: map[string]string{
-					"old_dimension": "new_dimension",
-				},
-			},
-		},
-	}
-
-	te, err := createMetricsExporter(context.Background(), component.ExporterCreateParams{Logger: zap.NewNop()}, config)
-	assert.NoError(t, err)
-	assert.NotNil(t, te)
-
-	// Validate that specified translation rules are loaded instead of default ones
-	assert.Equal(t, 1, len(config.TranslationRules))
-	assert.Equal(t, translation.ActionRenameDimensionKeys, config.TranslationRules[0].Action)
-	assert.Equal(t, 1, len(config.TranslationRules[0].Mapping))
-	assert.Equal(t, "new_dimension", config.TranslationRules[0].Mapping["old_dimension"])
-}
-
 func TestDefaultTranslationRules(t *testing.T) {
 	rules, err := loadDefaultTranslationRules()
 	require.NoError(t, err)

exporter/signalfxexporter/go.mod

@@ -12,11 +12,13 @@ require (
 	github.com/shirou/gopsutil v3.21.1+incompatible
 	github.com/signalfx/com_signalfx_metrics_protobuf v0.0.2
 	github.com/signalfx/signalfx-agent/pkg/apm v0.0.0-20201202163743-65b4fa925fc8
+	github.com/spf13/viper v1.7.1
 	github.com/stretchr/testify v1.7.0
 	go.opentelemetry.io/collector v0.19.1-0.20210127225953-68c5961f7bc2
 	go.uber.org/multierr v1.6.0 // indirect
 	go.uber.org/zap v1.16.0
 	google.golang.org/protobuf v1.25.0
+	gopkg.in/yaml.v2 v2.4.0
 )
 
 replace github.com/open-telemetry/opentelemetry-collector-contrib/internal/common => ../../internal/common

exporter/signalfxexporter/testdata/config.yaml

@@ -24,7 +24,6 @@ exporters:
       added-entry: "added value"
       dot.test: test
     access_token_passthrough: false
-    send_compatible_metrics: true
     translation_rules:
     - action: rename_dimension_keys
       mapping: