-
Notifications
You must be signed in to change notification settings - Fork 1.1k
.NET: Ensure Activity/TraceId preservation for OpenTelemetryAgent #3233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: rogerbarreto <[email protected]>
…ndaries Preserve Activity.Current across ConfigureAwait(false) boundaries by: 1. Capturing Activity.Current at start of RunCoreAsync/RunCoreStreamingAsync 2. Restoring it after each await with ConfigureAwait(false) 3. Ensuring it's set before yielding to consumer code in streaming This fixes distributed tracing when using ChatClientAgent with tools/MCP. Co-authored-by: rogerbarreto <[email protected]>
- Enhanced comments to clarify why Activity.Current restoration is needed - Specified what "consumer code" means in comments - Improved comment clarity for tracing context flow Co-authored-by: rogerbarreto <[email protected]>
…metryAgent - Remove Activity/telemetry concepts from ChatClientAgent (removed System.Diagnostics using) - Add Activity preservation logic to OpenTelemetryAgent's ForwardingChatClient - Update tests to reflect that Activity preservation is an OpenTelemetryAgent feature - All 155 ChatClientAgent tests pass - All 26 OpenTelemetryAgent tests pass - All 4 Activity tracing tests pass This keeps ChatClientAgent agnostic of telemetry concerns while enabling Activity/TraceId preservation through the OpenTelemetryAgent decorator. Co-authored-by: rogerbarreto <[email protected]>
- Add OpenTelemetryAgent_WithMockedMcpTool_PreservesTraceId unit test - Test validates Activity/TraceId preservation with mock MCP tool - Add ModelContextProtocol package reference for future integration tests - Include comprehensive README with manual integration test instructions - Provide standalone sample code for manual verification The mocked test runs automatically and validates the pattern works correctly. Full integration tests with real Azure OpenAI can be run manually following the README. Co-authored-by: rogerbarreto <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors Activity/TraceId preservation from ChatClientAgent to OpenTelemetryAgent, correctly applying separation of concerns through the decorator pattern. The core ChatClientAgent is now telemetry-agnostic, while OpenTelemetryAgent handles all distributed tracing concerns.
Changes:
- Removed all Activity manipulation logic from
ChatClientAgent(telemetry-agnostic) - Added Activity preservation in
OpenTelemetryAgent.ForwardingChatClientfor both sync and streaming scenarios - Added comprehensive test coverage for Activity/TraceId preservation patterns including MCP tool scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI/OpenTelemetryAgent.cs | Enhanced ForwardingChatClient with Activity.Current preservation logic in GetResponseAsync (after ConfigureAwait) and GetStreamingResponseAsync (before yields and after completion) |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ActivityTracingTests.cs | New test file with 5 comprehensive tests validating OpenTelemetryAgent's Activity preservation across various scenarios including mocked MCP tools, streaming, and nested agents |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/Microsoft.Agents.AI.UnitTests.csproj | Added ModelContextProtocol package reference (appears unused) |
dotnet/tests/Microsoft.Agents.AI.UnitTests/Microsoft.Agents.AI.UnitTests.csproj
Outdated
Show resolved
Hide resolved
dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ActivityTracingTests.cs
Show resolved
Hide resolved
dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ActivityTracingTests.cs
Outdated
Show resolved
Hide resolved
…ntAgent_ActivityTracingTests.cs Co-authored-by: Copilot <[email protected]>
….UnitTests.csproj Co-authored-by: Copilot <[email protected]>
Motivation and Context
ChatClientAgentwas modified to includeActivity.Currentmanipulation for distributed tracing, introducing telemetry concerns into a core component designed to be telemetry-agnostic. This violates separation of concerns—telemetry should be a decorator concern, not baked into the base agent.Description
Moved Activity preservation logic from core to decorator:
ChatClientAgent (cleaned - telemetry-agnostic)
System.DiagnosticsimportActivity.Currentcapture/restore logic (23 assignments removed)OpenTelemetryAgent (enhanced - owns telemetry)
ForwardingChatClient.GetResponseAsync: Captures Activity fromForwardedOptions, restores afterConfigureAwait(false)ForwardingChatClient.GetStreamingResponseAsync: Restores Activity before each yield and after streaming completesForwardedOptions.CurrentActivitypatternTests
OpenTelemetryAgent_*patternChatClientAgentwithOpenTelemetryAgentOpenTelemetryAgent_WithMockedMcpTool_PreservesTraceIdintegration testPattern:
Architecture: Decorator pattern correctly applied—
ChatClientAgentprovides core functionality,OpenTelemetryAgentadds telemetry as an opt-in concern.Contribution Checklist
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.