Add bounded concurrency for timeSeries.list fan-out#5
Conversation
GCP metrics scrapes fanned out one unbounded goroutine per metric descriptor, each issuing a paginated projects.timeSeries.list call. On high-cardinality projects this bursts dozens of simultaneous calls, tripping GCP soft-throttling (slow empty 200s) and stalling scrapes to the 240s context deadline. Add TimeSeriesListConcurrency to MonitoringCollectorOptions and bound the inner descriptor fan-out with a semaphore. A value <= 0 means unbounded, preserving previous behavior. Pagination, ingest-delay, filtering, and error accumulation are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds configurable concurrency and rate limiting for ChangesMonitoringCollector fan-out controls
Repository ignore rule
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@collectors/monitoring_collector.go`:
- Around line 100-103: TimeSeriesListConcurrency is currently enforced per
metric-descriptor batch inside metricDescriptorsFunction, so concurrent prefix
goroutines can تجاوز the intended scrape-wide cap. Move the semaphore creation
and sharing into reportMonitoringMetrics, and pass the shared limiter down so
all projects.timeSeries.list calls across MetricTypePrefixes and paginated
batches use one scrape-level limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4634f5c9-c4c9-4d0c-b8a0-6bf9f2e5235b
📒 Files selected for processing (2)
collectors/monitoring_collector.gocollectors/monitoring_collector_concurrency_test.go
The semaphore was created inside metricDescriptorsFunction, which runs once per metric-type prefix, so each prefix got its own limiter of size N. With P prefixes fanning out concurrently the effective cap was N*P, not the intended scrape-wide N — undercutting the fix for the GCP soft-throttle stalls. Create the semaphore once in reportMonitoringMetrics and share it across all prefix goroutines and descriptor batches. Update the concurrency test to use multiple prefixes; it now fails on the old per-prefix limiter and passes on the shared one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A concurrency semaphore caps simultaneous calls but not the rate over a window. GCP soft-throttles on per-second admission (54 calls in ~284ms), so capping concurrency alone leaves the burst intact once calls return quickly. Add a token-bucket rate limiter (golang.org/x/time/rate) over the timeSeries.list fan-out, mirroring the CloudWatch/YACE GlobalRateLimiter pattern. Config is Count-per-Duration (burst = Count); both <= 0 disables it, preserving current behavior. The limiter is per-collector, i.e. per-project, and each timeSeries.list page call waits on it before firing. Concurrency and rate limits are independent knobs. Add a test asserting the scrape cannot complete faster than the token-bucket pacing allows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nir-sagi
force-pushed
the
nirsagi/be-2312-integrations-agent-gcp-metrics-scrape-fires-unbounded
branch
from
9cb28a5 to
227482a
Compare
The agent runs the GCP client with telemetry disabled, so rate-limit behavior
was only observable via the eBPF sensor. Expose it directly: split the gate
into Allow() (admitted immediately) vs Wait() (throttled) and count each with
stackdriver_monitoring_time_series_list_rate_{allowed,waited}_total, mirroring
the CloudWatch/YACE RateLimitAllowed/Wait counters.
Extend the rate test to assert the burst is counted as allowed and the
remainder as waited.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nir-sagi
force-pushed
the
nirsagi/be-2312-integrations-agent-gcp-metrics-scrape-fires-unbounded
branch
from
9180702 to
04f9f48
Compare
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
GCP metrics scrapes fanned out one unbounded goroutine per metric descriptor, each issuing a paginated projects.timeSeries.list call. On high-cardinality projects this bursts dozens of simultaneous calls, tripping GCP soft-throttling (slow empty 200s) and stalling scrapes to the 240s context deadline.
Add TimeSeriesListConcurrency to MonitoringCollectorOptions and bound the inner descriptor fan-out with a semaphore. A value <= 0 means unbounded, preserving previous behavior. Pagination, ingest-delay, filtering, and error accumulation are unchanged.
Summary by CodeRabbit
TimeSeriesListConcurrencyto cap concurrent monitoring time-series requests per scrape.TimeSeriesListRateCountandTimeSeriesListRateDuration.