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..f607976ab9 100644 --- a/ddtrace/tracer/option.go +++ b/ddtrace/tracer/option.go @@ -141,8 +141,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 @@ -309,10 +309,6 @@ type config struct { // headerAsTags holds the header as tags configuration. headerAsTags dynamicConfig[[]string] - // dynamicInstrumentationEnabled controls if the target application can be modified by Dynamic Instrumentation or not. - // Value from DD_DYNAMIC_INSTRUMENTATION_ENABLED, default false. - dynamicInstrumentationEnabled bool - // globalSampleRate holds sample rate read from environment variables. globalSampleRate float64 @@ -379,7 +375,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, @@ -482,7 +478,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 { @@ -516,8 +511,6 @@ func newConfig(opts ...StartOption) (*config, error) { // is set, but DD_TRACE_PARTIAL_FLUSH_ENABLED is not true. Or just assume it should be enabled // if it's explicitly set, and don't require both variables to be configured. - c.dynamicInstrumentationEnabled, _, _ = stableconfig.Bool("DD_DYNAMIC_INSTRUMENTATION_ENABLED", false) - namingschema.LoadFromEnv() c.spanAttributeSchemaVersion = int(namingschema.GetVersion()) @@ -613,7 +606,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 @@ -1009,8 +1002,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/remote_config.go b/ddtrace/tracer/remote_config.go index 2a14dadb09..8c17c8b56a 100644 --- a/ddtrace/tracer/remote_config.go +++ b/ddtrace/tracer/remote_config.go @@ -384,7 +384,7 @@ func (t *tracer) startRemoteConfig(rcConfig remoteconfig.ClientConfig) error { var dynamicInstrumentationError, apmTracingError error - if t.config.dynamicInstrumentationEnabled { + if t.config.internalConfig.DynamicInstrumentationEnabled() { liveDebuggingError := remoteconfig.Subscribe( "LIVE_DEBUGGING", t.dynamicInstrumentationRCUpdate, ) 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_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..40c497d3d7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,16 +8,24 @@ package config import ( "net/url" "sync" + "sync/atomic" "time" + + "github.com/DataDog/dd-trace-go/v2/internal/telemetry" ) var ( - instance *config - configOnce sync.Once + useFreshConfig atomic.Bool + instance atomic.Value + once sync.Once ) // 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 @@ -49,8 +57,8 @@ type config struct { // 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"}) @@ -88,13 +96,46 @@ 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() { - instance = loadConfig() +func Get() *Config { + if useFreshConfig.Load() { + cfg := loadConfig() + instance.Store(cfg) + return cfg + } + + once.Do(func() { + cfg := loadConfig() + instance.Store(cfg) }) - return instance + return instance.Load().(*Config) } -func (c *config) Debug() bool { +func SetUseFreshConfig(use bool) { + useFreshConfig.Store(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) DynamicInstrumentationEnabled() bool { + c.mu.RLock() + defer c.mu.RUnlock() + return c.dynamicInstrumentationEnabled +} + +func (c *Config) SetDynamicInstrumentationEnabled(enabled bool, origin telemetry.Origin) { + c.mu.Lock() + defer c.mu.Unlock() + c.dynamicInstrumentationEnabled = enabled + telemetry.RegisterAppConfig("DD_DYNAMIC_INSTRUMENTATION_ENABLED", enabled, origin) +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000000..59e3f861f8 --- /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() { + once = sync.Once{} + instance = atomic.Value{} + useFreshConfig.Store(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{} -}