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..0a12108ae7 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 @@ -249,10 +249,6 @@ type config struct { // It defaults to time.Ticker; replaced in tests. tickChan <-chan time.Time - // noDebugStack disables the collection of debug stack traces globally. No traces reporting - // errors will record a stack trace when this option is set. - noDebugStack bool - // profilerHotspots specifies whether profiler Code Hotspots is enabled. profilerHotspots bool @@ -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 { @@ -613,7 +608,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 @@ -1002,15 +997,14 @@ func WithLogger(logger Logger) StartOption { // FinishOption. func WithDebugStack(enabled bool) StartOption { return func(c *config) { - c.noDebugStack = !enabled + c.internalConfig.SetDebugStack(enabled, telemetry.OriginCode) } } // 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/telemetry.go b/ddtrace/tracer/telemetry.go index 4704efd773..718e37e0f7 100644 --- a/ddtrace/tracer/telemetry.go +++ b/ddtrace/tracer/telemetry.go @@ -51,7 +51,7 @@ func startTelemetry(c *config) telemetry.Client { {Name: "agent_hostname", Value: c.hostname}, {Name: "runtime_metrics_v2_enabled", Value: c.runtimeMetricsV2}, {Name: "dogstatsd_addr", Value: c.dogstatsdAddr}, - {Name: "debug_stack_enabled", Value: !c.noDebugStack}, + {Name: "debug_stack_enabled", Value: c.internalConfig.DebugStack()}, {Name: "profiling_hotspots_enabled", Value: c.profilerHotspots}, {Name: "profiling_endpoints_enabled", Value: c.profilerEndpoints}, {Name: "trace_span_attribute_schema", Value: c.spanAttributeSchemaVersion}, 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..7cfb5dd1c7 100644 --- a/ddtrace/tracer/tracer.go +++ b/ddtrace/tracer/tracer.go @@ -764,7 +764,7 @@ func (t *tracer) StartSpan(operationName string, options ...StartSpanOption) *Sp if span.service == "" { span.service = t.config.serviceName } - span.noDebugStack = t.config.noDebugStack + span.noDebugStack = !t.config.internalConfig.DebugStack() if t.config.hostname != "" { span.setMeta(keyHostname, t.config.hostname) } 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..6fa9593fdb 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 @@ -45,12 +53,15 @@ type config struct { ciVisibilityAgentless bool logDirectory string traceRateLimitPerSecond float64 + // debugStack enables the collection of debug stack traces globally. No traces reporting + // errors will record a stack trace when this option is disabled. + debugStack bool } // 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"}) @@ -80,6 +91,7 @@ func loadConfig() *config { 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.debugStack = provider.getBool("DD_TRACE_DEBUG_STACK", true) return cfg } @@ -88,13 +100,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) DebugStack() bool { + c.mu.RLock() + defer c.mu.RUnlock() + return c.debugStack +} + +func (c *Config) SetDebugStack(enabled bool, origin telemetry.Origin) { + c.mu.Lock() + defer c.mu.Unlock() + c.debugStack = enabled + telemetry.RegisterAppConfig("DD_TRACE_DEBUG_STACK", 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{} -} diff --git a/internal/env/supported_configurations.gen.go b/internal/env/supported_configurations.gen.go index a427342d4f..0b9490469a 100644 --- a/internal/env/supported_configurations.gen.go +++ b/internal/env/supported_configurations.gen.go @@ -147,6 +147,7 @@ var SupportedConfigurations = map[string]struct{}{ "DD_TRACE_DEBUG": {}, "DD_TRACE_DEBUG_ABANDONED_SPANS": {}, "DD_TRACE_DEBUG_SEELOG_WORKAROUND": {}, + "DD_TRACE_DEBUG_STACK": {}, "DD_TRACE_ECHO_ANALYTICS_ENABLED": {}, "DD_TRACE_ELASTIC_ANALYTICS_ENABLED": {}, "DD_TRACE_ENABLED": {}, diff --git a/internal/env/supported_configurations.json b/internal/env/supported_configurations.json index fbda4b8416..18f46e10eb 100644 --- a/internal/env/supported_configurations.json +++ b/internal/env/supported_configurations.json @@ -414,6 +414,9 @@ "DD_TRACE_DEBUG_SEELOG_WORKAROUND": [ "A" ], + "DD_TRACE_DEBUG_STACK": [ + "A" + ], "DD_TRACE_ECHO_ANALYTICS_ENABLED": [ "A" ],