Enhance cron job configuration with timezone and retry options #3750
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| }); | ||
| const activeMissingStatusCount = await DeviceModel("airqo").countDocuments({ | ||
| isActive: true, | ||
| status: { $exists: false } || { $eq: null } || { $eq: "" }, |
There was a problem hiding this comment.
Correct the MongoDB query for status conditions
In your MongoDB queries, the use of JavaScript logical OR operators (||) does not function as intended. MongoDB queries require the use of the $or operator to combine multiple conditions.
Apply this diff to fix the query conditions:
For lines 28~ and 49~:
- status: { $exists: false } || { $eq: null } || { $eq: "" },
+ $or: [
+ { status: { $exists: false } },
+ { status: null },
+ { status: "" }
+ ],This change ensures that the query correctly identifies documents where the status field is missing, null, or an empty string.
Also applies to: 49-49
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/device-registry/bin/jobs/v2-check-network-status-job.js (1)
119-121: Enhance error logging with additional contextIncluding the error's stack trace is helpful. Consider adding more contextual information or using a standardized error logging format to aid in debugging.
For example, you might include the function name or additional details about the failed operation.
| const moment = require("moment-timezone"); | ||
| const TIMEZONE = moment.tz.guess(); | ||
| const UPTIME_THRESHOLD = 35; | ||
| const moment = require("moment-timezone"); // Keep using Moment.js |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider replacing Moment.js with a modern date-time library
Moment.js is in maintenance mode and has been deprecated. It's recommended to use modern alternatives like Luxon or date-fns for improved performance and future support.
Apply this diff to replace Moment.js with Luxon:
-const moment = require("moment-timezone"); // Keep using Moment.js
+const { DateTime } = require("luxon");And update the usage at line 137:
- const currentTime = moment.tz(TIMEZONE).format('YYYY-MM-DD HH:mm:ss');
+ const currentTime = DateTime.now().setZone(TIMEZONE).toFormat('yyyy-MM-dd HH:mm:ss');Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Thanks @Istiak-A-Tashrif , this is great and very helpful.
I have just left a few comments for your review. :)
| const ACTIVE_STATUS_THRESHOLD = 0; | ||
| const { logText, logObject } = require("@utils/log"); | ||
|
|
||
| // Configure timezone (e.g., 'UTC') | ||
| const TIMEZONE = 'UTC'; // You can change this to your desired timezone |
There was a problem hiding this comment.
Thanks for this!
Can we consider using moment.tz.guess() for this? More like:
const TIMEZONE = moment.tz.guess();
This is just to ensure consistent and accurate timezone handling.
| scheduled: true, | ||
| timezone: TIMEZONE, // Set the desired timezone |
There was a problem hiding this comment.
Sweet! I like the timezone aspect, much welcome. Thanks @Istiak-A-Tashrif
|
i have modified the timezone. please check it out snd accept for hacktoberfest. |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/device-registry/bin/jobs/v2-check-network-status-job.js (1 hunks)
🔇 Additional comments (1)
src/device-registry/bin/jobs/v2-check-network-status-job.js (1)
124-134: Well-implemented error handling and retry mechanismThe error handling implementation is robust with:
- Detailed error logging with stack traces
- Proper retry mechanism with delays
- Clear maximum retry limit handling
| const moment = require("moment-timezone"); // Keep using Moment.js | ||
| const ACTIVE_STATUS_THRESHOLD = process.env.ACTIVE_STATUS_THRESHOLD || 0; | ||
| const { logText, logObject } = require("@utils/log"); | ||
|
|
||
| const TIMEZONE = moment.tz.guess(); |
There was a problem hiding this comment.
Consider using explicit timezone configuration
Using moment.tz.guess() might lead to unexpected behavior in server environments. It's better to explicitly configure the timezone through environment variables.
Apply this diff:
-const TIMEZONE = moment.tz.guess();
+const TIMEZONE = process.env.TIMEZONE || 'UTC';📝 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 TIMEZONE = moment.tz.guess(); | |
| const TIMEZONE = process.env.TIMEZONE || 'UTC'; |
| const activeIncorrectStatusResult = await DeviceModel("airqo").aggregate([ | ||
| { | ||
| $match: { | ||
| isActive: true, | ||
| status: { $ne: "deployed" }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| $group: { | ||
| _id: null, | ||
| totalDevices: { $sum: 1 }, | ||
| offlineDevicesCount: { | ||
| $sum: { | ||
| $cond: [{ $eq: ["$isOnline", false] }, 1, 0], | ||
| }, | ||
| { | ||
| $group: { | ||
| _id: "$name", | ||
| }, | ||
| }, | ||
| }, | ||
| ]); | ||
| ]); | ||
|
|
||
| if (result.length === 0 || result[0].totalDevices === 0) { | ||
| logText("No deployed devices found"); | ||
| logger.info("No deployed devices found."); | ||
| return; | ||
| } | ||
|
|
||
| const { totalDevices, offlineDevicesCount } = result[0]; | ||
| const offlinePercentage = (offlineDevicesCount / totalDevices) * 100; | ||
| const activeMissingStatusResult = await DeviceModel("airqo").aggregate([ | ||
| { | ||
| $match: { | ||
| isActive: true, | ||
| $or: [ | ||
| { status: { $exists: false } }, | ||
| { status: null }, | ||
| { status: "" } | ||
| ], | ||
| }, | ||
| }, | ||
| { | ||
| $group: { | ||
| _id: "$name", | ||
| }, | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate aggregation pipelines
The aggregation pipelines for incorrect and missing status devices are very similar and could be combined to reduce database calls.
Consider combining the aggregations:
- const activeIncorrectStatusResult = await DeviceModel("airqo").aggregate([/*...*/]);
- const activeMissingStatusResult = await DeviceModel("airqo").aggregate([/*...*/]);
+ const deviceStatusResults = await DeviceModel("airqo").aggregate([
+ {
+ $match: {
+ isActive: true,
+ $or: [
+ { status: { $ne: "deployed" } },
+ { status: { $exists: false } },
+ { status: null },
+ { status: "" }
+ ]
+ }
+ },
+ {
+ $group: {
+ _id: "$name",
+ statusType: {
+ $cond: [
+ { $or: [
+ { $eq: ["$status", null] },
+ { $eq: ["$status", ""] }
+ ]},
+ "missing",
+ "incorrect"
+ ]
+ }
+ }
+ }
+ ]);
+
+ const activeIncorrectStatusResult = deviceStatusResults.filter(doc => doc.statusType === "incorrect");
+ const activeMissingStatusResult = deviceStatusResults.filter(doc => doc.statusType === "missing");📝 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 activeIncorrectStatusResult = await DeviceModel("airqo").aggregate([ | |
| { | |
| $match: { | |
| isActive: true, | |
| status: { $ne: "deployed" }, | |
| }, | |
| }, | |
| }, | |
| { | |
| $group: { | |
| _id: null, | |
| totalDevices: { $sum: 1 }, | |
| offlineDevicesCount: { | |
| $sum: { | |
| $cond: [{ $eq: ["$isOnline", false] }, 1, 0], | |
| }, | |
| { | |
| $group: { | |
| _id: "$name", | |
| }, | |
| }, | |
| }, | |
| ]); | |
| ]); | |
| if (result.length === 0 || result[0].totalDevices === 0) { | |
| logText("No deployed devices found"); | |
| logger.info("No deployed devices found."); | |
| return; | |
| } | |
| const { totalDevices, offlineDevicesCount } = result[0]; | |
| const offlinePercentage = (offlineDevicesCount / totalDevices) * 100; | |
| const activeMissingStatusResult = await DeviceModel("airqo").aggregate([ | |
| { | |
| $match: { | |
| isActive: true, | |
| $or: [ | |
| { status: { $exists: false } }, | |
| { status: null }, | |
| { status: "" } | |
| ], | |
| }, | |
| }, | |
| { | |
| $group: { | |
| _id: "$name", | |
| }, | |
| }, | |
| ]); | |
| const deviceStatusResults = await DeviceModel("airqo").aggregate([ | |
| { | |
| $match: { | |
| isActive: true, | |
| $or: [ | |
| { status: { $ne: "deployed" } }, | |
| { status: { $exists: false } }, | |
| { status: null }, | |
| { status: "" } | |
| ] | |
| } | |
| }, | |
| { | |
| $group: { | |
| _id: "$name", | |
| statusType: { | |
| $cond: [ | |
| { $or: [ | |
| { $eq: ["$status", null] }, | |
| { $eq: ["$status", ""] } | |
| ]}, | |
| "missing", | |
| "incorrect" | |
| ] | |
| } | |
| } | |
| } | |
| ]); | |
| const activeIncorrectStatusResult = deviceStatusResults.filter(doc => doc.statusType === "incorrect"); | |
| const activeMissingStatusResult = deviceStatusResults.filter(doc => doc.statusType === "missing"); |
| const schedule = '30 */2 * * *'; // At minute 30 of every 2nd hour | ||
| cron.schedule(schedule, () => { | ||
| const currentTime = moment.tz(TIMEZONE).format('YYYY-MM-DD HH:mm:ss'); | ||
| logger.info(`Running job at ${currentTime}`); | ||
| checkActiveStatuses(); | ||
| }, { |
There was a problem hiding this comment.
💡 Codebase verification
Timezone inconsistency detected across cron jobs
I've found an important timezone inconsistency that needs attention:
- Most services use explicit
"Africa/Nairobi"timezone v2-check-network-status-job.jsandv2/v3-store-readings-job.jsuse dynamic timezone detection withmoment.tz.guess()check-active-statuses.jsuses hardcoded'UTC'
These jobs running at "30 */2 * * *" could execute at different times due to these timezone inconsistencies, potentially causing race conditions or unexpected behavior.
Recommendations:
- Standardize timezone configuration across all cron jobs to
"Africa/Nairobi" - Update the following files:
src/device-registry/bin/jobs/v2-check-network-status-job.jssrc/device-registry/bin/jobs/v2-store-readings-job.jssrc/device-registry/bin/jobs/v3-store-readings-job.jssrc/device-registry/bin/jobs/check-active-statuses.js
🔗 Analysis chain
LGTM! Verify cron schedule across timezones
The cron schedule implementation looks good. The job will run every 2 hours at minute 30, with proper timezone handling and logging.
Let's verify the cron schedule usage across the codebase:
Let me verify the timezone handling across these cron jobs to ensure consistency.
These scripts will help us verify if all cron jobs are using consistent timezone handling, as timezone misconfigurations could lead to jobs running at unexpected times.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other cron schedules that might conflict
rg -l "cron.schedule" | xargs rg "(\*/2|\*/1|\*/3) \* \* \*" -A 2
Length of output: 1678
Script:
#!/bin/bash
# Search for timezone configurations in these files
rg -l "cron.schedule" | xargs rg -B2 "TIMEZONE|timezone"
Length of output: 7868
Script:
#!/bin/bash
# Look for timezone imports or configurations
rg "moment.tz|TIMEZONE" $(rg -l "cron.schedule")
Length of output: 2393
Script:
#!/bin/bash
# Check if timezone is defined in environment configs
fd -e json -e js -e env config | xargs rg "TIMEZONE"
Length of output: 52
| const activeIncorrectStatusCount = await DeviceModel("airqo").countDocuments({ | ||
| isActive: true, | ||
| status: { $ne: "deployed" }, | ||
| }); | ||
|
|
||
| const activeMissingStatusCount = await DeviceModel("airqo").countDocuments({ | ||
| isActive: true, | ||
| $or: [ | ||
| { status: { $exists: false } }, | ||
| { status: null }, | ||
| { status: "" } | ||
| ], | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database queries by combining count operations
The separate count queries for incorrect and missing statuses could be combined into a single aggregation pipeline for better performance.
Consider refactoring to:
- const activeIncorrectStatusCount = await DeviceModel("airqo").countDocuments({
- isActive: true,
- status: { $ne: "deployed" },
- });
-
- const activeMissingStatusCount = await DeviceModel("airqo").countDocuments({
- isActive: true,
- $or: [
- { status: { $exists: false } },
- { status: null },
- { status: "" }
- ],
- });
+ const statusCounts = await DeviceModel("airqo").aggregate([
+ {
+ $match: {
+ isActive: true
+ }
+ },
+ {
+ $group: {
+ _id: null,
+ incorrectStatus: {
+ $sum: {
+ $cond: [
+ { $and: [
+ { $ne: ["$status", "deployed"] },
+ { $ne: ["$status", null] },
+ { $ne: ["$status", ""] }
+ ]},
+ 1,
+ 0
+ ]
+ }
+ },
+ missingStatus: {
+ $sum: {
+ $cond: [
+ { $or: [
+ { $eq: ["$status", null] },
+ { $eq: ["$status", ""] }
+ ]},
+ 1,
+ 0
+ ]
+ }
+ }
+ }
+ }
+ ]);
+ const { incorrectStatus: activeIncorrectStatusCount, missingStatus: activeMissingStatusCount } = statusCounts[0] || { incorrectStatus: 0, missingStatus: 0 };📝 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 activeIncorrectStatusCount = await DeviceModel("airqo").countDocuments({ | |
| isActive: true, | |
| status: { $ne: "deployed" }, | |
| }); | |
| const activeMissingStatusCount = await DeviceModel("airqo").countDocuments({ | |
| isActive: true, | |
| $or: [ | |
| { status: { $exists: false } }, | |
| { status: null }, | |
| { status: "" } | |
| ], | |
| }); | |
| const statusCounts = await DeviceModel("airqo").aggregate([ | |
| { | |
| $match: { | |
| isActive: true | |
| } | |
| }, | |
| { | |
| $group: { | |
| _id: null, | |
| incorrectStatus: { | |
| $sum: { | |
| $cond: [ | |
| { $and: [ | |
| { $ne: ["$status", "deployed"] }, | |
| { $ne: ["$status", null] }, | |
| { $ne: ["$status", ""] } | |
| ]}, | |
| 1, | |
| 0 | |
| ] | |
| } | |
| }, | |
| missingStatus: { | |
| $sum: { | |
| $cond: [ | |
| { $or: [ | |
| { $eq: ["$status", null] }, | |
| { $eq: ["$status", ""] } | |
| ]}, | |
| 1, | |
| 0 | |
| ] | |
| } | |
| } | |
| } | |
| } | |
| ]); | |
| const { incorrectStatus: activeIncorrectStatusCount, missingStatus: activeMissingStatusCount } = statusCounts[0] || { incorrectStatus: 0, missingStatus: 0 }; |
added timezone configuration and retry mechanisms to the cron job in src/device-registry/bin/jobs/check-active-statuses.js for improved reliability.
Summary by CodeRabbit
New Features
Bug Fixes
Chores