From 81aab1e15950d5c468a0a2d8aace6e12365039a1 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 5 Feb 2025 11:31:30 +0000 Subject: [PATCH] [misc] fix logger.error(err) and logger.warn(err) calls (#23369) * [misc] fix logger.error(err) calls The signature is "logger.error({ err }, 'MESSAGE')". * [project-history] remove duplicate logger.err calls in health check The call-site is already logging any errors. Also, the logger.err call signature was not quite right. * [web] log userId when removeDropbox/removeGithub hook fails * [misc] fix logger.warn(err) calls The signature is "logger.warn({ err }, 'MESSAGE')". * [misc] fix logger.error(OError.tag(err)) calls * [web] make eslint happy GitOrigin-RevId: 7f528113a3f7e9f6293b7d2d45adc079380325bb --- services/history-v1/api/controllers/project_import.js | 4 ++-- services/history-v1/app.js | 8 +++++--- services/project-history/app.js | 7 ++++--- services/project-history/app/js/HealthChecker.js | 3 --- services/project-history/app/js/HistoryStoreManager.js | 2 +- .../web/app/src/Features/Subscription/FeaturesUpdater.js | 4 ++-- services/web/app/src/Features/User/UserController.js | 3 ++- services/web/app/src/Features/User/UserCreator.js | 3 ++- .../web/app/src/Features/User/UserPagesController.mjs | 3 ++- services/web/app/src/infrastructure/QueueWorkers.js | 4 ++-- 10 files changed, 22 insertions(+), 19 deletions(-) diff --git a/services/history-v1/api/controllers/project_import.js b/services/history-v1/api/controllers/project_import.js index ec4aa317b0..a91a6e6118 100644 --- a/services/history-v1/api/controllers/project_import.js +++ b/services/history-v1/api/controllers/project_import.js @@ -59,7 +59,7 @@ exports.importChanges = function importChanges(req, res, next) { try { changes = rawChanges.map(Change.fromRaw) } catch (err) { - logger.error(err) + logger.error({ err, projectId }, err.message) return render.unprocessableEntity(res) } @@ -129,7 +129,7 @@ exports.importChanges = function importChanges(req, res, next) { ) { // If we failed to apply operations, that's probably because they were // invalid. - logger.error(err) + logger.error({ err, projectId }, err.message) render.unprocessableEntity(res) } else if (err instanceof Chunk.NotFoundError) { render.notFound(res) diff --git a/services/history-v1/app.js b/services/history-v1/app.js index c96a2f5ac3..261f1001b6 100644 --- a/services/history-v1/app.js +++ b/services/history-v1/app.js @@ -84,16 +84,17 @@ function setupErrorHandling() { // Handle Swagger errors. app.use(function (err, req, res, next) { + const projectId = req.swagger?.params?.project_id?.value if (res.headersSent) { return next(err) } if (err.code === 'SCHEMA_VALIDATION_FAILED') { - logger.error(err) + logger.error({ err, projectId }, err.message) return res.status(HTTPStatus.UNPROCESSABLE_ENTITY).json(err.results) } if (err.code === 'INVALID_TYPE' || err.code === 'PATTERN') { - logger.error(err) + logger.error({ err, projectId }, err.message) return res.status(HTTPStatus.UNPROCESSABLE_ENTITY).json({ message: 'invalid type: ' + err.paramName, }) @@ -112,7 +113,8 @@ function setupErrorHandling() { }) app.use(function (err, req, res, next) { - logger.error(err) + const projectId = req.swagger?.params?.project_id?.value + logger.error({ err, projectId }, err.message) if (res.headersSent) { return next(err) diff --git a/services/project-history/app.js b/services/project-history/app.js index 1c2f93bfbc..a72af4f1b2 100644 --- a/services/project-history/app.js +++ b/services/project-history/app.js @@ -14,10 +14,11 @@ mongoClient .connect() .then(() => { app.listen(port, host, error => { - if (error != null) { - logger.error(OError.tag(error, 'could not start history server')) + if (error) { + error = OError.tag(error, 'could not start history server') + logger.error({ error }, error.message) } else { - logger.debug(`history starting up, listening on ${host}:${port}`) + logger.debug({}, `history starting up, listening on ${host}:${port}`) } }) }) diff --git a/services/project-history/app/js/HealthChecker.js b/services/project-history/app/js/HealthChecker.js index 74f1272e1e..c57f184aad 100644 --- a/services/project-history/app/js/HealthChecker.js +++ b/services/project-history/app/js/HealthChecker.js @@ -29,7 +29,6 @@ export function check(callback) { OError.tag(err, 'error checking lock for health check', { project_id: projectId, }) - logger.err(err) return cb(err) } else if ((res != null ? res.statusCode : undefined) !== 200) { return cb(new Error(`status code not 200, it's ${res.statusCode}`)) @@ -46,7 +45,6 @@ export function check(callback) { OError.tag(err, 'error flushing for health check', { project_id: projectId, }) - logger.err(err) return cb(err) } else if ((res != null ? res.statusCode : undefined) !== 204) { return cb(new Error(`status code not 204, it's ${res.statusCode}`)) @@ -63,7 +61,6 @@ export function check(callback) { OError.tag(err, 'error getting updates for health check', { project_id: projectId, }) - logger.err(err) return cb(err) } else if ((res != null ? res.statusCode : undefined) !== 200) { return cb(new Error(`status code not 200, it's ${res.statusCode}`)) diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index e8fd51d452..cca6143ccf 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/services/project-history/app/js/HistoryStoreManager.js @@ -250,7 +250,7 @@ export function sendChanges( statusCode: error.statusCode, body: error.body, }) - logger.warn(error) + logger.warn({ error, projectId, historyId, endVersion }, error.message) return callback(error) } callback() diff --git a/services/web/app/src/Features/Subscription/FeaturesUpdater.js b/services/web/app/src/Features/Subscription/FeaturesUpdater.js index bbc040c4dd..1e7a868fc0 100644 --- a/services/web/app/src/Features/Subscription/FeaturesUpdater.js +++ b/services/web/app/src/Features/Subscription/FeaturesUpdater.js @@ -57,7 +57,7 @@ async function refreshFeatures(userId, reason) { try { await Modules.promises.hooks.fire('removeDropbox', userId, reason) } catch (err) { - logger.error(err) + logger.error({ err, userId }, 'removeDropbox hook failed') } } @@ -66,7 +66,7 @@ async function refreshFeatures(userId, reason) { try { await Modules.promises.hooks.fire('removeGithub', userId, reason) } catch (err) { - logger.error(err) + logger.error({ err, userId }, 'removeGithub hook failed') } } diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index 404437d758..d4257f7912 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -266,7 +266,8 @@ async function tryDeleteUser(req, res, next) { errorData.info.public = { error: 'SubscriptionAdminDeletionError', } - logger.warn(OError.tag(err, errorData.message, errorData.info)) + const error = OError.tag(err, errorData.message, errorData.info) + logger.warn({ error, req }, error.message) return HttpErrorHandler.unprocessableEntity( req, res, diff --git a/services/web/app/src/Features/User/UserCreator.js b/services/web/app/src/Features/User/UserCreator.js index 808e4323aa..390d925a7f 100644 --- a/services/web/app/src/Features/User/UserCreator.js +++ b/services/web/app/src/Features/User/UserCreator.js @@ -100,7 +100,8 @@ async function createNewUser(attributes, options = {}) { await UserDeleter.promises.deleteMongoUser(user._id) throw OError.tag(error) } else { - logger.error(OError.tag(error)) + const err = OError.tag(error, 'adding affiliations failed') + logger.error({ err, userId: user._id }, err.message) } } } diff --git a/services/web/app/src/Features/User/UserPagesController.mjs b/services/web/app/src/Features/User/UserPagesController.mjs index c753d2bd78..bab9833d9c 100644 --- a/services/web/app/src/Features/User/UserPagesController.mjs +++ b/services/web/app/src/Features/User/UserPagesController.mjs @@ -79,7 +79,8 @@ async function settingsPage(req, res) { ) personalAccessTokens = results?.[0] ?? [] } catch (error) { - logger.error(OError.tag(error)) + const err = OError.tag(error, 'listPersonalAccessTokens hook failed') + logger.error({ err, userId }, err.message) } let currentManagedUserAdminEmail diff --git a/services/web/app/src/infrastructure/QueueWorkers.js b/services/web/app/src/infrastructure/QueueWorkers.js index caff895bf5..0b117954e7 100644 --- a/services/web/app/src/infrastructure/QueueWorkers.js +++ b/services/web/app/src/infrastructure/QueueWorkers.js @@ -66,7 +66,7 @@ function start() { await EmailHandler.promises.sendEmail(emailType, opts) } catch (e) { const error = OError.tag(e, 'failed to send deferred email') - logger.warn(error) + logger.warn({ error, emailType }, error.message) throw error } }) @@ -84,7 +84,7 @@ function start() { e, 'failed to send scheduled Group SSO account linking reminder' ) - logger.warn(error) + logger.warn({ error, userId, subscriptionId }, error.message) throw error } })