fix: reduce memory footprint by removing unused labels and shrinking eBPF maps#217
Merged
Conversation
Summary of ChangesHello, 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 significantly reduces the application's memory footprint, targeting both Go heap and kernel memory. By removing several unused metric labels that can be derived server-side and by shrinking the sizes of various eBPF maps and an internal connection statistics cache, the changes aim to prevent OOMKills in pods and improve overall resource efficiency. Highlights
🧠 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
Activity
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to reduce memory usage by removing unused metric labels and shrinking eBPF map sizes. While the changes are generally well-aligned with this goal, a critical vulnerability was introduced: a mismatch between the eBPF C code and the Go agent regarding the MAX_PAYLOAD_SIZE constant. This breaks L7 response monitoring functionality, as the Go agent will look for response data at the wrong memory offset. It is crucial to ensure that constants shared between eBPF and Go are kept in sync. Additionally, a minor refactoring is suggested in containers/container.go to improve code maintainability by reducing duplicated label arguments.
…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
mayankpande88
force-pushed
the
fix/reduce-memory-footprint
branch
from
9229da2 to
4d11a47
Compare
Without this, the Go agent reads L7 response data at the wrong offset, breaking all L7 protocol monitoring.
blue4209211
approved these changes
Mar 7, 2026
3 tasks
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>
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
2 participants
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
src_region,src_az,destination_workload_region,destination_workload_az,actual_destination_region,actual_destination_az,actual_destination_instance. Also removeddestination_instancefromNetLatency. These are derivable server-side viacontainer_infojoins (which already hasregionandazin VictoriaMetrics).MAX_CONNECTIONS: 1,000,000 → 100,000 (~100MB saved)MAX_PAYLOAD_SIZE: 8192 → 4096active_l7_requestsmax_entries: 8192 → 4096connectionStatsCacheSizefrom 4096 to 2048Pods are currently OOMKilled (exit code 137) at the 1Gi limit, with usage ranging 443Mi–749Mi and climbing. These changes target both Go heap (fewer labels = fewer allocations per scrape) and kernel memory (smaller eBPF maps).
Test plan
container_infojoin for region/az still works in VictoriaMetricsbpftool map show)