feat(otel): add request params, response metadata, tool call parity, and agent identification - #11261
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35f3a81b08
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| pub(super) fn record_request_params(span: &Span, model_config: &ModelConfig) { | ||
| if let Some(temperature) = model_config.temperature { | ||
| span.record("gen_ai.request.temperature", temperature as f64); | ||
| } | ||
| if let Some(max_tokens) = model_config.max_tokens { | ||
| span.record("gen_ai.request.max_tokens", max_tokens as i64); |
There was a problem hiding this comment.
When request_params overrides temperature or max_tokens, these attributes report the typed ModelConfig fields rather than the values actually sent. The OpenAI request builder applies request_params after those fields (and its test demonstrates configured 4096/unset temperature becoming 1/2.0); it also suppresses temperature for reasoning models, while this helper still records it. Traces for those configurations therefore describe a different request, so record the provider-normalized values instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this is technically correct. request_params is applied after temperature/max_tokens via obj.insert() in the OpenAI request builder, and reasoning models suppress temperature entirely.
However, fixing this properly would require threading the final resolved values back up from deep inside the provider format parsers (async streaming generators), or restructuring where telemetry recording happens relative to request construction. That's a significant architectural change that crosses the provider boundary.
In practice, request_params overriding temperature/max_tokens is rare (most users set them via ModelConfig directly), and the reasoning model suppression is a narrow edge case. Recording "what the user configured" is arguably useful for debugging even if it diverges from what the provider received after internal filtering.
Deferring to a follow-up PR to keep this one focused on the fields defined in #11140.
| if let Some(reasons) = &usage.finish_reasons { | ||
| let reasons_json = serde_json::to_string(reasons).unwrap_or_default(); | ||
| span.record("gen_ai.response.finish_reasons", reasons_json.as_str()); |
There was a problem hiding this comment.
gen_ai.response.finish_reasons is a string-array semantic-convention attribute, but serializing the vector and passing &str makes tracing-opentelemetry export a scalar string such as "[\"stop\"]". This affects every provider response that supplies a finish reason and breaks consumers that query or validate the attribute as an array; set an OpenTelemetry array-valued attribute rather than JSON-encoding it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct per the OTEL semantic convention spec, but there's a practical limitation: Rust's tracing crate's Span::record() API doesn't support array-valued fields.
Every attribute in this codebase is recorded through the tracing facade, not the opentelemetry SDK directly.
Fixing this would require either bypassing tracing and using opentelemetry types directly (mixing two tracing APIs), or implementing a custom tracing::Value for arrays — both are non-trivial and could interact poorly with the optional telemetry feature gate.
The JSON-encoded string ("[\"stop\"]") works with MLflow (our target consumer) today. Deferring to a follow-up where we can investigate the right approach without risking the plumbing in this PR.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2450d19088
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2450d19 to
b18d6ea
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b18d6ea0a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 179c8a2958
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 849c00d1e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69b59888d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ba81630f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| trace_output = tracing::field::Empty, | ||
| session.id = %session_config.id, | ||
| gen_ai.operation.name = "invoke_agent", | ||
| gen_ai.agent.name = tracing::field::Empty, |
There was a problem hiding this comment.
When a valid ElicitationResponse is submitted, reply_impl returns an empty stream at lines 1860-1892 before either the state-machine runner or the legacy session load records this field, so the root invoke_agent span for every accept/decline/cancel response has no gen_ai.agent.name. Fresh evidence beyond the earlier command-path finding is that this final revision still leaves the elicitation return ahead of all agent-name assignments; resolve the session name before this branch or record it while completing the elicitation.
Useful? React with 👍 / 👎.
|
LGTM |
# Conflicts: # crates/goose/src/agents/state_machine/ops_toolcalling.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b27d3e74c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let has_recorded_tokens = usage_effects.iter().any(|effect| { | ||
| matches!( | ||
| effect, | ||
| GooseEffect::RecordUsage(usage) | ||
| if usage.usage.input_tokens.is_some() | ||
| || usage.usage.output_tokens.is_some() | ||
| || usage.usage.total_tokens.is_some() | ||
| ) | ||
| }); | ||
| if !has_recorded_tokens { |
There was a problem hiding this comment.
When an OpenAI-compatible stream supplies only a response ID or finish reason, usage_effects already contains a metadata-only RecordUsage; this predicate enters the fallback but later appends a second record rather than updating the existing one. Fresh evidence after the earlier estimation finding is that SessionManager::apply records and emits every effect, so state-machine sessions insert two usage-ledger rows and emit two Usage events for one model call, with the first containing null token fields; merge the estimates into the existing metadata-bearing record instead.
AGENTS.md reference: AGENTS.md:L21-L23
Useful? React with 👍 / 👎.
* main: (70 commits) cli: remove recipe secret discovery (#11435) fix(openrouter): escape Gemini tool response ref keys (#11276) fix(security): honor MCP tool model visibility in Code Mode (#11425) fix(providers): estimate cost for Azure Foundry models via inferred catalog pricing (#11264) feat(providers): add Gondola as declarative OpenAI-compatible provider (#11421) feat(otel): add request params, response metadata, tool call parity, and agent identification (#11261) fix(providers): coalesce consecutive Thinking blocks in collect_stream (#11317) feat(hooks): add PreToolUseResult event and stable tool_call_id across tool lifecycle (#11120) add MCP conformance tests to goose CI (combines #10800 + #10801) (#10940) feat(desktop): sort configured providers to the top of the provider list (#11409) fix(cli): refuse symlink diagnostics outputs (#11398) test(plugins): isolate GOOSE_PATH_ROOT in discovery tests (#11407) fix(config): serialize secret mutations (#11388) fix: decouple source file and tool response limits (#11391) chore(deps): bump pctx_code_mode from 0.4.1 to 0.5.0 (#11245) fix(security): suppress sensitive OTLP traces (#11381) feat(openrouter): forward session_id and add app category header (#10868) feat(acp): derive and forward thinking effort from the ACP harness (#10949) fix(aws_bedrock): replace flat model list with routing table, add Gemma 4 Mantle support (#10297) Add GPT-5.6 follow-up support for Codex and Responses API (#10460) ...
* main: (107 commits) fix(providers): inform user of clipboard copy and remove copilot auth retry on timeout (aaif-goose#11160) feat(desktop): select saved recipes when creating a schedule (aaif-goose#10892) More provider test scripts (aaif-goose#10515) cli: remove recipe secret discovery (aaif-goose#11435) fix(openrouter): escape Gemini tool response ref keys (aaif-goose#11276) fix(security): honor MCP tool model visibility in Code Mode (aaif-goose#11425) fix(providers): estimate cost for Azure Foundry models via inferred catalog pricing (aaif-goose#11264) feat(providers): add Gondola as declarative OpenAI-compatible provider (aaif-goose#11421) feat(otel): add request params, response metadata, tool call parity, and agent identification (aaif-goose#11261) fix(providers): coalesce consecutive Thinking blocks in collect_stream (aaif-goose#11317) feat(hooks): add PreToolUseResult event and stable tool_call_id across tool lifecycle (aaif-goose#11120) add MCP conformance tests to goose CI (combines aaif-goose#10800 + aaif-goose#10801) (aaif-goose#10940) feat(desktop): sort configured providers to the top of the provider list (aaif-goose#11409) fix(cli): refuse symlink diagnostics outputs (aaif-goose#11398) test(plugins): isolate GOOSE_PATH_ROOT in discovery tests (aaif-goose#11407) fix(config): serialize secret mutations (aaif-goose#11388) fix: decouple source file and tool response limits (aaif-goose#11391) chore(deps): bump pctx_code_mode from 0.4.1 to 0.5.0 (aaif-goose#11245) fix(security): suppress sensitive OTLP traces (aaif-goose#11381) feat(openrouter): forward session_id and add app category header (aaif-goose#10868) ...
2 participants
Summary
Closes #11140
Adds missing OpenTelemetry GenAI semantic convention fields across both the legacy agent loop and state machine path:
gen_ai.request.temperatureandgen_ai.request.max_tokenson chat spans when configuredgen_ai.response.finish_reasons(e.g.["stop"],["tool_calls"]) andgen_ai.response.idcarried from provider format parsers throughProviderUsageto OTEL spansgen_ai.tool.call.argumentsandgen_ai.tool.call.resultadded to the state machine path (already existed in legacy), gated byOTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENTgen_ai.agent.nameon root spans — uses recipe title when available, explicit agent name if set, otherwise"goose"replyspan was closing ~100ms after creation (when the async fn returned the stream) instead of when the stream finished (~34s later), causing MLflow to show traces as "inprogress". Fixed by instrumenting the outer stream with the reply span, matching the state machine path's existing pattern.
All new logic lives in shared helpers in
gen_ai_telemetry.rsto avoid duplication between the two agent loop paths.Changed files
Core telemetry helpers (
crates/goose/src/agents/gen_ai_telemetry.rs):record_request_params()— records temperature/max_tokens fromModelConfigrecord_tool_arguments()/record_tool_result()— content-gated tool telemetryagent_name()— resolves agent name from session contextrecord_provider_usage()to includefinish_reasonsandresponse_idProvider format parsers (carry
finish_reasons/response_idthrough toProviderUsage):crates/goose-provider-types/src/formats/anthropic.rscrates/goose-provider-types/src/formats/openai.rscrates/goose-provider-types/src/formats/google.rscrates/goose-provider-types/src/formats/openai_responses.rscrates/goose-provider-types/src/conversation/token_usage.rsBoth agent loop paths:
crates/goose/src/agents/agent.rs— legacy loop spans + root span lifecycle fixcrates/goose/src/agents/reply_parts.rs— shared provider chat spanscrates/goose/src/agents/state_machine/ops_llm.rs— state machine chat spanscrates/goose/src/agents/state_machine/ops_toolcalling.rs— state machine tool spanscrates/goose/src/agents/state_machine/machine.rs— agent name on root spanSession (
crates/goose/src/session/session_manager.rs):agent_name: Option<String>toSessionTest plan
gen_ai_telemetry.rscovering request params, tool argument gating, tool result recording, provider usage with new fields, and agent name resolutionProviderUsageserialization tests (roundtrip, omit-none backward compat, deserialize-without-new-fields)finish_reasonsandresponse_idcargo fmtcleancargo clippy -p goose -p goose-provider-types -- -D warningscleanOTEL_TRACES_EXPORTER=console— verified spans contain new fields and root span timing is correctexport OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT=true