fix: scope message trace summaries by session#124
Conversation
Summary of ChangesHello @teng-lin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an architectural concern by implementing session-scoped message tracing summaries. Previously, trace statistics were aggregated globally, which could lead to inaccurate or misleading data when multiple sessions were active. The changes introduce mechanisms to associate traces with specific sessions and modify the summary generation logic to filter data based on the provided session ID, thereby providing more precise and relevant tracing insights per session. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements session-scoped message trace summaries, addressing a finding from an architecture review. The changes correctly track session ownership for traces and update the summary() method to compute statistics for a specific session. The addition of a regression test ensures this new behavior is verified. My review includes a suggestion to improve maintainability by refactoring the data structure for completed traces, and a critical recommendation to implement a bounded cache to prevent potential Out-of-Memory errors in long-running processes.
| private readonly completedTraceSessions: Array<string | undefined> = []; | ||
| private readonly completedTraces: number[] = []; // round-trip ms values |
There was a problem hiding this comment.
Using parallel arrays for completedTraceSessions and completedTraces can be fragile and harder to maintain. It's easy for them to get out of sync if a developer modifies one but not the other in the future. More critically, if these arrays are accumulating data in a long-running process, they can lead to unbounded memory growth and potential Out-of-Memory (OOM) errors.
Consider refactoring this to use a single array of objects, which would group related data together and make the code more robust and readable. Additionally, for long-running processes, implement a bounded cache with an eviction policy (e.g., FIFO) for this session-related data to prevent unbounded memory growth and potential OOM errors. This would involve changing how completed traces are added, evicted, and summarized.
For example:
// In summary()
for (const trace of this.completedTraces) {
if (trace.sessionId !== sessionId) continue;
complete += 1;
completeLatencyTotal += trace.latency;
}
// In emit()
this.completedTraces.push({ latency: elapsedMs, sessionId: state.sessionId });| private readonly completedTraceSessions: Array<string | undefined> = []; | |
| private readonly completedTraces: number[] = []; // round-trip ms values | |
| private readonly completedTraces: Array<{ latency: number; sessionId?: string }> = []; |
References
- Implement a bounded cache with an eviction policy (e.g., FIFO) for session-related maps or counters in long-running processes to prevent unbounded memory growth and potential Out-of-Memory (OOM) errors.
1 participant
Summary
Fixes the architecture-review finding: "Message tracing summary is global, not session-scoped" from
docs/review/architecture-review-2026-02-22.md.MessageTracerImpl.summary(sessionId)now returns stats scoped to the requested session instead of process-global aggregates.What Changed
src/core/messaging/message-tracer.ts.summary(sessionId)to compute:totalTraces,complete,stale,errors, andavgRoundTripMssessionId.src/core/messaging/message-tracer.test.tsto verify summaries exclude other sessions.Validation
pnpm exec vitest run src/core/messaging/message-tracer.test.ts src/core/messaging/message-tracer-security.test.tspnpm typecheckpnpm -r --include-workspace-root test -- --coverageAll commands passed, including coverage gates.