feat/be/hardware monitor type #1084
Conversation
…dware to handleHttp as they are identical
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several enhancements across multiple files. In Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
Server/db/models/Monitor.js (1)
Line range hint 1-71: Yo! Let's organize this schema better! 📝
The schema's getting kinda messy with fields scattered around. Let's group related fields together for better maintainability.
Consider reorganizing the fields like this:
- Core fields (id, name, description)
- Ownership fields (userId, teamId)
- Configuration fields (type, url, interval, secret)
- Status fields (status, isActive, uptimePercentage)
- Monitoring fields (thresholds, notifications)
Would you like me to show you a refactored version with better organization?
Server/db/models/HardwareCheck.js (3)
47-58: These new fields are straight fire! 🔥
The addition of responseTime, statusCode, and message fields creates a solid foundation for hardware monitoring. The index on statusCode is particularly clutch for performance.
Consider adding validation for statusCode to ensure it stays within expected ranges:
statusCode: {
type: Number,
index: true,
+ validate: {
+ validator: function(v) {
+ return Number.isInteger(v) && v >= 100 && v < 600;
+ },
+ message: props => `${props.value} is not a valid status code!`
+ }
},59-63: Expiry setup's looking clean, but let's make it cleaner!
The 30-day TTL index is solid for managing data retention, but that magic number's making me nervous like mom's spaghetti.
Consider extracting the expiry duration to a named constant:
+const HARDWARE_CHECK_RETENTION_DAYS = 30;
+
expiry: {
type: Date,
default: Date.now,
- expires: 60 * 60 * 24 * 30, // 30 days
+ expires: 60 * 60 * 24 * HARDWARE_CHECK_RETENTION_DAYS,
},Line range hint 1-83: This schema's structure is tighter than Eminem's rhymes!
The organization with separate sub-schemas for CPU, memory, disk, and host info is clean. However, let's drop some documentation to help future maintainers understand this masterpiece.
Consider adding JSDoc comments to describe the schema's purpose and field meanings:
+/**
+ * Schema for storing hardware monitoring check results
+ * @typedef {Object} HardwareCheck
+ * @property {ObjectId} monitorId - Reference to the parent monitor
+ * @property {Boolean} status - Overall check status
+ * @property {Number} responseTime - Time taken to complete the check
+ * @property {Number} statusCode - HTTP-like status code for the check
+ * @property {String} message - Human-readable check result message
+ * @property {Date} expiry - Automatic document expiration timestamp
+ */
const HardwareCheckSchema = mongoose.Schema(Server/service/statusService.js (1)
141-141: Mom's spaghetti says we need more error context! 🍝
While the optional chaining is good, we could provide more context in the error log.
Consider adding the monitor type to the error details:
-details: `Error inserting check for monitor: ${networkResponse?.monitorId}`,
+details: `Error inserting ${networkResponse?.type || 'unknown'} check for monitor: ${networkResponse?.monitorId}`,Server/tests/services/statusService.test.js (2)
176-195: Yo dawg, we need to beef up these hardware tests! 🍝
The test looks solid for the happy path, but there's more sauce we could add to make it even better:
- Add edge cases for when hardware stats are missing or incomplete
- Validate data types (numbers for CPU/memory usage)
- Use more realistic sample data (e.g., CPU: "45.2%", memory: "1.2GB/4GB")
Here's a suggested addition to make the tests more robust:
it("should handle partial hardware stats gracefully", () => {
const check = statusService.buildCheck({
monitorId: "test",
type: "hardware",
status: true,
responseTime: 100,
code: 200,
message: "Test message",
payload: { cpu: "45.2%" }, // Only CPU provided
});
expect(check.cpu).to.equal("45.2%");
expect(check.memory).to.be.undefined;
expect(check.disk).to.be.undefined;
expect(check.host).to.be.undefined;
});Line range hint 196-215: Mom's spaghetti moment: We're missing hardware-specific test cases in insertCheck! 🍝
The insertCheck test suite should verify that hardware checks are properly handled. According to the PR objectives, hardware checks are handled similarly to HTTP checks, but we should explicitly test this behaviour.
Add a test case for hardware check insertion:
it("should insert a hardware check into the database", async () => {
const hardwareCheck = {
monitorId: "test",
type: "hardware",
cpu: "45.2%",
memory: "1.2GB/4GB",
disk: "75%",
host: "server-01"
};
await statusService.insertCheck(hardwareCheck);
expect(statusService.db.createCheck.calledOnce).to.be.true;
expect(statusService.db.createCheck.firstCall.args[0]).to.deep.equal(hardwareCheck);
});Server/service/networkService.js (2)
183-185: Yo! Let's make this hardware-specific, know what I'm sayin'?
While reusing HTTP handling is efficient, hardware monitoring might need specialized processing. Consider:
- Hardware-specific error codes
- Response validation for hardware metrics
- Timeout configurations suitable for hardware checks
Would you like me to propose a more specialized implementation for hardware monitoring?
205-205: Let's add some logging for these hardware checks!
Consider adding debug logging for successful hardware checks to help with monitoring and debugging.
case this.TYPE_HARDWARE:
+ this.logger.debug({
+ message: 'Processing hardware check',
+ service: this.SERVICE_NAME,
+ method: 'getStatus',
+ });
return await this.requestHardware(job);Server/tests/services/networkService.test.js (2)
152-204: Yo! Add test case for invalid secret handling.
The hardware test suite looks solid, but there's one scenario we're missing - what happens when the secret is invalid? Consider adding a test case like:
it("should return a response object if hardware unsuccessful due to invalid secret", async () => {
const error = new Error("Unauthorized");
error.response = { status: 401 };
networkService.timeRequest = sinon
.stub()
.resolves({ response: null, responseTime: 1, error });
const job = {
data: {
url: "http://test.com",
_id: "123",
type: "hardware",
secret: "wrong_secret"
}
};
const httpResult = await networkService.requestHardware(job);
expect(httpResult.monitorId).to.equal("123");
expect(httpResult.type).to.equal("hardware");
expect(httpResult.responseTime).to.be.a("number");
expect(httpResult.status).to.be.false;
expect(httpResult.code).to.equal(401);
});235-240: Add assertion for HTTP method not being called.
Since hardware monitors use HTTP under the hood, let's make it crystal clear that we're not using the HTTP handler directly:
it("should call requestHardware if type is hardware", () => {
networkService.getStatus({ data: { type: "hardware" } });
expect(networkService.requestHardware.calledOnce).to.be.true;
expect(networkService.requestPing.notCalled).to.be.true;
expect(networkService.requestPagespeed.notCalled).to.be.true;
+ expect(networkService.requestHttp.notCalled).to.be.true;
});📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- Server/db/models/HardwareCheck.js (1 hunks)
- Server/db/models/Monitor.js (1 hunks)
- Server/service/jobQueue.js (0 hunks)
- Server/service/networkService.js (6 hunks)
- Server/service/statusService.js (3 hunks)
- Server/tests/services/networkService.test.js (8 hunks)
- Server/tests/services/statusService.test.js (1 hunks)
- Server/validation/joi.js (1 hunks)
💤 Files with no reviewable changes (1)
- Server/service/jobQueue.js
🧰 Additional context used
🪛 Biome
Server/service/networkService.js
[error] 111-111: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (6)
Server/db/models/Monitor.js (1)
Line range hint 37-38: Verify hardware monitor type usage across the codebase 🔍
Let's make sure the new hardware enum value is properly handled everywhere.
Server/db/models/HardwareCheck.js (1)
39-39: Yo, nice performance optimization with that index!
Adding an index to monitorId is gonna make those queries fly faster than mom's spaghetti sliding off a plate. Since this field is frequently used for lookups, this is a solid improvement.
Server/service/statusService.js (1)
131-131: Verify that createHardwareCheck exists in the database layer! 🍝
Let's ensure the database method exists to prevent runtime errors.
✅ Verification successful
Yo, the createHardwareCheck method is present and properly wired up! 🍝
The method exists in all the right places:
- Defined in
Server/db/mongo/modules/hardwareCheckModule.js - Properly exported and imported in
Server/db/mongo/MongoDB.js - Correctly referenced in
statusService.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for createHardwareCheck method definition
ast-grep --pattern 'createHardwareCheck = async function($$$) {
$$$
}'
Length of output: 74
Script:
#!/bin/bash
# Let's try different patterns to find createHardwareCheck
# Search in js files
rg -l "createHardwareCheck"
# Get more context around any matches
rg -B 2 -A 2 "createHardwareCheck"
# Try finding it as a class property
ast-grep --pattern 'createHardwareCheck = $_'
# Look for it in database related files specifically
fd "db|database" -e js -x rg "createHardwareCheck" {}
Length of output: 2279
Server/service/networkService.js (1)
96-96: Documentation looks good, eh!
The JSDoc updates properly document the new secret parameter and its purpose.
Server/tests/services/networkService.test.js (1)
76-76: LGTM! Type field additions are consistent.
The addition of the type field to the job objects in existing test cases maintains consistency with the new monitor type system.
Also applies to: 89-89, 103-103, 115-115, 128-128, 142-142
Server/validation/joi.js (1)
239-239: Same validation rules needed here too, buddy!
For consistency and security, the edit validation should match the create validation rules.
Apply the same validation rules as suggested for the create validation above.
| secret: { | ||
| type: String, | ||
| }, |
There was a problem hiding this comment.
Yo! We need to encrypt these secrets before they drop into the DB! 🔐
Storing authentication secrets in plain text is like leaving your house key under the doormat - it's asking for trouble! Here's what we need to address:
- Implement encryption before storage
- Add validation for the secret format/length
- Mark this field as sensitive in the schema
Consider implementing these security measures:
secret: {
type: String,
+ select: false, // Hide from query results by default
+ validate: {
+ validator: function(v) {
+ return v && v.length >= 8 && v.length <= 1024;
+ },
+ message: 'Secret must be between 8 and 1024 characters'
+ },
+ set: function(v) {
+ // Add encryption here
+ return v ? encrypt(v) : v;
+ }
},Would you like me to create a separate issue to track the encryption implementation?
Committable suggestion was skipped due to low confidence.
🛠️ Refactor suggestion
Yo! Let's add some middleware to protect this sensitive field! 🛡️
We should restrict access to the secret field at the schema level to prevent accidental exposure.
+// Add this before the model export
+MonitorSchema.pre('save', function(next) {
+ if (this.isModified('secret')) {
+ // Log secret changes for audit
+ console.info(`Monitor ${this._id} secret modified by user ${this.userId}`);
+ }
+ next();
+});
+MonitorSchema.pre('find', function() {
+ // Exclude secret by default unless explicitly requested
+ if (!this._fields || !this._fields.secret) {
+ this.select('-secret');
+ }
+});Committable suggestion was skipped due to low confidence.
Server/service/statusService.js
Outdated
| if (type === "hardware") { | ||
| check.cpu = payload.cpu; | ||
| check.memory = payload.memory; | ||
| check.disk = payload.disk; | ||
| check.host = payload.host; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo dawg, we need some validation up in here! 🍝
The hardware check payload handling could use some improvements:
- Add payload validation to prevent undefined property access
- Document the expected payload structure in JSDoc
Here's a more robust implementation:
if (type === "hardware") {
+ // Validate required payload properties
+ const { cpu, memory, disk, host } = payload || {};
+ if (!cpu || !memory || !disk || !host) {
+ throw new Error('Invalid hardware payload: missing required properties');
+ }
check.cpu = payload.cpu;
check.memory = payload.memory;
check.disk = payload.disk;
check.host = payload.host;
}Also, please add JSDoc documentation above the method to describe the expected payload structure:
/**
* @typedef {Object} HardwarePayload
* @property {Object} cpu - CPU statistics
* @property {Object} memory - Memory usage details
* @property {Object} disk - Disk usage information
* @property {string} host - Host system identifier
*/| ); | ||
|
|
||
| const httpResponse = { | ||
| monitorId: job.data._id, | ||
| type: "http", | ||
| type: job.data.type, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hold up! We should validate the type, eh?
The dynamic type assignment from job.data.type could lead to inconsistencies. Consider validating against allowed types.
+ const validTypes = [this.TYPE_PING, this.TYPE_HTTP, this.TYPE_PAGESPEED, this.TYPE_HARDWARE];
+ if (!validTypes.includes(job.data.type)) {
+ throw new Error(`Invalid monitor type: ${job.data.type}`);
+ }
type: job.data.type,Also applies to: 164-164
| const config = {}; | ||
|
|
||
| job.data.secret !== undefined && | ||
| (config.headers = { Authorization: `Bearer ${job.data.secret}` }); | ||
|
|
There was a problem hiding this comment.
Oh snap! Let's make the secret handling more robust, buddy!
A few concerns with the current secret handling:
- The assignment in expression makes the code harder to read
- Missing validation for secret format
- Direct exposure of secrets in headers
Here's a cleaner implementation:
const config = {};
-
-job.data.secret !== undefined &&
- (config.headers = { Authorization: `Bearer ${job.data.secret}` });
+
+if (job.data.secret) {
+ if (typeof job.data.secret !== 'string' || !job.data.secret.trim()) {
+ throw new Error('Invalid secret format');
+ }
+ config.headers = { Authorization: `Bearer ${job.data.secret.trim()}` };
+}📝 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.
| const config = {}; | |
| job.data.secret !== undefined && | |
| (config.headers = { Authorization: `Bearer ${job.data.secret}` }); | |
| const config = {}; | |
| if (job.data.secret) { | |
| if (typeof job.data.secret !== 'string' || !job.data.secret.trim()) { | |
| throw new Error('Invalid secret format'); | |
| } | |
| config.headers = { Authorization: `Bearer ${job.data.secret.trim()}` }; | |
| } |
🧰 Tools
🪛 Biome
[error] 111-111: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| describe("requestHardware", () => { | ||
| it("should return a response object if hardware successful", async () => { | ||
| const job = { data: { url: "http://test.com", _id: "123", type: "hardware" } }; | ||
| const httpResult = await networkService.requestHardware(job); | ||
| expect(httpResult.monitorId).to.equal("123"); | ||
| expect(httpResult.type).to.equal("hardware"); | ||
| expect(httpResult.responseTime).to.be.a("number"); | ||
| expect(httpResult.status).to.be.true; | ||
| }); | ||
| it("should return a response object if hardware successful and job jas a secret", async () => { | ||
| const job = { | ||
| data: { | ||
| url: "http://test.com", | ||
| _id: "123", | ||
| type: "hardware", | ||
| secret: "my_secret", | ||
| }, | ||
| }; | ||
| const httpResult = await networkService.requestHardware(job); | ||
| expect(httpResult.monitorId).to.equal("123"); | ||
| expect(httpResult.type).to.equal("hardware"); | ||
| expect(httpResult.responseTime).to.be.a("number"); | ||
| expect(httpResult.status).to.be.true; | ||
| }); | ||
| it("should return a response object if hardware unsuccessful", async () => { | ||
| const error = new Error("Test error"); | ||
| error.response = { status: 404 }; | ||
| networkService.timeRequest = sinon | ||
| .stub() | ||
| .resolves({ response: null, responseTime: 1, error }); | ||
| const job = { data: { url: "http://test.com", _id: "123", type: "hardware" } }; | ||
| const httpResult = await networkService.requestHardware(job); | ||
| expect(httpResult.monitorId).to.equal("123"); | ||
| expect(httpResult.type).to.equal("hardware"); | ||
| expect(httpResult.responseTime).to.be.a("number"); | ||
| expect(httpResult.status).to.be.false; | ||
| expect(httpResult.code).to.equal(404); | ||
| }); | ||
| it("should return a response object if hardware unsuccessful with unknown code", async () => { | ||
| const error = new Error("Test error"); | ||
| error.response = {}; | ||
| networkService.timeRequest = sinon | ||
| .stub() | ||
| .resolves({ response: null, responseTime: 1, error }); | ||
| const job = { data: { url: "http://test.com", _id: "123", type: "hardware" } }; | ||
| const httpResult = await networkService.requestHardware(job); | ||
| expect(httpResult.monitorId).to.equal("123"); | ||
| expect(httpResult.type).to.equal("hardware"); | ||
| expect(httpResult.responseTime).to.be.a("number"); | ||
| expect(httpResult.status).to.be.false; | ||
| expect(httpResult.code).to.equal(networkService.NETWORK_ERROR); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Hardware statistics payload needs test coverage
The test suite is missing assertions for hardware statistics payload. From statusService.js, we can see that hardware responses should include:
- cpu
- memory
- disk
- host
Current tests only verify:
- monitorId
- type
- responseTime
- status
- error codes
• Add assertions for hardware statistics in Server/tests/services/networkService.test.js successful cases to verify the payload structure matches what statusService expects.
🔗 Analysis chain
Verify test coverage for hardware statistics.
The PR objectives mention that hardware checks should populate hardware statistics. Let's verify the response payload structure:
Let me analyze the actual implementation to understand what properties we should be testing for in hardware responses.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hardware statistics schema or type definitions
rg -l "hardware.*statistics|statistics.*hardware" --type ts --type js
# Search for hardware response handling
ast-grep --pattern 'function $_(response) {
$$$
hardware
$$$
}'
Length of output: 143
Script:
#!/bin/bash
# Let's search for hardware response structure in the networkService implementation
rg -A 10 "requestHardware" --type js
# Also check for any hardware-related types or interfaces
rg -A 5 "hardware.*type|type.*hardware" --type js --type ts
# Look for hardware-related test assertions
rg "expect.*hardware|hardware.*expect" --type js
Length of output: 14046
| @@ -228,13 +228,15 @@ const createMonitorBodyValidation = joi.object({ | |||
| usage_disk: joi.number(), | |||
| }), | |||
| notifications: joi.array().items(joi.object()), | |||
| secret: joi.string(), | |||
There was a problem hiding this comment.
Yo! Let's beef up that secret field validation, eh?
The secret field needs some validation love to keep things secure. Consider adding these validation rules:
- Minimum length requirement
- Character complexity requirements
- Trim whitespace
- Prevent common security pitfalls
Here's a suggested improvement:
- secret: joi.string(),
+ secret: joi.string()
+ .min(8)
+ .trim()
+ .pattern(/^[\x20-\x7E]+$/) // Printable ASCII characters only
+ .messages({
+ 'string.min': 'Secret must be at least {#limit} characters',
+ 'string.pattern.base': 'Secret must contain only printable ASCII characters'
+ }),📝 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.
| secret: joi.string(), | |
| secret: joi.string() | |
| .min(8) | |
| .trim() | |
| .pattern(/^[\x20-\x7E]+$/) // Printable ASCII characters only | |
| .messages({ | |
| 'string.min': 'Secret must be at least {#limit} characters', | |
| 'string.pattern.base': 'Secret must contain only printable ASCII characters' | |
| }), |
💡 Codebase verification
Yo, we need to implement encryption for that secret field!
After sweating through the codebase like it's a rap battle, I can confirm there's no encryption implementation for the Monitor secret field. While we've got crypto being used in other parts (auth, recovery, invites), it's not touching our Monitor secrets. That's a security concern we need to address, eh?
- The secret field in the Monitor model is currently stored as plain text
- No encryption/decryption logic found in any Monitor-related files
- Other sensitive data in the system uses crypto, so we should follow that pattern
🔗 Analysis chain
Heads up about that secret field encryption, eh?
The PR objectives mention encrypting the secret field to protect against database leaks. While encryption isn't handled at the validation level, we should verify that it's being properly encrypted before storage.
Let's check if encryption is implemented in the Monitor model:
Also applies to: 239-239
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for encryption-related code in Monitor model and service
rg -i "encrypt|crypto" Server/db/models/Monitor.js
rg -i "encrypt|crypto" Server/service/monitorService.js
Length of output: 183
Script:
#!/bin/bash
# Let's first find the correct path for monitor service
fd -t f -e js "monitor.*service" Server/
# Search for encryption-related code in all relevant files
rg -i "encrypt|crypto" Server/
# Look for any crypto-related imports or requires
ast-grep --pattern 'require("crypto")'
ast-grep --pattern 'import $_ from "crypto"'
# Check for any encryption-related functions
ast-grep --pattern 'function encrypt$_($_) { $$$ }'
ast-grep --pattern 'const encrypt$_ = $_'
Length of output: 1101
Thanks for the review and the spelling catch! At least you know I wrote the tests myself if they include typos 😂 |
just corrected my own type sill -> still:) |
This PR implements handling of Hardware Monitors on the back end
HardwareCheckmodel to include standard fieldsMonitorto allow config for monitors that require authenticationNetworkServiceto handle hardware monitorshandleHttpfunctionhardwaretype monitors