Skip to content

[Part 4/6] feat(telemetry): add memory monitor with activity-aware recording and tests#8122

Merged
jacob314 merged 5 commits intogoogle-gemini:mainfrom
eLyiN:split/memory-monitor
Oct 7, 2025
Merged

[Part 4/6] feat(telemetry): add memory monitor with activity-aware recording and tests#8122
jacob314 merged 5 commits intogoogle-gemini:mainfrom
eLyiN:split/memory-monitor

Conversation

@eLyiN
Copy link
Copy Markdown
Contributor

@eLyiN eLyiN commented Sep 9, 2025

Summary

Adds comprehensive MemoryMonitor that provides activity-aware memory tracking, automatic lifecycle management, and integration APIs for global monitoring. Uses components from Parts 1-3 for intelligent recording strategies.

Dependencies

⚠️ This PR depends on:

Review Order: Please review Parts 1-3 first (#8110, #8111, #8113), then this PR.

Related PRs - Enhanced Performance Monitoring Split

This is Part 4 of 6 in the split implementation of #2127:

Changes in This PR

MemoryMonitor (packages/core/src/telemetry/memory-monitor.ts)

Complete memory monitoring solution with activity-aware recording, high-water mark tracking, rate limiting with priority bypass, and comprehensive metrics including heap used, heap total, RSS, and external memory.

Memory Snapshot Types

ProcessMetrics and MemorySnapshot interfaces providing structured memory data with component attribution and activity state context.


@eLyiN
Copy link
Copy Markdown
Contributor Author

eLyiN commented Sep 25, 2025

@jacob314 @joshualitt PR 4 Ready for Review

@eLyiN
Copy link
Copy Markdown
Contributor Author

eLyiN commented Sep 25, 2025

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive MemoryMonitor for telemetry, featuring activity-aware recording, high-water mark tracking, and rate limiting. The implementation is well-structured and includes a thorough test suite. My review focuses on improving the testability of the singleton pattern and the robustness and clarity of the tests to ensure long-term maintainability.

@joshualitt
Copy link
Copy Markdown
Contributor

Thanks for the PR! Thoughts on periodically calling the cleanup() methods on HighWaterMarkTracker and RateLimiter perhaps on a much longer interval than the monitoring itself? While not strictly necessary with the current static keys, it should hopefully be a small change with minimal performance impact, and would proactively safeguard against potential future memory leaks if dynamic keys are ever introduced.

@eLyiN eLyiN force-pushed the split/memory-monitor branch from 75744ac to bcf71bc Compare October 4, 2025 11:09
@eLyiN
Copy link
Copy Markdown
Contributor Author

eLyiN commented Oct 4, 2025

Thanks for the PR! Thoughts on periodically calling the cleanup() methods on HighWaterMarkTracker and RateLimiter perhaps on a much longer interval than the monitoring itself? While not strictly necessary with the current static keys, it should hopefully be a small change with minimal performance impact, and would proactively safeguard against potential future memory leaks if dynamic keys are ever introduced.

Great call on proactively cleaning up the tracker state. I've improved it, the latest commit now schedules periodic pruning of both the high-water mark tracker and the rate limiter, so even if we start emitting dynamic keys we won’t leak memory. While we were in there, we finished aligning the memory monitor with PR #10215 schema update, so the new metric attributes flow end-to-end. Tests are still all green.

@joshualitt joshualitt requested a review from jacob314 October 6, 2025 16:09
Copy link
Copy Markdown
Contributor

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jacob314 jacob314 enabled auto-merge October 7, 2025 23:15
@jacob314 jacob314 added this pull request to the merge queue Oct 7, 2025
Merged via the queue into google-gemini:main with commit 8cd2ec7 Oct 7, 2025
20 checks passed
giraffe-tree pushed a commit to giraffe-tree/gemini-cli that referenced this pull request Oct 10, 2025
…cording and tests (google-gemini#8122)

Co-authored-by: Jacob Richman <jacob314@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority/p2 Important but can be addressed in a future release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants