fix(go-adk): explicitly set timeout for traces and logs export#1638
fix(go-adk): explicitly set timeout for traces and logs export#1638supreme-gg-gg wants to merge 2 commits into
Conversation
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
There was a problem hiding this comment.
Pull request overview
Adjusts Go ADK OTLP gRPC exporter configuration to avoid mis-parsing OTEL_EXPORTER_OTLP_TIMEOUT (Go treating numeric values as ms vs Python treating them as seconds), which has been causing intermittent trace export timeouts.
Changes:
- Adds a shared
timeoutFromEnv(...)helper to parse OTLP timeout env vars (duration strings or integers) with a fallback default. - Explicitly applies
WithTimeout(...)to both OTLP trace and log gRPC exporters, using signal-specific env vars with a global fallback. - Introduces a package-level default OTLP export timeout constant.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // timeoutFromEnv reads OTLP timeout env vars and returns a duration. | ||
| // Supported formats: | ||
| // - duration strings (e.g. "15s", "15000ms") | ||
| // - plain integers: | ||
| // - <1000 treated as seconds (backward-compatible with chart values like 15) | ||
| // - >=1000 treated as milliseconds (OTEL default convention) | ||
| func timeoutFromEnv(keys ...string) time.Duration { | ||
| for _, key := range keys { | ||
| raw := strings.TrimSpace(os.Getenv(key)) | ||
| if raw == "" { | ||
| continue | ||
| } | ||
|
|
||
| if d, err := time.ParseDuration(raw); err == nil { | ||
| return d | ||
| } | ||
|
|
||
| if n, err := strconv.Atoi(raw); err == nil { | ||
| if n < 1000 { | ||
| return time.Duration(n) * time.Second | ||
| } | ||
| return time.Duration(n) * time.Millisecond | ||
| } |
There was a problem hiding this comment.
timeoutFromEnv changes the meaning of numeric-only OTLP timeout values: for values <1000 it treats them as seconds, whereas upstream OpenTelemetry Go treats numeric-only values as milliseconds. This divergence can surprise users who set OTEL_EXPORTER_OTLP_TIMEOUT=500 expecting 500ms and instead get 500s. Consider requiring explicit units (e.g. "15s") and parsing plain integers as milliseconds, or scoping the seconds-compat behavior to a kagent-specific legacy env var / chart-only knob.
| if d, err := time.ParseDuration(raw); err == nil { | ||
| return d | ||
| } | ||
|
|
||
| if n, err := strconv.Atoi(raw); err == nil { | ||
| if n < 1000 { | ||
| return time.Duration(n) * time.Second | ||
| } | ||
| return time.Duration(n) * time.Millisecond | ||
| } |
There was a problem hiding this comment.
timeoutFromEnv accepts negative numeric values (e.g. "-1"), which will produce a negative time.Duration and can lead to immediate failures or undefined exporter behavior. Consider treating non-positive parsed durations/integers as invalid and falling back to the next key (or the default) instead of returning them.
| // timeoutFromEnv reads OTLP timeout env vars and returns a duration. | ||
| // Supported formats: | ||
| // - duration strings (e.g. "15s", "15000ms") | ||
| // - plain integers: | ||
| // - <1000 treated as seconds (backward-compatible with chart values like 15) | ||
| // - >=1000 treated as milliseconds (OTEL default convention) | ||
| func timeoutFromEnv(keys ...string) time.Duration { | ||
| for _, key := range keys { | ||
| raw := strings.TrimSpace(os.Getenv(key)) | ||
| if raw == "" { | ||
| continue | ||
| } | ||
|
|
||
| if d, err := time.ParseDuration(raw); err == nil { | ||
| return d | ||
| } | ||
|
|
||
| if n, err := strconv.Atoi(raw); err == nil { | ||
| if n < 1000 { | ||
| return time.Duration(n) * time.Second | ||
| } | ||
| return time.Duration(n) * time.Millisecond | ||
| } | ||
| } | ||
|
|
||
| return defaultOTLPExportTimeout | ||
| } |
There was a problem hiding this comment.
This change introduces new timeout parsing/precedence logic (timeoutFromEnv) and applies it to both trace and log exporters, but there are no unit tests covering the parsing rules (duration strings vs integers, <1000 seconds behavior, key precedence, invalid inputs). Since this package already has tests, add focused tests for timeoutFromEnv using t.Setenv to prevent regressions.
| otlptracegrpc.WithTimeout(timeoutFromEnv( | ||
| "OTEL_EXPORTER_OTLP_TRACES_TIMEOUT", | ||
| "OTEL_EXPORTER_OTLP_TIMEOUT", | ||
| )), |
There was a problem hiding this comment.
WithTimeout(timeoutFromEnv(...)) now always sets a timeout even when no OTLP timeout env vars are provided (due to the hardcoded defaultOTLPExportTimeout). That changes behavior vs relying on the exporter’s built-in default, and can affect deployments that don’t set these env vars. Consider only adding WithTimeout when a relevant env var is present, or explicitly document/align the chosen default with the upstream default.
| otlptracegrpc.WithTimeout(timeoutFromEnv( | |
| "OTEL_EXPORTER_OTLP_TRACES_TIMEOUT", | |
| "OTEL_EXPORTER_OTLP_TIMEOUT", | |
| )), | |
| } | |
| if strings.TrimSpace(os.Getenv("OTEL_EXPORTER_OTLP_TRACES_TIMEOUT")) != "" || | |
| strings.TrimSpace(os.Getenv("OTEL_EXPORTER_OTLP_TIMEOUT")) != "" { | |
| opts = append(opts, otlptracegrpc.WithTimeout(timeoutFromEnv( | |
| "OTEL_EXPORTER_OTLP_TRACES_TIMEOUT", | |
| "OTEL_EXPORTER_OTLP_TIMEOUT", | |
| ))) |
Go OTLP gRPC exporter parses
OTEL_EXPORTER_OTLP_TIMEOUTas milliseconds by default but Python parses it as seconds. So our current value set via the controller chart will be parsed incorrectly, causing intermittent traces export issues liketraces export: exporter export timeout: rpc error: code = DeadlineExceeded desc = context deadline exceeded