feat/be/logger improvement #1058
Conversation
WalkthroughThis pull request involves significant changes across multiple files, primarily focusing on enhancing error handling and logging mechanisms. The Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (23)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🪛 Biome
🔇 Additional comments (21)
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 28
🧹 Outside diff range and nitpick comments (27)
Server/middleware/handleErrors.js (2)
8-12: Yo dawg, this structured logging is fire! 🔥The new structured logging format is much better for error tracking and analysis. However, we should consider sanitizing sensitive information from the stack trace before logging.
Consider adding this sanitization helper:
+ const sanitizeStackTrace = (stack) => { + if (!stack) return undefined; + // Remove sensitive info like auth tokens, passwords + return stack.replace(/Bearer\s+[A-Za-z0-9\-._~+/]+=*/g, 'Bearer [REDACTED]') + .replace(/password=([^&]*)/g, 'password=[REDACTED]'); + }; logger.error({ message: message, service: service, method: error.method, - stack: error.stack, + stack: sanitizeStackTrace(error.stack), });
Line range hint
4-14: Mom's spaghetti suggestion: Add request context! 🍝The error logging could benefit from additional request context to help with debugging.
Add these helpful context fields:
const handleErrors = (error, req, res, next) => { const status = error.status || 500; const message = error.message || errorMessages.FRIENDLY_ERROR; const service = error.service || errorMessages.UNKNOWN_SERVICE; logger.error({ message: message, service: service, method: error.method, stack: error.stack, + context: { + url: req.originalUrl, + method: req.method, + requestId: req.id, // Assuming you have request ID middleware + userId: req.user?.id, // If authentication is present + } });Server/db/models/User.js (1)
85-87: Mom's spaghetti suggestion: Add more context to that log! 🍝While the basic logging is good, we could make it even better by adding more context about the initialization.
Consider enhancing the log with additional metadata:
logger.info({ message: "User model initialized", + model: "User", + schema: Object.keys(UserSchema.paths).length + " fields", + timestamps: UserSchema.options.timestamps });Server/middleware/verifySuperAdmin.js (1)
40-45: Yo dawg, we heard you like error messages!The error logging structure looks solid, but you're including both
err.messageanddetailswith similar information. Consider removing thedetailsfield sinceerr.messagealready provides the necessary context.logger.error({ message: err.message, service: SERVICE_NAME, method: "verifySuperAdmin", stack: err.stack, - details: errorMessages.INVALID_AUTH_TOKEN, });Server/controllers/inviteController.js (1)
56-60: Yo! The logging structure is fire, but we can make it even better! 🔥The structured logging implementation looks solid with the essential debug info. However, consider these improvements to make it even more robust:
Here's a suggested enhancement to provide more context for troubleshooting:
logger.error({ message: error.message, service: SERVICE_NAME, method: "issueInvitation", stack: error.stack, + recipient: req.body.email, + level: 'ERROR', }); +// Consider notifying the user about the email delivery issue +res.status(200).json({ + success: true, + msg: "Invite created but email delivery delayed. Please try again later.", + data: inviteToken +});Server/service/emailService.js (1)
Line range hint
1-138: Consider implementing centralized error handling! 🤔While the logging improvements are great, we could enhance error handling across the service by implementing a higher-order function or decorator pattern to handle errors consistently.
Example approach:
const withErrorHandling = (methodName, fn) => async (...args) => { try { return await fn(...args); } catch (error) { logger.error({ message: error.message, service: SERVICE_NAME, method: methodName, stack: error.stack, }); throw error; } }; // Usage: this.loadTemplate = withErrorHandling('loadTemplate', (templateName) => { // ... existing implementation });This would:
- Standardize error handling across all methods
- Reduce code duplication
- Ensure consistent error logging and propagation
Server/index.js (1)
Line range hint
68-76: Mom's spaghetti moment: Let's organize this middleware!While attaching services to the request object works, it would be cleaner to move this middleware to a separate file, like
middleware/attachServices.js.// middleware/attachServices.js export const attachServices = (services) => (req, res, next) => { Object.entries(services).forEach(([key, service]) => { req[key] = service; }); next(); };Server/tests/services/emailService.test.js (3)
178-178: Yo dawg, we need to check that messageId too! 💯While the error message assertion is on point, we should also verify that
messageIdis undefined when the HTML build fails.Add this assertion after the error check:
expect(messageId).to.be.undefined;
190-190: Keep it consistent like mom's spaghetti! 🍝While the error message assertion is correct, let's maintain consistency with other test cases by adding a messageId check.
Add this assertion:
const messageId = await emailService.buildAndSendEmail(...); expect(messageId).to.be.undefined;
Line range hint
211-211: Yo, this empty test case is making me lose myself! 🎵There's an empty test case that needs attention. Either implement it or remove it to maintain test suite integrity.
Consider implementing the test case like this:
it("should log an error if buildHtml fails", async () => { mjml2htmlMock.throws(new Error("Build HTML error")); const messageId = await emailService.buildAndSendEmail( "welcomeEmailTemplate", {}, "[email protected]", "Welcome" ); expect(messageId).to.be.undefined; expect(loggerMock.error.calledOnce).to.be.true; expect(loggerMock.error.getCall(0).args[0].message).to.equal("Build HTML error"); });Server/db/mongo/modules/checkModule.js (1)
82-86: Heads up! The logging structure's missing some secret sauce! 🍝While the warning log is solid, let's keep it consistent with the error logging pattern by adding the filter value in the details field, eh?
Here's how to make it even better:
logger.warn({ message: "invalid filter", service: SERVICE_NAME, method: "getChecksCount", + details: `filter: ${filter}`, });Server/service/jobQueue.js (3)
110-113: Yo dawg, this logging looks tight but could use more sauce!The structured logging format is on point, but consider adding the maintenance window details to provide more context for debugging.
this.logger.info({ message: `Monitor ${monitorId} is in maintenance window`, service: SERVICE_NAME, method: "createWorker", + details: { + windowStart: start, + windowEnd: end, + repeatInterval: window.repeat + } });
203-211: Knees weak, arms heavy, but this error handling needs more context!While catching and logging the error is good, we could use more worker context for better debugging.
await worker.close().catch((error) => { this.logger.error({ message: error.message, service: SERVICE_NAME, method: "scaleWorkers", stack: error.stack, + details: { + workerId: worker.id, + workerStatus: worker.status, + totalWorkers: this.workers.length + } }); });
326-335: Lose yourself in the metrics, but make them more readable!While the metrics logging is functional, the metrics object could be more readable in logs.
this.logger.info({ - message: metrics, + message: 'Queue metrics retrieved', + service: SERVICE_NAME, + method: 'getMetrics', + metrics: { + ...metrics, + timestamp: new Date().toISOString() + } });Server/service/networkService.js (2)
Line range hint
279-280: There's a TODO in his code already - let's not forget it!The TODO comment about fetching hardware data needs to be properly tracked and implemented.
Would you like me to:
- Create a GitHub issue to track this TODO?
- Help implement the actual hardware data fetching functionality?
The implementation could involve using a system monitoring library like
systeminformationfor Node.js.
Line range hint
1-433: Time to clean up these long methods - snap back to reality!The code could benefit from some structural improvements:
- Methods are quite long (especially handlePagespeed and handleHardware)
- Error handling patterns are repeated
- Similar check creation logic is duplicated
Consider:
- Extracting check creation logic into separate methods for each type
- Creating a common error handling utility
- Using a strategy pattern for different check types
Example structure:
class BaseCheckStrategy { async execute(job) { try { const result = await this.performCheck(job); await this.createCheck(result); } catch (error) { await this.handleError(error, job); } } } class PingCheckStrategy extends BaseCheckStrategy { async performCheck(job) { // Ping specific logic } }Server/controllers/authController.js (3)
82-85: Yo! These logging improvements are fire! 🔥 Just one small suggestion.The structured logging format looks great and aligns with the standardization goals. However, for consistency, consider adding the
methodfield to the success log as well.logger.info({ message: successMessages.AUTH_CREATE_USER, service: SERVICE_NAME, + method: "registerUser", details: newUser._id, });Also applies to: 105-109
Line range hint
449-484: Yo dawg, we need to handle these promises with care! 🤔The cascade deletion logic looks solid, but there are a few concerns:
- The Promise.all could fail silently if any operation fails
- No logging for individual deletion operations
- No transaction management for atomic operations
Consider wrapping the operations in a transaction and adding logging:
if (user.role.includes("superadmin")) { + const session = await req.db.startSession(); + try { + await session.startTransaction(); result?.monitors.length > 0 && (await Promise.all( result.monitors.map(async (monitor) => { + logger.info({ + message: "Deleting monitor and associated data", + service: SERVICE_NAME, + method: "deleteUser", + details: { monitorId: monitor._id } + }); await req.jobQueue.deleteJob(monitor); await req.db.deleteChecks(monitor._id); await req.db.deletePageSpeedChecksByMonitorId(monitor._id); await req.db.deleteNotificationsByMonitorId(monitor._id); }) - )); + ).catch(error => { + logger.error({ + message: "Failed to delete monitor data", + service: SERVICE_NAME, + method: "deleteUser", + error: error.message, + stack: error.stack + }); + throw error; + })); await req.db.deleteTeam(user.teamId); await req.db.deleteAllOtherUsers(); await req.db.deleteMonitorsByUserId(user._id); + await session.commitTransaction(); + } catch (error) { + await session.abortTransaction(); + throw error; + } finally { + session.endSession(); + } }
Line range hint
487-495: Mom's spaghetti says we need consistency! 🍝The function works but doesn't follow the new logging standards and message patterns used elsewhere in the file.
Let's make it consistent:
const getAllUsers = async (req, res, next) => { try { const allUsers = await req.db.getAllUsers(req, res); + logger.info({ + message: successMessages.AUTH_GET_ALL_USERS, + service: SERVICE_NAME, + method: "getAllUsers", + }); - res.status(200).json({ success: true, msg: "Got all users", data: allUsers }); + res.status(200).json({ + success: true, + msg: successMessages.AUTH_GET_ALL_USERS, + data: allUsers + }); } catch (error) { next(handleError(error, SERVICE_NAME, "getAllUsersController")); } };Also, add
AUTH_GET_ALL_USERSto your messages utility if it's not already there.Server/tests/services/networkService.test.js (1)
791-793: Consider adding error type verification.While the test correctly verifies the error message for unknown job types, consider also verifying that the error is of the expected type for more thorough testing.
expect(loggerMock.error.firstCall.args[0].message).to.equal( "Unsupported type: unknown" ); +expect(loggerMock.error.firstCall.args[0]).to.be.instanceof(Error);Server/tests/controllers/authController.test.js (3)
Line range hint
606-652: Enhance test coverage for resetPasswordThe resetPassword test suite could benefit from additional test cases to cover edge cases:
- Password validation edge cases (e.g., password just meeting/failing minimum requirements)
- Token expiration scenarios
- Rate limiting scenarios
Here's a suggested additional test case:
it('should reject when password barely misses requirements', async () => { req.body = { recoveryToken: 'recovery-token', password: 'Almost1' // Missing special character }; await resetPassword(req, res, next); expect(next.firstCall.args[0]).to.be.an('error'); expect(next.firstCall.args[0].status).to.equal(422); });
Line range hint
442-486: Standardize error message testing approachThe error testing approach varies across test suites. For consistency, consider always asserting specific error messages, similar to how it's done in the loginUser tests.
Example standardization for refreshAuthToken error tests:
it('should reject if no refresh token is provided', async () => { delete req.headers['x-refresh-token']; await refreshAuthToken(req, res, next); expect(next.firstCall.args[0]).to.be.an('error'); expect(next.firstCall.args[0].message).to.equal(errorMessages.NO_REFRESH_TOKEN); expect(next.firstCall.args[0].status).to.equal(401); // Add assertion for error code if applicable expect(next.firstCall.args[0].code).to.equal('AUTH_ERROR'); });
Line range hint
32-40: Ensure consistent stub cleanup across all test suitesWhile most test suites use afterEach for cleanup, some stubs might not be properly restored. Consider adding afterEach blocks consistently across all test suites.
Example pattern to follow:
describe('Auth Controller - someMethod', () => { let stubs = []; beforeEach(() => { // Create stubs stubs.push(sinon.stub(someObj, 'someMethod')); }); afterEach(() => { // Restore all stubs stubs.forEach(stub => stub.restore()); stubs = []; sinon.restore(); }); });Server/tests/controllers/monitorController.test.js (3)
473-494: Yo! Let's make these error messages more consistent, dawg!The error messages in the DNS resolution test could be more consistent with the validation error messages. Consider standardizing the error message format across all test cases.
Apply this change to improve consistency:
- expect(errorPassedToNext.message).to.include("DNS resolution failed"); + expect(errorPassedToNext.message).to.equal("DNS resolution failed");
Line range hint
556-591: Mom's spaghetti moment: Let's clean up this repeated error message!There's a repeated error message string across multiple test cases. We should extract it to a constant to follow the DRY principle.
Add this at the beginning of the describe block:
+ const getErrorMessage = (monitor) => + `Error deleting associated records for monitor ${monitor._id} with name ${monitor.name}`; it("should log an error if deleteJob throws an error", async () => { const error = new Error("Job error"); const monitor = { name: "test_monitor", _id: "123" }; req.db.deleteMonitor.returns(monitor); req.jobQueue.deleteJob.rejects(error); await deleteMonitor(req, res, next); expect(logger.error.calledOnce).to.be.true; - expect(logger.error.firstCall.args[0].message).to.equal( - `Error deleting associated records for monitor ${monitor._id} with name ${monitor.name}` - ); + expect(logger.error.firstCall.args[0].message).to.equal(getErrorMessage(monitor));
Line range hint
666-702: Knees weak, arms heavy - same error message pattern spotted!The error message pattern is repeated here too. We could share the error message handling between deleteMonitor and deleteAllMonitors tests.
Consider moving the error message function to a shared test utility:
// testUtils.js export const getMonitorErrorMessage = (monitor) => `Error deleting associated records for monitor ${monitor._id} with name ${monitor.name}`;Then use it in both test suites:
+import { getMonitorErrorMessage } from '../testUtils.js'; it("should log an error if deleteChecks throws an error", async () => { const monitors = [{ name: "test_monitor", _id: "123" }]; req.settingsService.getSettings.returns({ jwtSecret: "my_secret" }); req.db.deleteAllMonitors.returns({ monitors, deletedCount: 1 }); const error = new Error("Check error"); req.db.deleteChecks.rejects(error); await deleteAllMonitors(req, res, next); expect(logger.error.calledOnce).to.be.true; - expect(logger.error.firstCall.args[0].message).to.equal( - `Error deleting associated records for monitor ${monitors[0]._id} with name ${monitors[0].name}` - ); + expect(logger.error.firstCall.args[0].message).to.equal(getMonitorErrorMessage(monitors[0]));Server/utils/logger.js (1)
72-77: Include the missingstackparameter in the JSDoc comments for theerrormethod.The
errormethod utilizesconfig.stack, but it's not documented in the JSDoc comments. Documenting all parameters ensures comprehensive understanding and proper usage of the method.Please update the JSDoc comments as follows:
/** * Logs an error message. * @param {Object} config - The configuration object. * @param {string} config.message - The message to log. * @param {string} config.service - The service name. * @param {string} config.method - The method name. * @param {Object} config.details - Additional details. + * @param {string} config.stack - The error stack trace. */
🛑 Comments failed to post (28)
Server/db/models/User.js (1)
83-83:
⚠️ Potential issueKnees weak, arms heavy... we've got a double export! 😰
The model is being exported twice, which could lead to initialization timing issues or confusion. We should stick to a single export after the initialization is complete.
Here's how to fix it:
const User = mongoose.model("User", UserSchema); User.init().then(() => { logger.info({ message: "User model initialized", }); }); -export default mongoose.model("User", UserSchema); +export default User;Also applies to: 89-89
Server/middleware/verifySuperAdmin.js (1)
53-57:
⚠️ Potential issueMom's spaghetti alert! We've got a bug! 🍝
There's a critical issue in this error logging block:
err.stackreferences an undefinederrvariable - this will cause a runtime error- The logging structure is inconsistent with the previous error block
Apply this fix:
logger.error({ message: errorMessages.INVALID_AUTH_TOKEN, service: SERVICE_NAME, method: "verifySuperAdmin", - stack: err.stack, + stack: new Error().stack, });📝 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.logger.error({ message: errorMessages.INVALID_AUTH_TOKEN, service: SERVICE_NAME, method: "verifySuperAdmin", stack: new Error().stack,Server/db/models/PageSpeedCheck.js (1)
104-109: 🛠️ Refactor suggestion
Yo, let's make this error logging even more fire! 🎵
While the structured error logging is solid, we could make it even better by including additional context.
Consider adding these fields to the error log:
logger.error({ message: error.message, service: "PageSpeedCheck", method: "pre-save", stack: error.stack, + monitorId: this.monitorId, + severity: "high", });This would help with:
- Tracking which monitor caused the error
- Prioritizing error resolution based on severity
Committable suggestion was skipped due to low confidence.
Server/tests/utils/logger.test.js (4)
19-49: 🛠️ Refactor suggestion
Yo, these constructor tests are looking a bit slim!
The test coverage is sweaty right now - we're only testing the happy path with object messages.
Consider adding these test cases to make it more robust:
it("should handle primitive message types", function () { const primitiveMessage = "test message"; // ... test implementation }); it("should handle null/undefined messages", function () { const nullMessage = null; // ... test implementation }); it("should handle circular references in objects", function () { const circularObj = { a: 1 }; circularObj.self = circularObj; // ... test implementation });
73-93: 🛠️ Refactor suggestion
These warn tests are making me lose myself in the duplication!
There's a lot of copy-pasta between the info and warn tests that's making me nervous.
Consider using a shared test factory:
const testLoggerMethod = (methodName, stub) => { it(`should log a ${methodName} message`, () => { const config = { message: `${methodName} message`, service: "TestService", method: "TestMethod", details: { key: "value" } }; logger[methodName](config); expect(stub.calledOnce).to.be.true; expect(stub.calledWith(config.message, { service: config.service, method: config.method, details: config.details })).to.be.true; }); }; describe("logger methods", () => { ['info', 'warn'].forEach(method => { describe(method, () => { testLoggerMethod(method, method === 'info' ? infoStub : warnStub); }); }); });
95-118: 🛠️ Refactor suggestion
The error tests need more sauce than mom's spaghetti!
We're missing some crucial error handling scenarios that could come back to haunt us.
Add these test cases:
it("should handle Error objects", () => { const error = new Error("Test error"); const config = { message: error.message, error, service: "TestService", method: "TestMethod" }; logger.error(config); expect(errorStub.calledOnce).to.be.true; expect(errorStub.firstCall.args[1].stack).to.equal(error.stack); }); it("should handle non-Error objects in error field", () => { const config = { message: "Error message", error: { custom: "error" }, service: "TestService", method: "TestMethod" }; logger.error(config); expect(errorStub.calledOnce).to.be.true; expect(errorStub.firstCall.args[1].details).to.deep.include({ custom: "error" }); });
1-4: 🛠️ Refactor suggestion
Yo dawg, we need to clean up these imports!
There's a double import of the logger module that's making me nervous like mom's spaghetti. We're importing both the default export and the
Loggerclass separately.Consider consolidating the imports:
-import logger from "../../utils/logger.js"; -import { Logger } from "../../utils/logger.js"; +import logger, { Logger } from "../../utils/logger.js";📝 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.import sinon from "sinon"; import logger, { Logger } from "../../utils/logger.js"; import winston from "winston";Server/service/emailService.js (3)
107-111:
⚠️ Potential issueMom's spaghetti moment in buildHtml! 🍝
The error logging is solid, but we're letting the function continue after an error occurs. This could lead to undefined behavior when trying to use the html result.
Let's clean this up:
} catch (error) { this.logger.error({ message: error.message, service: SERVICE_NAME, method: "buildAndSendEmail", stack: error.stack, }); + throw error; // Prevent undefined html from being used }📝 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.this.logger.error({ message: error.message, service: SERVICE_NAME, method: "buildAndSendEmail", stack: error.stack, }); throw error; // Prevent undefined html from being used
47-51:
⚠️ Potential issueYo! The error handling needs some love in loadTemplate! 🍝
While the error logging is on point, the function continues execution after logging the error. Since template loading is crucial for email functionality, we should either throw the error or return a meaningful value.
Here's how we can fix this, keeping our palms less sweaty:
} catch (error) { this.logger.error({ message: error.message, service: SERVICE_NAME, method: "loadTemplate", stack: error.stack, }); + throw error; // Re-throw to prevent silent failures }📝 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.this.logger.error({ message: error.message, service: SERVICE_NAME, method: "loadTemplate", stack: error.stack, }); throw error; // Re-throw to prevent silent failures
125-129:
⚠️ Potential issueKnees weak, arms heavy in sendEmail error handling! 🍝
The error logging is consistent, but we've got some potential issues:
- No explicit error handling after logging
- The parent function tries to access
info?.messageIdbutinfocould be undefinedHere's how we can make it better:
} catch (error) { this.logger.error({ message: error.message, service: SERVICE_NAME, method: "sendEmail", stack: error.stack, }); + throw error; // Ensure calling code knows the email failed }Also, consider adding a type check in the parent function:
- return info?.messageId; + if (!info) { + throw new Error('Failed to send email'); + } + return info.messageId;Committable suggestion was skipped due to low confidence.
Server/index.js (5)
54-54: 🛠️ Refactor suggestion
Yo dawg, we need to make that PORT more flexible!
The hardcoded port number might come back to haunt us later. Consider using an environment variable instead, with 5000 as a fallback.
- const PORT = 5000; + const PORT = process.env.PORT || 5000;📝 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 PORT = process.env.PORT || 5000;
165-171: 🛠️ Refactor suggestion
Palms are sweaty handling these errors - let's categorize them!
While the error logging is structured, we could improve error categorization for better monitoring and alerting.
logger.error({ message: error.message, service: SERVICE_NAME, method: "startApp", stack: error.stack, + errorType: error.name || 'UnknownError', + severity: 'CRITICAL', + impact: 'SERVICE_UNAVAILABLE' });📝 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.logger.error({ message: error.message, service: SERVICE_NAME, method: "startApp", stack: error.stack, errorType: error.name || 'UnknownError', severity: 'CRITICAL', impact: 'SERVICE_UNAVAILABLE' }); process.exit(1);
112-115:
⚠️ Potential issueKnees weak, arms are heavy - we've got a potential race condition!
The server starts listening before all services are initialized. Let's ensure everything's ready before we start accepting connections.
- await db.connect(); - app.listen(PORT, () => { - logger.info({ message: `server started on port:${PORT}` }); - }); + await db.connect(); + // Initialize all services first + const settingsService = new SettingsService(AppSettings); + await settingsService.loadSettings(); + // ... other service initializations ... + + app.listen(PORT, () => { + logger.info({ message: `server started on port:${PORT}` }); + });Committable suggestion was skipped due to low confidence.
92-100: 🛠️ Refactor suggestion
Let's beef up this health check, make it heavy like mom's spaghetti!
While the logging is improved, the health check could be more comprehensive by checking critical service dependencies.
app.use("/api/v1/healthy", async (req, res) => { try { logger.info({ message: "Checking Health of the server." }); + const dbStatus = await db.isConnected(); + const queueStatus = await jobQueue.isReady(); + if (!dbStatus || !queueStatus) { + throw new Error("Critical service unavailable"); + } return res.status(200).json({ - message: "Healthy" + status: "healthy", + services: { + database: dbStatus, + queue: queueStatus + } });Committable suggestion was skipped due to low confidence.
140-154: 🛠️ Refactor suggestion
There's vomit on his sweater already - we need a cleanup timeout!
The cleanup function could hang indefinitely. Let's add a timeout to ensure we eventually exit.
const cleanup = async () => { + const CLEANUP_TIMEOUT = 5000; if (cleaningUp) { logger.warn({ message: "Already cleaning up" }); return; } cleaningUp = true; + const timeoutPromise = new Promise((_, reject) => + setTimeout(() => reject(new Error('Cleanup timeout')), CLEANUP_TIMEOUT) + ); try { - logger.info({ message: "shutting down gracefully" }); + await Promise.race([ + (async () => { + logger.info({ message: "shutting down gracefully" }); + await jobQueue.obliterate(); + await db.disconnect(); + logger.info({ message: "shut down gracefully" }); + })(), + timeoutPromise + ]); - await jobQueue.obliterate(); - await db.disconnect(); - logger.info({ message: "shut down gracefully" }); } catch (error) {📝 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.logger.warn({ message: "Already cleaning up" }); return; } cleaningUp = true; const CLEANUP_TIMEOUT = 5000; const timeoutPromise = new Promise((_, reject) => setTimeout(() => reject(new Error('Cleanup timeout')), CLEANUP_TIMEOUT) ); try { await Promise.race([ (async () => { logger.info({ message: "shutting down gracefully" }); await jobQueue.obliterate(); await db.disconnect(); logger.info({ message: "shut down gracefully" }); })(), timeoutPromise ]); } catch (error) { logger.error({ message: error.message, service: SERVICE_NAME, method: "cleanup", stack: error.stack,Server/db/mongo/MongoDB.js (2)
34-48:
⚠️ Potential issueMom's spaghetti moment in the error handling! 🍝
While the logging is on point, we might want to propagate errors to the caller instead of swallowing them. Consider:
const disconnect = async () => { try { logger.info({ message: "Disconnecting from MongoDB" }); await mongoose.disconnect(); logger.info({ message: "Disconnected from MongoDB" }); - return; } catch (error) { logger.error({ message: error.message, service: SERVICE_NAME, method: "disconnect", stack: error.stack, }); + throw error; } };📝 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 disconnect = async () => { try { logger.info({ message: "Disconnecting from MongoDB" }); await mongoose.disconnect(); logger.info({ message: "Disconnected from MongoDB" }); } catch (error) { logger.error({ message: error.message, service: SERVICE_NAME, method: "disconnect", stack: error.stack, }); throw error; } };
22-22: 💡 Codebase verification
Yo, we need to mask that connection string in error logs! 🍝
Looking at the error handling in MongoDB.js, the error object is being logged with its full message and stack trace. Since MongoDB connection errors often include the connection string, this could expose sensitive credentials in the logs. Here's what needs attention:
- In
Server/db/mongo/MongoDB.js, the error logging in the catch block should sanitize the error message and stack trace to remove any connection string details before logging- The connection string appears in several configuration files and scripts that were found, but those are expected locations and not a security concern
Consider adding a helper function to sanitize MongoDB error messages before logging them.
🔗 Analysis chain
Heads up about sensitive data in logs! 👀
The logging implementation looks solid, but we should ensure the connection string isn't accidentally logged in error messages. Consider masking sensitive parts of the connection string if it appears in logs.
Let's check if there are any other instances where connection strings might be logged:
Also applies to: 24-29
Let me check the error handling in MongoDB.js to see if the connection string might be exposed in error logs.
Let me try a different approach to check the error handling code.
Let me get the full context around the connection handling to make a final assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential connection string logging rg -i "connection.*string"Length of output: 7605
Script:
#!/bin/bash # Check error handling in MongoDB.js ast-grep --pattern 'catch ($_) { $$$ }'Length of output: 43
Script:
#!/bin/bash # Get the full content of MongoDB.js to analyze error handling rg -A 5 "catch" Server/db/mongo/MongoDB.jsLength of output: 403
Script:
#!/bin/bash # Get the full content around connection handling rg -B 5 -A 10 "connectionString" Server/db/mongo/MongoDB.jsLength of output: 617
Server/service/jobQueue.js (1)
347-347: 🛠️ Refactor suggestion
Palms are sweaty reviewing these scattered logs!
Consider consolidating these logs into a single, more informative message.
-this.logger.info({ message: metrics }); -this.logger.info({ message: metrics }); -this.logger.info({ message: successMessages.JOB_QUEUE_OBLITERATE }); +this.logger.info({ + message: successMessages.JOB_QUEUE_OBLITERATE, + service: SERVICE_NAME, + method: 'obliterate', + details: { + metricsBeforeObliterate: metrics, + metricsAfterObliterate: finalMetrics, + workersTerminated: this.workers.length + } +});Also applies to: 363-364
Server/service/networkService.js (4)
59-64:
⚠️ Potential issueYo dawg, we need to sanitize that error before it drops!
While the structured logging format is fire 🔥, we should sanitize the error object before logging to prevent potential sensitive information leakage.
Here's a suggested fix to keep it clean:
this.logger.error({ message: error.message, service: this.SERVICE_NAME, method: "handleNotification", details: `notification error for monitor: ${monitor._id}`, - stack: error.stack, + stack: this.sanitizeError(error.stack), });Add this helper method to sanitize errors:
sanitizeError(stack) { if (!stack) return ''; // Remove sensitive patterns like auth tokens, passwords, etc. return stack.replace(/auth=[\w-]+/g, 'auth=***') .replace(/password=[\w-]+/g, 'password=***'); }
84-90: 🛠️ Refactor suggestion
Mom's spaghetti moment - we got duplicate error logging patterns!
Both error logging blocks could benefit from:
- Error sanitization (same as previous comment)
- DRY principle application - these logging patterns are repeated
Let's make it cleaner:
// Add this helper method logError(error, method, details) { this.logger.error({ message: error.message, service: this.SERVICE_NAME, method, details, stack: this.sanitizeError(error.stack), }); }Then simplify both blocks:
- this.logger.error({ - message: error.message, - service: this.SERVICE_NAME, - method: "handleStatusUpdate", - stack: error.stack, - details: `monitor lookup error for monitor: ${_id}`, - }); + this.logError(error, "handleStatusUpdate", `monitor lookup error for monitor: ${_id}`);Also applies to: 106-111
424-429:
⚠️ Potential issueCatch this error like it's the last chance to blow!
The error logging in logAndStoreCheck could use more specific error details to help with debugging.
Enhance the error details:
this.logger.error({ message: error.message, service: this.SERVICE_NAME, method: "logAndStoreCheck", - details: `Error writing check for ${data.monitorId}`, + details: `Failed to ${writeToDB.name} for monitor ${data.monitorId}. Check data: ${JSON.stringify(data, null, 2)}`, stack: error.stack, });Committable suggestion was skipped due to low confidence.
397-402:
⚠️ Potential issueLose yourself in the consistency - we're missing some fields here!
The error logging structure in getStatus differs from other error logs in the file. Let's keep the same vibe throughout.
Apply this change to maintain consistency:
this.logger.error({ message: `Unsupported type: ${job.data.type}`, service: this.SERVICE_NAME, method: "getStatus", + details: `Job ID: ${job.id}`, + stack: new Error().stack });📝 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.this.logger.error({ message: `Unsupported type: ${job.data.type}`, service: this.SERVICE_NAME, method: "getStatus", details: `Job ID: ${job.id}`, stack: new Error().stack });Server/tests/services/jobQueue.test.js (2)
21-24: 🛠️ Refactor suggestion
Simplify the removeRepeatable implementation using direct boolean return.
The current implementation can be more concise by directly returning the result of the find operation.
- if (removedJob) { - return true; - } - return false; + return !!removedJob;📝 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.return !!removedJob;
564-567: 💡 Codebase verification
⚠️ Potential issueYo! These test expectations are still valid and should be uncommented! 🍝
Looking at the test context, these expectations are testing critical functionality in the
deleteJobmethod:
- Checking if the job was actually removed from the queue
- Verifying logging behaviour
- Ensuring worker stats are checked
- Confirming worker scaling is triggered
The test setup clearly shows:
- A job is added to the queue (
jobQueue.queue.jobs = [job])- Stubs are set up for
getWorkerStatsandscaleWorkers- The logger is being tracked
These expectations aren't duplicated elsewhere in the file and are essential for verifying the complete behaviour of
deleteJob. Commenting them out leaves the test incomplete and potentially masks bugs.🔗 Analysis chain
Yo! These commented-out test expectations need attention! 🍝
These commented test expectations are leaving critical functionality untested. We should either:
- Remove them if they're no longer valid, or
- Update them to match the new boolean return type from removeRepeatable
Let's verify if these tests are still needed:
Let me gather more context about these tests to make a better assessment.
This will help us understand:
- The test case context where these expectations were commented out
- If there are other test cases covering the removeRepeatable functionality
- Whether these expectations are still relevant or need updating
Let me check one more thing to see if there are any recent changes to this test file or the related functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other test files that might have moved these expectations rg -l "expect\(jobQueue\.queue\.jobs\.length\).*\.to\.equal\(0\)" --type jsLength of output: 116
Script:
#!/bin/bash # Let's see the surrounding context of these expectations and their test case rg -B 10 -A 10 "expect\(jobQueue\.queue\.jobs\.length\).*\.to\.equal\(0\)" Server/tests/services/jobQueue.test.js # Also check for any related test cases that might be testing similar functionality rg "removeRepeatable" Server/tests/services/jobQueue.test.jsLength of output: 1483
Script:
#!/bin/bash # Let's check the implementation of deleteJob to understand the expected behavior ast-grep --pattern 'deleteJob($$$) { $$$ }' # Also check if there are any other assertions in this test file rg "expect\(" -A 1 Server/tests/services/jobQueue.test.jsLength of output: 3659
Server/tests/services/networkService.test.js (1)
822-829: 🛠️ Refactor suggestion
Consider consolidating duplicate error tests.
The three test cases for database write errors (null, undefined, and error thrown) have similar expectations and could be consolidated using test parameters for better maintainability.
-it("should thrown an error if the check is not inserted (null)", async () => { - writeToDB.resolves(null); - await networkService.logAndStoreCheck(data, writeToDB); - expect(loggerMock.error.calledOnce).to.be.true; - expect(loggerMock.error.firstCall.args[0].details).to.equal( - `Error writing check for ${data.monitorId}` - ); -}); -it("should thrown an error if the check is not inserted (undefined)", async () => { - writeToDB.resolves(undefined); - await networkService.logAndStoreCheck(data, writeToDB); - expect(loggerMock.error.calledOnce).to.be.true; - expect(loggerMock.error.firstCall.args[0].details).to.equal( - `Error writing check for ${data.monitorId}` - ); -}); +const invalidResults = [null, undefined]; +invalidResults.forEach((result) => { + it(`should throw an error if the check is not inserted (${result})`, async () => { + writeToDB.resolves(result); + await networkService.logAndStoreCheck(data, writeToDB); + expect(loggerMock.error.calledOnce).to.be.true; + expect(loggerMock.error.firstCall.args[0].details).to.equal( + `Error writing check for ${data.monitorId}` + ); + }); +});Also applies to: 842-844, 849-853
Server/utils/logger.js (3)
7-8:
⚠️ Potential issueAvoid reassigning function parameters for clearer code.
Reassigning the
messageparameter within the function may lead to unexpected behaviour and can make the code harder to understand. It's better to use a new variable to store the formatted message.Apply this diff to prevent reassignment:
- if (message instanceof Object) { - message = JSON.stringify(message, null, 2); - } + let formattedMessage = message; + if (formattedMessage instanceof Object) { + formattedMessage = JSON.stringify(formattedMessage, null, 2); + }Don't forget to update subsequent usages of
messagetoformattedMessage.Committable suggestion was skipped due to low confidence.
31-34: 🛠️ Refactor suggestion
Consider making the log filename configurable to enhance flexibility.
Hardcoding the log filename as
"app.log"might limit the logger's adaptability in different environments. Allowing the filename to be configurable can make the logging system more versatile and easier to integrate with various deployment setups.You could modify the filename setting to use an environment variable or a configuration parameter:
new transports.File({ format: format.combine(format.json()), - filename: "app.log", + filename: process.env.LOG_FILE || "app.log", }),This way, the default remains
"app.log", but you have the option to specify a different filename if needed.📝 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.new transports.File({ format: format.combine(format.json()), filename: process.env.LOG_FILE || "app.log", }),
11-14:
⚠️ Potential issueReplace assignments within expressions with explicit
ifstatements for better readability.Using assignments inside expressions like
service && (msg += ...)can be confusing and might trip up someone reading the code. Switching toifstatements makes the intent clearer and the code easier to maintain.Here's how you can adjust the code:
- service && (msg += ` [${service}]`); - method && (msg += `(${method})`); - message && (msg += ` ${message}`); - details && (msg += ` (details: ${details})`); + if (service) { + msg += ` [${service}]`; + } + if (method) { + msg += `(${method})`; + } + if (formattedMessage) { + msg += ` ${formattedMessage}`; + } + if (details) { + msg += ` (details: ${details})`; + }This change promotes clarity and aligns with best practices.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 11-11: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 12-12: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 13-13: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 14-14: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
This PR improves and cleans up the logging on the BE. The logger was written on the first day of the project by a different dev, so I've attempted to bring it in line with the rest of the project.
This PR also standardizes logging on the BE as there was a mix of
console.logand thelogger.console.logstatements withloggerstatements