feat(observability): add OTEL traces and logs to harnesses - #2805
Conversation
|
This recording shows the traces and logs for ADK, Claude, and Codex harness in Grafana input.mp4 |
krisztianfekete
left a comment
There was a problem hiding this comment.
Thanks @supreme-gg-gg, this is looking really good, added some comments/questions!
cc. @dhaifley w/r/t the flushing problems you were working on recently.
| return nil | ||
| } | ||
| span := trace.SpanFromContext(ctx) | ||
| span.End() |
There was a problem hiding this comment.
Since this ends otelhttp's server span, so some attributes are likely lost. You can start your own child span in Before and end that one instead, or capture the status yourself before ending.
| corev1.EnvVar{Name: "OTEL_LOG_USER_PROMPTS", Value: "1"}, | ||
| corev1.EnvVar{Name: "OTEL_LOG_TOOL_DETAILS", Value: "1"}, |
There was a problem hiding this comment.
These are all off by default, so maybe we should wire these up to Helm, so it's opt-in? We should do this for all similar stuff as well (outside of this PR).
There was a problem hiding this comment.
For this and the following comments, I think we can have two helm values:
- one for including details like user prompt, tool, assistant response etc. which will apply to relevant settings for both traces and logs, defaulted to false (an alternative could be to have them configurable for traces and logs separately, but I assume most user would want both since traces would not contain all the details and would result in very granular settings)
- the other one for logging API bodies, since that one could be noisy and rarely useful compared to the first one
| corev1.EnvVar{Name: "OTEL_LOG_ASSISTANT_RESPONSES", Value: "1"}, | ||
| corev1.EnvVar{Name: "OTEL_LOG_RAW_API_BODIES", Value: "1"}, |
There was a problem hiding this comment.
These should potentially be opt-in as well, controlled separately from prompt logging as I'd expect this to be quite noisy/expensive?
|
|
||
| parsed, err := url.Parse(endpoint) | ||
| if err != nil || (parsed.Scheme != "http" && parsed.Scheme != "https") || parsed.Hostname() == "" || parsed.User != nil || parsed.RawQuery != "" || parsed.Fragment != "" { | ||
| return signalConfig{}, fmt.Errorf("OTLP %s endpoint must be an absolute HTTP(S) URL without credentials, query, or fragment", signal) |
There was a problem hiding this comment.
Can we check this at startup, and log a warning if it's wrong instead?
| if cfg.Telemetry != nil { | ||
| native.Otel = &nativeOtel{ | ||
| LogUserPrompt: cfg.Telemetry.CaptureContent, | ||
| Environment: "kagent", |
There was a problem hiding this comment.
What's the Codex default here?
There was a problem hiding this comment.
The default value for environment should be "dev" here: https://learn.chatgpt.com/docs/config-file/config-advanced#observability-and-telemetry; I'll remove this kagent setting
| environment = append(environment, traceConfig.Environment()...) | ||
| environment = append(environment, logConfig.Environment()...) |
There was a problem hiding this comment.
I think we have to set OTEL_SERVICE_NAME and OTEL_RESOURCE_ATTRIBUTES so we are fully consistent with our wrappers, don't we? Likely in both Codex and Claude.
There was a problem hiding this comment.
I don't see an option to configure service name in the docs unfortunately. We're already propagating traceparent and tracestate so that would give us trace correlation. Maybe we can just let each native CLI own their service identity?
There was a problem hiding this comment.
Most SDKs should honor OTEL_RESOURCE_ATTRIBUTES by defaukt, can you please check if that's the case here?
There was a problem hiding this comment.
I tried setting this for both harness but I don't seem to see the custom attributes. For Codex I think this is a known issue openai/codex#30987, not too sure why it's not working for Claude
| for _, variable := range input.Harness.Spec.Env { | ||
| if _, reserved := ownedEnvironment[variable.Name]; reserved || strings.HasPrefix(variable.Name, mcpCredentialPrefix) { | ||
| _, reserved := ownedEnvironment[variable.Name] | ||
| if reserved || strings.HasPrefix(variable.Name, mcpCredentialPrefix) || strings.HasPrefix(variable.Name, otelEnvironmentPrefix) { |
There was a problem hiding this comment.
We have to pass some OTEL_ env vars to not break functionality (I am thinking mainly about the export intervals, but maybe even the endpoints?)
| // TraceConfig is the controller-owned trace export configuration compiled into | ||
| // each runtime revision. | ||
| type TraceConfig struct { | ||
| Enabled bool | ||
| Endpoint string | ||
| Protocol string | ||
| hostname string | ||
| } | ||
|
|
||
| // LogConfig is the controller-owned log export configuration compiled into | ||
| // each runtime revision. | ||
| type LogConfig struct { | ||
| Enabled bool | ||
| Endpoint string | ||
| Protocol string | ||
| hostname string | ||
| } |
There was a problem hiding this comment.
Maybe this could be a SignalConfig instead?
EItanya
left a comment
There was a problem hiding this comment.
🤖 AI-generated review. Validated the HTTP response attribute loss discussed below.
|
@krisztianfekete I've addressed your comments in ad7cf7f, thanks for the review! |
krisztianfekete
left a comment
There was a problem hiding this comment.
Looks good to me! I am a bit paranoid about potential flushing problems still, can we maybe have an e2e test to ensure that part os robust as well?
|
@krisztianfekete I've added a flush-traces E2E test in 57cf9a7, can you take a look? |
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
6959402 to
e35ffda
Compare
Adds correlated OpenTelemetry tracing and logging across the Kagent, Codex, and Claude harnesses. Refactors shared tracing configuration and setup code. Builds on top of previous V2 observability works like #2663 and #2786. Also closes #2760 and partially addresses #2762.
Key Changes
Notes
OTEL_TRACING_ENABLEDandOTEL_LOGGING_ENABLEDgo/adk/pkg/a2a/server/server.go. However, during a pending HITL, since we do not quit the Codex or Claude process, it is not guaranteed that all traces are delivered immediately.