fix: eliminate metrics allocation storm, deadlocks, and memory overhead#222
Closed
mayankpande88 wants to merge 6 commits into
Closed
fix: eliminate metrics allocation storm, deadlocks, and memory overhead#222mayankpande88 wants to merge 6 commits into
mayankpande88 wants to merge 6 commits into
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Removes the 3-layer WrapRegistererWith wrapping chain for container metrics that caused ~200MB of transient allocations per scrape on busy nodes. Container metrics now embed const labels directly, and L7 CounterVec/HistogramVec use ConstLabels on their opts for zero per-scrape overhead. ip2fqdn and LLM metrics retain minimal wrapping. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e shadowing Addresses PR review: replaces []string with named struct fields for node-level const labels. Also fixes variable shadowing where loop variable `c` in GetSensitiveCounters range shadowed the Container receiver, causing a compile error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ing redundant API calls - Add SetTransform functions to all 9 informers to strip unneeded fields (ManagedFields, Annotations, Spec.Containers, Status.Conditions, etc.) before objects enter the informer cache, reducing memory by ~80%. - Remove getFullClusterSnapshot() which did redundant List() API calls for all resource types at startup (informers already do their own initial List). Replace with factory.WaitForCacheSync() + updateIpMapping() to eliminate the double-fetch memory spike. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Root cause: Collect() held c.lock.RLock() for extended periods while reading connectionStats, failedConnectionAttempts, activeConnections, and logSamples. Event handlers (onConnectionOpen, onL7Request, etc.) need c.lock.Lock() — creating a deadlock when a pending writer blocks recursive readers. Additionally, Registry.Collect() sent to trafficStatsUpdateCh (consumed by the event handler goroutine), so if the event handler was blocked on c.lock.Lock(), Registry.Collect() blocked too → Gather() never returns → zombie goroutines accumulate. Changes: - New TCPMetrics struct (CounterVec/GaugeVec) updated by event handlers at event time, not scrape time. Collect() calls tcpMetrics.collect(ch) without any c.lock — same pattern as existing L7Stats. - Remove connectionStats LRU cache (no longer needed for accumulation) - Remove failedConnectionAttempts map (push directly to CounterVec) - onConnectionOpen/onRetransmission push metrics before acquiring c.lock - updateConnectionTrafficStats pushes byte deltas to TCPMetrics - Active connections gauge refreshed periodically by event handler - Move updateStatsFromEbpfMapsIfNecessary() from Registry.Collect() into event handler ticker — eliminates trafficStatsUpdateCh deadlock - Remove trafficStatsUpdateCh/nodejsStatsUpdateCh/pythonStatsUpdateCh channels — event handler calls container methods directly - logSamples changed from map[string]string to sync.Map for lock-free LoadOrStore (eliminates c.lock.Lock() in Collect for log samples) - Restarts/OOMKills pushed to TCPMetrics counter at event time Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
prometheus.Registry.Unregister does not work for unchecked collectors (empty Describe). It only searches collectorsByID, never the uncheckedCollectors slice. This meant every container replacement (process restart within same pod) leaked the old Container object in the registry — Gather() collected duplicate metrics and returned HTTP 500. Register a single containerCollector on the raw registry that iterates containersById under RLock. Individual container Register/Unregister calls removed entirely.
Picks up fix/json-log-pattern-hashing (PR #16) which stabilizes JSON log pattern hashing by extracting only message fields.
mayankpande88
force-pushed
the
fix/eliminate-wrapping-metric-allocs
branch
from
813d039 to
9ab002b
Compare
Contributor
Author
|
Closing: branch had stale commits already merged via #219. Replaced by new PR from clean branch. |
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.
Description
Eliminates several critical performance and correctness issues in the metrics collection pipeline that caused OOM kills, Prometheus scrape failures (HTTP 500), and excessive memory allocation.
Type of change
Changes
Push-model TCPMetrics (
ec67408)Moves TCP metric updates from scrape-time (inside
c.lock) to event-handler time, eliminating a deadlock whereCollect()heldc.lockwhile Prometheus held the registry lock, and vice versa.Single containerCollector (
0b01e86)Replaces per-container
prometheus.Register/Unregisterwith a single collector that iteratescontainersById. Fixes HTTP 500 from duplicate metric registration —prometheus.Registry.Unregister()silently fails for unchecked collectors (emptyDescribe), so re-registering a container with the same ID caused duplicates.WrapRegistererWith elimination (
841874d)Removes
WrapRegistererWithwhich allocated ~200MB of transient wrappers per Prometheus scrape by creating newwrappedMetricobjects for every metric on every collection.K8s informer memory reduction (
19606a3)Adds
SetTransformto strip unneeded fields (managed fields, annotations, last-applied-config) from cached K8s objects (~80% cache size reduction). Removes redundantgetFullClusterSnapshot()startup API calls.NodeConstLabels struct (
7bb8b43)Replaces magic slice indices for node constant labels with a named struct. Fixes a variable shadowing bug.
Logparser dependency update (
813d039)Updates
github.com/nudgebee/logparserto pick up stable JSON log pattern hashing (nudgebee/logparser#16).How Has This Been Tested?
nudgebee-agentnamespace) — verified no OOM kills, no HTTP 500 on scrapes