Refactor trace ID retrieval for consistent invocation responses#7865
Refactor trace ID retrieval for consistent invocation responses#7865therealjohn wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors trace ID extraction/printing in the Azure AI Agents CLI extension to ensure a consistent trace ID is shown for HTTP responses by preferring x-request-id and falling back to apim-request-id.
Changes:
- Added
responseTraceID(*http.Response) stringhelper to centralize trace ID extraction logic. - Updated
responsesLocal,responsesRemote, andhandleInvocationResponseto use the helper and standardize the printed label format.
jongio
left a comment
There was a problem hiding this comment.
CI is blocked by a gofmt issue in the test file - the struct literal has mixed spaces/tabs.
Issues to address:
- invoke_test.go:960 - formatting: run
gofmt -w internal/cmd/invoke_test.goto fix
Refactor logic is clean - all 3 call sites updated, header preference matches the CORS allowed-headers in operations.go and foundry_toolsets_client.go, and tests cover all 4 header combinations.
jongio
left a comment
There was a problem hiding this comment.
Addresses my previous feedback. The gofmt fix and int32 overflow clamping (min(i, math.MaxInt32)) look correct - all instances are covered and CI is green.
wbreza
left a comment
There was a problem hiding this comment.
Code Review — PR #7865
Refactor trace ID retrieval for consistent invocation responses
Already Addressed by Prior Reviewers
- ✅ Missing unit tests → added in commit 2
- ✅ gofmt mixed indentation → fixed in commit 3
- ✅ Overflow clamping confirmed covering all instances → approved by @jongio
New Observations
🟡 Low — PR description doesn't mention index overflow fixes
The description only covers trace ID refactoring, but 4 of the 6 changed files (\init.go, \init_from_code.go, \init_models.go, \helpers.go) are index overflow clamping with \int32(min(i, math.MaxInt32)). The third commit message documents this well, but the PR body could be updated to reflect all changes.
🟢 Nitpick — Output format padding change is undocumented
The trace ID output changed from "Trace ID: %s"\ to "Trace ID: %s"\ (extra spaces for alignment with other labels like "Session: "). Presumably intentional for visual consistency.
What Looks Good ✅
esponseTraceID()\ helper — clean extraction with correct header preference- Tests — table-driven, parallel, covers all 4 header combinations
- Overflow clamping — correct pattern with //nolint:gosec\ annotations
- Follows repo conventions (naming, error handling, test structure)
Verdict: Clean, well-scoped PR. No blocking issues.
|
/reopen |
When intermediaries like APIM combine duplicate response headers per RFC 7230 section 3.2.2, x-request-id can arrive as a comma-joined value (e.g. `<id>,<id>`), causing the printed trace ID to display the same value twice. Split on comma and return the first non-empty token so a single ID is shown regardless of folding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // and falling back to apim-request-id. If a header value is comma-folded (which | ||
| // can happen when an intermediary like APIM combines duplicate headers per | ||
| // RFC 7230 §3.2.2), the first non-empty token is returned. | ||
| func responseTraceID(resp *http.Response) string { |
There was a problem hiding this comment.
Taking both of these and returning them as a single thing is going to make it harder to debug requests, as we won't have a way of knowing which header it actually corresponds to.
What does "Trace ID" actually mean in terms of what it is tracking and why we are displaying it? What is the actual debugging component that we should be providing here?
There was a problem hiding this comment.
The goal is to return the x-request-id, but if that doesnt exist, use the apim-request-id.
For whatever reason, something to do with APIM, there are multiple x-request-ids in the response call (same values) that get wrapped into value,value which displays poorly.
wbreza
left a comment
There was a problem hiding this comment.
Clean, well-tested refactoring. The responseTraceID() helper correctly centralizes trace ID retrieval with proper x-request-id → apim-request-id fallback and RFC 7230 §3.2.2 comma-folded header handling. Test coverage is comprehensive (8 scenarios). No issues found.
jongio
left a comment
There was a problem hiding this comment.
Reviewed 2 changed files after rebase. The responseTraceID helper is clean — header preference logic, comma-folded handling, and test coverage all look good. No new findings beyond my earlier review.
This pull request refactors how trace IDs are extracted and displayed from HTTP responses in the Azure AI Agents CLI extension. The main improvement is the introduction of a helper function to consistently retrieve the trace ID, preferring the
x-request-idheader and falling back toapim-request-id. This change reduces code duplication and ensures the trace ID is displayed in a uniform format across different parts of the codebase.Trace ID extraction and display improvements:
responseTraceIDhelper function to extract the trace ID from the response headers, preferringx-request-idand falling back toapim-request-id(cli/azd/extensions/azure.ai.agents/internal/cmd/invoke.go).responsesLocal,responsesRemote, andhandleInvocationResponseto use the newresponseTraceIDfunction and standardized the output format (cli/azd/extensions/azure.ai.agents/internal/cmd/invoke.go) [1] [2] [3].Fixes #7864