Skip to content

fix: eliminate scrape deadlock, Prometheus wrapping allocs, and K8s informer memory#219

Merged
mayankpande88 merged 6 commits into
mainfrom
fix/eliminate-wrapping-metric-allocs
Mar 8, 2026
Merged

fix: eliminate scrape deadlock, Prometheus wrapping allocs, and K8s informer memory#219
mayankpande88 merged 6 commits into
mainfrom
fix/eliminate-wrapping-metric-allocs

Conversation

@mayankpande88

@mayankpande88 mayankpande88 commented Mar 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes OOMKills, deadlocked scrapes, and HTTP 500 metric errors on nodes with many containers.

1. Push-model metrics to eliminate c.lock deadlock

Collect() held c.lock.RLock() while reading connectionStats, failedConnectionAttempts, and logSamples. Event handlers 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), creating a circular wait.

Fixed by moving TCP/connection metrics to pre-registered CounterVec/GaugeVec updated by event handlers at event time. Collect() now calls tcpMetrics.collect(ch) without any c.lock.

2. Single containerCollector instead of per-container registration

prometheus.Registry.Unregister() does not work for unchecked collectors (empty Describe()). It only searches collectorsByID, never the uncheckedCollectors slice. Every container replacement (process restart within same pod) leaked the old Container in the registry — Gather() collected duplicate metrics and returned HTTP 500.

Fixed by registering a single containerCollector on the raw registry that iterates containersById under RLock. Individual container Register/Unregister calls removed.

3. Eliminate WrapRegistererWith allocation storm

Container metrics no longer use 3-layer WrapRegistererWith wrapping. Const labels are baked into each container's metric descriptors directly, avoiding ~200MB of transient wrappingMetric allocations per scrape on busy nodes.

4. K8s informer cache stripping

SetTransform functions on all 9 informers strip unneeded fields (ManagedFields, Annotations, Spec.Containers, etc.) before objects enter the cache (~80% reduction). Removed redundant getFullClusterSnapshot() startup API calls.

Key changes

  • containers/tcp_metrics.go — New push-model TCPMetrics struct (CounterVec/GaugeVec updated by event handlers)
  • containers/container.go — Removed connectionStats LRU, failedConnectionAttempts map, restarts/oomKills fields. Collect() no longer holds c.lock for TCP metrics. logSamples changed to sync.Map.
  • containers/registry.go — Single containerCollector replaces per-container registration. Removed trafficStatsUpdateCh/nodejsStatsUpdateCh/pythonStatsUpdateCh channels. eBPF stats update moved to event handler ticker. NodeConstLabels struct.
  • containers/metrics.go — constLabelNames, helper constructors for CounterVec/GaugeVec/Counter
  • containers/l7.go — L7Stats uses promConstLabels for CounterVec/HistogramVec const labels
  • common/ip_resolver.go — Informer transform functions, removed getFullClusterSnapshot()
  • main.go — Passes raw registry alongside wrapped registerer

Test plan

  • Deploy to dev cluster and verify metrics emitted with correct labels
  • Verify no deadlocked scrape goroutines (blocked=0 on all 15 pods)
  • Verify no HTTP 500 duplicate metric errors (was affecting 4 pods, now 0)
  • Victoria Metrics agent scraping all pods successfully (zero errors in vmagent logs)
  • Monitor memory usage over 24h — should stay stable instead of growing

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>
@gemini-code-assist

Copy link
Copy Markdown

Summary of Changes

Hello, 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 focuses on a critical performance improvement by refactoring how Prometheus metrics are handled for containers. The core objective is to drastically reduce transient memory allocations, particularly the substantial ~200MB per scrape observed on high-load systems, by eliminating inefficient WrapRegistererWith operations. This is achieved through a more direct approach to embedding constant labels into metrics and optimizing the initialization of L7 metric vectors, resulting in a significantly more efficient and scalable metrics collection system.

