just reducing logging #3596
just reducing logging #3596
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #3596 +/- ##
========================================
Coverage 29.61% 29.61%
========================================
Files 184 184
Lines 25023 25022 -1
Branches 3327 3327
========================================
Hits 7411 7411
+ Misses 17485 17484 -1
Partials 127 127
|
|
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: 1
🧹 Outside diff range and nitpick comments (3)
src/device-registry/utils/create-event.js (3)
Line range hint
2516-2610: Refactor theinsertfunction for improved maintainability and error handlingThe
insertfunction is quite complex and handles multiple responsibilities. Consider the following improvements:
Break down the function into smaller, more focused functions. This will improve readability and make the code easier to maintain and test.
Replace
console.logstatements with proper logging using theloggerobject. This is more suitable for production environments and provides consistent logging across the application.Implement more robust error handling. Consider creating custom error types for different scenarios and handle them specifically.
Consider using database transactions to ensure data consistency, especially when performing multiple update operations.
Example refactoring:
async function insert(tenant, measurements, next) { try { const transformedMeasurements = await transformMeasurements(measurements); const insertionResult = await insertMeasurements(tenant, transformedMeasurements); return handleInsertionResult(insertionResult); } catch (error) { logger.error(`🐛🐛 Internal Server Error ${error.message}`); throw new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { message: error.message }); } } async function transformMeasurements(measurements) { // ... implementation } async function insertMeasurements(tenant, measurements) { // ... implementation using transactions if possible } function handleInsertionResult(result) { // ... implementation }This refactoring separates concerns, improves error handling, and makes the code more modular and easier to maintain.
Line range hint
2558-2564: Optimize and improve robustness of database operationsThe current implementation uses a single
updateOneoperation with upsert. While this works, it could be optimized and made more robust:
- Consider using
bulkWritefor better performance when inserting multiple documents:const bulkOps = measurements.map(measurement => ({ updateOne: { filter: { day: measurement.day, site_id: measurement.site_id, device_id: measurement.device_id, nValues: { $lt: parseInt(constants.N_VALUES || 500) }, $or: [ { "values.time": { $ne: measurement.time } }, { "values.device": { $ne: measurement.device } }, { "values.frequency": { $ne: measurement.frequency } }, { "values.device_id": { $ne: measurement.device_id } }, { "values.site_id": { $ne: measurement.site_id } }, { day: { $ne: measurement.day } }, ], }, update: { $push: { values: measurement }, $min: { first: measurement.time }, $max: { last: measurement.time }, $inc: { nValues: 1 }, }, upsert: true } })); const result = await EventModel(tenant).bulkWrite(bulkOps);
- Implement retry logic for database operations to handle temporary network issues or database unavailability:
const result = await asyncRetry( async (bail) => { try { return await EventModel(tenant).bulkWrite(bulkOps); } catch (error) { if (error.name === 'MongoError' && error.code === 11000) { // Duplicate key error, don't retry bail(error); } throw error; // Retry for other errors } }, { retries: 3, factor: 2, minTimeout: 1000, maxTimeout: 5000, } );These changes will improve the performance for bulk inserts and add resilience to temporary database issues.
Line range hint
2572-2610: Standardize the response structure for consistencyThe current implementation returns different response structures based on the operation's success or failure. Standardizing this would improve consistency and make it easier for clients to handle responses. Consider the following structure:
function createResponse(success, message, data = null, errors = []) { return { success, message, data, errors, status: success ? httpStatus.OK : httpStatus.INTERNAL_SERVER_ERROR }; } // Usage: if (errors.length > 0 && isEmpty(eventsAdded)) { logger.error("API: failed to store measurements, most likely DB cast errors or duplicate records"); return createResponse(false, "Finished the operation with some errors", null, errors); } else { logger.info("API: successfully added the events"); return createResponse(true, "Successfully added the events", { eventsAdded, eventsRejected }, errors); }This approach:
- Provides a consistent response structure.
- Always includes a
successboolean for easy checking.- Includes
dataanderrorsfields, even if they're empty.- Uses appropriate HTTP status codes.
Additionally, consider categorizing errors (e.g., validation errors, database errors) to provide more structured error information to the client.
| console.log( | ||
| "API: failed to store measurements, most likely DB cast errors or duplicate records" |
There was a problem hiding this comment.
Improve logging consistency and error handling
The current implementation mixes console.log and logger.error for logging. Standardize on using the logger object for all logging to ensure consistency and better control over log levels.
Replace:
console.log("API: failed to store measurements, most likely DB cast errors or duplicate records");and
console.log("API: successfully added the events");with:
logger.error("API: failed to store measurements, most likely DB cast errors or duplicate records");and
logger.info("API: successfully added the events");Additionally, consider adding more granular error handling. Instead of catching all errors in a single try-catch block, handle specific error types separately. This will allow for more precise error messages and potentially different handling strategies for different types of errors.
Example:
try {
// ... existing code
} catch (error) {
if (error instanceof MongoError) {
logger.error(`Database error: ${error.message}`);
// Handle database-specific errors
} else if (error instanceof ValidationError) {
logger.error(`Validation error: ${error.message}`);
// Handle validation errors
} else {
logger.error(`Unexpected error: ${error.message}`);
// Handle other types of errors
}
next(new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { message: error.message }));
}This approach provides more detailed error logging and allows for more specific error handling strategies.
Also applies to: 2564-2571
Description
just reducing logging for failed measurement insertion
Summary by CodeRabbit