chore: test to prod#204
Conversation
…200) * fix: resolve external IPs to FQDNs in metric labels using DNS cache DestinationKey is set at TCP connection time, but DNS may not be cached yet due to per-CPU perf buffer race conditions. This causes all metric labels (destination, destination_workload_name) to show raw IPs instead of FQDNs for external services. Add enrichDestinationKey() that checks the DNS cache at metric emission time and substitutes the FQDN when available. Applied to all TCP metrics (bytes, connections, retransmits) and all L7 protocol metrics (HTTP, Postgres, Redis, etc). * fix: aggregate connection stats by enriched key to prevent duplicate metrics Multiple IPs resolving to the same FQDN (e.g., Google's shared IPs for monitoring.googleapis.com) caused duplicate metric errors when enriched to the same label set. Aggregate stats by enriched DestinationKey before emitting TCP metrics. * fix: migrate connection DestinationKey when DNS becomes available Root cause fix for duplicate metric errors. When a TCP connection opens before DNS is cached (per-CPU perf buffer race), the DestinationKey stores the raw IP. Later connections get an FQDN-based key. Both enrich to the same FQDN at emit time, causing "collected before with same label values" errors. migrateConnectionKeyIfNeeded updates conn.DestinationKey in-place and migrates connectionStats from the old IP-based key to the FQDN-based key. Called from updateConnectionTrafficStats and onL7RequestWithResult. Collect() aggregation kept as safety net for stale entries. * fix: use minimal structs and O(1) pod lookup in ip_resolver Replace full K8s API objects with minimal structs (MinimalOwnerInfo, MinimalService, MinimalNode) to reduce memory ~10x per resource. Add PodNameIndex for O(1) ResolvePodOwner instead of O(pods) scan. Deduplicate service resolution into resolveServiceWorkload(). Simplify getControllerOfOwner from 7 switch cases to unified lookup. * fix: eliminate duplicate HTTP parsing and remove dead code Remove 3 unused functions (ParseHTTPResponse, ParseHttpResponse, ParseHostFromHttpRequest). Fix parseRequest() to call ParseHTTPRequest once instead of both ParseHttp + ParseHTTPRequest. Pass pre-parsed method/path to observe() instead of re-parsing payload a third time. Fix pre-existing RawPath bug in ParseHTTPRequest URL construction. * fix: race conditions, missing informer, stale IP cleanup, and eBPF bounds checks - Fix concurrent map access in getMounts/getListens/ping (copy c.processes under lock) - Fix conn.Closed written outside lock in onConnectionClose - Add missing CronJob informer handler for owner chain resolution - Fix InstanceMeta missing Instance field in initial snapshot - Fix storeWorkloadsIP check-then-act race with mutex - Clean stale ClusterIPs on Service update and old pod IPs on Pod update - Add bounds checks in memcached/redis eBPF parsers to prevent OOB reads - Remove misleading defer req.Body.Close() in ParseHTTPRequest * fix: correct ssl_st struct offsets for OpenSSL 3.x TLS capture In OpenSSL 3.x, the SSL struct was split into ssl_st (base) and ssl_connection_st (connection data). rbio/wbio moved from offset 16/24 to 80/88. The eBPF code was reading garbage pointers for the BIO, causing FD extraction to fail (CONN_NOT_FOUND with invalid FDs like 2868022864). HTTP payload was captured correctly since it comes from function parameters, but the connection couldn't be matched. Adds GET_FD_V3 macro that reads rbio/wbio at the correct offsets for OpenSSL 3.x (verified against OpenSSL 3.5.4 with a test program). * fix: prefer system libssl over psycopg2 bundled lib for TLS uprobes Processes like the notification server load both psycopg2's bundled libssl-*.so and the system /usr/lib/libssl.so.3. The agent was picking the first match from /proc/pid/maps (psycopg2's), but Python's ssl module uses the system lib for HTTPS. This caused SSL uprobes to fire on the wrong library, missing Slack/Teams API calls. * fix: reduce log noise by raising debug log levels Per-event hot-path logs (L7_EVENT_REGISTRY, CONTAINER_FOUND, TIMESTAMP_MISMATCH) were at V(2), producing ~15K lines/min with KLOG_V=2. Raised to V(5). Per-request logs (HTTP2_COMPLETED, LLM_TRACK) raised to V(4). Occasional logs raised to V(3). TLS per-process debug logs moved from unconditional Infof to V(2)/V(3). * fix: align L7 metric labels with TCP and remove dead connection fields - Rename L7 labels to match TCP: destination_region → destination_workload_region, destination_az → destination_workload_az, destination_instance → actual_destination_instance - Add missing actual_destination_region and actual_destination_az labels to L7 metrics - Fix L7 destination_workload_region/az to use destinationWorkload (was using actualDestWorkload) - Remove dead fields from ConnectionKey (srcWorkload, dstWorkload, actualDestWorkload — never set) - Remove dead fields from ActiveConnection (dstWorkload, actualDestWorkload — set but never read) - Remove redundant DNS re-enrichment in trace creation (migrateConnectionKeyIfNeeded already handles it) * style: fix gofmt formatting in container.go and http.go
#203) * 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.
chore: main to test
Summary of ChangesHello @mayankpande88, 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 refactors and enhances the agent's ability to collect and process metrics and traces, particularly focusing on Kubernetes and systemd environments, network connection enrichment, and improved eBPF uprobe attachment. It introduces new data structures for more efficient Kubernetes resource handling, refines systemd service detection, and improves the robustness of L7 tracing by addressing race conditions in DNS resolution and connection tracking. Additionally, it updates the build process, adds new configuration flags, and includes a new utility for ELF symbol parsing. 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
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 introduces a significant number of improvements and refactorings across the agent. Key changes include a major optimization of the Kubernetes IP resolver to reduce memory usage, fixes for several race conditions, and a solution for a metric duplication issue related to DNS resolution. The TLS uprobe attachment logic has been nicely refactored into a reusable ELF utility, and support for OpenSSL 3.x has been added. I've found one area for improvement in the Dockerfile to reduce the final image size. Overall, these are excellent changes that enhance the agent's performance, stability, and maintainability.
| @@ -114,7 +115,7 @@ func NewL7Stats() L7Stats { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Changing the receiver for observe and other L7Stats methods from a value receiver (s L7Stats) to a pointer receiver (s *L7Stats) is a critical fix. With a value receiver, each call would operate on a copy of the struct, meaning the internal maps and mutex would not be shared, and state changes would be lost. This change ensures that all operations correctly modify the single shared L7Stats instance.
| } | ||
| if len(body) > 0 || body != nil { | ||
| p := body | ||
| req.Body = io.NopCloser(bytes.NewReader(p)) | ||
| defer req.Body.Close() | ||
| if len(body) > 0 { | ||
| req.Body = io.NopCloser(bytes.NewReader(body)) | ||
| } |
There was a problem hiding this comment.
The removal of defer req.Body.Close() is a good fix. In the previous implementation, the body was closed immediately after being set, which would cause issues for any code attempting to read it later. The new implementation correctly leaves the responsibility of closing the body to the consumer of the http.Request object.
| func (resolver *K8sIPResolver) ResolvePodOwner(podName string, podNamespace string) Workload { | ||
| // This function is now more complex because we don't have the full pod object. | ||
| // We need to find the pod in our minimal cache. | ||
| var targetPod *MinimalPod | ||
| resolver.snapshot.Pods.Range(func(key, value any) bool { | ||
| pod, ok := value.(MinimalPod) | ||
| if !ok { | ||
| return true // continue | ||
| } | ||
| if pod.Name == podName && pod.Namespace == podNamespace { | ||
| targetPod = &pod | ||
| return false // stop | ||
| if uidVal, ok := resolver.snapshot.PodNameIndex.Load(podNamespace + "/" + podName); ok { | ||
| if podVal, ok := resolver.snapshot.Pods.Load(uidVal.(types.UID)); ok { | ||
| pod := podVal.(MinimalPod) | ||
| return resolver.resolvePodDescriptor(&pod) | ||
| } | ||
| return true | ||
| }) | ||
|
|
||
| if targetPod != nil { | ||
| return resolver.resolvePodDescriptor(targetPod) | ||
| } |
There was a problem hiding this comment.
Using a PodNameIndex map for a direct lookup of pods by namespace/name is an excellent performance optimization. This avoids iterating over the entire list of pods, which could be a significant bottleneck in large clusters.
| RUN apt-get update && apt-get install -y \ | ||
| curl git build-essential pkg-config libsystemd-dev |
There was a problem hiding this comment.
To reduce the Docker image size, it's a good practice to clean up the apt cache after installing packages. You can do this by adding && rm -rf /var/lib/apt/lists/* to the end of this RUN command.
RUN apt-get update && apt-get install -y \
curl git build-essential pkg-config libsystemd-dev && \
rm -rf /var/lib/apt/lists/*
| if len(p.uprobes) > 0 { | ||
| uprobes := p.uprobes | ||
| p.uprobes = nil | ||
| go func() { | ||
| for _, u := range uprobes { | ||
| _ = u.Close() | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
Detaching the uprobes in a separate goroutine is a great improvement for robustness. This prevents the Close() method from blocking if closing an uprobe takes a long time, and setting p.uprobes to nil immediately helps prevent race conditions if Close() were to be called concurrently.
| c.lock.Lock() | ||
| conn := c.connectionsByPidFd[PidFd{Pid: e.Pid, Fd: e.Fd}] | ||
| c.lock.Unlock() | ||
| if conn != nil { | ||
| if conn.Timestamp != 0 && conn.Timestamp != e.Timestamp { | ||
| return | ||
| } | ||
| if conn.Closed.IsZero() { | ||
| if e.TrafficStats != nil { | ||
| c.lock.Lock() | ||
| c.updateConnectionTrafficStats(conn, e.TrafficStats.BytesSent, e.TrafficStats.BytesReceived) | ||
| c.lock.Unlock() | ||
| } | ||
| conn.Closed = time.Now() | ||
| if conn == nil { | ||
| c.lock.Unlock() | ||
| return | ||
| } | ||
| if conn.Timestamp != 0 && conn.Timestamp != e.Timestamp { | ||
| c.lock.Unlock() | ||
| return | ||
| } | ||
| if conn.Closed.IsZero() { | ||
| if e.TrafficStats != nil { | ||
| c.updateConnectionTrafficStats(conn, e.TrafficStats.BytesSent, e.TrafficStats.BytesReceived) | ||
| } | ||
| conn.Closed = time.Now() | ||
| } | ||
| c.lock.Unlock() |
There was a problem hiding this comment.
Refactoring the onConnectionClose logic to use a single lock at the beginning of the function and early returns is a good improvement. It makes the code cleaner and safer by avoiding nested locks and ensuring the lock is held for the entire duration of the critical section.
| if !os.IsNotExist(err) { | ||
| klog.Warningf("failed to read link '%s': %s", entry.Name(), err) | ||
| } |
There was a problem hiding this comment.
The change from if os.IsNotExist(err) to if !os.IsNotExist(err) is a good bug fix. The previous logic would log a warning if the file did not exist, which is a normal condition for a process that has exited. The new logic correctly logs warnings only for unexpected errors, reducing log noise.
3 participants
No description provided.