Conversation
…currency Each scrape fetches time series for every metric descriptor of every configured project concurrently, with no limit. When google.projects.filter (or a long google.project-ids list) resolves to many projects, this can spawn far more concurrent Monitoring API requests/JSON decodes than a memory-constrained pod can handle, leading to OOM kills. Add monitoring.max-concurrency (default 0, unbounded) which caps concurrent TimeSeries.List requests via a single semaphore shared across all projects, so the limit holds regardless of how many projects are resolved. Signed-off-by: Pavan Nalam <pavan.nalam3693@gmail.com>
Move the concurrency test into monitoring_collector_test.go (the corresponding _test.go file for the changed code), extract semaphore construction into a small newRequestLimiter helper with its own table-driven test, and add direct unit tests for acquire/releaseRequestLimiter covering the nil (unbounded) and blocking-when-full cases. Also assert Config.MaxConcurrentRequests defaults correctly in TestNewConfigWithDefaults. Signed-off-by: Pavan Nalam <pavan.nalam3693@gmail.com>
…ription Signed-off-by: Pavan Nalam <pavan.nalam3693@gmail.com>
ntmspavan
changed the title
ntmspavan
changed the title
The manual monitoring.max-concurrency flag required an operator to guess a concurrency number correlated only loosely with actual memory usage, and did nothing by default (unbounded) unless explicitly set. Replace it with two fixes that require no configuration: - Cap concurrent Monitoring API TimeSeries.List requests process-wide at a fixed internal limit (maxConcurrentTimeSeriesRequests), so a scrape can never hold more than that many decoded API responses in memory at once, regardless of how many projects or metric descriptors it fans out across. - Read the container's cgroup memory limit at startup and set Go's GOMEMLIMIT to 90% of it via automemlimit, so the garbage collector reclaims more aggressively as usage approaches the container's actual resource limit. Signed-off-by: Pavan Nalam <pavan.nalam3693@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #537. Replaces the earlier
monitoring.max-concurrencyflag (from ba53328/6e46846/4e1593c on this branch) with two fixes that require no configuration, since a manual flag defaulting to "unbounded unless you know to set it" doesn't actually solve "respect the pod's resource limits by default."Problem
reportMonitoringMetricsspawns one goroutine per metric descriptor per project, with no cap on fan-out. Whengoogle.projects.filter(or a longgoogle.project-idslist) resolves to many projects, or a project has many metric descriptors, the number of concurrently in-flightTimeSeries.Listrequests -- each holding a fully decoded JSON response in memory -- is unbounded. In a memory-constrained pod this can spike the heap past the container's memory limit and get it OOM-killed.Fix
Bound the actual fan-out.
TimeSeries.Listrequests are now capped process-wide at a fixed internal limit (maxConcurrentTimeSeriesRequests = 20incollectors/monitoring_collector.go), shared across every project's collector. This directly caps the peak live memory a scrape can hold, regardless of how many projects or descriptors are involved. This is a hardcoded internal constant, not a flag -- it's a bug fix to unbounded behavior, not a new tunable.Make the GC aware of the container's actual limit. At startup, stackdriver_exporter now reads the container's cgroup memory limit and sets Go's
GOMEMLIMITto 90% of it via automemlimit (https://github.com/KimMachineGun/automemlimit). This is a defense-in-depth backstop: the GC reclaims more aggressively as usage approaches the real limit. It activates automatically wheneverresources.limits.memoryis set on the container; it's a no-op otherwise (unlimited pod, bare VM with no cgroup cap, non-Linux). Can be overridden with theGOMEMLIMIT/AUTOMEMLIMITenv vars.Neither of these reads or depends on
resources.limits.cpu/GOMAXPROCS-- that's a separate, unaddressed concern (tracked as a possible future automaxprocs addition, not part of this PR).Testing
go build ./...,go vet ./...,gofmt -l .all clean.go test -race ./...passes.TestTimeSeriesRequestLimiterBoundsConcurrency, which fails if concurrency ever exceeds the limit or never reaches it (so it can't pass without exercising real contention).