[Part 1/6] feat(telemetry): add rate limiter and high-water mark tracker with tests#8110
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 core utilities for optimizing telemetry data collection, specifically focusing on memory usage and metric recording frequency. These additions aim to make future performance monitoring more efficient by filtering out insignificant data and controlling the rate at which metrics are captured, without altering existing CLI behavior or dependencies.
Highlights
- New Telemetry Primitives: Introduces two new self-contained telemetry primitives:
HighWaterMarkTrackerandRateLimiter, which are foundational for upcoming performance monitoring work. - HighWaterMarkTracker: A new utility designed to reduce noise in memory metric recording by only triggering when memory usage (e.g., heap/RSS) increases by a significant, configurable threshold.
- RateLimiter: A new utility to throttle telemetry metric recording, ensuring that metrics are not sent too frequently, and includes an optional high-priority path for critical events.
- Comprehensive Testing: Both
HighWaterMarkTrackerandRateLimitercome with comprehensive unit tests written using Vitest, ensuring their reliability and correct behavior. - API Exposure: The newly added
HighWaterMarkTrackerandRateLimiterAPIs are exposed via the main telemetry index for external use.
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. ↩
a963f0a to
fab793f
Compare
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 code is well-structured and clear. My review focuses on improving robustness and correctness by adding input validation to prevent unexpected behavior from invalid configurations, addressing a potential memory leak in HighWaterMarkTracker, and fixing a bug in RateLimiter's time calculation logic.
There was a problem hiding this comment.
The constructor for RateLimiter does not validate the minIntervalMs parameter. If a negative value is provided, the rate limiting logic will be ineffective as now - lastRecordTime >= interval will almost always be true. To ensure the rate limiter behaves correctly, you should add a check to ensure minIntervalMs is non-negative.
constructor(minIntervalMs: number = 60000) {
if (minIntervalMs < 0) {
throw new Error('minIntervalMs must be non-negative.');
}
// Default: 1 minute
this.minIntervalMs = minIntervalMs;
}There was a problem hiding this comment.
The getTimeUntilNextAllowed method does not account for the isHighPriority flag that shouldRecord uses. It always calculates the remaining time based on the full minIntervalMs, even if a high-priority event could be recorded sooner (after minIntervalMs / 2). This can provide misleading information to consumers of this method. To fix this, the method should accept an optional isHighPriority parameter and use it to calculate the correct interval.
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);
}5d648d6 to
8bbbe5e
Compare
Head branch was pushed to by a user without write access
8bbbe5e to
0cf5560
Compare
…revent negative thresholds from causing incorrect high-water recordings and mitigate potential memory growth when metric types are unbounded. Adds explicit cleanup API and aligns resets to clear all state. Includes tests to enforce behavior.
…ng in RateLimiter\n\nWHY: Negative intervals would break rate limiting; add validation. Expose accurate remaining time for high-priority events by parameterizing and rounding the halved interval, and centralize the divisor. Remove redundant comments. Tests updated to lock behavior.
0cf5560 to
ffe826d
Compare
|
Hey @jacob314 , addressed all points:
On cleanup: if metric keys are bounded (heap_used/rss/etc.), opt‑in cleanup is sufficient. If keys can grow, I recommend host‑scheduled cleanup (e.g., every 5m, prune >1h idle). I can add a small startAutoCleanup/stopAutoCleanup helper if you prefer. |

Summary
Adds foundational telemetry utilities required for enhanced performance monitoring: a RateLimiter to prevent telemetry spam with high-priority bypass, and a HighWaterMarkTracker to reduce noise by only recording significant changes.
Dependencies
Review Order: This is Part 1/6 - review this PR first before the dependent parts.
Related PRs - Enhanced Performance Monitoring Split
This is Part 1 of 6 in the split implementation of #2127:
Changes in This PR
RateLimiter (
packages/core/src/telemetry/rate-limiter.ts)HighWaterMarkTracker (
packages/core/src/telemetry/high-water-mark-tracker.ts)Exports (
packages/core/src/telemetry/index.ts)Testing
✅ Comprehensive unit tests using vitest:
rate-limiter.test.ts- 22 test cases covering rate limiting, high-priority bypass, edge caseshigh-water-mark-tracker.test.ts- 17 test cases covering threshold tracking, configuration scenarios✅ Build verification:
npm run preflight(lint, typecheck, test, build)Technical Notes
Benefits
Next Steps: After this PR merges, Parts 2-6 will build upon these primitives to create comprehensive performance monitoring capabilities.