feat(vmcp): log active sessions on creation, expiry, and periodically#4019
feat(vmcp): log active sessions on creation, expiry, and periodically#4019
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00fd4857c7
ℹ️ 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.
Pull request overview
Adds session visibility to the vMCP /status endpoint to support operational debugging and compliance auditing when SessionManagementV2 is enabled.
Changes:
- Extend
/statusresponse schema to optionally include active vMCP sessions and backend session ID mappings. - Add
ListActiveSessions()to the session manager and expose the data throughbuildStatusResponse. - Add unit tests for
ListActiveSessions()behavior and a regression test thatsessionsis omitted when V2 is disabled.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/server/status.go | Adds optional sessions block to /status when SessionManagementV2 is enabled. |
| pkg/vmcp/server/status_test.go | Updates status response test DTOs; adds tests for sessions omission and backend field redaction. |
| pkg/vmcp/server/sessionmanager/session_manager.go | Introduces SessionInfo and implements ListActiveSessions(). |
| pkg/vmcp/server/sessionmanager/list_sessions_test.go | Adds tests for listing behavior across empty, placeholder-only, and populated stores. |
| pkg/vmcp/server/session_manager_interface.go | Extends the SessionManager interface with ListActiveSessions(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4019 +/- ##
=======================================
Coverage 68.62% 68.63%
=======================================
Files 445 445
Lines 45374 45426 +52
=======================================
+ Hits 31140 31178 +38
- Misses 11827 11843 +16
+ Partials 2407 2405 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jerm-dro
left a comment
There was a problem hiding this comment.
Is this change really necessary? I get that it'd be nice to have visibility into active sessions, but exposing an endpoint seems heavy-handed. Alternative approaches:
- periodically log all the active sessions in memory
- log sessions as they are created and expired
What do you think?
Add session visibility to the /status endpoint for operational debugging and compliance auditing when sessionManagementV2 is enabled. Closes: #3876
ok this task came generated by claude, based on the rfc. A bit overkill, i agree. So i modified the issue to log content, as you were saying, and i modified the pr. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace the /status endpoint session fields with structured logging: - CreateSession logs Info with session ID, backend count, and names - Terminate logs Info with session ID and backend count on expiry - StartPeriodicLogging starts a background goroutine that logs active session counts every minute via logActiveSessions() - Server.Start() wires up periodic logging when SessionManagementV2 is on Remove ListActiveSessions() from the SessionManager interface and drop the /status session fields (Sessions, SessionsStatus, BackendUsage). Closes: #3876 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> fix(vmcp): drop else after return in Terminate (revive lint) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| // SupportsRange reports whether the current storage backend supports Range(). | ||
| // Returns true only for LocalStorage; distributed backends (Redis, Valkey) return false. | ||
| func (m *Manager) SupportsRange() bool { | ||
| _, ok := m.storage.(*LocalStorage) | ||
| return ok | ||
| } | ||
|
|
There was a problem hiding this comment.
Blocker: How about we wait to implement this until we can support all session storage types?
We're imminently going to introduce Redis storage. That will come along with a LRU cache for in-memory sessions. That will be easier to instrument in a way that works for single and multi replica use cases.
Log session lifecycle events and periodic active session counts for operational debugging and compliance auditing when sessionManagementV2 is enabled.
Changes
Manager.CreateSession(): upgraded log toInfowith session ID, backend count, and backend namesManager.Terminate(): logInfowith session ID and backend count when a MultiSession expires; separate log for placeholder terminationManager.StartPeriodicLogging(ctx, interval): new method that starts a background goroutine logging active session counts every minuteManager.logActiveSessions(): internal helper that iterates sessions viaRange(), aggregates per-backend session counts, and logs a summary; logs a one-time warning when distributed storage makes enumeration unavailableServer.Start(): callsStartPeriodicLoggingwhenSessionManagementV2is enabledRemoved
/statusendpoint session fields (Sessions,SessionsStatus,BackendUsage) — session data is now surfaced through logs onlyListActiveSessions()fromSessionManagerinterfaceCloses: #3876