fix(otel): gate content attributes behind captureContent in toolCalli…#4938
fix(otel): gate content attributes behind captureContent in toolCalli…#4938openjny wants to merge 2 commits intomicrosoft:mainfrom
Conversation
…ngLoop and chatMLFetcher Content attributes (INPUT_MESSAGES, OUTPUT_MESSAGES, TOOL_DEFINITIONS) and user_message span events were unconditionally set via span.setAttribute() in toolCallingLoop.ts and chatMLFetcher.ts. Because ReadableSpan is immutable once created, these attributes leaked to the OTLP exporter even when captureContent was false. Gate all content attribute writes behind `otelService.config.captureContent`, matching the existing pattern in genAiEvents.ts and BYOK providers. Note: With captureContent=false, the Debug Panel will also not show content. This matches the documented behavior in agent_monitoring_arch.md. Fixes microsoft/vscode#307407
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR updates OpenTelemetry (OTel) instrumentation to ensure prompt/response content is only attached to spans when captureContent is enabled, preventing content leakage to OTLP exporters and aligning debug-panel visibility with the single captureContent privacy flag.
Changes:
- Gate content-bearing span attributes/events in
toolCallingLoop.tsbehindthis._otelService.config.captureContent. - Gate
GenAiAttr.INPUT_MESSAGEScapture inchatMLFetcher.tsbehindthis._otelService.config.captureContent. - Add a new Vitest suite (
contentGating.spec.ts) validating content gating behavior for agent and inference span patterns.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/extension/intents/node/toolCallingLoop.ts | Wraps span content capture (INPUT_MESSAGES, OUTPUT_MESSAGES, TOOL_DEFINITIONS, user_message) behind captureContent. |
| src/extension/prompt/node/chatMLFetcher.ts | Wraps INPUT_MESSAGES capture behind captureContent to prevent unintended export. |
| src/platform/otel/common/test/contentGating.spec.ts | Adds unit tests asserting content attributes/events are gated by captureContent. |
| // Capture full request content — gated by captureContent to prevent OTLP leakage | ||
| if (otelInferenceSpan && this._otelService.config.captureContent) { | ||
| const capiMessages = (requestBody.messages ?? requestBody.input) as ReadonlyArray<{ role?: string; content?: string | unknown[] }> | undefined; | ||
| if (capiMessages) { | ||
| // Normalize non-string content (Anthropic arrays, Responses API parts) to strings for OTel schema |
There was a problem hiding this comment.
GenAiAttr.INPUT_MESSAGES is now gated on captureContent, but this block still sets other content-bearing attributes (notably CopilotChatAttr.USER_REQUEST and GenAiAttr.SYSTEM_INSTRUCTIONS) regardless of captureContent. SYSTEM_INSTRUCTIONS is explicitly classified as opt-in content in genAiAttributes.ts, and genAiEvents.ts only emits it when otel.config.captureContent is true. To avoid continued prompt leakage when captureContent=false (and to match the documented single-flag behavior), gate these attributes (and any other prompt/response content attributes set on this span) behind this._otelService.config.captureContent as well.
| if (this._otelService.config.captureContent) { | ||
| const lastRound = result.toolCallRounds.at(-1); | ||
| if (lastRound?.response) { | ||
| const responseText = Array.isArray(lastRound.response) ? lastRound.response.join('') : lastRound.response; |
There was a problem hiding this comment.
This new captureContent gate encloses setting OUTPUT_MESSAGES/TOOL_DEFINITIONS, but TOOL_DEFINITIONS is still serialized without truncateForOTel(). Tool schemas/descriptions can get large enough to exceed backend attribute limits and cause OTLP export/batch failures; elsewhere (e.g. emitInferenceDetailsEvent) content attributes are truncated. Consider truncating the serialized tool definitions too for consistency and exporter robustness when captureContent is enabled.
| /** | ||
| * Tests that content attributes (INPUT_MESSAGES, OUTPUT_MESSAGES, user_message events) | ||
| * are properly gated behind captureContent in agent and inference span patterns. | ||
| * | ||
| * Validates the fix for https://github.com/microsoft/vscode/issues/307407 |
There was a problem hiding this comment.
The new tests validate gating for INPUT_MESSAGES/OUTPUT_MESSAGES/TOOL_DEFINITIONS and user_message, but they don’t cover other content-bearing attributes that are currently emitted by the same spans (e.g. GenAiAttr.SYSTEM_INSTRUCTIONS, CopilotChatAttr.USER_REQUEST, CopilotChatAttr.REASONING_CONTENT). Adding assertions for these would better prevent future regressions where captureContent=false still leaks prompt/response content.
There was a problem hiding this comment.
The test concern is fair, but there's a bigger issue - these tests duplicate the if (captureContent) check inline rather than calling through ToolCallingLoop or ChatMLFetcher, so adding more attributes to assert on wouldn't help. They'd still pass even if the guards got removed from the production code. Either the tests need to exercise the real code paths, or they're not providing the regression safety they look like they do.
|
@openjny thanks for the pr. Can you take a look and address the copilot comments which seem valid ? |
|
@zhichli Appreciate for checking it, I'll work on that |
- chatMLFetcher: gate USER_REQUEST, SYSTEM_INSTRUCTIONS, OUTPUT_MESSAGES, and REASONING_CONTENT behind captureContent (previously only INPUT_MESSAGES was gated) - Replace inline-duplicating unit tests in contentGating.spec.ts with integration tests that exercise the real ToolCallingLoop code path, ensuring guards cannot be silently removed without test failures - Add CapturingOTelService config smoke tests
Summary
Fixes microsoft/vscode#307407
When
captureContent=false,toolCallingLoop.tsandchatMLFetcher.tsunconditionallyset
INPUT_MESSAGES,OUTPUT_MESSAGES,TOOL_DEFINITIONS, anduser_messagespan eventsvia
span.setAttribute(). BecauseReadableSpanis immutable once created, these attributesleaked to the OTLP exporter regardless of the
captureContentsetting.Changes
toolCallingLoop.ts: GateINPUT_MESSAGES,OUTPUT_MESSAGES,TOOL_DEFINITIONS,and
user_messageevents behindthis._otelService.config.captureContentchatMLFetcher.ts: GateINPUT_MESSAGESbehindthis._otelService.config.captureContentcontentGating.spec.ts(new): 7 tests verifying content attributes are set/unsetbased on
captureContentfor both agent and inference span patternsPattern
Matches the existing
if (otel.config.captureContent)pattern used ingenAiEvents.tsand BYOK providers (
anthropicProvider.ts).Debug Panel Impact
With
captureContent=false, the Debug Panel will also no longer display content.This is consistent with the design documented in
agent_monitoring_arch.md:Testing
npm run lint✅npm run compile✅ (0 errors)