Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calculate network/device uptime statistics over any specified period #3586

Draft
wants to merge 5 commits into
base: staging
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/device-registry/controllers/create-device.js
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,59 @@
return;
});
},
getUptimeStatistics: async (req, res, next) => {
try {
const errors = extractErrorsFromRequest(req);
if (errors) {
next(

Check warning on line 965 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L962-L965

Added lines #L962 - L965 were not covered by tests
new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors)
);
return;

Check warning on line 968 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L968

Added line #L968 was not covered by tests
}

const request = req;
const defaultTenant = constants.DEFAULT_TENANT || "airqo";
request.query.tenant = isEmpty(req.query.tenant)

Check warning on line 973 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L971-L973

Added lines #L971 - L973 were not covered by tests
? defaultTenant
: req.query.tenant;

logObject("req.query", req.query);

Check warning on line 977 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L977

Added line #L977 was not covered by tests

const result = await createDeviceUtil.getUptimeStatistics(request, next);

Check warning on line 979 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L979

Added line #L979 was not covered by tests

if (isEmpty(result) || res.headersSent) {
return;

Check warning on line 982 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L981-L982

Added lines #L981 - L982 were not covered by tests
}

if (result.success === true) {
const status = result.status ? result.status : httpStatus.OK;
return res.status(status).json({

Check warning on line 987 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L985-L987

Added lines #L985 - L987 were not covered by tests
success: true,
message: result.message,
uptime_statistics: result.data,
});
} else if (result.success === false) {
const status = result.status

Check warning on line 993 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L992-L993

Added lines #L992 - L993 were not covered by tests
? result.status
: httpStatus.INTERNAL_SERVER_ERROR;
return res.status(status).json({

Check warning on line 996 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L996

Added line #L996 was not covered by tests
success: false,
message: result.message,
errors: result.errors ? result.errors : { message: "" },
});
}
} catch (error) {
logger.error(`🐛🐛 Internal Server Error ${error.message}`);
next(

Check warning on line 1004 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L1003-L1004

Added lines #L1003 - L1004 were not covered by tests
new HttpError(
"Internal Server Error",
httpStatus.INTERNAL_SERVER_ERROR,
{ message: error.message }
)
);
return;

Check warning on line 1011 in src/device-registry/controllers/create-device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/controllers/create-device.js#L1011

Added line #L1011 was not covered by tests
}
},
};

module.exports = device;
243 changes: 243 additions & 0 deletions src/device-registry/models/Device.js
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,249 @@
},
};

/***
*
* To use the function (getUptimeStatistics) below
* const uptimeStats = await DeviceModel(tenant).getUptimeStatistics({
devices: ['device1Id', 'device2Id'],
startDate: '2023-01-01',
endDate: '2023-12-31',
timeFrame: 'monthly',
networkFilter: 'airqo'
});
*/

deviceSchema.statics.getUptimeStatistics = async function({
devices = [],
startDate = new Date(new Date().setDate(new Date().getDate() - 14)),
endDate = new Date(),
timeFrame,
networkFilter,
} = {}) {
const pipeline = [];

Check warning on line 736 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L736

Added line #L736 was not covered by tests

// Match stage
const matchStage = {};
if (devices.length > 0) {
matchStage._id = { $in: devices };

Check warning on line 741 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L739-L741

Added lines #L739 - L741 were not covered by tests
}
if (networkFilter) {
matchStage.network = networkFilter;

Check warning on line 744 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L743-L744

Added lines #L743 - L744 were not covered by tests
}
Comment on lines +739 to +745
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


// Add date range to match stage if provided
if (startDate && endDate) {
matchStage.createdAt = {

Check warning on line 749 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L748-L749

Added lines #L748 - L749 were not covered by tests
$gte: new Date(startDate),
$lte: new Date(endDate),
};
}
Comment on lines +749 to +753
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 startDate is provided: Filter records from startDate onwards.
  • Only endDate is provided: Filter records up to endDate.

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


logObject("matchStage", matchStage);
pipeline.push({ $match: matchStage });

Check warning on line 756 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L755-L756

Added lines #L755 - L756 were not covered by tests

// Group by device and calculate uptime
pipeline.push({

Check warning on line 759 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L759

Added line #L759 was not covered by tests
$group: {
_id: "$_id",
name: { $first: "$name" },
network: { $first: "$network" },
isOnline: { $first: "$isOnline" },
createdAt: { $first: "$createdAt" },
totalTime: {
$sum: {
$cond: [{ $eq: [true, "$isOnline"] }, 1, 0],
},
},
uptime: {
$sum: {
$cond: [
{
$and: [
{ $eq: [true, "$isOnline"] },
{ $gt: ["$createdAt", startDate] },
],
},
1,
0,
],
},
},
},
});

// Calculate uptime percentage
pipeline.push({

Check warning on line 789 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L789

Added line #L789 was not covered by tests
$project: {
name: 1,
network: 1,
isOnline: 1,
createdAt: 1,
uptimePercentage: {
$multiply: [{ $divide: ["$uptime", "$totalTime"] }, 100],
},
Comment on lines +796 to +797
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
$multiply: [{ $divide: ["$uptime", "$totalTime"] }, 100],
},
uptimePercentage: {
$cond: {
if: { $eq: ["$totalTime", 0] },
then: 0,
else: {
$multiply: [{ $divide: ["$uptime", "$totalTime"] }, 100],
},
},
},

},
});

