Add OpenBSD support #59
Conversation
This allows spoting data problems on foreign platforms
Note that this doesn't use latest OpenBSD version because cross-platform-actions/action doesn't yet have a tag with its support. Also disable fail-fast for cross-platform tests, as it allows to see whether just a specific system failed, or all cross-platform, or all BSD systems.
This is based on FreeBSD code, but lacks some fields
WalkthroughThe pull request introduces updates to enhance cross-platform testing, specifically for OpenBSD and FreeBSD. The workflow file Changes
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
=======================================
Coverage 55.68% 55.68%
=======================================
Files 5 5
Lines 264 264
=======================================
Hits 147 147
Misses 117 117 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
Cargo.toml (1)
48-50: LGTM! Consider using a version range forlibc.The addition of the OpenBSD-specific dependency aligns well with the PR objective of adding OpenBSD support. The structure and version of the
libcdependency are consistent with the FreeBSD configuration.To allow for minor updates and reduce the need for frequent version bumps, consider using a version range for
libc. For example:[target.'cfg(target_os = "openbsd")'.dependencies] -libc = "0.2.159" +libc = "0.2.159" # Alternatively, use a range like "0.2"This change would allow for minor updates within the 0.2.x range, potentially reducing maintenance overhead while still maintaining compatibility.
src/collector.rs (2)
86-102: LGTM: OpenBSD-specific test function added correctly.The new test function
test_collect_internal_ok_openbsdis well-implemented and correctly verifies the expected behavior ofcollect()on OpenBSD. It's appropriate to have a separate test at this stage, given the differences in available metrics.The TODO comment about potentially merging this test with
test_collect_internal_okin the future is a good reminder for maintainability.Consider adding a brief comment explaining why certain metrics return
Nonefor OpenBSD. This would provide context for future developers and align with the PR author's note about lacking certain metrics.
Line range hint
1-124: Overall: Excellent implementation of OpenBSD support.The changes in this file consistently and correctly implement OpenBSD support across conditional compilation, error handling, and testing. The modifications align well with the PR objective and acknowledge current limitations.
To improve maintainability and clarity, consider the following suggestions:
- Add a comment in the
Metricsstruct documenting which fields are currently unsupported on OpenBSD.- Create a tracking issue for implementing the missing metrics on OpenBSD, as mentioned in the PR description.
These changes would help future contributors understand the current state of OpenBSD support and provide a clear path for further improvements.
.github/workflows/test.yml (3)
107-115: LGTM! Well-structured matrix strategy for cross-platform testing.The matrix strategy is well-defined, allowing for efficient testing across multiple operating systems. The inclusion of specific preparation commands for each OS is a good practice.
Consider adding a TODO comment or creating an issue to track the addition of OpenBSD 7.6 support when it becomes available in the action.
135-136: LGTM! Correct test execution with OS-specific preparation.The use of the matrix variable for the preparation command ensures the correct setup for each OS. Running the test command with
--nocaptureis helpful for debugging.Consider adding error handling to the preparation command, such as:
run: | set -e ${{matrix.prepare_cmd}} cargo test -- --nocaptureThis will ensure that the workflow fails if the preparation command encounters any errors.
Line range hint
107-136: Overall, excellent implementation of cross-platform testing.The changes to this workflow file successfully introduce OpenBSD support and enhance the flexibility of cross-platform testing. The use of a matrix strategy, dynamic job naming, and OS-specific preparation commands are all well-implemented. This setup will make it easier to add support for more platforms in the future.
To further improve the workflow:
- Consider adding a job that runs on the latest stable versions of all supported platforms to catch any breaking changes quickly.
- Implement a strategy to periodically update the OS versions in the matrix to ensure testing on the most recent stable releases.
src/collector/openbsd.rs (3)
37-38: Usesize_ofinstead ofsize_of_valfor clarityIn line 37, using
std::mem::size_of::<libc::kinfo_proc>()is clearer and directly conveys the intention to obtain the size oflibc::kinfo_proc, rather than usingsize_of_valon aMaybeUninitinstance.Apply this diff for improved clarity:
- let kinfo_proc_size = std::mem::size_of_val(&kinfo_proc) as libc::size_t; + let kinfo_proc_size = std::mem::size_of::<libc::kinfo_proc>() as libc::size_t;
76-81: Offer assistance with implementing missing metricsThe TODO comments indicate missing metrics such as
virtual_memory_bytes,virtual_memory_max_bytes,threads, andopen_fds. If needed, I can help research alternative methods to obtain these metrics on OpenBSD.Would you like me to explore possible ways to retrieve these metrics on OpenBSD or open a GitHub issue to track this enhancement?
79-79: Fix unclosed parenthesis in commentThe comment on line 79 has an unclosed parenthesis:
// - threads (no corresponding field in kinfo_proc(Apply this diff to correct the typo:
- // - threads (no corresponding field in kinfo_proc( + // - threads (no corresponding field in kinfo_proc)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .github/workflows/test.yml (2 hunks)
- Cargo.toml (1 hunks)
- src/collector.rs (3 hunks)
- src/collector/openbsd.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
src/collector.rs (3)
5-5: LGTM: OpenBSD-specific implementation path added correctly.The new conditional compilation attribute for OpenBSD is correctly implemented, following the established pattern for other operating systems. This change aligns well with the PR objective of adding OpenBSD support.
16-17: LGTM: OpenBSD correctly added to the list of supported platforms.The compile error condition has been appropriately updated to include OpenBSD. This ensures that the "dummy" feature is only required for truly unsupported platforms, which now excludes OpenBSD. This change is consistent with the PR objective.
108-108: LGTM: OpenBSD correctly excluded from the dummy test.The addition of
#[cfg(not(target_os = "openbsd"))]to the dummy test function is correct. This ensures that the dummy test only runs on truly unsupported platforms, now excluding OpenBSD. This change is consistent with the overall goal of adding OpenBSD support..github/workflows/test.yml (3)
116-117: LGTM! Appropriate runner and clear job naming.The use of
ubuntu-latestas the runner is correct for cross-platform testing withcross-platform-actions/action. The dynamic job name clearly identifies which OS and version is being tested in each matrix combination.
128-128: LGTM! Improved caching strategy.The updated caching key now includes the OS and version from the matrix. This ensures that each OS and version combination has its own cache, preventing potential issues with incompatible caches between different environments.
132-133: LGTM! Correct use of matrix variables in cross-platform action.The
operating_systemandversionare correctly set using matrix variables, allowing the workflow to dynamically configure the environment for each matrix combination. This is consistent with the matrix strategy defined earlier.src/collector/openbsd.rs (4)
5-14:getrusagefunction is correctly implementedThe
getrusagefunction correctly wraps thelibc::getrusagesystem call with appropriate handling of unsafe code and safety comments.
16-25:getrlimitfunction is properly implementedThe
getrlimitfunction accurately wraps thelibc::getrlimitsystem call, with correct usage of unsafe blocks and safety explanations.
82-87: Calculation ofcpu_seconds_totalis accurateThe computation of
cpu_seconds_totalcorrectly sums user and system CPU times, converting microseconds to seconds.
102-104: Verify the correctness ofstart_time_secondsAssigning
kinfo_proc.p_ustart_sectometrics.start_time_secondsassumes this field represents the process start time in seconds since the epoch. Please verify thatp_ustart_secis in the expected format and units.Run the following script to confirm the correctness:
| // this is required because MIB is array of ints, and is safe | ||
| // as long size of kinfo_proc structure doesn't exceed 2GB | ||
| kinfo_proc_size.try_into().unwrap(), | ||
| 1, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Handle potential failure in try_into conversion
At line 48, kinfo_proc_size.try_into().unwrap() could panic if the conversion fails. Although the comment mentions safety due to the size constraint, it's safer to handle the Result explicitly to prevent possible panics.
Apply this diff to gracefully handle conversion errors:
let mib = [
libc::CTL_KERN,
libc::KERN_PROC,
libc::KERN_PROC_PID,
pid,
- kinfo_proc_size.try_into().unwrap(),
+ match kinfo_proc_size.try_into() {
+ Ok(size) => size,
+ Err(_) => return None, // Handle the error appropriately
+ },
1,
];Alternatively, if you're confident the size will always fit, consider using expect with a descriptive message:
kinfo_proc_size.try_into().expect("kinfo_proc_size exceeds libc::c_int range"),Committable suggestion was skipped due to low confidence.
| fn translate_rlim(rlim: libc::rlim_t) -> u64 { | ||
| if rlim == libc::RLIM_INFINITY { | ||
| 0 | ||
| } else { | ||
| rlim as u64 | ||
| } | ||
| } |
There was a problem hiding this comment.
Review handling of RLIM_INFINITY in translate_rlim function
In the translate_rlim function, mapping RLIM_INFINITY to 0 might be misleading, as 0 could be interpreted as no resource limit rather than an infinite limit. Consider using u64::MAX or a sentinel value that more clearly represents an infinite limit.
Apply this diff to adjust the handling:
fn translate_rlim(rlim: libc::rlim_t) -> u64 {
if rlim == libc::RLIM_INFINITY {
- 0
+ u64::MAX
} else {
rlim as u64
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn translate_rlim(rlim: libc::rlim_t) -> u64 { | |
| if rlim == libc::RLIM_INFINITY { | |
| 0 | |
| } else { | |
| rlim as u64 | |
| } | |
| } | |
| fn translate_rlim(rlim: libc::rlim_t) -> u64 { | |
| if rlim == libc::RLIM_INFINITY { | |
| u64::MAX | |
| } else { | |
| rlim as u64 | |
| } | |
| } |
|
Could you update README as well? |
|
Oops, forgot to push |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
README.md (3)
45-55: LGTM: Updated metrics table with OpenBSD support.The metrics table has been accurately updated to include OpenBSD support, clearly showing which metrics are available on this platform. This is consistent with the PR objectives and provides valuable information to users.
Consider adding a note or legend explaining what the 'x' marks represent in the table, for improved clarity.
Line range hint
137-143: Consider updating the Features section.While the Features section remains unchanged, it might be worth considering if any updates are needed to reflect the addition of OpenBSD support. For example, you could mention if any specific features are related to or affect OpenBSD support.
Line range hint
1-148: Overall LGTM: README.md updated to reflect OpenBSD support.The changes to the README.md file accurately reflect the addition of OpenBSD support, which is the main objective of this PR. The updates to the supported platforms list and the metrics table provide clear and valuable information to users about the crate's capabilities on OpenBSD.
Consider adding a brief note in the Usage section mentioning that the usage remains the same across all supported platforms, including OpenBSD. This would reassure users that no platform-specific code is needed when using the crate on OpenBSD.
🧰 Tools
🪛 LanguageTool
[style] ~42-~42: ‘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] ~57-~57: Using many exclamation marks might seem excessive (in this case: 7 exclamation marks for a text that’s 4851 characters long)
Context: ... | | 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 (1)
README.md (1)
11-14: *LGTM: Updated supported platforms and acknowledgment of BSD support.The changes accurately reflect the addition of OpenBSD support and acknowledge the broader *BSD support. This aligns well with the PR objectives and provides clear information to users about the crate's capabilities.
|
Thanks a lot 🎉 |
I was curious about supporting other platforms with the help of
cross-platform-actions, and it turned out not too hard to add OpenBSD. It still lacks part of the metrics, hopefully OpenBSD uses will improve it (I myself don't have OpenBSD installed).This PR also makes cross-platform testing more flexible and allows add support for more platforms in the future.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation