Skip to content

Commit e12933d

Browse files
committed
Refactor telemetry config to use config overlay approach
Address PR review comments by implementing config overlay pattern: **Changes:** - Remove ForceEnableTelemetry flag (no longer needed) - Change EnableTelemetry from bool to *bool pointer - nil = use server feature flag (default) - true = client forces enabled (overrides server) - false = client forces disabled (overrides server) - Simplify priority logic to 3 clear layers: 1. Client Config (explicit) - highest priority 2. Server Config (feature flag) - when client doesn't set 3. Fail-Safe Default (disabled) - when server unavailable **Benefits:** - Simpler mental model: client > server > default - Client config naturally has priority (no special bypass flag) - Foundation for general config overlay system - All 89 telemetry tests passing Addresses review comments: - "if we take the config overlay approach that client side config take priority? we won't need the concept of force enablement?" - Sets foundation for extending to all driver configs (not just telemetry)
1 parent 38e41de commit e12933d

2 files changed

Lines changed: 116 additions & 127 deletions

File tree

telemetry/config.go

Lines changed: 29 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,12 @@ type Config struct {
1212
// Enabled controls whether telemetry is active
1313
Enabled bool
1414

15-
// ForceEnableTelemetry bypasses server-side feature flag checks
16-
// When true, telemetry is always enabled regardless of server flags
17-
ForceEnableTelemetry bool
18-
19-
// EnableTelemetry indicates user wants telemetry enabled if server allows
20-
// Respects server-side feature flags and rollout percentage
21-
EnableTelemetry bool
15+
// EnableTelemetry is the client-side telemetry preference
16+
// - nil: not set by client, use server feature flag (default behavior)
17+
// - true: client wants telemetry enabled (overrides server)
18+
// - false: client wants telemetry disabled (overrides server)
19+
// This implements config overlay: client > server > default
20+
EnableTelemetry *bool
2221

2322
// BatchSize is the number of metrics to batch before flushing
2423
BatchSize int
@@ -43,12 +42,12 @@ type Config struct {
4342
}
4443

4544
// DefaultConfig returns default telemetry configuration.
46-
// Note: Telemetry is disabled by default and requires explicit opt-in.
45+
// Note: Telemetry uses config overlay - enabled by default, controlled by server feature flags.
46+
// Clients can override by explicitly setting enableTelemetry=true/false.
4747
func DefaultConfig() *Config {
4848
return &Config{
49-
Enabled: false, // Disabled by default, requires explicit opt-in
50-
ForceEnableTelemetry: false,
51-
EnableTelemetry: false,
49+
Enabled: false, // Will be set based on overlay logic
50+
EnableTelemetry: nil, // nil = use server feature flag (config overlay)
5251
BatchSize: 100,
5352
FlushInterval: 5 * time.Second,
5453
MaxRetries: 3,
@@ -63,22 +62,16 @@ func DefaultConfig() *Config {
6362
func ParseTelemetryConfig(params map[string]string) *Config {
6463
cfg := DefaultConfig()
6564

66-
// Check for forceEnableTelemetry flag (bypasses server feature flags)
67-
if v, ok := params["forceEnableTelemetry"]; ok {
68-
if v == "true" || v == "1" {
69-
cfg.ForceEnableTelemetry = true
70-
cfg.Enabled = true // Also set Enabled for backward compatibility
71-
}
72-
}
73-
74-
// Check for enableTelemetry flag (respects server feature flags)
65+
// Config overlay approach: client setting overrides server feature flag
66+
// Priority:
67+
// 1. Client explicit setting (enableTelemetry=true/false) - overrides server
68+
// 2. Server feature flag (when client doesn't set) - server controls
69+
// 3. Default disabled (when server flag unavailable) - fail-safe
7570
if v, ok := params["enableTelemetry"]; ok {
76-
if v == "true" || v == "1" {
77-
cfg.EnableTelemetry = true
78-
} else if v == "false" || v == "0" {
79-
cfg.EnableTelemetry = false
80-
}
71+
enabled := (v == "true" || v == "1")
72+
cfg.EnableTelemetry = &enabled
8173
}
74+
// Note: If not present in params, stays nil (use server feature flag)
8275

8376
if v, ok := params["telemetry_batch_size"]; ok {
8477
if size, err := strconv.Atoi(v); err == nil && size > 0 {
@@ -96,14 +89,12 @@ func ParseTelemetryConfig(params map[string]string) *Config {
9689
}
9790

9891
// isTelemetryEnabled checks if telemetry should be enabled for this connection.
99-
// Implements the priority-based decision tree for telemetry enablement.
92+
// Implements config overlay approach with clear priority order.
10093
//
101-
// Priority (highest to lowest):
102-
// 1. forceEnableTelemetry=true - Bypasses all server checks (testing/internal)
103-
// 2. enableTelemetry=false - Explicit opt-out (always disabled)
104-
// 3. enableTelemetry=true + Server Feature Flag - User opt-in with server control
105-
// 4. Server Feature Flag Only - Default behavior (Databricks-controlled)
106-
// 5. Default - Disabled (false)
94+
// Config Overlay Priority (highest to lowest):
95+
// 1. Client Config - enableTelemetry explicitly set (true/false) - overrides server
96+
// 2. Server Config - feature flag controls when client doesn't specify
97+
// 3. Fail-Safe Default - disabled when server flag unavailable/errors
10798
//
10899
// Parameters:
109100
// - ctx: Context for the request
@@ -114,26 +105,17 @@ func ParseTelemetryConfig(params map[string]string) *Config {
114105
// Returns:
115106
// - bool: true if telemetry should be enabled, false otherwise
116107
func isTelemetryEnabled(ctx context.Context, cfg *Config, host string, httpClient *http.Client) bool {
117-
// Priority 1: Force enable bypasses all server checks
118-
if cfg.ForceEnableTelemetry {
119-
return true
120-
}
121-
122-
// Priority 2: Explicit opt-out always disables
123-
// When enableTelemetry is explicitly set to false, respect that
124-
if !cfg.EnableTelemetry {
125-
return false
108+
// Priority 1: Client explicitly set enableTelemetry (overrides server)
109+
if cfg.EnableTelemetry != nil {
110+
return *cfg.EnableTelemetry
126111
}
127112

128-
// Priority 3 & 4: Check server-side feature flag
129-
// This handles both:
130-
// - User explicitly opted in (enableTelemetry=true) - respect server decision
131-
// - Default behavior (no explicit setting) - server controls enablement
113+
// Priority 2: Check server-side feature flag
132114
flagCache := getFeatureFlagCache()
133115
serverEnabled, err := flagCache.isTelemetryEnabled(ctx, host, httpClient)
134116
if err != nil {
135-
// On error, respect default (disabled)
136-
// This ensures telemetry failures don't impact driver operation
117+
// Priority 3: Fail-safe default (disabled)
118+
// On error, default to disabled to ensure telemetry failures don't impact driver
137119
return false
138120
}
139121

0 commit comments

Comments
 (0)