// Group by network
pipeline.push({

Check warning on line 802 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L802

Added line #L802 was not covered by tests
$group: {
_id: "$network",
devices: { $push: "$$ROOT" },
totalDevices: { $sum: 1 },
avgUptimePercentage: { $avg: "$uptimePercentage" },
},
});

// Calculate online and offline devices based on current status
pipeline.push({

Check warning on line 812 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L812

Added line #L812 was not covered by tests
$project: {
network: "$_id",
totalDevices: 1,
avgUptimePercentage: 1,
devices: 1,
onlineDevices: {
$size: {
$filter: {
input: "$devices",
as: "device",
cond: { $eq: ["$$device.isOnline", true] },
},
},
},
offlineDevices: {
$size: {
$filter: {
input: "$devices",
as: "device",
cond: { $ne: ["$$device.isOnline", true] },
},
},
},
},
});

// Calculate percentages and prepare device names
pipeline.push({

Check warning on line 840 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L840

Added line #L840 was not covered by tests
$project: {
network: 1,
totalDevices: 1,
avgUptimePercentage: 1,
onlineDevices: 1,
offlineDevices: 1,
onlinePercentage: {
$multiply: [{ $divide: ["$onlineDevices", "$totalDevices"] }, 100],
},
offlinePercentage: {
$multiply: [{ $divide: ["$offlineDevices", "$totalDevices"] }, 100],
},
onlineDeviceNames: {
$map: {
input: {
$filter: {
input: "$devices",
as: "device",
cond: { $eq: ["$$device.isOnline", true] },
},
},
as: "device",
in: "$$device.name",
},
},
offlineDeviceNames: {
$map: {
input: {
$filter: {
input: "$devices",
as: "device",
cond: { $ne: ["$$device.isOnline", true] },
},
},
as: "device",
in: "$$device.name",
},
},
},
});

// Apply time frame aggregation if specified
if (timeFrame) {
pipeline.push({

Check warning on line 884 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L883-L884

Added lines #L883 - L884 were not covered by tests
$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" },
},
});
pipeline.push({

Check warning on line 921 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L921

Added line #L921 was not covered by tests
$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"] },
},
},
},
});
}

// Sort by network and timeFrame if applicable
pipeline.push({

Check warning on line 950 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L950

Added line #L950 was not covered by tests
$sort: timeFrame ? { network: 1, timeFrame: 1 } : { network: 1 },
});

// Execute the aggregation pipeline
const result = await this.aggregate(pipeline);

Check warning on line 955 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L955

Added line #L955 was not covered by tests

return result;

Check warning on line 957 in src/device-registry/models/Device.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/models/Device.js#L957

Added line #L957 was not covered by tests
};
Comment on lines +955 to +958
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
};


const DeviceModel = (tenant) => {
try {
let devices = mongoose.model("devices");
Expand Down
52 changes: 52 additions & 0 deletions src/device-registry/routes/v2/devices.js
Original file line number Diff line number Diff line change
Expand Up @@ -1763,4 +1763,56 @@
]),
deviceController.generateQRCode
);

/***
* get network and device statistics
*/
router.get(
"/uptime-stats",
oneOf([
query("tenant")
.optional()
.notEmpty()
.withMessage("tenant cannot be empty if provided")
.trim()
.toLowerCase()
.isIn(constants.NETWORKS)
.withMessage("the tenant value is not among the expected ones"),
query("devices")
.optional()
.isString()
.withMessage("devices should be a comma-separated list of device IDs")
.custom((value) => {
if (value) {
return value

Check warning on line 1787 in src/device-registry/routes/v2/devices.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/routes/v2/devices.js#L1786-L1787

Added lines #L1786 - L1787 were not covered by tests
.split(",")
.every((device) => /^[a-zA-Z0-9-_]+$/.test(device.trim()));

Check warning on line 1789 in src/device-registry/routes/v2/devices.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/routes/v2/devices.js#L1789

Added line #L1789 was not covered by tests
}
return true;

Check warning on line 1791 in src/device-registry/routes/v2/devices.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/routes/v2/devices.js#L1791

Added line #L1791 was not covered by tests
})
.withMessage("each device id should be a valid alphanumeric string"),
query("startDate")
.optional()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"startDate must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)"
),
query("endDate")
.optional()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"endDate must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)"
),
query("timeFrame")
.optional()
.isIn(["daily", "weekly", "monthly"])
.withMessage("timeFrame must be either 'daily', 'weekly', or 'monthly'"),
query("networkFilter")
.optional()
.isIn(constants.NETWORKS)
.withMessage("networkFilter must be one of the valid networks"),
]),
deviceController.getUptimeStatistics
);

module.exports = router;
Loading
Loading