chore: upstream rebase #153
Conversation
* update version Signed-off-by: Vanshikav123 <vanshikav928@gmail.com> * addressing review Signed-off-by: Vanshikav123 <vanshikav928@gmail.com> * addressing review Signed-off-by: Vanshikav123 <vanshikav928@gmail.com> --------- Signed-off-by: Vanshikav123 <vanshikav928@gmail.com>
…iqueness metrics: enhance the `instance` label uniqueness to avoid collisions
containers: add support for ZFS filesystem
add FoundationDB application type
add support for Oracle Cloud metadata
add support for disabling log monitoring via container ENV variable
cgroup v1: use hierarchical total_* counters for accurate RSS/memory usage
FoundationDB support
eBPF traces improvements
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @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 is a rebase that brings several significant updates and improvements across various components. It focuses on updating core system requirements, expanding support for new technologies like FoundationDB and Oracle Cloud, and enhancing the accuracy and configurability of existing monitoring capabilities. The changes aim to improve compatibility, provide more granular control over data collection, and ensure more precise metric reporting.
Highlights
- Kernel and Go Version Updates: The minimum required Linux kernel version has been updated from 4.16 to 5.1, and the Go version requirement remains at v1.23.
- FoundationDB Protocol Support: Added comprehensive support for the FoundationDB protocol, including eBPF-based request/response parsing, application type detection, and new Prometheus metrics for requests and latency.
- Improved Cgroup and Container Identification: Enhanced cgroup parsing logic to correctly identify containers using 'lxc.payload' paths and refined the general container identification process.
- Enhanced Disk and Network Device Recognition: Expanded the regex for block devices to include 'mmcblk' (eMMC/SD card devices) and updated network device filtering to recognize additional 'enP' interface naming conventions.
- Oracle Cloud Metadata Integration: Implemented detection and metadata collection for Oracle Cloud infrastructure, allowing for better visibility in Oracle Cloud environments.
- Process-Specific Feature Toggles: Introduced new process flags (COROOT_EBPF_TRACES and COROOT_LOG_MONITORING) to allow disabling eBPF tracing and log monitoring on a per-process basis via environment variables.
- Configurable Trace Sampling: Added a new 'traces-sampling' flag to control the rate at which OpenTelemetry traces are sampled, providing more flexibility for trace data volume.
- Prometheus Instance Label Refinement: Modified the generation of the Prometheus 'instance' label to incorporate the system UUID, ensuring more robust and unique instance identification.
- Memory Cgroup Metric Updates: Adjusted memory cgroup metric collection to use 'total_rss' and 'total_cache' fields for more accurate reporting.
- ZFS Mount Info Parsing: Updated mount information parsing to correctly handle ZFS filesystem types, improving volume detection for ZFS-based mounts.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request appears to be an upstream rebase, incorporating a significant number of changes. Key additions include support for FoundationDB tracing, Oracle Cloud metadata discovery, and various smaller enhancements like ZFS mount support and improved device detection. There are also several refactorings for better code structure and performance. My review focuses on potential issues in the new and modified code, including suggestions for improved robustness and performance.
| if len(parts) > 2 { | ||
| cgPath = "/" + pp[1] | ||
| } |
There was a problem hiding this comment.
The condition len(parts) > 2 is always true within this loop, as parts is the result of strings.SplitN(line, ":", 3). This could lead to a panic if cgPath is malformed (e.g., an empty string or just "/"), causing an out-of-bounds access on pp[1]. A more robust check would be on the length of pp, such as len(pp) > 1, to ensure there is at least one path component to access.
| if len(parts) > 2 { | |
| cgPath = "/" + pp[1] | |
| } | |
| if len(pp) > 1 { | |
| cgPath = "/" + pp[1] | |
| } |
| for _, p := range c.processes { | ||
| if p.Flags.EbpfTracesDisabled { | ||
| ebpfTracesDisabled = true | ||
| break | ||
| } |
There was a problem hiding this comment.
This loop iterates over all processes in the container for every L7 request, which can be inefficient if a container has many processes. A more performant approach would be to cache the ebpfTracesDisabled flag at the container level. You could update this cached flag whenever a process is added or removed, thus avoiding the loop on this hot path.
| for _, p := range c.processes { | ||
| if p.Flags.LogMonitoringDisabled { | ||
| klog.InfoS("skipping log monitoring due to COROOT_LOG_MONITORING=disabled", "cg", c.cgroup.Id) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop iterates over all processes in the container every time this function is called, which can be inefficient. A more performant approach would be to cache the LogMonitoringDisabled flag at the container level. You could update this cached flag whenever a process is added or removed, thus avoiding this loop.
| // All FoundationDB replies use composed FileIdentifiers with ErrorOr wrapper (ID 2) | ||
| // Format: (2 << 24) | base_file_identifier | ||
| // Extract the base FileIdentifier from the lower 24 bits | ||
| __u32 base_id = file_id & 0x00FFFFFF; |
There was a problem hiding this comment.
The comment at line 228 mentions that FDB replies use a composed FileIdentifier with an ErrorOr wrapper (ID 2), implying the top 8 bits should be 0x02. However, the code only masks out the lower 24 bits for base_id and doesn't validate that the upper 8 bits are indeed 2. This could lead to misidentifying packets from other protocols as FDB replies if they happen to share a base_id. It would be more robust to add a check like if ((file_id >> 24) != 2) { return 0; } before the switch statement.
7 participants
No description provided.