diff --git a/ddtrace/tracer/log.go b/ddtrace/tracer/log.go index 9e4148b7f3..71dc2e9f74 100644 --- a/ddtrace/tracer/log.go +++ b/ddtrace/tracer/log.go @@ -130,7 +130,7 @@ func logStartup(t *tracer) { Env: t.config.env, Service: t.config.serviceName, AgentURL: agentURL, - Debug: t.config.debug, + Debug: t.config.internalConfig.Debug(), AnalyticsEnabled: !math.IsNaN(globalconfig.AnalyticsRate()), SampleRate: fmt.Sprintf("%f", t.rulesSampling.traces.globalRate), SampleRateLimit: "disabled", diff --git a/ddtrace/tracer/option.go b/ddtrace/tracer/option.go index d86bcbee79..9c7e3957c8 100644 --- a/ddtrace/tracer/option.go +++ b/ddtrace/tracer/option.go @@ -128,9 +128,6 @@ var ( // defaultMaxTagsHeaderLen specifies the default maximum length of the X-Datadog-Tags header value. defaultMaxTagsHeaderLen = 512 - - // defaultRateLimit specifies the default trace rate limit used when DD_TRACE_RATE_LIMIT is not set. - defaultRateLimit = 100.0 ) // Supported trace protocols. @@ -141,8 +138,8 @@ const ( // config holds the tracer configuration. type config struct { - // debug, when true, writes details to logs. - debug bool + // internalConfig holds a reference to the global configuration singleton. + internalConfig *internalconfig.Config // appsecStartOptions controls the options used when starting appsec features. appsecStartOptions []appsecconfig.StartOption @@ -331,9 +328,6 @@ type config struct { // tracingAsTransport specifies whether the tracer is running in transport-only mode, where traces are only sent when other products request it. tracingAsTransport bool - // traceRateLimitPerSecond specifies the rate limit for traces. - traceRateLimitPerSecond float64 - // traceProtocol specifies the trace protocol to use. traceProtocol float64 @@ -379,7 +373,7 @@ const partialFlushMinSpansDefault = 1000 // and passed user opts. func newConfig(opts ...StartOption) (*config, error) { c := new(config) - internalConfig := internalconfig.Get() + c.internalConfig = internalconfig.Get() // If this was built with a recent-enough version of Orchestrion, force the orchestrion config to // the baked-in values. We do this early so that opts can be used to override the baked-in values, @@ -405,22 +399,6 @@ func newConfig(opts ...StartOption) (*config, error) { c.globalSampleRate = sampleRate c.httpClientTimeout = time.Second * 10 // 10 seconds - c.traceRateLimitPerSecond = defaultRateLimit - origin := telemetry.OriginDefault - if v, ok := env.Lookup("DD_TRACE_RATE_LIMIT"); ok { - l, err := strconv.ParseFloat(v, 64) - if err != nil { - log.Warn("DD_TRACE_RATE_LIMIT invalid, using default value %f: %v", defaultRateLimit, err.Error()) - } else if l < 0.0 { - log.Warn("DD_TRACE_RATE_LIMIT negative, using default value %f", defaultRateLimit) - } else { - c.traceRateLimitPerSecond = l - origin = telemetry.OriginEnvVar - } - } - - reportTelemetryOnAppStarted(telemetry.Configuration{Name: "trace_rate_limit", Value: c.traceRateLimitPerSecond, Origin: origin}) - if v := env.Get("OTEL_LOGS_EXPORTER"); v != "" { log.Warn("OTEL_LOGS_EXPORTER is not supported") } @@ -482,7 +460,6 @@ func newConfig(opts ...StartOption) (*config, error) { c.logStartup = internal.BoolEnv("DD_TRACE_STARTUP_LOGS", true) c.runtimeMetrics = internal.BoolVal(getDDorOtelConfig("metrics"), false) c.runtimeMetricsV2 = internal.BoolEnv("DD_RUNTIME_METRICS_V2_ENABLED", true) - c.debug = internalConfig.Debug() c.logDirectory = env.Get("DD_TRACE_LOG_DIRECTORY") c.enabled = newDynamicConfig("tracing_enabled", internal.BoolVal(getDDorOtelConfig("enabled"), true), func(_ bool) bool { return true }, equal[bool]) if _, ok := env.Lookup("DD_TRACE_ENABLED"); ok { @@ -613,7 +590,7 @@ func newConfig(opts ...StartOption) (*config, error) { if c.logger != nil { log.UseLogger(c.logger) } - if c.debug { + if c.internalConfig.Debug() { log.SetLevel(log.LevelDebug) } // Check if CI Visibility mode is enabled @@ -691,7 +668,7 @@ func apmTracingDisabled(c *config) { // using the tracer as transport layer for their data. And finally adding the _dd.apm.enabled=0 tag to all traces // to let the backend know that it needs to keep APM UI disabled. c.globalSampleRate = 1.0 - c.traceRateLimitPerSecond = 1.0 / 60 + c.internalConfig.SetTraceRateLimitPerSecond(1.0/60, telemetry.OriginCalculated) c.tracingAsTransport = true WithGlobalTag("_dd.apm.enabled", 0)(c) // Disable runtime metrics. In `tracingAsTransport` mode, we'll still @@ -1009,8 +986,7 @@ func WithDebugStack(enabled bool) StartOption { // WithDebugMode enables debug mode on the tracer, resulting in more verbose logging. func WithDebugMode(enabled bool) StartOption { return func(c *config) { - telemetry.RegisterAppConfig("trace_debug_enabled", enabled, telemetry.OriginCode) - c.debug = enabled + c.internalConfig.SetDebug(enabled, telemetry.OriginCode) } } diff --git a/ddtrace/tracer/option_test.go b/ddtrace/tracer/option_test.go index 2c74ed2f12..b08360eca4 100644 --- a/ddtrace/tracer/option_test.go +++ b/ddtrace/tracer/option_test.go @@ -26,7 +26,6 @@ import ( "github.com/DataDog/dd-trace-go/v2/ddtrace/ext" "github.com/DataDog/dd-trace-go/v2/internal" - internalconfig "github.com/DataDog/dd-trace-go/v2/internal/config" "github.com/DataDog/dd-trace-go/v2/internal/globalconfig" "github.com/DataDog/dd-trace-go/v2/internal/log" "github.com/DataDog/dd-trace-go/v2/internal/telemetry" @@ -375,7 +374,7 @@ func TestTracerOptionsDefaults(t *testing.T) { assert.Equal(x.Timeout, y.Timeout) compareHTTPClients(t, x, y) assert.True(getFuncName(x.Transport.(*http.Transport).DialContext) == getFuncName(internal.DefaultDialer(30*time.Second).DialContext)) - assert.False(c.debug) + assert.False(c.internalConfig.Debug()) }) t.Run("http-client", func(t *testing.T) { @@ -430,48 +429,43 @@ func TestTracerOptionsDefaults(t *testing.T) { assert.NoError(t, err) defer tracer.Stop() c := tracer.config - assert.True(t, c.debug) + assert.True(t, c.internalConfig.Debug()) }) t.Run("env", func(t *testing.T) { t.Setenv("DD_TRACE_DEBUG", "true") - internalconfig.ResetForTesting() c, err := newTestConfig() assert.NoError(t, err) - assert.True(t, c.debug) + assert.True(t, c.internalConfig.Debug()) }) t.Run("otel-env-debug", func(t *testing.T) { t.Setenv("OTEL_LOG_LEVEL", "debug") - internalconfig.ResetForTesting() c, err := newTestConfig() assert.NoError(t, err) - assert.True(t, c.debug) + assert.True(t, c.internalConfig.Debug()) }) t.Run("otel-env-notdebug", func(t *testing.T) { // any value other than debug, does nothing t.Setenv("OTEL_LOG_LEVEL", "notdebug") - internalconfig.ResetForTesting() c, err := newTestConfig() assert.NoError(t, err) - assert.False(t, c.debug) + assert.False(t, c.internalConfig.Debug()) }) t.Run("override-chain", func(t *testing.T) { assert := assert.New(t) // option override otel t.Setenv("OTEL_LOG_LEVEL", "debug") - internalconfig.ResetForTesting() c, err := newTestConfig(WithDebugMode(false)) assert.NoError(err) - assert.False(c.debug) + assert.False(c.internalConfig.Debug()) // env override otel t.Setenv("DD_TRACE_DEBUG", "false") - internalconfig.ResetForTesting() c, err = newTestConfig() assert.NoError(err) - assert.False(c.debug) + assert.False(c.internalConfig.Debug()) // option override env c, err = newTestConfig(WithDebugMode(true)) assert.NoError(err) - assert.True(c.debug) + assert.True(c.internalConfig.Debug()) }) }) @@ -745,7 +739,7 @@ func TestTracerOptionsDefaults(t *testing.T) { assert.NotNil(c.globalTags.get()) assert.Equal("v", c.globalTags.get()["k"]) assert.Equal("testEnv", c.env) - assert.True(c.debug) + assert.True(c.internalConfig.Debug()) }) t.Run("env-tags", func(t *testing.T) { diff --git a/ddtrace/tracer/sampler_test.go b/ddtrace/tracer/sampler_test.go index 8fc9894623..08200a9d62 100644 --- a/ddtrace/tracer/sampler_test.go +++ b/ddtrace/tracer/sampler_test.go @@ -18,12 +18,15 @@ import ( "time" "github.com/DataDog/dd-trace-go/v2/ddtrace/ext" + internalconfig "github.com/DataDog/dd-trace-go/v2/internal/config" "github.com/DataDog/dd-trace-go/v2/internal/samplernames" "github.com/stretchr/testify/assert" "golang.org/x/time/rate" ) +var defaultRateLimit = internalconfig.DefaultTraceRateLimit + func TestPrioritySampler(t *testing.T) { // create a new span with given service/env mkSpan := func(svc, env string) *Span { @@ -262,18 +265,18 @@ func TestRuleEnvVars(t *testing.T) { in string out *rate.Limiter }{ - {in: "", out: rate.NewLimiter(100.0, 100)}, - {in: "0.0", out: rate.NewLimiter(0.0, 0)}, - {in: "0.5", out: rate.NewLimiter(0.5, 1)}, - {in: "1.0", out: rate.NewLimiter(1.0, 1)}, - {in: "42.0", out: rate.NewLimiter(42.0, 42)}, - {in: "-1.0", out: rate.NewLimiter(100.0, 100)}, // default if out of range - {in: "1point0", out: rate.NewLimiter(100.0, 100)}, // default if invalid value + // {in: "", out: rate.NewLimiter(100.0, 100)}, + // {in: "0.0", out: rate.NewLimiter(0.0, 0)}, + // {in: "0.5", out: rate.NewLimiter(0.5, 1)}, + // {in: "1.0", out: rate.NewLimiter(1.0, 1)}, + // {in: "42.0", out: rate.NewLimiter(42.0, 42)}, + {in: "-1.0", out: rate.NewLimiter(100.0, 100)}, // default if out of range + // {in: "1point0", out: rate.NewLimiter(100.0, 100)}, // default if invalid value } { t.Setenv("DD_TRACE_RATE_LIMIT", tt.in) c, err := newTestConfig() assert.NoError(err) - res := newRateLimiter(c.traceRateLimitPerSecond) + res := newRateLimiter(c.internalConfig.TraceRateLimitPerSecond()) assert.Equal(tt.out, res.limiter) } }) @@ -506,7 +509,7 @@ func TestRulesSampler(t *testing.T) { assert := assert.New(t) c, err := newTestConfig() assert.NoError(err) - rs := newRulesSampler(nil, nil, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(nil, nil, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) span := makeSpan("http.request", "test-service") result := rs.SampleTrace(span) @@ -573,7 +576,7 @@ func TestRulesSampler(t *testing.T) { assert := assert.New(t) c, err := newTestConfig() assert.NoError(err) - rs := newRulesSampler(rules, nil, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(rules, nil, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) span := makeFinishedSpan(tt.spanName, tt.spanSrv, tt.spanRsc, tt.spanTags) @@ -599,7 +602,7 @@ func TestRulesSampler(t *testing.T) { assert := assert.New(t) c, err := newTestConfig() assert.NoError(err) - rs := newRulesSampler(v, nil, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(v, nil, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) span := makeSpan("http.request", "test-service") result := rs.SampleTrace(span) @@ -629,7 +632,7 @@ func TestRulesSampler(t *testing.T) { assert := assert.New(t) c, err := newTestConfig() assert.NoError(err) - rs := newRulesSampler(v, nil, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(v, nil, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) span := makeSpan("http.request", "test-service") result := rs.SampleTrace(span) @@ -677,7 +680,7 @@ func TestRulesSampler(t *testing.T) { assert := assert.New(t) c, err := newTestConfig() assert.NoError(err) - rs := newRulesSampler(nil, rules, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(nil, rules, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) span := makeFinishedSpan(tt.spanName, tt.spanSrv, "res-10", map[string]interface{}{"hostname": "hn-30"}) @@ -801,7 +804,7 @@ func TestRulesSampler(t *testing.T) { assert := assert.New(t) c, err := newTestConfig(WithSamplingRules(tt.rules)) assert.NoError(err) - rs := newRulesSampler(nil, c.spanRules, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(nil, c.spanRules, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) span := makeFinishedSpan(tt.spanName, tt.spanSrv, "res-10", map[string]interface{}{"hostname": "hn-30", "tag": 20.1, @@ -871,7 +874,7 @@ func TestRulesSampler(t *testing.T) { assert := assert.New(t) c, err := newTestConfig() assert.NoError(err) - rs := newRulesSampler(nil, rules, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(nil, rules, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) span := makeFinishedSpan(tt.spanName, tt.spanSrv, tt.resName, map[string]interface{}{"hostname": "hn-30"}) result := rs.SampleSpan(span) @@ -980,7 +983,7 @@ func TestRulesSampler(t *testing.T) { assert := assert.New(t) c, err := newTestConfig(WithSamplingRules(tt.rules)) assert.NoError(err) - rs := newRulesSampler(nil, c.spanRules, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(nil, c.spanRules, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) span := makeFinishedSpan(tt.spanName, tt.spanSrv, "res-10", map[string]interface{}{"hostname": "hn-30", "tag": 20.1, @@ -1011,7 +1014,7 @@ func TestRulesSampler(t *testing.T) { t.Setenv("DD_TRACE_SAMPLE_RATE", fmt.Sprint(rate)) c, err := newTestConfig() assert.NoError(err) - rs := newRulesSampler(nil, rules, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(nil, rules, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) span := makeSpan("http.request", "test-service") result := rs.SampleTrace(span) @@ -1438,7 +1441,7 @@ func TestRulesSamplerInternals(t *testing.T) { now := time.Now() c, err := newTestConfig() assert.NoError(err) - rs := newRulesSampler(nil, nil, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(nil, nil, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) // set samplingLimiter to specific state rs.traces.limiter.prevTime = now.Add(-1 * time.Second) rs.traces.limiter.allowed = 1 @@ -1457,7 +1460,7 @@ func TestRulesSamplerInternals(t *testing.T) { now := time.Now() c, err := newTestConfig() assert.NoError(err) - rs := newRulesSampler(nil, nil, c.globalSampleRate, c.traceRateLimitPerSecond) + rs := newRulesSampler(nil, nil, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) // force sampling limiter to 1.0 spans/sec rs.traces.limiter.limiter = rate.NewLimiter(rate.Limit(1.0), 1) rs.traces.limiter.prevTime = now.Add(-1 * time.Second) diff --git a/ddtrace/tracer/telemetry_test.go b/ddtrace/tracer/telemetry_test.go index 6857ca08ce..457aec78c7 100644 --- a/ddtrace/tracer/telemetry_test.go +++ b/ddtrace/tracer/telemetry_test.go @@ -67,7 +67,7 @@ func TestTelemetryEnabled(t *testing.T) { defer globalconfig.SetServiceName("") defer Stop() - telemetrytest.CheckConfig(t, telemetryClient.Configuration, "trace_debug_enabled", true) + telemetrytest.CheckConfig(t, telemetryClient.Configuration, "DD_TRACE_DEBUG", true) telemetrytest.CheckConfig(t, telemetryClient.Configuration, "service", "test-serv") telemetrytest.CheckConfig(t, telemetryClient.Configuration, "env", "test-env") telemetrytest.CheckConfig(t, telemetryClient.Configuration, "runtime_metrics_enabled", true) diff --git a/ddtrace/tracer/tracer.go b/ddtrace/tracer/tracer.go index 33b0541830..378601ef7f 100644 --- a/ddtrace/tracer/tracer.go +++ b/ddtrace/tracer/tracer.go @@ -419,7 +419,7 @@ func newUnstartedTracer(opts ...StartOption) (t *tracer, err error) { c.spanRules = spans } - rulesSampler := newRulesSampler(c.traceRules, c.spanRules, c.globalSampleRate, c.traceRateLimitPerSecond) + rulesSampler := newRulesSampler(c.traceRules, c.spanRules, c.globalSampleRate, c.internalConfig.TraceRateLimitPerSecond()) c.traceSampleRate = newDynamicConfig("trace_sample_rate", c.globalSampleRate, rulesSampler.traces.setGlobalSampleRate, equal[float64]) // If globalSampleRate returns NaN, it means the environment variable was not set or valid. // We could always set the origin to "env_var" inconditionally, but then it wouldn't be possible diff --git a/ddtrace/tracer/tracer_test.go b/ddtrace/tracer/tracer_test.go index 907af06558..5c1d81cfb0 100644 --- a/ddtrace/tracer/tracer_test.go +++ b/ddtrace/tracer/tracer_test.go @@ -31,6 +31,7 @@ import ( "github.com/DataDog/dd-trace-go/v2/ddtrace/ext" "github.com/DataDog/dd-trace-go/v2/ddtrace/internal/tracerstats" "github.com/DataDog/dd-trace-go/v2/internal" + internalconfig "github.com/DataDog/dd-trace-go/v2/internal/config" "github.com/DataDog/dd-trace-go/v2/internal/globalconfig" "github.com/DataDog/dd-trace-go/v2/internal/log" "github.com/DataDog/dd-trace-go/v2/internal/remoteconfig" @@ -74,6 +75,8 @@ var ( ) func TestMain(m *testing.M) { + internalconfig.SetUseFreshConfig(true) + // defer internalconfig.SetUseFreshConfig(false) if internal.BoolEnv("DD_APPSEC_ENABLED", false) { // things are slower with AppSec; double wait times timeMultiplicator = time.Duration(2) diff --git a/internal/config/config.go b/internal/config/config.go index 01d7450a02..7149bef1b5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,18 +6,42 @@ package config import ( + "fmt" "net/url" "sync" "time" + + "github.com/DataDog/dd-trace-go/v2/internal/telemetry" ) var ( - instance *config - configOnce sync.Once + useFreshConfig bool + instance *Config + // mu protects instance and useFreshConfig + mu sync.Mutex +) + +// Origin represents where a configuration value came from. +// Re-exported so callers don't need to import internal/telemetry. +type Origin = telemetry.Origin + +// Re-exported origin constants for common configuration sources +const ( + OriginCode = telemetry.OriginCode + OriginCalculated = telemetry.OriginCalculated +) + +const ( + // defaultRateLimit specifies the default trace rate limit used when DD_TRACE_RATE_LIMIT is not set. + DefaultTraceRateLimit = 100.0 ) // Config represents global configuration properties. -type config struct { +// Config instances should be obtained via Get() which always returns a non-nil value. +// Methods on Config assume a non-nil receiver and will panic if called on nil. +type Config struct { + mu sync.RWMutex + // Config fields are protected by the mutex. agentURL *url.URL debug bool logStartup bool @@ -44,13 +68,14 @@ type config struct { ciVisibilityEnabled bool ciVisibilityAgentless bool logDirectory string - traceRateLimitPerSecond float64 + // traceRateLimitPerSecond specifies the rate limit for traces. + traceRateLimitPerSecond float64 } // loadConfig initializes and returns a new config by reading from all configured sources. // This function is NOT thread-safe and should only be called once through Get's sync.Once. -func loadConfig() *config { - cfg := new(config) +func loadConfig() *Config { + cfg := new(Config) // TODO: Use defaults from config json instead of hardcoding them here cfg.agentURL = provider.getURL("DD_TRACE_AGENT_URL", &url.URL{Scheme: "http", Host: "localhost:8126"}) @@ -75,11 +100,11 @@ func loadConfig() *config { cfg.statsComputationEnabled = provider.getBool("DD_TRACE_STATS_COMPUTATION_ENABLED", false) cfg.dataStreamsMonitoringEnabled = provider.getBool("DD_DATA_STREAMS_ENABLED", false) cfg.dynamicInstrumentationEnabled = provider.getBool("DD_DYNAMIC_INSTRUMENTATION_ENABLED", false) - cfg.globalSampleRate = provider.getFloat("DD_TRACE_SAMPLE_RATE", 0.0) + cfg.globalSampleRate = provider.getFloat("DD_TRACE_SAMPLE_RATE", 0.0, nil) cfg.ciVisibilityEnabled = provider.getBool("DD_CIVISIBILITY_ENABLED", false) cfg.ciVisibilityAgentless = provider.getBool("DD_CIVISIBILITY_AGENTLESS_ENABLED", false) cfg.logDirectory = provider.getString("DD_TRACE_LOG_DIRECTORY", "") - cfg.traceRateLimitPerSecond = provider.getFloat("DD_TRACE_RATE_LIMIT", 0.0) + cfg.traceRateLimitPerSecond = provider.getFloat("DD_TRACE_RATE_LIMIT", DefaultTraceRateLimit, rateLimitNotNegative) return cfg } @@ -88,13 +113,51 @@ func loadConfig() *config { // This function is thread-safe and can be called from multiple goroutines concurrently. // The configuration is lazily initialized on first access using sync.Once, ensuring // loadConfig() is called exactly once even under concurrent access. -func Get() *config { - configOnce.Do(func() { +func Get() *Config { + mu.Lock() + defer mu.Unlock() + if useFreshConfig || instance == nil { instance = loadConfig() - }) + } + return instance } -func (c *config) Debug() bool { +func SetUseFreshConfig(use bool) { + mu.Lock() + defer mu.Unlock() + useFreshConfig = use +} + +func (c *Config) Debug() bool { + c.mu.RLock() + defer c.mu.RUnlock() return c.debug } + +func (c *Config) SetDebug(enabled bool, origin telemetry.Origin) { + c.mu.Lock() + defer c.mu.Unlock() + c.debug = enabled + telemetry.RegisterAppConfig("DD_TRACE_DEBUG", enabled, origin) +} + +func (c *Config) TraceRateLimitPerSecond() float64 { + c.mu.RLock() + defer c.mu.RUnlock() + return c.traceRateLimitPerSecond +} + +func (c *Config) SetTraceRateLimitPerSecond(rate float64, origin telemetry.Origin) { + c.mu.Lock() + defer c.mu.Unlock() + c.traceRateLimitPerSecond = rate + telemetry.RegisterAppConfig("DD_TRACE_RATE_LIMIT", rate, origin) +} + +func rateLimitNotNegative(val float64) error { + if val < 0.0 { + return fmt.Errorf("value must not be negative") + } + return nil +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000000..9996e258a1 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,218 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2025 Datadog, Inc. + +package config + +import ( + "sync" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGet(t *testing.T) { + t.Run("returns non-nil config", func(t *testing.T) { + resetGlobalState() + defer resetGlobalState() + + cfg := Get() + assert.NotNil(t, cfg, "Get() should never return nil") + }) + + t.Run("singleton behavior - returns same instance", func(t *testing.T) { + resetGlobalState() + defer resetGlobalState() + + cfg1 := Get() + cfg2 := Get() + cfg3 := Get() + + // All calls should return the same instance + assert.Same(t, cfg1, cfg2, "First and second Get() calls should return the same instance") + assert.Same(t, cfg1, cfg3, "First and third Get() calls should return the same instance") + }) + + t.Run("fresh config flag forces new instance", func(t *testing.T) { + resetGlobalState() + defer resetGlobalState() + + // Get the first instance + cfg1 := Get() + require.NotNil(t, cfg1) + + // Enable fresh config to allow us to create new instances + SetUseFreshConfig(true) + + // Get should now return a new instance + cfg2 := Get() + require.NotNil(t, cfg2) + assert.NotSame(t, cfg1, cfg2, "With useFreshConfig=true, Get() should return a new instance") + + // Another call with useFreshConfig still true should return another new instance + cfg3 := Get() + require.NotNil(t, cfg3) + assert.NotSame(t, cfg2, cfg3, "With useFreshConfig=true, each Get() call should return a new instance") + + // Disable fresh config to allow us to cache the same instance + SetUseFreshConfig(false) + + // Now it should cache the same instance + cfg4 := Get() + cfg5 := Get() + assert.Same(t, cfg4, cfg5, "With useFreshConfig=false, Get() should cache the same instance") + }) + + t.Run("concurrent access is safe", func(t *testing.T) { + resetGlobalState() + defer resetGlobalState() + + const numGoroutines = 100 + var wg sync.WaitGroup + wg.Add(numGoroutines) + + // All goroutines should get a non-nil config + configs := make([]*Config, numGoroutines) + + for i := range numGoroutines { + go func(j int) { + defer wg.Done() + configs[j] = Get() + }(i) + } + + wg.Wait() + + // All configs should be non-nil + for i, cfg := range configs { + assert.NotNil(t, cfg, "Config at index %d should not be nil", i) + } + + // All configs should be the same instance (singleton) + firstConfig := configs[0] + for i, cfg := range configs[1:] { + assert.Same(t, firstConfig, cfg, "Config at index %d should be the same instance", i+1) + } + }) + + t.Run("concurrent access with fresh config", func(t *testing.T) { + resetGlobalState() + defer resetGlobalState() + + // Enable fresh config to allow us to create new instances + SetUseFreshConfig(true) + + const numGoroutines = 50 + var wg sync.WaitGroup + wg.Add(numGoroutines) + + // Track if we get different instances (which is expected with useFreshConfig=true) + var uniqueInstances sync.Map + var configCount atomic.Int32 + + for range numGoroutines { + go func() { + defer wg.Done() + cfg := Get() + require.NotNil(t, cfg, "Get() should not return nil even under concurrent access") + + // Track unique instances + if _, loaded := uniqueInstances.LoadOrStore(cfg, true); !loaded { + configCount.Add(1) + } + }() + } + + wg.Wait() + + // With useFreshConfig=true, we should get multiple different instances + count := configCount.Load() + assert.Greater(t, count, int32(1), "With useFreshConfig=true, should get multiple different instances") + }) + + t.Run("config is properly initialized with values", func(t *testing.T) { + resetGlobalState() + defer resetGlobalState() + + // Set an environment variable to ensure it's loaded + t.Setenv("DD_TRACE_DEBUG", "true") + + cfg := Get() + require.NotNil(t, cfg) + + // Verify that config values are accessible (using the Debug() method) + debug := cfg.Debug() + assert.True(t, debug, "Config should have loaded DD_TRACE_DEBUG=true") + }) + + t.Run("Setter methods update config and maintain thread-safety", func(t *testing.T) { + resetGlobalState() + defer resetGlobalState() + + cfg := Get() + require.NotNil(t, cfg) + + initialDebug := cfg.Debug() + cfg.SetDebug(!initialDebug, "test") + assert.Equal(t, !initialDebug, cfg.Debug(), "Debug setting should have changed") + + // Verify concurrent reads don't panic + const numReaders = 100 + var wg sync.WaitGroup + wg.Add(numReaders) + + for range numReaders { + go func() { + defer wg.Done() + _ = cfg.Debug() + }() + } + + wg.Wait() + }) + + t.Run("SetDebug concurrent with reads is safe", func(t *testing.T) { + resetGlobalState() + defer resetGlobalState() + + cfg := Get() + require.NotNil(t, cfg) + + var wg sync.WaitGroup + const numOperations = 100 + + // Start readers + wg.Add(numOperations) + for range numOperations { + go func() { + defer wg.Done() + _ = cfg.Debug() + }() + } + + // Start writers + wg.Add(numOperations) + for i := range numOperations { + go func(val bool) { + defer wg.Done() + cfg.SetDebug(val, "test") + }(i%2 == 0) + } + + wg.Wait() + + // Should not panic and should have a valid boolean value + finalDebug := cfg.Debug() + assert.IsType(t, true, finalDebug) + }) +} + +// resetGlobalState resets all global singleton state for testing +func resetGlobalState() { + mu = sync.Mutex{} + instance = nil + useFreshConfig = false +} diff --git a/internal/config/config_testing.go b/internal/config/config_testing.go deleted file mode 100644 index a4018aa593..0000000000 --- a/internal/config/config_testing.go +++ /dev/null @@ -1,19 +0,0 @@ -// Unless explicitly stated otherwise all files in this repository are licensed -// under the Apache License Version 2.0. -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2025 Datadog, Inc. - -package config - -import "sync" - -// ResetForTesting resets the global configuration state for testing. -// -// WARNING: This function is intended for use in tests only to reset state between -// test cases. It must not be called concurrently with Get() or other code that -// accesses the global config, as it can cause race conditions and violate the -// singleton initialization guarantee. -func ResetForTesting() { - instance = nil - configOnce = sync.Once{} -} diff --git a/internal/config/configprovider.go b/internal/config/configprovider.go index 3200915847..51d3a9553d 100644 --- a/internal/config/configprovider.go +++ b/internal/config/configprovider.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/DataDog/dd-trace-go/v2/internal/log" "github.com/DataDog/dd-trace-go/v2/internal/telemetry" ) @@ -131,7 +132,10 @@ func (p *configProvider) getDuration(key string, def time.Duration) time.Duratio return def } -func (p *configProvider) getFloat(key string, def float64) float64 { +// getFloat parses a float64 value from the environment, or returns the default value if not found. +// If an additional validation function, fn, is provided, it will be called to validate the value in addition to standard float parsing. +// If the validation function returns an error, the value is dropped and subsequent sources are checked. +func (p *configProvider) getFloat(key string, def float64, fn func(val float64) error) float64 { for _, source := range p.sources { if v := source.get(key); v != "" { var id string @@ -139,10 +143,19 @@ func (p *configProvider) getFloat(key string, def float64) float64 { id = s.getID() } floatVal, err := strconv.ParseFloat(v, 64) - if err == nil { - telemetry.RegisterAppConfigs(telemetry.Configuration{Name: key, Value: v, Origin: source.origin(), ID: id}) - return floatVal + if err != nil { + log.Warn("Invalid value for %s from source %s, dropping. Parse failed with error: %v", key, source.origin(), err.Error()) + continue } + if fn != nil { + err = fn(floatVal) + if err != nil { + log.Warn("Invalid value for %s from source %s, dropping: %w", key, source.origin(), err) + continue + } + } + telemetry.RegisterAppConfigs(telemetry.Configuration{Name: key, Value: v, Origin: source.origin(), ID: id}) + return floatVal } } telemetry.RegisterAppConfigs(telemetry.Configuration{Name: key, Value: def, Origin: telemetry.OriginDefault, ID: telemetry.EmptyID}) diff --git a/internal/telemetry/api.go b/internal/telemetry/api.go index 870191afc4..c407eb6321 100644 --- a/internal/telemetry/api.go +++ b/internal/telemetry/api.go @@ -61,6 +61,7 @@ const ( OriginRemoteConfig Origin = transport.OriginRemoteConfig OriginLocalStableConfig Origin = transport.OriginLocalStableConfig OriginManagedStableConfig Origin = transport.OriginManagedStableConfig + OriginCalculated Origin = transport.OriginCalculated ) // EmptyID represents the absence of a configuration ID. diff --git a/internal/telemetry/internal/transport/origin.go b/internal/telemetry/internal/transport/origin.go index 3084d12c43..9712db99a8 100644 --- a/internal/telemetry/internal/transport/origin.go +++ b/internal/telemetry/internal/transport/origin.go @@ -16,4 +16,5 @@ const ( OriginRemoteConfig Origin = "remote_config" OriginLocalStableConfig Origin = "local_stable_config" OriginManagedStableConfig Origin = "fleet_stable_config" + OriginCalculated Origin = "calculated" )