Calculate network/device uptime statistics over any specified period #3586
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new static method Changes
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
Baalmart
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #3586 +/- ##
===========================================
- Coverage 27.04% 26.98% -0.07%
===========================================
Files 146 146
Lines 21340 21397 +57
Branches 274 274
===========================================
+ Hits 5772 5774 +2
- Misses 15568 15623 +55
|
|
Device registry changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
src/device-registry/models/Device.js (1)
729-983: Consider adding unit tests for the new methodThe
getUptimeStatisticsmethod adds significant functionality to the codebase. Adding unit tests will ensure the method works as intended and will help catch any future regressions.Would you like assistance in creating unit tests for this method?
🧰 Tools
🪛 Biome
[error] 917-917: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 922-922: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 926-926: Do not add then to an object.
(lint/suspicious/noThenProperty)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/device-registry/models/Device.js (1 hunks)
🧰 Additional context used
🪛 Biome
src/device-registry/models/Device.js
[error] 917-917: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 922-922: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 926-926: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (1)
src/device-registry/models/Device.js (1)
914-926: Address static analysis errors related to 'then' propertyThe static analysis tool reports errors on lines 917, 922, and 926 regarding the use of the
thenproperty. In the context of MongoDB aggregation pipelines, usingcaseandthenwithin the$switchoperator is correct.🧰 Tools
🪛 Biome
[error] 917-917: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 922-922: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 926-926: Do not add then to an object.
(lint/suspicious/noThenProperty)
src/device-registry/models/Device.js
Outdated
| deviceSchema.statics.getUptimeStatistics = async function({ | ||
| devices = [], | ||
| startDate, | ||
| endDate, | ||
| timeFrame, | ||
| networkFilter, | ||
| } = {}) { |
There was a problem hiding this comment.
Add input validation for function parameters
The getUptimeStatistics method accepts parameters like devices, startDate, endDate, timeFrame, and networkFilter without validating them. To enhance robustness, consider adding validation to ensure:
startDateandendDateare valid dates andstartDateis not afterendDate.timeFrameis one of the expected values:"daily","weekly", or"monthly".
This will prevent potential runtime errors due to invalid inputs.
| matchStage.createdAt = { | ||
| $gte: new Date(startDate), | ||
| $lte: new Date(endDate), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle cases where date ranges are partially provided
Currently, the code checks if both startDate and endDate are provided before adding date range filters. However, users might provide only one of these dates. Consider supporting scenarios where:
- Only
startDateis provided: Filter records fromstartDateonwards. - Only
endDateis provided: Filter records up toendDate.
Updating the conditions will make the method more flexible.
Apply this diff to adjust the date range handling:
// Add date range to match stage if provided
-if (startDate && endDate) {
+if (startDate || endDate) {
matchStage.createdAt = {
- $gte: new Date(startDate),
- $lte: new Date(endDate),
+ ...(startDate && { $gte: new Date(startDate) }),
+ ...(endDate && { $lte: new Date(endDate) }),
};
}Similarly, adjust the status history date range filtering:
// Match status history within the date range
-if (startDate && endDate) {
+if (startDate || endDate) {
pipeline.push({
$match: {
"statusHistory.timestamp": {
- $gte: new Date(startDate),
- $lte: new Date(endDate),
+ ...(startDate && { $gte: new Date(startDate) }),
+ ...(endDate && { $lte: new Date(endDate) }),
},
},
});
}Also applies to: 760-770
| const matchStage = {}; | ||
| if (devices.length > 0) { | ||
| matchStage._id = { $in: devices }; | ||
| } | ||
| if (networkFilter) { | ||
| matchStage.network = networkFilter; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize the match stage in the aggregation pipeline
To improve performance, consider moving the initial $match stage after the $unwind of statusHistory if the filters are primarily on statusHistory fields. Additionally, ensure indexes are in place for the fields used in the $match stage to speed up query execution.
| const result = await this.aggregate(pipeline); | ||
|
|
||
| return result; | ||
| }; |
There was a problem hiding this comment.
Add error handling for the aggregation pipeline execution
Currently, the aggregation execution does not handle potential errors that may occur during database operations. Wrapping the aggregation call in a try-catch block will allow the method to handle exceptions gracefully and provide meaningful error messages.
Apply this diff to add error handling:
// Execute the aggregation pipeline
-const result = await this.aggregate(pipeline);
+let result;
+try {
+ result = await this.aggregate(pipeline);
+} catch (error) {
+ logger.error(`Aggregation error: ${error.message}`);
+ throw new Error('Error fetching uptime statistics');
+}
return result;📝 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 result = await this.aggregate(pipeline); | |
| return result; | |
| }; | |
| // Execute the aggregation pipeline | |
| let result; | |
| try { | |
| result = await this.aggregate(pipeline); | |
| } catch (error) { | |
| logger.error(`Aggregation error: ${error.message}`); | |
| throw new Error('Error fetching uptime statistics'); | |
| } | |
| return result; | |
| }; |
| $multiply: [{ $divide: ["$uptime", "$totalTime"] }, 100], | ||
| }, |
There was a problem hiding this comment.
Prevent division by zero in uptime percentage calculation
The calculation of uptimePercentage could result in a division by zero if totalTime is zero. To avoid runtime errors, add a check to ensure totalTime is not zero before performing the division.
Apply this diff to handle cases where totalTime is zero:
uptimePercentage: {
- $multiply: [{ $divide: ["$uptime", "$totalTime"] }, 100],
+ $cond: {
+ if: { $eq: ["$totalTime", 0] },
+ then: 0,
+ else: {
+ $multiply: [{ $divide: ["$uptime", "$totalTime"] }, 100],
+ },
+ },
},📝 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.
| $multiply: [{ $divide: ["$uptime", "$totalTime"] }, 100], | |
| }, | |
| uptimePercentage: { | |
| $cond: { | |
| if: { $eq: ["$totalTime", 0] }, | |
| then: 0, | |
| else: { | |
| $multiply: [{ $divide: ["$uptime", "$totalTime"] }, 100], | |
| }, | |
| }, | |
| }, |
src/device-registry/models/Device.js
Outdated
| pipeline.push({ | ||
| $group: { | ||
| _id: { | ||
| network: "$network", | ||
| timeFrame: { | ||
| $switch: { | ||
| branches: [ | ||
| { | ||
| case: { $eq: [timeFrame, "daily"] }, | ||
| then: { | ||
| $dateToString: { format: "%Y-%m-%d", date: "$$NOW" }, | ||
| }, | ||
| }, | ||
| { | ||
| case: { $eq: [timeFrame, "weekly"] }, | ||
| then: { $dateToString: { format: "%Y-W%V", date: "$$NOW" } }, | ||
| }, | ||
| { | ||
| case: { $eq: [timeFrame, "monthly"] }, | ||
| then: { $dateToString: { format: "%Y-%m", date: "$$NOW" } }, | ||
| }, | ||
| ], | ||
| default: "all", | ||
| }, | ||
| }, | ||
| }, | ||
| network: { $first: "$network" }, | ||
| totalDevices: { $sum: "$totalDevices" }, | ||
| avgUptimePercentage: { $avg: "$avgUptimePercentage" }, | ||
| onlineDevices: { $sum: "$onlineDevices" }, | ||
| offlineDevices: { $sum: "$offlineDevices" }, | ||
| onlinePercentage: { $avg: "$onlinePercentage" }, | ||
| offlinePercentage: { $avg: "$offlinePercentage" }, | ||
| onlineDeviceNames: { $addToSet: "$onlineDeviceNames" }, | ||
| offlineDeviceNames: { $addToSet: "$offlineDeviceNames" }, | ||
| }, | ||
| }); | ||
|
|
||
| // Flatten device names arrays | ||
| pipeline.push({ | ||
| $project: { | ||
| network: 1, | ||
| timeFrame: "$_id.timeFrame", | ||
| totalDevices: 1, | ||
| avgUptimePercentage: 1, | ||
| onlineDevices: 1, | ||
| offlineDevices: 1, | ||
| onlinePercentage: 1, | ||
| offlinePercentage: 1, | ||
| onlineDeviceNames: { | ||
| $reduce: { | ||
| input: "$onlineDeviceNames", | ||
| initialValue: [], | ||
| in: { $setUnion: ["$$value", "$$this"] }, | ||
| }, | ||
| }, | ||
| offlineDeviceNames: { | ||
| $reduce: { | ||
| input: "$offlineDeviceNames", | ||
| initialValue: [], | ||
| in: { $setUnion: ["$$value", "$$this"] }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Refine the time frame grouping logic
The use of $$NOW in the $dateToString expressions will result in all documents being grouped under the current date, week, or month rather than the dates from the data. To accurately group data based on the statusHistory.timestamp, consider using the timestamp from the records.
Apply this diff to correct the time frame grouping:
timeFrame: {
$switch: {
branches: [
{
case: { $eq: [timeFrame, "daily"] },
then: {
- $dateToString: { format: "%Y-%m-%d", date: "$$NOW" },
+ $dateToString: { format: "%Y-%m-%d", date: "$statusHistory.timestamp" },
},
},
{
case: { $eq: [timeFrame, "weekly"] },
then: {
- $dateToString: { format: "%Y-W%V", date: "$$NOW" },
+ $dateToString: { format: "%Y-W%V", date: "$statusHistory.timestamp" },
},
},
{
case: { $eq: [timeFrame, "monthly"] },
then: {
- $dateToString: { format: "%Y-%m", date: "$$NOW" },
+ $dateToString: { format: "%Y-%m", date: "$statusHistory.timestamp" },
},
},
],
default: "all",
},
},This change ensures that the grouping is based on the actual timestamps of the status history.
📝 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.
| pipeline.push({ | |
| $group: { | |
| _id: { | |
| network: "$network", | |
| timeFrame: { | |
| $switch: { | |
| branches: [ | |
| { | |
| case: { $eq: [timeFrame, "daily"] }, | |
| then: { | |
| $dateToString: { format: "%Y-%m-%d", date: "$$NOW" }, | |
| }, | |
| }, | |
| { | |
| case: { $eq: [timeFrame, "weekly"] }, | |
| then: { $dateToString: { format: "%Y-W%V", date: "$$NOW" } }, | |
| }, | |
| { | |
| case: { $eq: [timeFrame, "monthly"] }, | |
| then: { $dateToString: { format: "%Y-%m", date: "$$NOW" } }, | |
| }, | |
| ], | |
| default: "all", | |
| }, | |
| }, | |
| }, | |
| network: { $first: "$network" }, | |
| totalDevices: { $sum: "$totalDevices" }, | |
| avgUptimePercentage: { $avg: "$avgUptimePercentage" }, | |
| onlineDevices: { $sum: "$onlineDevices" }, | |
| offlineDevices: { $sum: "$offlineDevices" }, | |
| onlinePercentage: { $avg: "$onlinePercentage" }, | |
| offlinePercentage: { $avg: "$offlinePercentage" }, | |
| onlineDeviceNames: { $addToSet: "$onlineDeviceNames" }, | |
| offlineDeviceNames: { $addToSet: "$offlineDeviceNames" }, | |
| }, | |
| }); | |
| // Flatten device names arrays | |
| pipeline.push({ | |
| $project: { | |
| network: 1, | |
| timeFrame: "$_id.timeFrame", | |
| totalDevices: 1, | |
| avgUptimePercentage: 1, | |
| onlineDevices: 1, | |
| offlineDevices: 1, | |
| onlinePercentage: 1, | |
| offlinePercentage: 1, | |
| onlineDeviceNames: { | |
| $reduce: { | |
| input: "$onlineDeviceNames", | |
| initialValue: [], | |
| in: { $setUnion: ["$$value", "$$this"] }, | |
| }, | |
| }, | |
| offlineDeviceNames: { | |
| $reduce: { | |
| input: "$offlineDeviceNames", | |
| initialValue: [], | |
| in: { $setUnion: ["$$value", "$$this"] }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| } | |
| pipeline.push({ | |
| $group: { | |
| _id: { | |
| network: "$network", | |
| timeFrame: { | |
| $switch: { | |
| branches: [ | |
| { | |
| case: { $eq: [timeFrame, "daily"] }, | |
| then: { | |
| $dateToString: { format: "%Y-%m-%d", date: "$statusHistory.timestamp" }, | |
| }, | |
| }, | |
| { | |
| case: { $eq: [timeFrame, "weekly"] }, | |
| then: { $dateToString: { format: "%Y-W%V", date: "$statusHistory.timestamp" } }, | |
| }, | |
| { | |
| case: { $eq: [timeFrame, "monthly"] }, | |
| then: { $dateToString: { format: "%Y-%m", date: "$statusHistory.timestamp" } }, | |
| }, | |
| ], | |
| default: "all", | |
| }, | |
| }, | |
| }, | |
| network: { $first: "$network" }, | |
| totalDevices: { $sum: "$totalDevices" }, | |
| avgUptimePercentage: { $avg: "$avgUptimePercentage" }, | |
| onlineDevices: { $sum: "$onlineDevices" }, | |
| offlineDevices: { $sum: "$offlineDevices" }, | |
| onlinePercentage: { $avg: "$onlinePercentage" }, | |
| offlinePercentage: { $avg: "$offlinePercentage" }, | |
| onlineDeviceNames: { $addToSet: "$onlineDeviceNames" }, | |
| offlineDeviceNames: { $addToSet: "$offlineDeviceNames" }, | |
| }, | |
| }); | |
| // Flatten device names arrays | |
| pipeline.push({ | |
| $project: { | |
| network: 1, | |
| timeFrame: "$_id.timeFrame", | |
| totalDevices: 1, | |
| avgUptimePercentage: 1, | |
| onlineDevices: 1, | |
| offlineDevices: 1, | |
| onlinePercentage: 1, | |
| offlinePercentage: 1, | |
| onlineDeviceNames: { | |
| $reduce: { | |
| input: "$onlineDeviceNames", | |
| initialValue: [], | |
| in: { $setUnion: ["$$value", "$$this"] }, | |
| }, | |
| }, | |
| offlineDeviceNames: { | |
| $reduce: { | |
| input: "$offlineDeviceNames", | |
| initialValue: [], | |
| in: { $setUnion: ["$$value", "$$this"] }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| } |
🧰 Tools
🪛 Biome
[error] 917-917: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 922-922: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 926-926: Do not add then to an object.
(lint/suspicious/noThenProperty)
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
|
Device registry changes in this PR available for preview here |
Baalmart
changed the title
Baalmart
changed the title
Description
calculate network/device uptime statistics over the specified period
Changes Made
startDateandendDate. These are used to filter the devices and their status history within the specified date range. The function calculates the uptime based on the status changes within this period.timeFrame:This parameter allows for aggregating results by daily, weekly, or monthly periods. If specified, the results are grouped according to the chosen time frame.Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
This functionality or feature provides more comprehensive and flexible uptime statistics. It allows for analysing device performance over specific time periods and can show trends when using the
timeFrameparameter.Summary by CodeRabbit