Refactor Data Processing Job for Improved Efficiency and Maintainability #3606
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #3606 +/- ##
===========================================
- Coverage 27.05% 27.04% -0.01%
===========================================
Files 146 146
Lines 21336 21346 +10
Branches 273 273
===========================================
+ Hits 5772 5774 +2
- Misses 15564 15572 +8
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a scheduled job that processes and stores device readings every 30 minutes using the 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
CodeRabbit Configuration File (
|
|
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: 4
🧹 Outside diff range and nitpick comments (4)
src/device-registry/bin/jobs/test/ut_v3-store-readings-job.js (3)
31-94: Comprehensive test coverage for fetchAndStoreDataIntoReadingsModelThe tests for this function cover the main scenarios effectively: successful processing, handling of no events, and error handling during processing. This is a solid foundation for ensuring the reliability of this critical function.
To further enhance the test suite, consider adding a test for the scenario where the EventModel.fetch call fails. This would ensure that the function handles database connectivity issues gracefully.
Here's a suggested additional test:
it("should handle errors during event fetching", async () => { const fetchStub = sinon.stub(EventModel, "fetch").returns( Promise.reject(new Error("Database error")) ); await fetchAndStoreDataIntoReadingsModel(); expect(loggerStub.calledWith(sinon.match(/Error fetching events/))).to.be.true; });This test would verify that the function logs an appropriate error message when the event fetching process fails.
96-140: Solid foundation for processDocument tests with room for expansionThe current tests for processDocument provide a good starting point, covering successful processing and logging of missing details. However, to ensure robustness, consider adding the following tests:
- Test error handling when DeviceModel.findOne or SiteModel.findOne fails.
- Test the scenario where ReadingModel.updateOne fails.
- Verify that the correct data is passed to ReadingModel.updateOne.
Here's an example of an additional test you could add:
it("should handle errors when updating ReadingModel", async () => { const mockDoc = { device_id: "device1", site_id: "site1", time: new Date(), }; sinon.stub(DeviceModel, "findOne").resolves({ lastActive: new Date() }); sinon.stub(SiteModel, "findOne").resolves({ lastActive: new Date() }); sinon.stub(DeviceModel, "updateOne").resolves(); sinon.stub(SiteModel, "updateOne").resolves(); const updateOneStub = sinon.stub(ReadingModel, "updateOne").rejects(new Error("Database error")); await processDocument(mockDoc); expect(loggerStub.calledWith(sinon.match(/Error updating ReadingModel/))).to.be.true; });This test would ensure that the function handles errors during the ReadingModel update process gracefully.
199-225: Solid fetchAllData tests with opportunity for enhancementThe current tests for fetchAllData provide good coverage of the basic scenarios: successful data retrieval and error handling. This is a strong foundation for ensuring the reliability of this function.
To further strengthen the test suite, consider adding a test that verifies the pagination logic more explicitly. This could involve checking that the function correctly handles multiple pages of data and combines them into a single result set.
Here's a suggested additional test to verify pagination:
it("should correctly paginate through all data", async () => { const mockEntities1 = [{ _id: "entity1" }, { _id: "entity2" }]; const mockEntities2 = [{ _id: "entity3" }]; const findStub = sinon.stub(DeviceModel, "find"); findStub.onFirstCall().returns({ limit: () => ({ skip: () => Promise.resolve(mockEntities1), }), }); findStub.onSecondCall().returns({ limit: () => ({ skip: () => Promise.resolve(mockEntities2), }), }); findStub.onThirdCall().returns({ limit: () => ({ skip: () => Promise.resolve([]), }), }); const result = await fetchAllData(DeviceModel); expect(findStub.callCount).to.equal(3); expect(result).to.deep.equal([...mockEntities1, ...mockEntities2]); });This test ensures that the function correctly handles multiple pages of data and combines them into a single result set, verifying the pagination logic.
src/device-registry/bin/jobs/v3-store-readings-job.js (1)
55-55: Remove Unused VariableupdateResultThe variable
updateResultis assigned but never utilized. If the result ofModel.updateOneis not needed, you can remove the variable assignment to streamline the code.Apply this diff:
- const updateResult = await Model.updateOne(filter, updateData); + await Model.updateOne(filter, updateData);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 55-55: src/device-registry/bin/jobs/v3-store-readings-job.js#L55
Added line #L55 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/device-registry/bin/jobs/test/ut_v3-store-readings-job.js (1 hunks)
- src/device-registry/bin/jobs/v3-store-readings-job.js (1 hunks)
- src/device-registry/bin/server.js (1 hunks)
🧰 Additional context used
🪛 Biome
src/device-registry/bin/jobs/v3-store-readings-job.js
[error] 106-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: codecov/patch
src/device-registry/bin/jobs/v3-store-readings-job.js
[warning] 21-26: src/device-registry/bin/jobs/v3-store-readings-job.js#L21-L26
Added lines #L21 - L26 were not covered by tests
[warning] 31-31: src/device-registry/bin/jobs/v3-store-readings-job.js#L31
Added line #L31 was not covered by tests
[warning] 33-34: src/device-registry/bin/jobs/v3-store-readings-job.js#L33-L34
Added lines #L33 - L34 were not covered by tests
[warning] 36-36: src/device-registry/bin/jobs/v3-store-readings-job.js#L36
Added line #L36 was not covered by tests
[warning] 39-39: src/device-registry/bin/jobs/v3-store-readings-job.js#L39
Added line #L39 was not covered by tests
[warning] 42-42: src/device-registry/bin/jobs/v3-store-readings-job.js#L42
Added line #L42 was not covered by tests
[warning] 45-49: src/device-registry/bin/jobs/v3-store-readings-job.js#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 55-55: src/device-registry/bin/jobs/v3-store-readings-job.js#L55
Added line #L55 was not covered by tests
[warning] 57-57: src/device-registry/bin/jobs/v3-store-readings-job.js#L57
Added line #L57 was not covered by tests
[warning] 62-62: src/device-registry/bin/jobs/v3-store-readings-job.js#L62
Added line #L62 was not covered by tests
[warning] 65-65: src/device-registry/bin/jobs/v3-store-readings-job.js#L65
Added line #L65 was not covered by tests
[warning] 69-70: src/device-registry/bin/jobs/v3-store-readings-job.js#L69-L70
Added lines #L69 - L70 were not covered by tests
[warning] 72-72: src/device-registry/bin/jobs/v3-store-readings-job.js#L72
Added line #L72 was not covered by tests
[warning] 74-75: src/device-registry/bin/jobs/v3-store-readings-job.js#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 84-84: src/device-registry/bin/jobs/v3-store-readings-job.js#L84
Added line #L84 was not covered by tests
[warning] 87-88: src/device-registry/bin/jobs/v3-store-readings-job.js#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 97-97: src/device-registry/bin/jobs/v3-store-readings-job.js#L97
Added line #L97 was not covered by tests
[warning] 101-101: src/device-registry/bin/jobs/v3-store-readings-job.js#L101
Added line #L101 was not covered by tests
[warning] 104-107: src/device-registry/bin/jobs/v3-store-readings-job.js#L104-L107
Added lines #L104 - L107 were not covered by tests
[warning] 111-111: src/device-registry/bin/jobs/v3-store-readings-job.js#L111
Added line #L111 was not covered by tests
[warning] 121-122: src/device-registry/bin/jobs/v3-store-readings-job.js#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 124-124: src/device-registry/bin/jobs/v3-store-readings-job.js#L124
Added line #L124 was not covered by tests
[warning] 127-128: src/device-registry/bin/jobs/v3-store-readings-job.js#L127-L128
Added lines #L127 - L128 were not covered by tests
[warning] 135-135: src/device-registry/bin/jobs/v3-store-readings-job.js#L135
Added line #L135 was not covered by tests
[warning] 141-141: src/device-registry/bin/jobs/v3-store-readings-job.js#L141
Added line #L141 was not covered by tests
[warning] 144-144: src/device-registry/bin/jobs/v3-store-readings-job.js#L144
Added line #L144 was not covered by tests
[warning] 146-146: src/device-registry/bin/jobs/v3-store-readings-job.js#L146
Added line #L146 was not covered by tests
[warning] 151-152: src/device-registry/bin/jobs/v3-store-readings-job.js#L151-L152
Added lines #L151 - L152 were not covered by tests
[warning] 155-155: src/device-registry/bin/jobs/v3-store-readings-job.js#L155
Added line #L155 was not covered by tests
[warning] 158-158: src/device-registry/bin/jobs/v3-store-readings-job.js#L158
Added line #L158 was not covered by tests
🔇 Additional comments (7)
src/device-registry/bin/server.js (3)
27-28: Overall, these changes significantly enhance system architecture.The updates to both job imports and API versioning demonstrate a commitment to evolving the system architecture in a structured manner. These changes align perfectly with the PR's objectives of improving efficiency and maintainability.
A few final thoughts:
- Consider adding a CHANGELOG.md file (if not already present) to document these significant version changes.
- Ensure that the team is aware of these changes, particularly the API version update, as it may impact other parts of the system or external integrations.
- Update any relevant documentation, including API docs and README files, to reflect these changes.
Great work on this refactor! The attention to versioning and modular structure will certainly pay dividends in long-term maintainability.
Also applies to: 72-72
Line range hint
72-72: Commendable API version update, enhancing system evolution.The transition from
/api/v1/devicesto/api/v2/devicesis a positive step towards maintaining a clear versioning strategy for your API. This change aligns well with the PR's focus on improving maintainability.Key points to address:
- Ensure that all clients and documentation referencing the devices API are updated to reflect this new version.
- Consider adding a brief comment explaining the major changes or improvements in v2 compared to v1.
- It's crucial to maintain backwards compatibility or provide a deprecation plan for v1 if it's being completely replaced.
Let's verify the implementation of the new routes and check for any lingering v1 references:
#!/bin/bash # Verify v2 routes implementation and check for v1 references echo "Checking v2 routes implementation:" cat $(fd -t f v2.js -d 1 routes) 2>/dev/null || echo "v2 routes file not found" echo -e "\nChecking for any remaining v1 device API references:" rg -i "api/v1/devices" --type js
27-28: Excellent update to job imports, aligning with refactoring goals.The introduction of
v3-store-readings-jobandv2-check-network-status-jobdemonstrates a thoughtful evolution of the data processing mechanism. This change aligns well with the PR's objective of improving efficiency and maintainability.A few points to consider:
- The version increments (v3 and v2) suggest significant improvements. It would be beneficial to briefly document the key enhancements in these new versions, perhaps in the PR description or in comments within the respective job files.
- Ensure that any dependencies or configurations related to these jobs have been updated accordingly throughout the codebase.
- Consider adding a brief comment above these imports explaining their purpose and any critical changes from previous versions.
To ensure a smooth transition, let's verify the implementation of these new job files:
src/device-registry/bin/jobs/test/ut_v3-store-readings-job.js (3)
1-29: Excellent test setup and organization!The initial setup demonstrates a well-structured approach to unit testing. The use of Sinon for stubbing, particularly for logging functions, is a great practice that will help isolate the tests from side effects. The beforeEach and afterEach hooks ensure a clean slate for each test, which is crucial for maintaining test independence.
142-197: Excellent test coverage for updateEntityStatus functionThe tests for the updateEntityStatus function are well-structured and comprehensive. They cover the three main scenarios:
- Successful update of an entity's status
- Handling of a non-existent entity
- Graceful error handling during the update process
This thorough approach ensures that the function behaves correctly under various conditions, which is crucial for maintaining the reliability of the system. The use of Sinon stubs for mocking database operations is particularly commendable, as it allows for isolated testing of the function's logic.
1-225: Excellent test suite with room for minor enhancementsThis unit test file for the v3-store-readings-job demonstrates a thorough and well-structured approach to testing. The use of Mocha, Chai, and Sinon shows adherence to best practices in JavaScript testing.
Key strengths:
- Comprehensive coverage of main functions
- Good use of mocking and stubbing for isolation
- Clear test descriptions and organization
Suggestions for improvement:
- Add tests for database connectivity issues in fetchAndStoreDataIntoReadingsModel
- Expand error handling tests in processDocument
- Include more explicit pagination testing in fetchAllData
Overall, this is a solid foundation for ensuring the reliability and maintainability of the v3-store-readings-job. The suggested additions would further enhance the robustness of the test suite.
src/device-registry/bin/jobs/v3-store-readings-job.js (1)
259-260: Avoid Logging Full Error Objects to Prevent Sensitive Data ExposureLogging the entire error object using
stringify(error)might inadvertently expose sensitive information, such as stack traces or internal details. Consider logging only essential error messages to maintain security best practices.[security]
Apply this diff to log a concise error message:
- logger.error(`🐛🐛 Internal Server Error ${stringify(error)}`); + logger.error(`🐛🐛 Internal Server Error: ${error.message}`);
| if (error.isFatal) { | ||
| bail(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure error.isFatal Property Exists
The code checks error.isFatal to determine if it should bail out of the retry loop. The standard Error object does not include an isFatal property by default. Ensure that this property is correctly set elsewhere in your application when throwing errors that should halt retries.
If error.isFatal is not defined, non-recoverable errors may not be handled as intended.
| for (const doc of batch) { | ||
| if (doc.device_id) activeDeviceIds.add(doc.device_id); | ||
| if (doc.site_id) activeSiteIds.add(doc.site_id); | ||
| await processDocument(doc); // Call processDocument directly | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Batch Processing with Concurrent Execution
Currently, documents within each batch are processed sequentially, which may lead to longer processing times. To enhance performance, consider processing the documents concurrently using Promise.all.
Apply this diff to process documents concurrently:
- for (const doc of batch) {
- if (doc.device_id) activeDeviceIds.add(doc.device_id);
- if (doc.site_id) activeSiteIds.add(doc.site_id);
- await processDocument(doc);
- }
+ await Promise.all(
+ batch.map(async (doc) => {
+ if (doc.device_id) activeDeviceIds.add(doc.device_id);
+ if (doc.site_id) activeSiteIds.add(doc.site_id);
+ await processDocument(doc);
+ })
+ );📝 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.
| for (const doc of batch) { | |
| if (doc.device_id) activeDeviceIds.add(doc.device_id); | |
| if (doc.site_id) activeSiteIds.add(doc.site_id); | |
| await processDocument(doc); // Call processDocument directly | |
| } | |
| await Promise.all( | |
| batch.map(async (doc) => { | |
| if (doc.device_id) activeDeviceIds.add(doc.device_id); | |
| if (doc.site_id) activeSiteIds.add(doc.site_id); | |
| await processDocument(doc); | |
| }) | |
| ); |
| delete updateDoc._id; | ||
| await ReadingModel("airqo").updateOne(filter, updateDoc, { |
There was a problem hiding this comment.
Avoid Using delete Operator for Performance Reasons
Using the delete operator can adversely affect performance because it alters the object's structure, potentially causing de-optimization in JavaScript engines. Instead of deleting the _id property, consider excluding it when creating the updateDoc object or setting it to undefined.
Option 1 (Preferred): Exclude _id when creating updateDoc:
- const updateDoc = { ...doc, time: docTime.toDate() };
- delete updateDoc._id;
+ const { _id, ...docWithoutId } = doc;
+ const updateDoc = { ...docWithoutId, time: docTime.toDate() };Option 2: Set _id to undefined:
const updateDoc = { ...doc, time: docTime.toDate() };
- delete updateDoc._id;
+ updateDoc._id = undefined;Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 106-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: codecov/patch
[warning] 104-107: src/device-registry/bin/jobs/v3-store-readings-job.js#L104-L107
Added lines #L104 - L107 were not covered by tests
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/device-registry/bin/jobs/v3-store-readings-job.js (3)
1-19: Well-structured imports and constantsThe imports and constant declarations are well-organized and appropriate for the job's requirements. The use of environment-specific logging and utility functions demonstrates good modularity.
To further enhance readability, consider grouping related imports together. For example, you could group all model imports consecutively.
Consider reordering the imports as follows:
const constants = require("@config/constants"); const log4js = require("log4js"); +const cron = require("node-cron"); +const moment = require("moment-timezone"); +const asyncRetry = require("async-retry"); const EventModel = require("@models/Event"); const DeviceModel = require("@models/Device"); const SiteModel = require("@models/Site"); const ReadingModel = require("@models/Reading"); const { logText, logObject } = require("@utils/log"); const stringify = require("@utils/stringify"); -const asyncRetry = require("async-retry"); const generateFilter = require("@utils/generate-filter"); -const cron = require("node-cron"); -const moment = require("moment-timezone");
20-29: Effective logging of document detailsThe
logDocumentDetailsfunction is well-implemented, providing crucial information for debugging and monitoring. The use of optional chaining (||) to handle potentially undefined properties is a good practice.To maintain consistency with modern JavaScript practices, consider using nullish coalescing (
??) instead of logical OR (||) for the default values. This would only use the default value when the property isnullorundefined, not for other falsy values like an empty string.Consider updating the default value assignments:
- const deviceId = doc.device_id || "N/A"; - const device = doc.device || "N/A"; - const time = doc.time || "N/A"; - const siteId = doc.site_id || "N/A"; - const site = doc.site || "N/A"; + const deviceId = doc.device_id ?? "N/A"; + const device = doc.device ?? "N/A"; + const time = doc.time ?? "N/A"; + const siteId = doc.site_id ?? "N/A"; + const site = doc.site ?? "N/A";🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-26: src/device-registry/bin/jobs/v3-store-readings-job.js#L21-L26
Added lines #L21 - L26 were not covered by tests
1-278: Summary of review findings and recommendationsThe
v3-store-readings-job.jsfile implements a scheduled job for processing and storing device readings. While the overall structure and logic are sound, several areas for improvement have been identified:
- Error handling: Implement more robust error handling and recovery mechanisms throughout the job.
- Performance optimizations: Use bulk operations where possible, especially for updating device and site statuses.
- Code organization: Break down large functions into smaller, more focused ones to improve maintainability and testability.
- Logic corrections: Address the issue in the
isEntityActivefunction to ensure correct activity status determination.- Consistency: Update variable assignments to use modern JavaScript practices like nullish coalescing.
These improvements will enhance the reliability, efficiency, and maintainability of the job. Additionally, consider adding comprehensive unit tests to cover the various functions and edge cases in this file.
Would you like assistance in implementing any of these suggested improvements or in creating unit tests for this job?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-26: src/device-registry/bin/jobs/v3-store-readings-job.js#L21-L26
Added lines #L21 - L26 were not covered by tests
[warning] 31-31: src/device-registry/bin/jobs/v3-store-readings-job.js#L31
Added line #L31 was not covered by tests
[warning] 33-34: src/device-registry/bin/jobs/v3-store-readings-job.js#L33-L34
Added lines #L33 - L34 were not covered by tests
[warning] 36-36: src/device-registry/bin/jobs/v3-store-readings-job.js#L36
Added line #L36 was not covered by tests
[warning] 39-39: src/device-registry/bin/jobs/v3-store-readings-job.js#L39
Added line #L39 was not covered by tests
[warning] 42-42: src/device-registry/bin/jobs/v3-store-readings-job.js#L42
Added line #L42 was not covered by tests
[warning] 45-49: src/device-registry/bin/jobs/v3-store-readings-job.js#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 55-55: src/device-registry/bin/jobs/v3-store-readings-job.js#L55
Added line #L55 was not covered by tests
[warning] 57-57: src/device-registry/bin/jobs/v3-store-readings-job.js#L57
Added line #L57 was not covered by tests
[warning] 62-62: src/device-registry/bin/jobs/v3-store-readings-job.js#L62
Added line #L62 was not covered by tests
[warning] 65-65: src/device-registry/bin/jobs/v3-store-readings-job.js#L65
Added line #L65 was not covered by tests
[warning] 69-70: src/device-registry/bin/jobs/v3-store-readings-job.js#L69-L70
Added lines #L69 - L70 were not covered by tests
[warning] 72-72: src/device-registry/bin/jobs/v3-store-readings-job.js#L72
Added line #L72 was not covered by tests
[warning] 74-75: src/device-registry/bin/jobs/v3-store-readings-job.js#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 84-84: src/device-registry/bin/jobs/v3-store-readings-job.js#L84
Added line #L84 was not covered by tests
[warning] 87-88: src/device-registry/bin/jobs/v3-store-readings-job.js#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 97-97: src/device-registry/bin/jobs/v3-store-readings-job.js#L97
Added line #L97 was not covered by tests
[warning] 101-101: src/device-registry/bin/jobs/v3-store-readings-job.js#L101
Added line #L101 was not covered by tests
[warning] 104-107: src/device-registry/bin/jobs/v3-store-readings-job.js#L104-L107
Added lines #L104 - L107 were not covered by tests
[warning] 111-111: src/device-registry/bin/jobs/v3-store-readings-job.js#L111
Added line #L111 was not covered by tests
[warning] 121-122: src/device-registry/bin/jobs/v3-store-readings-job.js#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 124-124: src/device-registry/bin/jobs/v3-store-readings-job.js#L124
Added line #L124 was not covered by tests
[warning] 127-128: src/device-registry/bin/jobs/v3-store-readings-job.js#L127-L128
Added lines #L127 - L128 were not covered by tests
[warning] 135-135: src/device-registry/bin/jobs/v3-store-readings-job.js#L135
Added line #L135 was not covered by tests
[warning] 141-141: src/device-registry/bin/jobs/v3-store-readings-job.js#L141
Added line #L141 was not covered by tests
[warning] 144-144: src/device-registry/bin/jobs/v3-store-readings-job.js#L144
Added line #L144 was not covered by tests
[warning] 146-146: src/device-registry/bin/jobs/v3-store-readings-job.js#L146
Added line #L146 was not covered by tests
[warning] 151-152: src/device-registry/bin/jobs/v3-store-readings-job.js#L151-L152
Added lines #L151 - L152 were not covered by tests
[warning] 155-155: src/device-registry/bin/jobs/v3-store-readings-job.js#L155
Added line #L155 was not covered by tests
[warning] 158-158: src/device-registry/bin/jobs/v3-store-readings-job.js#L158
Added line #L158 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/device-registry/bin/jobs/v3-store-readings-job.js (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/device-registry/bin/jobs/v3-store-readings-job.js
[warning] 21-26: src/device-registry/bin/jobs/v3-store-readings-job.js#L21-L26
Added lines #L21 - L26 were not covered by tests
[warning] 31-31: src/device-registry/bin/jobs/v3-store-readings-job.js#L31
Added line #L31 was not covered by tests
[warning] 33-34: src/device-registry/bin/jobs/v3-store-readings-job.js#L33-L34
Added lines #L33 - L34 were not covered by tests
[warning] 36-36: src/device-registry/bin/jobs/v3-store-readings-job.js#L36
Added line #L36 was not covered by tests
[warning] 39-39: src/device-registry/bin/jobs/v3-store-readings-job.js#L39
Added line #L39 was not covered by tests
[warning] 42-42: src/device-registry/bin/jobs/v3-store-readings-job.js#L42
Added line #L42 was not covered by tests
[warning] 45-49: src/device-registry/bin/jobs/v3-store-readings-job.js#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 55-55: src/device-registry/bin/jobs/v3-store-readings-job.js#L55
Added line #L55 was not covered by tests
[warning] 57-57: src/device-registry/bin/jobs/v3-store-readings-job.js#L57
Added line #L57 was not covered by tests
[warning] 62-62: src/device-registry/bin/jobs/v3-store-readings-job.js#L62
Added line #L62 was not covered by tests
[warning] 65-65: src/device-registry/bin/jobs/v3-store-readings-job.js#L65
Added line #L65 was not covered by tests
[warning] 69-70: src/device-registry/bin/jobs/v3-store-readings-job.js#L69-L70
Added lines #L69 - L70 were not covered by tests
[warning] 72-72: src/device-registry/bin/jobs/v3-store-readings-job.js#L72
Added line #L72 was not covered by tests
[warning] 74-75: src/device-registry/bin/jobs/v3-store-readings-job.js#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 84-84: src/device-registry/bin/jobs/v3-store-readings-job.js#L84
Added line #L84 was not covered by tests
[warning] 87-88: src/device-registry/bin/jobs/v3-store-readings-job.js#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 97-97: src/device-registry/bin/jobs/v3-store-readings-job.js#L97
Added line #L97 was not covered by tests
[warning] 101-101: src/device-registry/bin/jobs/v3-store-readings-job.js#L101
Added line #L101 was not covered by tests
[warning] 104-107: src/device-registry/bin/jobs/v3-store-readings-job.js#L104-L107
Added lines #L104 - L107 were not covered by tests
[warning] 111-111: src/device-registry/bin/jobs/v3-store-readings-job.js#L111
Added line #L111 was not covered by tests
[warning] 121-122: src/device-registry/bin/jobs/v3-store-readings-job.js#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 124-124: src/device-registry/bin/jobs/v3-store-readings-job.js#L124
Added line #L124 was not covered by tests
[warning] 127-128: src/device-registry/bin/jobs/v3-store-readings-job.js#L127-L128
Added lines #L127 - L128 were not covered by tests
[warning] 135-135: src/device-registry/bin/jobs/v3-store-readings-job.js#L135
Added line #L135 was not covered by tests
[warning] 141-141: src/device-registry/bin/jobs/v3-store-readings-job.js#L141
Added line #L141 was not covered by tests
[warning] 144-144: src/device-registry/bin/jobs/v3-store-readings-job.js#L144
Added line #L144 was not covered by tests
[warning] 146-146: src/device-registry/bin/jobs/v3-store-readings-job.js#L146
Added line #L146 was not covered by tests
[warning] 151-152: src/device-registry/bin/jobs/v3-store-readings-job.js#L151-L152
Added lines #L151 - L152 were not covered by tests
[warning] 155-155: src/device-registry/bin/jobs/v3-store-readings-job.js#L155
Added line #L155 was not covered by tests
[warning] 158-158: src/device-registry/bin/jobs/v3-store-readings-job.js#L158
Added line #L158 was not covered by tests
🔇 Additional comments (1)
src/device-registry/bin/jobs/v3-store-readings-job.js (1)
160-278: 🛠️ Refactor suggestionOptimize main job function and improve error handling
The
fetchAndStoreDataIntoReadingsModelfunction effectively orchestrates the data processing job. However, there are several areas where we can improve error handling, performance, and code organization:
- Error handling: The function catches errors but doesn't provide a mechanism for retrying the entire job or notifying administrators of failures.
- Performance: The function processes events in batches but updates devices and sites individually, which could be optimized.
- Code organization: The function is quite long and handles multiple concerns, making it harder to maintain and test.
Consider the following improvements:
- Implement a retry mechanism for the entire job and add notifications for critical failures.
- Use bulk operations for updating devices and sites.
- Break down the function into smaller, more focused functions.
Here's a high-level refactor suggestion:
const processEventBatch = async (batch) => { // ... (implement batch processing logic) }; const updateInactiveEntities = async (activeIds, Model, entityType) => { // ... (implement bulk update for inactive entities) }; const fetchAndStoreDataIntoReadingsModel = async () => { try { const filter = generateFilter.fetch({ query: { tenant: "airqo", recent: "yes", metadata: "site_id", active: "yes", brief: "yes", }, }); const allEvents = await fetchAllData(EventModel, filter, {}, 100, true); if (!allEvents || allEvents.length === 0) { logger.warn("No Events found to insert into Readings"); return; } const activeDeviceIds = new Set(); const activeSiteIds = new Set(); // Process events in batches for (let i = 0; i < allEvents.length; i += BATCH_SIZE) { const batch = allEvents.slice(i, i + BATCH_SIZE); await processEventBatch(batch, activeDeviceIds, activeSiteIds); } // Update inactive devices and sites await Promise.all([ updateInactiveEntities(activeDeviceIds, DeviceModel, "Device"), updateInactiveEntities(activeSiteIds, SiteModel, "Site"), ]); logger.info("Data processing job completed successfully"); } catch (error) { logger.error(`Job failed: ${error.message}`); // Implement notification mechanism for critical failures // Consider implementing a retry mechanism here } }; // Set up the cron job const schedule = "30 * * * *"; cron.schedule(schedule, fetchAndStoreDataIntoReadingsModel, { scheduled: true, timezone: TIMEZONE, });This refactored version breaks down the main function into smaller, more manageable pieces. It also suggests implementing a notification mechanism for critical failures and considering a retry mechanism for the entire job.
To ensure the cron job is set up correctly, you can run the following command:
This will list all cron jobs and filter for the one related to this script, allowing you to verify its schedule.
| async function updateEntityStatus(Model, filter, time, entityType) { | ||
| try { | ||
| const entity = await Model.findOne(filter); | ||
| if (entity) { | ||
| const isActive = isEntityActive(entity, time); | ||
| const updateData = { | ||
| lastActive: moment(time) | ||
| .tz(TIMEZONE) | ||
| .toDate(), | ||
| isOnline: isActive, | ||
| }; | ||
| const updateResult = await Model.updateOne(filter, updateData); | ||
| } else { | ||
| logger.warn( | ||
| `🙀🙀 ${entityType} not found with filter: ${stringify(filter)}` | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| logger.error( | ||
| `🐛🐛 Error updating ${entityType}'s status: ${error.message}` | ||
| ); | ||
| logger.error(`🐛🐛 Stack trace: ${error.stack}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize updateEntityStatus function and address isEntityActive dependency
The updateEntityStatus function is well-structured with appropriate error handling. However, there are a few points to consider:
-
It relies on the
isEntityActivefunction, which we previously identified as having incorrect logic. Ensure you update this function after correctingisEntityActive. -
The function performs a find operation followed by an update, which could be optimized into a single operation.
-
The
isActivevariable is calculated but not used in the update operation.
Consider the following optimizations:
- Update the
isEntityActivecall once that function is corrected. - Use
findOneAndUpdateto combine the find and update operations. - Include the
isActivestatus in the update operation.
Here's a suggested refactor:
async function updateEntityStatus(Model, filter, time, entityType) {
try {
const currentTime = moment().tz(TIMEZONE).toDate();
const updateData = {
lastActive: currentTime,
isOnline: currentTime - moment(time).tz(TIMEZONE).toDate() < INACTIVE_THRESHOLD
};
const result = await Model.findOneAndUpdate(
filter,
updateData,
{ new: true, upsert: false }
);
if (!result) {
logger.warn(`🙀🙀 ${entityType} not found with filter: ${stringify(filter)}`);
}
} catch (error) {
logger.error(`🐛🐛 Error updating ${entityType}'s status: ${error.message}`);
logger.error(`🐛🐛 Stack trace: ${error.stack}`);
}
}This refactored version combines the find and update operations, directly calculates the isOnline status, and simplifies the overall structure of the function.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-49: src/device-registry/bin/jobs/v3-store-readings-job.js#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 55-55: src/device-registry/bin/jobs/v3-store-readings-job.js#L55
Added line #L55 was not covered by tests
[warning] 57-57: src/device-registry/bin/jobs/v3-store-readings-job.js#L57
Added line #L57 was not covered by tests
[warning] 62-62: src/device-registry/bin/jobs/v3-store-readings-job.js#L62
Added line #L62 was not covered by tests
[warning] 65-65: src/device-registry/bin/jobs/v3-store-readings-job.js#L65
Added line #L65 was not covered by tests
| async function processDocument(doc) { | ||
| try { | ||
| const docTime = moment(doc.time).tz(TIMEZONE); | ||
|
|
||
| const updatePromises = []; | ||
|
|
||
| if (doc.site_id) { | ||
| updatePromises.push( | ||
| updateEntityStatus( | ||
| SiteModel("airqo"), | ||
| { _id: doc.site_id }, | ||
| docTime.toDate(), | ||
| "Site" | ||
| ) | ||
| ); | ||
| } else { | ||
| logDocumentDetails(doc); | ||
| } | ||
|
|
||
| if (doc.device_id) { | ||
| updatePromises.push( | ||
| updateEntityStatus( | ||
| DeviceModel("airqo"), | ||
| { _id: doc.device_id }, | ||
| docTime.toDate(), | ||
| "Device" | ||
| ) | ||
| ); | ||
| } else { | ||
| logDocumentDetails(doc); | ||
| } | ||
|
|
||
| // Wait for both updates to complete | ||
| await Promise.all(updatePromises); | ||
|
|
||
| // Update Reading | ||
| const filter = { site_id: doc.site_id, time: docTime.toDate() }; | ||
| const { _id, ...docWithoutId } = doc; | ||
| const updateDoc = { ...docWithoutId, time: docTime.toDate() }; | ||
| await ReadingModel("airqo").updateOne(filter, updateDoc, { | ||
| upsert: true, | ||
| }); | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Error processing document: ${error.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and efficiency in processDocument
The processDocument function is well-structured, utilizing promises for concurrent updates of sites and devices. However, there are opportunities to improve error handling and efficiency:
- The function catches errors but doesn't rethrow them, which might mask issues in the calling function.
- The
updatePromisesarray is always created, even when it might remain empty. - The reading update is performed sequentially after the entity updates, which could be parallelized.
Consider the following improvements:
- Rethrow caught errors to allow proper handling in the calling function.
- Create the
updatePromisesarray only when needed. - Include the reading update in the concurrent operations.
Here's a suggested refactor:
async function processDocument(doc) {
const docTime = moment(doc.time).tz(TIMEZONE);
const updatePromises = [];
if (doc.site_id) {
updatePromises.push(
updateEntityStatus(SiteModel("airqo"), { _id: doc.site_id }, docTime.toDate(), "Site")
);
} else {
logDocumentDetails(doc);
}
if (doc.device_id) {
updatePromises.push(
updateEntityStatus(DeviceModel("airqo"), { _id: doc.device_id }, docTime.toDate(), "Device")
);
} else {
logDocumentDetails(doc);
}
const filter = { site_id: doc.site_id, time: docTime.toDate() };
const { _id, ...docWithoutId } = doc;
const updateDoc = { ...docWithoutId, time: docTime.toDate() };
updatePromises.push(
ReadingModel("airqo").updateOne(filter, updateDoc, { upsert: true })
);
try {
await Promise.all(updatePromises);
} catch (error) {
logger.error(`🐛🐛 Error processing document: ${error.message}`);
throw error; // Rethrow the error for proper handling in the calling function
}
}This refactored version combines all updates into a single Promise.all call, potentially improving efficiency. It also rethrows the caught error, allowing for proper error handling in the calling function.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 69-70: src/device-registry/bin/jobs/v3-store-readings-job.js#L69-L70
Added lines #L69 - L70 were not covered by tests
[warning] 72-72: src/device-registry/bin/jobs/v3-store-readings-job.js#L72
Added line #L72 was not covered by tests
[warning] 74-75: src/device-registry/bin/jobs/v3-store-readings-job.js#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 84-84: src/device-registry/bin/jobs/v3-store-readings-job.js#L84
Added line #L84 was not covered by tests
[warning] 87-88: src/device-registry/bin/jobs/v3-store-readings-job.js#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 97-97: src/device-registry/bin/jobs/v3-store-readings-job.js#L97
Added line #L97 was not covered by tests
[warning] 101-101: src/device-registry/bin/jobs/v3-store-readings-job.js#L101
Added line #L101 was not covered by tests
[warning] 104-107: src/device-registry/bin/jobs/v3-store-readings-job.js#L104-L107
Added lines #L104 - L107 were not covered by tests
[warning] 111-111: src/device-registry/bin/jobs/v3-store-readings-job.js#L111
Added line #L111 was not covered by tests
| const fetchAllData = async ( | ||
| Model, | ||
| filter = {}, | ||
| projection = {}, | ||
| pageSize = 100, | ||
| isEventModel = false | ||
| ) => { | ||
| const allData = []; | ||
| let page = 0; | ||
|
|
||
| while (true) { | ||
| let response; | ||
|
|
||
| if (isEventModel) { | ||
| response = await Model("airqo").fetch({ | ||
| ...filter, | ||
| limit: pageSize, | ||
| skip: page * pageSize, | ||
| }); | ||
|
|
||
| // Check if the response is successful and contains data | ||
| if ( | ||
| !response.success || | ||
| !response.data || | ||
| response.data.length === 0 || | ||
| response.data[0].data.length === 0 | ||
| ) { | ||
| break; // Exit loop if no more data | ||
| } | ||
|
|
||
| allData.push(...response.data[0].data); | ||
| } else { | ||
| const entities = await Model("airqo") | ||
| .find(filter, projection) | ||
| .limit(pageSize) | ||
| .skip(page * pageSize); | ||
|
|
||
| if (entities.length === 0) break; // Exit if no more data | ||
| allData.push(...entities); | ||
| } | ||
|
|
||
| page++; | ||
| } | ||
|
|
||
| return allData; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance readability and error handling in fetchAllData
The fetchAllData function effectively handles pagination and includes special logic for the EventModel. However, there are opportunities to improve its readability and error handling:
- The function uses a
while(true)loop, which can be hard to reason about. - Error handling is not explicitly implemented, which could lead to infinite loops or unhandled exceptions.
- The special handling for EventModel makes the function less generic and harder to maintain.
Consider the following improvements:
- Replace the
while(true)loop with a more explicit condition. - Implement proper error handling.
- Consider separating the EventModel logic into a separate function for better separation of concerns.
Here's a suggested refactor:
const fetchAllData = async (
Model,
filter = {},
projection = {},
pageSize = 100,
isEventModel = false
) => {
const allData = [];
let page = 0;
let hasMoreData = true;
while (hasMoreData) {
try {
let response;
if (isEventModel) {
response = await Model("airqo").fetch({
...filter,
limit: pageSize,
skip: page * pageSize,
});
if (
!response.success ||
!response.data ||
response.data.length === 0 ||
response.data[0].data.length === 0
) {
hasMoreData = false;
} else {
allData.push(...response.data[0].data);
}
} else {
const entities = await Model("airqo")
.find(filter, projection)
.limit(pageSize)
.skip(page * pageSize);
if (entities.length === 0) {
hasMoreData = false;
} else {
allData.push(...entities);
}
}
page++;
} catch (error) {
logger.error(`Error fetching data: ${error.message}`);
throw error; // Rethrow to allow handling in the calling function
}
}
return allData;
};This refactored version uses a more explicit loop condition, includes error handling, and maintains the existing functionality. For further improvement, consider creating a separate function for fetching EventModel data to improve the function's cohesion and maintainability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 121-122: src/device-registry/bin/jobs/v3-store-readings-job.js#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 124-124: src/device-registry/bin/jobs/v3-store-readings-job.js#L124
Added line #L124 was not covered by tests
[warning] 127-128: src/device-registry/bin/jobs/v3-store-readings-job.js#L127-L128
Added lines #L127 - L128 were not covered by tests
[warning] 135-135: src/device-registry/bin/jobs/v3-store-readings-job.js#L135
Added line #L135 was not covered by tests
[warning] 141-141: src/device-registry/bin/jobs/v3-store-readings-job.js#L141
Added line #L141 was not covered by tests
[warning] 144-144: src/device-registry/bin/jobs/v3-store-readings-job.js#L144
Added line #L144 was not covered by tests
[warning] 146-146: src/device-registry/bin/jobs/v3-store-readings-job.js#L146
Added line #L146 was not covered by tests
[warning] 151-152: src/device-registry/bin/jobs/v3-store-readings-job.js#L151-L152
Added lines #L151 - L152 were not covered by tests
[warning] 155-155: src/device-registry/bin/jobs/v3-store-readings-job.js#L155
Added line #L155 was not covered by tests
[warning] 158-158: src/device-registry/bin/jobs/v3-store-readings-job.js#L158
Added line #L158 was not covered by tests
| function isEntityActive(entity, time) { | ||
| const inactiveThreshold = INACTIVE_THRESHOLD; | ||
|
|
||
| if (!entity || !entity.lastActive) { | ||
| return false; | ||
| } | ||
| const currentTime = moment() | ||
| .tz(TIMEZONE) | ||
| .toDate(); | ||
| const measurementTime = moment(time) | ||
| .tz(TIMEZONE) | ||
| .toDate(); | ||
| return currentTime - measurementTime < inactiveThreshold; | ||
| } |
There was a problem hiding this comment.
Incorrect logic in isEntityActive function
The current implementation of isEntityActive compares the current time with the measurement time (time parameter) instead of the entity's lastActive timestamp. This could lead to incorrect determinations of an entity's active status.
To fix this issue, we should compare the current time with the entity's lastActive timestamp.
Here's the corrected implementation:
-function isEntityActive(entity, time) {
+function isEntityActive(entity) {
const inactiveThreshold = INACTIVE_THRESHOLD;
if (!entity || !entity.lastActive) {
return false;
}
const currentTime = moment()
.tz(TIMEZONE)
.toDate();
- const measurementTime = moment(time)
+ const lastActiveTime = moment(entity.lastActive)
.tz(TIMEZONE)
.toDate();
- return currentTime - measurementTime < inactiveThreshold;
+ return currentTime - lastActiveTime < inactiveThreshold;
}This change ensures that we're correctly determining if an entity is active based on its last recorded active timestamp.
📝 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.
| function isEntityActive(entity, time) { | |
| const inactiveThreshold = INACTIVE_THRESHOLD; | |
| if (!entity || !entity.lastActive) { | |
| return false; | |
| } | |
| const currentTime = moment() | |
| .tz(TIMEZONE) | |
| .toDate(); | |
| const measurementTime = moment(time) | |
| .tz(TIMEZONE) | |
| .toDate(); | |
| return currentTime - measurementTime < inactiveThreshold; | |
| } | |
| function isEntityActive(entity) { | |
| const inactiveThreshold = INACTIVE_THRESHOLD; | |
| if (!entity || !entity.lastActive) { | |
| return false; | |
| } | |
| const currentTime = moment() | |
| .tz(TIMEZONE) | |
| .toDate(); | |
| const lastActiveTime = moment(entity.lastActive) | |
| .tz(TIMEZONE) | |
| .toDate(); | |
| return currentTime - lastActiveTime < inactiveThreshold; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 31-31: src/device-registry/bin/jobs/v3-store-readings-job.js#L31
Added line #L31 was not covered by tests
[warning] 33-34: src/device-registry/bin/jobs/v3-store-readings-job.js#L33-L34
Added lines #L33 - L34 were not covered by tests
[warning] 36-36: src/device-registry/bin/jobs/v3-store-readings-job.js#L36
Added line #L36 was not covered by tests
[warning] 39-39: src/device-registry/bin/jobs/v3-store-readings-job.js#L39
Added line #L39 was not covered by tests
[warning] 42-42: src/device-registry/bin/jobs/v3-store-readings-job.js#L42
Added line #L42 was not covered by tests
Description
Refactor Data Processing Job for Improved Efficiency and Maintainability
Changes Made
Testing
Affected Services
API Documentation Updated?
Additional Notes
Refactor Data Processing Job for Improved Efficiency and Maintainability
Summary by CodeRabbit
New Features
Bug Fixes
Tests