feat(telemetry): add rate limiter and high‑water mark tracker with tests#8109
feat(telemetry): add rate limiter and high‑water mark tracker with tests#8109eLyiN wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @eLyiN, 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 introduces foundational telemetry primitives, a HighWaterMarkTracker and a RateLimiter, designed to enhance performance monitoring by intelligently managing and reducing noise in metric collection. These additions are part of a larger effort to improve performance insights without overwhelming the system with excessive data.
Highlights
- HighWaterMarkTracker Introduction: Implements a mechanism to track and trigger recordings for memory metrics (like heap/RSS) only when significant growth occurs, effectively filtering out minor fluctuations and reducing noise.
- RateLimiter Introduction: Provides a utility to throttle the frequency of metric recordings, including a high-priority path for critical events, ensuring efficient data collection and preventing excessive telemetry.
- New API Exposure: The newly developed HighWaterMarkTracker and RateLimiter classes are now exported via the telemetry index, making them available for use in other parts of the codebase.
- Comprehensive Testing: Both new primitives come with extensive unit tests using Vitest, ensuring their reliability and correctness across various scenarios.
- Minimal Impact: The changes are self-contained within the telemetry module and do not affect CLI behavior, configuration, or introduce new dependencies, maintaining system stability.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces two useful telemetry primitives, HighWaterMarkTracker and RateLimiter, along with comprehensive tests. The implementation is clean and well-structured. I've identified a critical potential memory leak in HighWaterMarkTracker due to the absence of a cleanup mechanism. Additionally, I've found a couple of high-severity issues in RateLimiter: a bug in getTimeUntilNextAllowed that ignores high-priority events, and a potential crash in getStats when handling a large number of metrics. My review includes suggestions to address these points.
| * Only triggers when memory usage increases by a significant threshold | ||
| */ | ||
| export class HighWaterMarkTracker { | ||
| private waterMarks: Map<string, number> = new Map(); |
There was a problem hiding this comment.
The waterMarks map stores high-water marks for each metric type but never removes entries. If a large number of unique metricType strings are used over the lifetime of a HighWaterMarkTracker instance, this map will grow indefinitely, causing a memory leak.
A cleanup mechanism should be implemented to periodically remove stale metric types that haven't been updated in a long time. The test file high-water-mark-tracker.test.ts even contains a describe('time-based cleanup', ...) block, which suggests this functionality was intended but is now missing. A possible solution would be to store timestamps along with watermarks and add a cleanup method similar to the one in RateLimiter.
| getTimeUntilNextAllowed(metricKey: string): number { | ||
| const now = Date.now(); | ||
| const lastRecordTime = this.lastRecordTimes.get(metricKey) || 0; | ||
| const nextAllowedTime = lastRecordTime + this.minIntervalMs; | ||
|
|
||
| return Math.max(0, nextAllowedTime - now); | ||
| } |
There was a problem hiding this comment.
The getTimeUntilNextAllowed method does not account for the isHighPriority flag. It always calculates the remaining time based on this.minIntervalMs, while shouldRecord correctly uses a shorter interval for high-priority events. This inconsistency can lead to incorrect scheduling if a consumer of this class relies on getTimeUntilNextAllowed for high-priority events.
The method should accept an isHighPriority parameter to calculate the correct interval. Also, the corresponding tests should be updated to cover this case.
| getTimeUntilNextAllowed(metricKey: string): number { | |
| const now = Date.now(); | |
| const lastRecordTime = this.lastRecordTimes.get(metricKey) || 0; | |
| const nextAllowedTime = lastRecordTime + this.minIntervalMs; | |
| return Math.max(0, nextAllowedTime - now); | |
| } | |
| getTimeUntilNextAllowed(metricKey: string, isHighPriority: boolean = false): number { | |
| const now = Date.now(); | |
| const lastRecordTime = this.lastRecordTimes.get(metricKey) || 0; | |
| const interval = isHighPriority ? this.minIntervalMs / 2 : this.minIntervalMs; | |
| const nextAllowedTime = lastRecordTime + interval; | |
| return Math.max(0, nextAllowedTime - now); | |
| } |
| const oldest = Math.min(...recordTimes); | ||
| const newest = Math.max(...recordTimes); |
There was a problem hiding this comment.
The use of the spread operator (...) with Math.min and Math.max can cause a "Maximum call stack size exceeded" error if the recordTimes array becomes very large. While the cleanup method limits the growth of the underlying map, it's safer to use an approach that doesn't have this limitation, such as using Array.prototype.reduce().
| const oldest = Math.min(...recordTimes); | |
| const newest = Math.max(...recordTimes); | |
| const oldest = recordTimes.reduce((a, b) => Math.min(a, b)); | |
| const newest = recordTimes.reduce((a, b) => Math.max(a, b)); |
Summary
Introduces two self-contained telemetry primitives used by subsequent performance monitoring work:
• HighWaterMarkTracker: triggers on significant growth for memory metrics (e.g., heap/rss), reducing noise
• RateLimiter: throttles metric recording with an optional high-priority path
This is the first PR in a series that splits the enhanced performance monitoring work into smaller, reviewable chunks.
Changes
Test Plan
npm run build)npm run typecheck)npm run lint)npm run preflight)Part of #2127