Highlights

  • Performance Optimization: Eliminated a 3-layer WrapRegistererWith chain for container metrics, significantly reducing transient memory allocations (estimated ~200MB per scrape on busy nodes) during Prometheus scrapes.
  • Direct Label Embedding: Implemented direct embedding of constant labels (container_id, app_id, machine_id, system_uuid, az, region) into container metrics via new Container.counter() and Container.gauge() methods, removing wrapping overhead.
  • L7 Metrics Optimization: Configured L7 CounterVec and HistogramVec to use ConstLabels in their options, ensuring zero per-scrape allocation for these metrics.
  • Unchecked Collectors: Registered containers as unchecked collectors in Prometheus, managing label uniqueness directly within the metrics rather than relying on Prometheus descriptors.
  • Breaking Change: Introduced a breaking change where non-cloud nodes will now include az="" and region="" labels for container metrics, which were previously absent.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • containers/container.go
    • Added a constLabels field to the Container struct to store constant labels.
    • Initialized constLabels and promConstLabels during the NewContainer creation process.
    • Modified the NewL7Stats call to pass promConstLabels for L7 metric initialization.
    • Updated the Describe method to indicate that containers are now unchecked collectors, sending no descriptors.
    • Replaced all direct calls to global gauge and counter functions with new c.gauge and c.counter methods, which automatically prepend constant labels.
    • Changed the call to jvmMetrics to use the new c.jvmMetrics method.
  • containers/jvm.go
    • Converted the jvmMetrics function into a method of the Container struct (c.jvmMetrics).
    • Updated calls to gauge and counter within jvmMetrics to use the c.gauge and c.counter methods.
  • containers/l7.go
    • Added a promConstLabels field to the L7Stats struct to hold Prometheus constant labels.
    • Modified the NewL7Stats constructor to accept prometheus.Labels for initialization.
    • Included s.promConstLabels in prometheus.CounterOpts and prometheus.HistogramOpts when creating CounterVec and HistogramVec.
  • containers/metrics.go
    • Introduced a global constLabelNames slice defining common constant label names for container metrics.
    • Modified the metric helper function to prepend constLabelNames to all metric descriptors it creates.
    • Updated the Ip2Fqdn metric definition to explicitly use prometheus.NewDesc without constLabelNames, as its labels are handled differently.
  • containers/registry.go
    • Added rawReg (a raw Prometheus Registerer) and nodeConstLabels fields to the Registry struct.
    • Updated the NewRegistry function signature to accept rawReg and node-specific constant labels (machineId, systemUuid, az, region).
    • Initialized rawReg and nodeConstLabels within the Registry constructor.
    • Modified the Collect method for Ip2Fqdn to use prometheus.MustNewConstMetric directly for metric creation.
    • Changed container Unregister and Register calls to use r.rawReg instead of prometheus.WrapRegistererWith.
  • main.go
    • Declared and initialized az and region variables from node metadata.
    • Updated the call to containers.NewRegistry to pass both the wrapped registerer (for specific metrics like ip2fqdn and LLM) and the registry (raw) along with machineId, systemUuid, az, and region.
Activity
  • No human activity (comments, reviews, or progress updates) has been recorded for this pull request yet.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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

  1. 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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively addresses the performance issue of high memory allocations during Prometheus scrapes by eliminating the WrapRegistererWith calls for container metrics. The approach of embedding constant labels directly into the metrics is well-implemented. For container-level metrics, this is done by creating new helper methods on the Container struct that prepend the constant labels. For L7 metrics, this is achieved by using the ConstLabels option in CounterVec and HistogramVec. The code is cleaner and more performant. I have one suggestion to improve the robustness of how node-level labels are handled to avoid potential issues with label ordering in the future.

