[exporter/datadog] Move Sanitize functionality into Validate and …

…`Unmarshal` (#8829)

* [exporter/datadog] Move validation to `Validate`

* [exporter/datadog] Move some unmarshaling steps to `Unmarshal`; deprecate `Sanitize`

* [exporter/datadog] Move endpoint override logic to `Unmarshal`

* [exporter/datadog] Remove unnecessary calls to `Sanitize`

* Fix changelog

* Update deprecation version

CHANGELOG.md

@@ -6,6 +6,8 @@
 
 ### 🚩 Deprecations 🚩
 
+- `datadogexporter`: Deprecate `Sanitize` method of `Config` struct (#8829)
+
 ### 🚀 New components 🚀
 
 - `expvarreceiver`: Include `expvarreceiver` in components (#10847)
@@ -14,6 +16,7 @@
 
 - `transformprocessor`: Add byte slice literal to the grammar.  Add new SpanID and TraceID functions that take a byte slice and return a Span/Trace ID. (#10487)
 - `elasticsearchreceiver`: Add integration test for elasticsearch receiver (#10165)
+- `datadogexporter`: Some config validation and unmarshaling steps are now done on `Validate` and `Unmarshal` instead of `Sanitize` (#8829)
 
 ### 🧰 Bug fixes 🧰
 

exporter/datadogexporter/config/config.go

@@ -18,6 +18,7 @@ import (
 	"encoding"
 	"errors"
 	"fmt"
+	"os"
 	"regexp"
 	"strings"
 
@@ -435,11 +436,16 @@ type Config struct {
 }
 
 // Sanitize tries to sanitize a given configuration
+// Deprecated: [v0.54.0] Will be unexported in a future minor version.
 func (c *Config) Sanitize(logger *zap.Logger) error {
-	if c.TagsConfig.Env == "" {
-		c.TagsConfig.Env = "none"
+	for _, err := range c.warnings {
+		logger.Warn(fmt.Sprintf("Deprecated: %v", err))
 	}
 
+	return nil
+}
+
+func (c *Config) Validate() error {
 	if c.OnlyMetadata && (!c.HostMetadata.Enabled || c.HostMetadata.HostnameSource != HostnameSourceFirstResource) {
 		return errNoMetadata
 	}
@@ -452,30 +458,6 @@ func (c *Config) Sanitize(logger *zap.Logger) error {
 		return errUnsetAPIKey
 	}
 
-	c.API.Key = strings.TrimSpace(c.API.Key)
-
-	// Set default site
-	if c.API.Site == "" {
-		c.API.Site = "datadoghq.com"
-	}
-
-	// Set the endpoint based on the Site
-	if c.Metrics.TCPAddr.Endpoint == "" {
-		c.Metrics.TCPAddr.Endpoint = fmt.Sprintf("https://api.%s", c.API.Site)
-	}
-
-	if c.Traces.TCPAddr.Endpoint == "" {
-		c.Traces.TCPAddr.Endpoint = fmt.Sprintf("https://trace.agent.%s", c.API.Site)
-	}
-
-	for _, err := range c.warnings {
-		logger.Warn(fmt.Sprintf("Deprecated: %v", err))
-	}
-
-	return nil
-}
-
-func (c *Config) Validate() error {
 	if c.Traces.IgnoreResources != nil {
 		for _, entry := range c.Traces.IgnoreResources {
 			_, err := regexp.Compile(entry)
@@ -514,6 +496,18 @@ func (c *Config) Unmarshal(configMap *confmap.Conf) error {
 		return err
 	}
 
+	c.API.Key = strings.TrimSpace(c.API.Key)
+
+	// If an endpoint is not explicitly set, override it based on the site.
+	// TODO (#8396) Remove DD_URL check.
+	if !configMap.IsSet("metrics::endpoint") && os.Getenv("DD_URL") == "" {
+		c.Metrics.TCPAddr.Endpoint = fmt.Sprintf("https://api.%s", c.API.Site)
+	}
+	// TODO (#8396) Remove DD_APM_URL check.
+	if !configMap.IsSet("traces::endpoint") && os.Getenv("DD_APM_URL") == "" {
+		c.Traces.TCPAddr.Endpoint = fmt.Sprintf("https://trace.agent.%s", c.API.Site)
+	}
+
 	// Add deprecation warnings for deprecated settings.
 	renamingWarnings, err := handleRenamedSettings(configMap, c)
 	if err != nil {
@@ -531,7 +525,7 @@ func (c *Config) Unmarshal(configMap *confmap.Conf) error {
 	if c.Version != "" {
 		c.warnings = append(c.warnings, fmt.Errorf(deprecationTemplate, "version", "v0.52.0", 8783))
 	}
-	if c.Env != "" {
+	if configMap.IsSet("env") {
 		c.warnings = append(c.warnings, fmt.Errorf(deprecationTemplate, "env", "v0.52.0", 9016))
 	}
 	if c.Traces.SampleRate != 0 {

exporter/datadogexporter/config/config_test.go

@@ -18,10 +18,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"go.opentelemetry.io/collector/config/confignet"
 	"go.opentelemetry.io/collector/confmap"
-	"go.uber.org/zap"
 )
 
 func TestHostTags(t *testing.T) {
@@ -81,103 +78,108 @@ func TestHostTags(t *testing.T) {
 	)
 }
 
-// TestOverrideMetricsURL tests that the metrics URL is overridden
-// correctly when set manually.
-func TestOverrideMetricsURL(t *testing.T) {
+func TestValidate(t *testing.T) {
 
-	const DebugEndpoint string = "http://localhost:8080"
-
-	cfg := Config{
-		API: APIConfig{Key: "notnull", Site: DefaultSite},
-		Metrics: MetricsConfig{
-			TCPAddr: confignet.TCPAddr{
-				Endpoint: DebugEndpoint,
+	tests := []struct {
+		name string
+		cfg  *Config
+		err  string
+	}{
+		{
+			name: "no api::key",
+			cfg:  &Config{},
+			err:  errUnsetAPIKey.Error(),
+		},
+		{
+			name: "invalid hostname",
+			cfg: &Config{
+				API:        APIConfig{Key: "notnull"},
+				TagsConfig: TagsConfig{Hostname: "invalid_host"},
 			},
+			err: "hostname field is invalid: 'invalid_host' is not RFC1123 compliant",
 		},
-	}
-
-	err := cfg.Sanitize(zap.NewNop())
-	require.NoError(t, err)
-	assert.Equal(t, cfg.Metrics.Endpoint, DebugEndpoint)
-}
-
-// TestDefaultSite tests that the Site option is set to the
-// default value when no value was set prior to running Sanitize
-func TestDefaultSite(t *testing.T) {
-	cfg := Config{
-		API: APIConfig{Key: "notnull"},
-	}
-
-	err := cfg.Sanitize(zap.NewNop())
-	require.NoError(t, err)
-	assert.Equal(t, cfg.API.Site, DefaultSite)
-}
-
-func TestDefaultTLSSettings(t *testing.T) {
-	cfg := Config{
-		API: APIConfig{Key: "notnull"},
-	}
-
-	err := cfg.Sanitize(zap.NewNop())
-	require.NoError(t, err)
-	assert.Equal(t, cfg.LimitedHTTPClientSettings.TLSSetting.InsecureSkipVerify, false)
-}
-
-func TestTLSSettings(t *testing.T) {
-	cfg := Config{
-		API: APIConfig{Key: "notnull"},
-		LimitedHTTPClientSettings: LimitedHTTPClientSettings{
-			TLSSetting: LimitedTLSClientSettings{
-				InsecureSkipVerify: true,
+		{
+			name: "no metadata",
+			cfg: &Config{
+				API:          APIConfig{Key: "notnull"},
+				OnlyMetadata: true,
+				SendMetadata: false,
+			},
+			err: errNoMetadata.Error(),
+		},
+		{
+			name: "span name remapping valid",
+			cfg: &Config{
+				API:    APIConfig{Key: "notnull"},
+				Traces: TracesConfig{SpanNameRemappings: map[string]string{"old.opentelemetryspan.name": "updated.name"}},
+			},
+		},
+		{
+			name: "span name remapping empty val",
+			cfg: &Config{
+				API:    APIConfig{Key: "notnull"},
+				Traces: TracesConfig{SpanNameRemappings: map[string]string{"oldname": ""}},
+			},
+			err: "'' is not valid value for span name remapping",
+		},
+		{
+			name: "span name remapping empty key",
+			cfg: &Config{
+				API:    APIConfig{Key: "notnull"},
+				Traces: TracesConfig{SpanNameRemappings: map[string]string{"": "newname"}},
+			},
+			err: "'' is not valid key for span name remapping",
+		},
+		{
+			name: "ignore resources valid",
+			cfg: &Config{
+				API:    APIConfig{Key: "notnull"},
+				Traces: TracesConfig{IgnoreResources: []string{"[123]"}},
+			},
+		},
+		{
+			name: "ignore resources missing bracket",
+			cfg: &Config{
+				API:    APIConfig{Key: "notnull"},
+				Traces: TracesConfig{IgnoreResources: []string{"[123"}},
+			},
+			err: "'[123' is not valid resource filter regular expression",
+		},
+		{
+			name: "invalid histogram settings",
+			cfg: &Config{
+				API: APIConfig{Key: "notnull"},
+				Metrics: MetricsConfig{
+					HistConfig: HistogramConfig{
+						Mode:         HistogramModeNoBuckets,
+						SendCountSum: false,
+					},
+				},
+			},
+			err: "'nobuckets' mode and `send_count_sum_metrics` set to false will send no histogram metrics",
+		},
+		{
+			name: "TLS settings are valid",
+			cfg: &Config{
+				API: APIConfig{Key: "notnull"},
+				LimitedHTTPClientSettings: LimitedHTTPClientSettings{
+					TLSSetting: LimitedTLSClientSettings{
+						InsecureSkipVerify: true,
+					},
+				},
 			},
 		},
 	}
-
-	err := cfg.Sanitize(zap.NewNop())
-	require.NoError(t, err)
-	assert.Equal(t, cfg.TLSSetting.InsecureSkipVerify, true)
-}
-
-func TestAPIKeyUnset(t *testing.T) {
-	cfg := Config{}
-	err := cfg.Sanitize(zap.NewNop())
-	assert.Equal(t, err, errUnsetAPIKey)
-}
-
-func TestNoMetadata(t *testing.T) {
-	cfg := Config{
-		OnlyMetadata: true,
-		SendMetadata: false,
+	for _, testInstance := range tests {
+		t.Run(testInstance.name, func(t *testing.T) {
+			err := testInstance.cfg.Validate()
+			if testInstance.err != "" {
+				assert.EqualError(t, err, testInstance.err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
 	}
-
-	err := cfg.Sanitize(zap.NewNop())
-	assert.Equal(t, err, errNoMetadata)
-}
-
-func TestInvalidHostname(t *testing.T) {
-	cfg := Config{TagsConfig: TagsConfig{Hostname: "invalid_host"}}
-
-	err := cfg.Sanitize(zap.NewNop())
-	require.Error(t, err)
-}
-
-func TestIgnoreResourcesValidation(t *testing.T) {
-	validCfg := Config{Traces: TracesConfig{IgnoreResources: []string{"[123]"}}}
-	invalidCfg := Config{Traces: TracesConfig{IgnoreResources: []string{"[123"}}}
-
-	noErr := validCfg.Validate()
-	err := invalidCfg.Validate()
-	require.NoError(t, noErr)
-	require.Error(t, err)
-}
-
-func TestSpanNameRemappingsValidation(t *testing.T) {
-	validCfg := Config{Traces: TracesConfig{SpanNameRemappings: map[string]string{"old.opentelemetryspan.name": "updated.name"}}}
-	invalidCfg := Config{Traces: TracesConfig{SpanNameRemappings: map[string]string{"oldname": ""}}}
-	noErr := validCfg.Validate()
-	err := invalidCfg.Validate()
-	require.NoError(t, noErr)
-	require.Error(t, err)
 }
 
 func TestUnmarshal(t *testing.T) {

exporter/datadogexporter/config/warn_envvars.go

@@ -19,6 +19,7 @@ import (
 	"time"
 
 	"go.opentelemetry.io/collector/config"
+	"go.opentelemetry.io/collector/config/confignet"
 	"go.opentelemetry.io/collector/confmap"
 	"go.opentelemetry.io/collector/exporter/exporterhelper"
 )
@@ -30,9 +31,18 @@ func futureDefaultConfig() *Config {
 		TimeoutSettings: exporterhelper.TimeoutSettings{
 			Timeout: 15 * time.Second,
 		},
+		API: APIConfig{
+			Site: "datadoghq.com",
+		},
+		TagsConfig: TagsConfig{
+			Env: "none",
+		},
 		RetrySettings: exporterhelper.NewDefaultRetrySettings(),
 		QueueSettings: exporterhelper.NewDefaultQueueSettings(),
 		Metrics: MetricsConfig{
+			TCPAddr: confignet.TCPAddr{
+				Endpoint: "https://api.datadoghq.com",
+			},
 			SendMonotonic: true,
 			DeltaTTL:      3600,
 			Quantiles:     true,
@@ -52,6 +62,9 @@ func futureDefaultConfig() *Config {
 			},
 		},
 		Traces: TracesConfig{
+			TCPAddr: confignet.TCPAddr{
+				Endpoint: "https://trace.agent.datadoghq.com",
+			},
 			IgnoreResources: []string{},
 		},
 		HostMetadata: HostMetadataConfig{