[Part 6/6] feat(cli): add activity monitoring hooks and integrate telemetry documentation#8125
[Part 6/6] feat(cli): add activity monitoring hooks and integrate telemetry documentation#8125eLyiN wants to merge 17 commits intogoogle-gemini:mainfrom
Conversation
|
Please let me know when this is ready to review. Would be great to start tracking these metrics. |
c07d364 to
f6a4972
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request completes the performance monitoring implementation by adding React hooks for activity monitoring and integrating them into the CLI. The changes include new hooks useActivityMonitoring, useActivityRecorder, and useHistoryWithActivity, updates to useGeminiStream to record user activities, and comprehensive documentation.
My review identifies a few critical issues. The useActivityRecorder hook incorrectly logs all activities as the same type. In useGeminiStream, a generic activity recording function is used instead of the new specific recorders, which prevents detailed activity tracking. Additionally, useHistoryWithActivity doesn't correctly propagate the enableActivityMonitoring flag. These issues should be addressed to ensure the new monitoring system functions as intended.
f6a4972 to
27c3fc6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request completes the performance monitoring implementation by adding React hooks for activity monitoring and integrating them into the CLI. It also includes comprehensive documentation for the new telemetry system.
My review focuses on improving maintainability and type safety in the new hooks.
- I've pointed out that
useHistoryWithActivityduplicates logic fromuseHistoryinstead of wrapping it, which could lead to maintenance issues. - I've also suggested strengthening the type safety of the
useActivityMonitoringhook to prevent potential bugs with invalid activity types.
The changes in useGeminiStream.ts to record activities at various points seem correct and well-placed. The new tests and documentation are also valuable additions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request completes the performance monitoring implementation by adding React hooks for activity monitoring and integrating them into the CLI. The changes are well-structured, introducing useActivityMonitoring and useHistoryWithActivity hooks, and correctly instrumenting key user interactions in useGeminiStream. However, there is a critical bug in useHistoryManager where the new callbacks for history changes will never be triggered due to an incorrect pattern for handling side effects with asynchronous state updates. Additionally, a test for useActivityMonitoring is flawed and misleading. My review includes comments to address these critical and high-severity issues.
b2905c5 to
c4f61f0
Compare
|
@jacob314, finally, the last PR is ready for review (let me know if you have any requests or thoughts). |
c4f61f0 to
fcb0665
Compare
Tests passing, ready for re-review. |
eecc1e9 to
7a9ef72
Compare
…e memory monitoring details
…mized activity recording
…ing functions in useGeminiStream
…proper activity tracking
…ithActivity Introduce optional UseHistoryOptions to useHistory hook for extensibility, allowing callbacks on item addition, update, history clearing, and loading. Refactor useHistoryWithActivity to leverage the base hook with activity recording callbacks, eliminating code duplication and reducing overall lines by 131.
…ed activity tracking
Replace useHistory with useHistoryWithActivityMonitoring to complete the integration of activity tracking hooks at the top level. Changes: - AppContainer.tsx: Replace useHistory() with useHistoryWithActivityMonitoring(config) - AppContainer.test.tsx: Update mock to use the new hook name This completes the activity monitoring integration. Activity recording is now automatically enabled when isPerformanceMonitoringActive() returns true, providing complete activity tracking for all history operations (add/update/clear/load) combined with existing stream events.
…ring to useHistoryManagerWithActivity
- Fix mock path for useHistoryManagerWithActivity in AppContainer test - Add error handling to prevent activity recording crashes - Remove unused config parameter from useActivityRecorder - Fix test environment to enable jsdom for React hooks testing All 14 activity monitoring tests now pass. Changes improve code robustness and eliminate unused parameters across the codebase.
Add concrete configuration examples and verification steps for enabling activity monitoring. Include: - JSON configuration example with local/google targets - Verification steps for checking monitoring is active - Complete list of hardcoded defaults (timeouts, thresholds) Makes it easier for users to understand how to enable and verify activity monitoring in their telemetry setup.
7a9ef72 to
eda7499
Compare
|
Hey @jacob314 ! I rebased the branch onto origin/main and resolved the conflicts in useGeminiStream.ts. The conflicts occurred because main added a runInDevTraceSpan wrapper around submitQuery. I preserved your evolved approach with typed recording functions (recordStreamStart/End) and integrated them with the new tracing wrapper. FYI: Your branch has evolved significantly from the original PR-6 plan:
The evolution is great for code quality (type safety, semantic names), but it's a larger scope than originally planned. Let me know if you want to:
The rebase is done and tests should pass. Please review the conflict resolution in useGeminiStream.ts (lines 821-984) to ensure I preserved your intent correctly. |
|
Hi @eLyiN, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
|
Thank you for submission to the Gemini CLI project. At this time, we are closing this pull request in order to allow us to better triage and support more recent pull requests against the latest code changes. If you feel like this pull request is a critical contribution to the Gemini CLI project, please associate the pull request with an existing GitHub issue (instructions here: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue) before reopening. After Monday January 26 2026, any pull requests submitted by contributors without an associated issue will be automatically closed (more information here: #16706). If you do choose to reopen and submit this pull request, please ensure you rebase your changes onto the current main branch before resubmitting. This will help avoid merge conflicts and ensure your contribution is compatible with the latest codebase. |
Summary
Completes the enhanced performance monitoring implementation by adding CLI hooks for activity monitoring and integrating them throughout the CLI application. Includes comprehensive telemetry documentation and full AppContainer integration.
Dependencies
Review Order: Please review Parts 1-5 first, then this PR.
split/activity-monitorand includes all previous changes. Only review the NEW CLI integration components added in this PR.Related PRs - Enhanced Performance Monitoring Split
This is Part 6 of 6 in the split implementation of #2127:
Changes in This PR (NEW Components Only)
🆕 CLI Files Added:
packages/cli/src/ui/hooks/useActivityMonitoring.ts- React hook for activity monitoring (166 lines)packages/cli/src/ui/hooks/useActivityMonitoring.test.ts- Comprehensive hook unit tests (235 lines, 14 tests)packages/cli/src/ui/hooks/useHistoryManagerWithActivityMonitoring.ts- History management with activity tracking (45 lines)✏️ CLI Files Modified:
packages/cli/src/ui/hooks/useHistoryManager.ts- Added callback support (onItemAdded, onItemUpdated, onHistoryCleared, onHistoryLoaded)packages/cli/src/ui/hooks/useGeminiStream.ts- Integrated activity recording in stream flowspackages/cli/src/ui/AppContainer.tsx- Wired up useHistoryWithActivityMonitoringpackages/cli/src/ui/AppContainer.test.tsx- Updated mocks for new hook📚 Documentation Added:
docs/telemetry.md- Complete telemetry system documentation (217 insertions)CLI Activity Hooks
useActivityMonitoring Hook:
Features:
isPerformanceMonitoringActive()settinguseHistoryWithActivityMonitoring Hook:
Enhancements:
UseHistoryManagerReturninterfaceStream Integration
Activity Recording in
useGeminiStream.ts:recordToolCall()after slash command tool schedulingrecordUserInput()when adding normal user messagesrecordStreamStart()andrecordStreamEnd()in submitQueryAppContainer Integration
Complete Top-Level Integration:
useHistory()withuseHistoryWithActivityMonitoring(config)History Manager Enhancements
Callback Support in
useHistoryManager.ts:Benefits:
Telemetry Documentation
Comprehensive Documentation (
docs/telemetry.md):Testing (NEW Tests Only)
✅ CLI Hook tests:
useActivityMonitoring.test.ts- 14 tests covering hook behavior, lifecycle, activity recording✅ Integration validation:
✅ Build verification:
Technical Implementation
React Integration Architecture:
Zero Breaking Changes:
isPerformanceMonitoringActive()Benefits
Addresses Original Issues
Issue #1685 (startup/exit problems):
Issue #1687 (rate limiting + memory issues):
Performance optimization:
🎉 COMPLETE IMPLEMENTATION: This completes the enhanced performance monitoring implementation from #2127, providing production-ready monitoring with full CLI integration and comprehensive documentation.
Ready for Production: All components tested, documented, and integrated. Activity monitoring is feature-flagged and safe to activate in CLI workflows.