-
Notifications
You must be signed in to change notification settings - Fork 614
Add session lifecycle hooks for observability #179
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?
Add session lifecycle hooks for observability #179
Conversation
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 adds opt-in OpenTelemetry instrumentation to the .NET SDK for distributed tracing and metrics, following OpenTelemetry GenAI Semantic Conventions.
Changes:
- Adds telemetry infrastructure with ActivitySource and Meter for spans and metrics
- Implements SessionTelemetryTracker to convert session events into OpenTelemetry spans
- Integrates telemetry tracking into the CopilotSession lifecycle
- Adds comprehensive unit tests (10 tests) for telemetry functionality
- Documents the observability features in README with examples
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/Telemetry/CopilotTelemetry.cs | Main telemetry class providing ActivitySource, Meter, and metric recording methods with opt-in enable/disable logic |
| dotnet/src/Telemetry/OpenTelemetryConstants.cs | Defines constants for span names, attribute names, and metric names following GenAI semantic conventions |
| dotnet/src/Telemetry/SessionTelemetryTracker.cs | Converts session events to OpenTelemetry spans, managing lifecycle of session, turn, tool, subagent, hook, and inference activities |
| dotnet/src/Session.cs | Integrates telemetry tracker into session, instantiating it on construction and disposing on cleanup |
| dotnet/src/GitHub.Copilot.SDK.csproj | Adds OpenTelemetry.Api package reference and InternalsVisibleTo for test project |
| dotnet/test/TelemetryTests.cs | Comprehensive unit tests covering all major span types and telemetry scenarios |
| dotnet/README.md | Documents how to enable and configure OpenTelemetry, lists available spans and metrics, includes example trace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for contributing this, @reachanihere! Rather than integrating otel concepts/dependencies into the SDK directly here (and diverging from the other-language SDKs), the way we'd normally approach this is by suggesting there be an Aspire-specific package for the GitHub Copilot SDK (e.g., Of course for this to be possible, we need Perhaps the one missing piece is some way to register a callback on |
|
Thank you a lot for your feedback, @SteveSandersonMS! Totally agree, keeping the SDK lean and letting observability live in a separate Aspire package would be a better approach. I think the session lifecycle hooks would unblock this scenario. I also verified that all the events I was using (turns, tools, inference, usage, etc.) are already exposed via Would it be ok if I update this PR to just add those hooks (and removing all OTel implementation)? And for the Aspire package itself, is that something your team would maintain alongside the SDK? |
|
@reachanihere Yes, that sounds great. Thanks!
Not currently. We're trying to keep our focus more tightly defined. |
…etrics Add opt-in OpenTelemetry instrumentation to the .NET SDK, enabling distributed tracing and metrics collection for Copilot sessions. ## Features - ActivitySource "GitHub.Copilot.SDK" for distributed tracing - Meter "GitHub.Copilot.SDK" for metrics - Spans for session, turn, tool execution, subagent, hook, and inference events - Metrics for token usage, cost, tool executions, errors, and duration - Follows OpenTelemetry GenAI Semantic Conventions ## Enabling Telemetry Telemetry is disabled by default. Enable via: - AppContext switch: `GitHub.Copilot.EnableOpenTelemetry` - Environment variable: `GITHUB_COPILOT_ENABLE_OPEN_TELEMETRY=true` ## New Files - src/Telemetry/CopilotTelemetry.cs - Main telemetry class - src/Telemetry/OpenTelemetryConstants.cs - Span/attribute constants - src/Telemetry/SessionTelemetryTracker.cs - Event to span conversion - test/TelemetryTests.cs - Unit tests
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Add _disposeLock to synchronize ProcessEvent and Dispose - Properly dispose all orphaned activities during Dispose - Prevents race conditions and resource leaks
4df6a5e to
64af6b9
Compare
Thanks @SteveSandersonMS, request to please review the updated PR |
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Tests verify: - SessionCreated fires when sessions are created - SessionDestroyed fires when sessions are disposed - Events fire correctly for multiple sessions - SessionCreated fires when sessions are resumed
When ResumeSessionAsync replaces an existing session, clear the old session's OnDisposed callback to prevent it from firing SessionDestroyed if disposed later. Added test to verify old session disposal doesn't fire spurious events.
5fb9f14 to
63adfee
Compare
Summary
Add session lifecycle events to enable external observability integration.
Based on feedback, OpenTelemetry instrumentation is better suited for a separate Aspire package rather than being built into the core SDK. This PR adds the minimal hooks needed to enable that external integration.
Changes
New Events on
CopilotClientSessionCreatedCopilotSessioninstanceSessionDestroyedUsage
Closes #181