Add FreeBSD support #53
Conversation
Though FreeBSD does support procfs/linprocfs, these are not required to be mounted (and are commontly not), so use standard always available interfaces instead: - Take CPU time from getrusage(2) - Take max open files and max virtual memory size from getrlimit(2) - Take number of open files from /dev/fd (which is the same as /proc/self/fd on Linux) - Take remaining fields from kernel process structure retrieved from sysctl(3) Note that this code may likely be reused for other BSD systems (OpenBSD, NetBSD) with minimal changes.
WalkthroughThe changes introduce support for the FreeBSD operating system in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
Cargo.toml (1)
45-47: LGTM! Consider using a caret requirement for the libc dependency.The addition of the libc dependency for FreeBSD is correct and aligns well with the PR objectives. It's properly placed in the FreeBSD-specific target section and uses a recent version of the crate.
Consider using a caret requirement (
^) for the libc dependency to allow for compatible updates:[target.'cfg(target_os = "freebsd")'.dependencies] -libc = "0.2.159" +libc = "^0.2.159"This change would allow for minor and patch updates within the 0.2.x series, which can include bug fixes and performance improvements while maintaining compatibility.
src/collector.rs (2)
11-16: LGTM: FreeBSD added to supported platforms list.The update correctly includes FreeBSD in the list of supported platforms, ensuring that the compile error for unsupported platforms is not triggered for FreeBSD.
Consider alphabetizing the list of operating systems for better readability:
not(any( - target_os = "macos", - target_os = "linux", - target_os = "windows", - target_os = "freebsd" + target_os = "freebsd", + target_os = "linux", + target_os = "macos", + target_os = "windows" ))
61-66: LGTM: FreeBSD added to test configuration.The update correctly includes FreeBSD in the list of operating systems for which the
test_collect_internal_oktest should run, ensuring that the test will be compiled and executed on FreeBSD.Consider alphabetizing the list of operating systems for consistency with the previous suggestion:
#[cfg(any( - target_os = "macos", - target_os = "linux", - target_os = "windows", - target_os = "freebsd" + target_os = "freebsd", + target_os = "linux", + target_os = "macos", + target_os = "windows" ))]src/collector/freebsd.rs (2)
78-80: Nitpick: Rename variablelimit_astolimit_nofilefor clarityIn lines 78 to 80, the variable
limit_asis used to store the result ofgetrlimit(libc::RLIMIT_NOFILE). To enhance readability and accurately reflect the resource, consider renaminglimit_astolimit_nofile.Apply this diff to rename the variable:
-if let Some(limit_as) = getrlimit(libc::RLIMIT_NOFILE) { - metrics.max_fds = Some(translate_rlim(limit_as.rlim_cur)); +if let Some(limit_nofile) = getrlimit(libc::RLIMIT_NOFILE) { + metrics.max_fds = Some(translate_rlim(limit_nofile.rlim_cur)); }
94-94: Nitpick: Moveusestatement to the top-level scopeAt line 94, the
use std::convert::TryInto as _;statement is inside thecollectfunction. For consistency and to adhere to Rust conventions, consider moving thisusestatement to the top of the file.Apply this diff to relocate the
usestatement:+use std::convert::TryInto as _; use super::Metrics; fn getrusage(who: libc::c_int) -> Option<libc::rusage> { // existing code } - use std::convert::TryInto as _;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Cargo.toml (1 hunks)
- src/collector.rs (3 hunks)
- src/collector/freebsd.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
src/collector.rs (3)
4-4: LGTM: FreeBSD-specific conditional compilation attribute added correctly.The addition of the FreeBSD-specific conditional compilation attribute is consistent with the existing structure for other operating systems and correctly specifies the path to the FreeBSD implementation file.
87-87: LGTM: FreeBSD excluded from dummy test configuration.The addition of the FreeBSD-specific conditional compilation check correctly excludes FreeBSD from running the dummy test when the "dummy" feature is enabled. This is consistent with the existing structure for other operating systems.
Line range hint
1-103: Overall: FreeBSD support successfully implemented.The changes to
src/collector.rsconsistently and correctly implement FreeBSD support across the file. The conditional compilation attributes, unsupported platform checks, and test configurations have been updated appropriately.To ensure the FreeBSD-specific implementation is complete, please run the following verification script:
This script will help confirm that the FreeBSD-specific implementation file exists and contains the necessary
collect()function.✅ Verification successful
Line range hint
1-103: Overall: FreeBSD support successfully implemented.The verification confirms that
src/collector/freebsd.rsexists and contains the necessarycollect()function. FreeBSD support insrc/collector.rshas been correctly integrated and is functioning as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify FreeBSD-specific implementation file exists and contains necessary functions. # Test 1: Check if FreeBSD-specific implementation file exists fd -t f "freebsd.rs" src/collector # Test 2: Verify that the FreeBSD implementation file contains necessary functions rg -t rust "fn collect\(\)" src/collector/freebsd.rsLength of output: 144
src/collector/freebsd.rs (6)
25-31: Approved:translate_rlimfunction logic is correctThe
translate_rlimfunction correctly translatesrlim_tvalues, handlingRLIM_INFINITYappropriately.
3-12: Approved:getrusagefunction implementation is safe and correctThe use of
MaybeUninitandunsafeblocks ingetrusageis appropriate and handles the initialization safely.
14-23: Approved:getrlimitfunction implementation is safe and correctThe
getrlimitfunction correctly retrieves resource limits using safe patterns forunsafecode.
33-62: Approved:kinfo_getprocfunction correctly retrieves process informationThe implementation of
kinfo_getprocis sound, and references to external resources are helpful for future maintenance.
64-112: Approved:collectfunction accurately gathers system metricsThe
collectfunction aggregates system metrics effectively, utilizing the helper functions defined. The use ofunsafeblocks is justified and handled correctly.
107-109: Verify the accuracy of open file descriptors countWhen counting open file descriptors using
/dev/fd, ensure that the count accurately reflects the number of open files, excluding any special entries that may not represent actual file descriptors.You can run the following script to list and count the actual file descriptors:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
README.md (1)
Line range hint
1-54: Consider additional documentation updates for FreeBSD supportWhile the changes accurately reflect the addition of FreeBSD support in the project description and metrics table, consider the following suggestions to further improve the documentation:
- Update the usage examples to mention FreeBSD compatibility, if applicable.
- Add a note in the Features section about FreeBSD support, possibly mentioning any specific considerations or requirements for FreeBSD users.
These additions would provide a more comprehensive overview of the new FreeBSD support for users.
🧰 Tools
🪛 LanguageTool
[style] ~41-~41: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...lient_golang]) provides. > [!NOTE] > > Prior to version 2.0.0, the `process_cpu_seconds...(EN_WORDINESS_PREMIUM_PRIOR_TO)
[style] ~56-~56: Using many exclamation marks might seem excessive (in this case: 7 exclamation marks for a text that’s 4678 characters long)
Context: ...x | x | | x | > [!NOTE] > > If you only need to compile th...(EN_EXCESSIVE_EXCLAMATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- README.md (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
README.md (2)
11-12: LGTM: FreeBSD support added to project descriptionThe addition of FreeBSD to the list of supported platforms accurately reflects the new feature implemented in this PR.
44-54: Verifyprocess_heap_bytesmetric supportThe addition of the FreeBSD column in the metrics table is great and accurately reflects the new support. However, could you please verify if the
process_heap_bytesmetric is indeed only supported on FreeBSD and not on other platforms? This seems unusual and might be worth double-checking.
README.md
Outdated
| [metrics] crate, supporting Linux, macOS, and Windows. The collector code is | ||
| manually rewritten in Rust from the official Prometheus client for Go | ||
| [metrics] crate, supporting Linux, macOS, Windows, and FreeBSD. The collector | ||
| code is manually rewritten in Rust from the official Prometheus client for Go |
There was a problem hiding this comment.
Is this also the case for your FreeBSD addition? If not, maybe "Some collector code..."?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
===========================================
- Coverage 68.69% 52.74% -15.95%
===========================================
Files 5 4 -1
Lines 214 237 +23
===========================================
- Hits 147 125 -22
- Misses 67 112 +45 ☔ View full report in Codecov by Sentry. |
|
@AMDmi3 Thanks a lot! I was wondering if we could use https://github.com/cross-platform-actions/action to ensure that |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/test.yml (1)
107-127: LGTM! Good addition of FreeBSD testing.The new
test-cross-platform-freebsdjob is well-structured and appropriately integrated into the existing workflow. It correctly uses cross-platform-actions to test on FreeBSD 14.1, which enhances the project's cross-platform compatibility testing.Consider the following improvements:
- Make the job conditional to run only on specific branches or events to optimize CI resources:
test-cross-platform-freebsd: if: github.event_name == 'pull_request' || github.ref == 'refs/heads/main' runs-on: ubuntu-latest # ... rest of the job configuration
- Use a more specific cache key for FreeBSD testing:
- uses: actions/cache@v4 with: path: | ~/.cargo/bin/ ~/.cargo/registry/index/ ~/.cargo/registry/cache/ ~/.cargo/git/db/ target/ key: freebsd-14.1-cargo-${{ hashFiles('**/Cargo.lock') }}
- Pin package versions for better reproducibility:
run: | sudo pkg update && sudo pkg install -y rust-1.70.0 cmake-3.25.2 rust-bindgen-cli-0.65.1 cargo testThese suggestions can improve the job's efficiency, caching, and reproducibility.
|
Done. See https://github.com/lambdalisue/rs-metrics-process/actions/runs/11264911567/job/31325788843?pr=53 for example run. The only preparation steps inside the VM would be to install |
|
Thanks! |
This makes sense
And this does not as FreeBSD is rolling release. |
Though FreeBSD does support procfs/linprocfs, these are not required to be mounted (and are commontly not), so use standard always available interfaces instead:
Note that this code may likely be reused for other BSD systems (OpenBSD, NetBSD) with minimal changes.
Example
cargo test -- --nocaptureoutput on my system:Notably, after
ulimit -n 100 -v $((1024*1024)):Summary by CodeRabbit
New Features
Bug Fixes