src: store kernelVersion metadata in info.proto #290
Conversation
a39831b to
aa6e58e
Compare
santigimeno
left a comment
There was a problem hiding this comment.
Codewise LGTM. We need to add a test or modify the existing one in https://github.com/nodesource/nsolid/blob/node-v22.x-nsolid-v5.x/test/agents/test-grpc-info.mjs
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript (lib/nsolid.js)
participant Native as Native Addon (nsolid_api.cc)
participant System as System (Linux Kernel)
participant Proto as Protobuf (InfoBody)
JS->>Native: Call getKernelVersion()
alt On Linux
Native->>System: uname() or read /proc/version_signature
System-->>Native: kernel version info
Native-->>JS: kernel version number
else Not Linux
Native-->>JS: zero
end
JS->>Proto: Set kernelVersion field in InfoBody
Proto-->>JS: InfoBody with kernelVersion
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
agents/grpc/src/proto/info.pb.cc (2)
576-584: Generated code parsing logic forhasEbpfSupportfieldThe parsing logic for the
hasEbpfSupportfield is generated, but there's no corresponding field in the proto definition. This further confirms the mismatch between the proto file and the generated code.
767-771: Generated code serialization logic forhasEbpfSupportfieldThe serialization logic for the
hasEbpfSupportfield is generated, but there's no corresponding field in the proto definition. This further confirms the mismatch.
🧹 Nitpick comments (1)
agents/grpc/src/grpc_agent.cc (1)
24-29: Include headers with proper commentsThe added headers are correctly wrapped in an
#ifdef __linux__conditional compilation block. However, the comment could be more specific about what these headers are used for.-// Include headers for eBPF support detection +// Include headers for kernel version detection on Linux #ifdef __linux__ #include <sys/stat.h> #include <sys/utsname.h> #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
agents/grpc/proto/info.proto(1 hunks)agents/grpc/src/grpc_agent.cc(2 hunks)agents/grpc/src/proto/info.pb.cc(13 hunks)agents/grpc/src/proto/info.pb.h(4 hunks)test/agents/test-grpc-info.mjs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
agents/grpc/src/grpc_agent.cc (2)
agents/grpc/src/proto/info.pb.cc (2)
body(1039-1042)body(1040-1040)agents/grpc/src/proto/info.pb.h (3)
body(635-635)body(1436-1439)body(1436-1436)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-windows
🔇 Additional comments (5)
test/agents/test-grpc-info.mjs (1)
63-63: Test correctly verifies the newkernelVersionfieldThe test has been appropriately updated to verify that the
kernelVersionfield is present in the response and is a string.agents/grpc/src/proto/info.pb.h (4)
238-238: Field number added for the new hasEbpfSupport fieldThe PR adds a new field number constant
kHasEbpfSupportFieldNumber = 16to the enum of field numbers. This corresponds to the new eBPF support detection feature mentioned in the PR objectives.
443-450: API methods added for hasEbpfSupport boolean fieldThese public and private accessor methods for the new
hasEbpfSupportboolean field follow the standard Protocol Buffer pattern for boolean fields:
- Public methods:
clear_hasebpfsupport(),hasebpfsupport(), andset_hasebpfsupport(bool)- Private methods:
_internal_hasebpfsupport()and_internal_set_hasebpfsupport(bool)The implementation properly maintains the pattern used throughout the file.
479-479: Added storage member for the hasEbpfSupport fieldThe boolean member variable
hasebpfsupport_is correctly added to the internal implementation struct to store the actual field value.
1309-1327: Implementation of hasEbpfSupport accessor methodsThe actual implementation of the accessor methods for the
hasEbpfSupportfield:
- Properly initializes the default value to
falsein the clear method- Correctly implements getters and setters
- Maintains the Protocol Buffer insertion points for extensibility
- Follows consistent patterns with other boolean field implementations in the file
This implementation correctly follows Protocol Buffer's C++ code generation patterns.
afba5ee to
ccb9c12
Compare
17f7910 to
a1fea5d
Compare
01578d6 to
89ab926
Compare
|
@santigimeno I couldn't test it locally (just the test/agents/test-grpc-info.mjs). What's the best way to test this PR? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/nsolid/nsolid_api.cc (1)
2540-2556: Implementation looks good but could add error logging.The function correctly retrieves the kernel version on Linux using
uname()and returns an empty string on other platforms. The implementation is straightforward and follows the established patterns in the codebase.Consider adding debug logging when
uname()fails to help with troubleshooting:if (uname(&info) == 0) { kernel_version = info.release; + } else { + // Consider adding debug logging here for troubleshooting + // e.g., fprintf(stderr, "Failed to get kernel version: %s\n", strerror(errno)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
agents/grpc/proto/info.proto(1 hunks)agents/grpc/src/proto/info.pb.cc(13 hunks)agents/grpc/src/proto/info.pb.h(4 hunks)lib/nsolid.js(1 hunks)src/nsolid/nsolid_api.cc(4 hunks)test/agents/test-grpc-info.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/nsolid.js
🚧 Files skipped from review as they are similar to previous changes (4)
- test/agents/test-grpc-info.mjs
- agents/grpc/proto/info.proto
- agents/grpc/src/proto/info.pb.cc
- agents/grpc/src/proto/info.pb.h
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: lint-js-and-md
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: build-tarball
- GitHub Check: test-macOS
- GitHub Check: build-docs
- GitHub Check: coverage-windows
- GitHub Check: coverage-linux
- GitHub Check: coverage-linux-without-intl
🔇 Additional comments (3)
src/nsolid/nsolid_api.cc (3)
20-22: Clean platform-specific header inclusion.Good use of conditional compilation to include the
<sys/utsname.h>header only on Linux platforms, keeping the code portable and avoiding unnecessary dependencies on other platforms.
2980-2980: Proper method registration.The new
getKernelVersionfunction is correctly registered with the V8 binding using theSetMethodfunction, following the same pattern as other functions in this file.
3113-3113: Proper external reference registration.The
GetKernelVersionfunction is correctly registered with the external reference registry, ensuring it will be properly accessible from JavaScript.
|
In addition to those changes, don't forget to update PopulateInfoEvent in grpc_agent.cc so it sets the kernel version. |
On top of modifying the grpc test I would also modify and https://github.com/nodesource/nsolid/tree/6d8741c4f298758fae27beab6748819e06e839d6/test/addons/nsolid-get-infoThe check I would do is:
|
89ab926 to
7f53b27
Compare
|
PTAL @santigimeno |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
agents/grpc/proto/info.proto(1 hunks)agents/grpc/src/grpc_agent.cc(1 hunks)agents/grpc/src/proto/info.pb.cc(15 hunks)agents/grpc/src/proto/info.pb.h(4 hunks)lib/nsolid.js(2 hunks)src/nsolid/nsolid_api.cc(4 hunks)test/addons/nsolid-get-info/nsolid-get-info.js(1 hunks)test/agents/test-grpc-info.mjs(1 hunks)test/parallel/test-nsolid-info.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- agents/grpc/src/grpc_agent.cc
🚧 Files skipped from review as they are similar to previous changes (6)
- agents/grpc/proto/info.proto
- test/agents/test-grpc-info.mjs
- src/nsolid/nsolid_api.cc
- lib/nsolid.js
- agents/grpc/src/proto/info.pb.cc
- agents/grpc/src/proto/info.pb.h
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: coverage-linux-without-intl
- GitHub Check: test-macOS
- GitHub Check: coverage-linux
- GitHub Check: lint-js-and-md
- GitHub Check: build-tarball
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-windows
test/parallel/test-nsolid-info.js
Outdated
| assert.strictEqual(typeof info.kernelVersion, 'string'); | ||
| assert.ok(info.kernelVersion.length > 0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add platform-specific validation for kernel version.
The test correctly verifies that the kernelVersion exists and is a non-empty string. However, based on the PR discussion, we should add platform-specific validation:
- On Linux: Check that kernelVersion represents a version >= 2.0.0
- On non-Linux: Verify that kernelVersion is '0'
assert.strictEqual(typeof info.kernelVersion, 'string');
assert.ok(info.kernelVersion.length > 0);
+ if (process.platform === 'linux') {
+ // Ensure kernel version is >= 2.0.0 on Linux
+ assert.ok(info.kernelVersion !== '0', 'Linux kernel version should not be 0');
+ } else {
+ // On non-Linux platforms, kernel version should be '0'
+ assert.strictEqual(info.kernelVersion, '0', `Kernel version should be 0 on ${process.platform}`);
+ }🤖 Prompt for AI Agents
In test/parallel/test-nsolid-info.js around lines 79 to 80, enhance the
kernelVersion validation by adding platform-specific checks: if the platform is
Linux, parse kernelVersion as a version string and assert it is greater than or
equal to 2.0.0; if the platform is not Linux, assert that kernelVersion equals
'0'. This requires detecting the platform and performing conditional assertions
accordingly.
|
TODO for tomorrow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
agents/grpc/proto/info.proto(1 hunks)agents/grpc/src/grpc_agent.cc(1 hunks)src/nsolid/nsolid_api.cc(4 hunks)test/addons/nsolid-get-info/nsolid-get-info.js(2 hunks)test/agents/test-grpc-info.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/agents/test-grpc-info.mjs
- agents/grpc/proto/info.proto
- test/addons/nsolid-get-info/nsolid-get-info.js
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-windows
- GitHub Check: test-macOS
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: build-docs
- GitHub Check: build-tarball
- GitHub Check: coverage-linux
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-linux-without-intl
🔇 Additional comments (4)
agents/grpc/src/grpc_agent.cc (1)
255-257: 💡 Verification agent🧩 Analysis chain
Implementation correctly added for kernelVersion field retrieval
The code now correctly retrieves and sets the
kernelVersionfield from the JSON info object to the protobuf message. This change aligns with the PR objective to store the kernel version metadata in the info.proto file.
🏁 Script executed:
#!/bin/bash # Verify that all kernel version related changes work together correctly # Check for the implementation of the getKernelVersion native method echo "Checking for kernel version implementation in nsolid_api.cc..." rg -A 10 "getKernelVersion" src/nsolid/nsolid_api.cc # Check if the field was properly added to the proto file echo "Checking for kernelVersion field in info.proto..." rg "kernelVersion" agents/grpc/proto/info.proto # Check if tests have been updated as recommended in PR comments echo "Checking for test updates..." rg "kernelVersion" test/agents/test-grpc-info.mjs rg "kernelVersion" test/parallel/test-nsolid-info.js rg "kernelVersion" test/addons/nsolid-get-infoLength of output: 1698
🏁 Script executed:
#!/bin/bash # Inspect the implementation of GetKernelVersion in the native Node API echo "=== src/nsolid/nsolid_api.cc (GetKernelVersion context) ===" rg -A 5 -B 5 "GetKernelVersion" src/nsolid/nsolid_api.cc || true # Show the content of the Node API test that asserts a string echo "=== test/parallel/test-nsolid-info.js ===" sed -n '1,200p' test/parallel/test-nsolid-info.js # Show the add-on test expectations for kernelVersion echo "=== test/addons/nsolid-get-info/nsolid-get-info.js ===" sed -n '1,200p' test/addons/nsolid-get-info/nsolid-get-info.jsLength of output: 8588
Fix typo in native API and align JS test with numeric kernelVersion
There’s a typo in
GetKernelVersionand a mismatch in the pure-JS test’s expected type:• In
src/nsolid/nsolid_api.cc, change:- if (kernel_versin != 0) { + if (kernel_version != 0) {so it references the declared
kernel_versionand compiles correctly.• In
test/parallel/test-nsolid-info.js, update the assertions to expect a number (to match the addon test and proto definition):- assert.strictEqual(typeof info.kernelVersion, 'string'); - assert.ok(info.kernelVersion.length > 0); + assert.strictEqual(typeof info.kernelVersion, 'number'); + assert.ok(info.kernelVersion > 0);These fixes ensure the native binding, gRPC agent, and both test suites agree on a numeric
kernelVersion.Likely an incorrect or invalid review comment.
src/nsolid/nsolid_api.cc (3)
2540-2605: The implementation of the Linux kernel version parser is thorough and well-designed.The function handles various Linux distributions like Ubuntu and Debian, correctly parses kernel version strings, and addresses edge cases like the UNAME26 personality. The caching mechanism with a static variable is also a good performance optimization.
3046-3046: LGTM: The method is properly registered to be accessible from JavaScript.The
getKernelVersionmethod is correctly exposed to JavaScript.
3179-3179: LGTM: The method is properly registered in the external reference registry.This ensures the native function can be called from JavaScript and is properly included in snapshots.
1566823 to
2f2766d
Compare
2f2766d to
84991ba
Compare
|
I'll fix the build error soon (need to import the header) |
5994dec to
48da108
Compare
PR-URL: #290 Refs: nodesource/ebpf-roadmap#1 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
|
Landed in d4cd851 |
PR-URL: #290 Refs: nodesource/ebpf-roadmap#1 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
PR-URL: #290 Refs: nodesource/ebpf-roadmap#1 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> PR-URL: #359 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Refs: https://github.com/nodesource/ebpf-roadmap/issues/1
Summary by CodeRabbit
New Features
Tests