Comment thread containers/container.go
mayankpande88 and others added 3 commits March 8, 2026 01:16
…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>
@mayankpande88 mayankpande88 changed the title fix: eliminate WrapRegistererWith allocation storm on Prometheus scrapes fix: eliminate Prometheus wrapping allocs and reduce K8s informer memory Mar 7, 2026
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>
@mayankpande88 mayankpande88 changed the title fix: eliminate Prometheus wrapping allocs and reduce K8s informer memory fix: eliminate scrape deadlock, Prometheus wrapping allocs, and K8s informer memory Mar 8, 2026
@mayankpande88

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

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.
@mayankpande88 mayankpande88 merged commit 7a23f42 into main Mar 8, 2026
2 checks passed
@mayankpande88 mayankpande88 deleted the fix/eliminate-wrapping-metric-allocs branch March 8, 2026 06:41
mayankpande88 added a commit that referenced this pull request Mar 8, 2026
* fix: reduce memory usage, fix race conditions, and prevent corrupted eBPF spans (#206)

* fix: add missing patternsPerLevelLimit param to logparser.NewParser calls

* fix: update logparser dep, sanitize log paths, replace test credential

* fix: update logparser dep to fix sensitive pattern loading

The previous version had a corrupted sensitive_patterns.json that caused
"cannot unmarshal object into Go value of type []SensitivePattern" errors,
breaking sensitive data detection in log parsing.

* fix: reduce memory usage, fix race conditions, and fd leaks

Memory optimizations:
- Auto-tune GOMEMLIMIT to 90% of cgroup memory limit
- Reduce connectionStats LRU cache from 8192 to 4096
- Reduce event channel buffer from 10000 to 2000
- Cap ip2fqdn map at 10000 entries
- Reduce label interner max from 10000 to 5000
- Reduce LLM stream limits (maxActiveStreams 1000->200, buffer 1MB->64KB)
- Cache stripped Go binary paths to avoid repeated uprobe attach attempts
- Reduce GC interval from 10min to 5min
- Clean up HTTP/2 HPACK parsers for dead connections in gc()

Race condition fixes in Container.Collect():
- Snapshot connectionStats LRU under RLock (not concurrent-safe)
- Call getListens/getProxiedListens under RLock
- Protect logSamples map reads/writes with lock
- Read restarts/oomKills under RLock
- Read c.processes under RLock in onConnectionOpen before lock scope

File descriptor leak fixes in logs/tail_reader.go:
- Close file on Stat/Seek failure in NewTailReader
- Close old file before setting nil in moved()
- Cap partial line buffer at 64KB to prevent unbounded growth
- Fix prefix accumulation bug (was replacing instead of appending)

* fix: validate eBPF durations to prevent corrupted spans causing OTel OOM

eBPF kernel probes occasionally produce garbage duration values where the
uint64 has the high bit set. Casting to time.Duration (int64) produces
negative durations, creating spans where end_time < start_time. These
poison spans cause ClickHouse batch rejections, OTel collector retry
accumulation, and eventual OOMKill (165 restarts observed).

Source-level fix (eBPF event parsing):
- Add safeDuration() that returns 0 for values >= 1 hour or == 0
- Replace all uint64->time.Duration casts in perf/ringbuf readers

HTTP/2 parser fix:
- Add safeKernelDuration() for kernel timestamp subtraction
- Prevents unsigned underflow when events arrive out of order

Defense-in-depth (span creation):
- createSpan: drop spans with duration <= 0 or > 1 hour
- LLMRequest: drop spans with zero/invalid timestamps or > 1 hour

* fix: address review comments

- Use double-checked locking for logSamples to avoid redundant writes
- Use strconv.ParseInt instead of fmt.Sscanf for stricter cgroup parsing

* fix: gofmt alignment in llm_stream.go

* fix: use debug.SetMemoryLimit instead of runtime.SetMemoryLimit

* chore(deps): bump go.opentelemetry.io/otel/sdk from 1.37.0 to 1.40.0 (#205)

Bumps [go.opentelemetry.io/otel/sdk](https://github.com/open-telemetry/opentelemetry-go) from 1.37.0 to 1.40.0.
- [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases)
- [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md)
- [Commits](open-telemetry/opentelemetry-go@v1.37.0...v1.40.0)

---
updated-dependencies:
- dependency-name: go.opentelemetry.io/otel/sdk
  dependency-version: 1.40.0
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: resolve IP-based destinations to FQDN via HTTP headers (#215)

* fix: resolve IP-based destinations to FQDN via HTTP headers and eBPF dport extraction

Two complementary fixes for the ip_to_fqdn resolution gap where external
destinations show raw IPs instead of FQDNs in metrics:

1. Go: Use HTTP Host header and HTTP/2 :authority pseudo-header to resolve
   IP-based DestinationKeys to FQDNs when DNS capture fails (due to Go
   goroutine migration breaking pid-based eBPF tracking). Extracts
   migrateConnectionKeyToFQDN helper from migrateConnectionKeyIfNeeded.

2. eBPF: Extract destination port from connect() sockaddr arguments so
   non-TCP connections (UDP DNS) get proper dport in active_connections.
   Previously sys_exit_connect created entries with dport=0 for UDP,
   preventing DNS protocol detection for port 53.

* fix: address review feedback - return ip2fqdn for HTTP/2 and add early return guard

* fix: reduce memory footprint by removing unused labels and shrinking eBPF maps (#217)

* fix: reduce memory footprint by removing unused labels and shrinking eBPF maps

- Remove 7 unused labels from TCP/L7 metrics (src_region, src_az,
  destination_workload_region, destination_workload_az,
  actual_destination_region, actual_destination_az,
  actual_destination_instance). These are derivable server-side via
  container_info joins.
- Reduce MAX_CONNECTIONS from 1M to 100K (saves ~100MB kernel memory)
- Reduce MAX_PAYLOAD_SIZE from 8192 to 4096
- Reduce active_l7_requests max_entries from 8192 to 4096
- Reduce connectionStatsCacheSize from 4096 to 2048

* fix: sync MaxPayloadSize Go constant with eBPF MAX_PAYLOAD_SIZE (4096)

Without this, the Go agent reads L7 response data at the wrong offset,
breaking all L7 protocol monitoring.

* fix: prevent OOMKills by lowering GOMEMLIMIT and reducing memory allocations (#218)

- Lower GOMEMLIMIT from 90% to 60% of cgroup limit. The cgroup OOM
  killer counts kernel memory (~80MB for eBPF maps) and page cache
  (~200MB from /proc reads) which GOMEMLIMIT doesn't control.
- Fix Event struct Payload/Response arrays: use MaxPayloadSize (4096)
  instead of hardcoded 8192. Saves ~16MB in the 2000-entry event
  channel buffer.
- Reduce l7_events ring buffer from 16MB to 8MB. With 4KB payloads,
  8MB still holds ~2000 events.

* fix: detect Go applications on listen events for container_application_type (#216)

attachTlsUprobes (which detects Go binaries via buildinfo) was only
called on outbound connection events. Go servers that only listen for
incoming connections were never detected as golang apps. Add the same
call on ListenOpen events so server-only Go processes are properly
identified.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* fix: eliminate scrape deadlock, Prometheus wrapping allocs, and K8s informer memory (#219)

* fix: eliminate WrapRegistererWith allocation storm on Prometheus scrapes

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>

* fix: use NodeConstLabels struct instead of magic indices, fix variable 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>

* fix: reduce K8s informer memory by stripping cached objects and removing 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>

* fix: push-model metrics to eliminate c.lock deadlock on scrapes

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>

* fix: use single containerCollector instead of per-container registration

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.

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Raman Kumar <raman.kharche@nudgebee.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants