Improved logging + DOCKERFILE changes#190
Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes backend logging around structured JSON output to stdout (for docker logs consumption), adds a shared logError helper for consistent error reporting with context, and updates the backend Docker image configuration to support runtime image writes in a writable temp directory.
Changes:
- Replace ad-hoc
logger.error(...)usage with structuredlogError(logger, message, error, context)across controllers/services and update related tests. - Update Winston logger formatting to emit JSON-line logs to stdout and remove file-based transports.
- Adjust backend Dockerfile runtime env to set
IMAGE_DIR(and stop creating/copying an/imagesdirectory), plus README guidance for querying logs.
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/backend/src/walkie/walkie.service.ts | Add module child logger; async handler |
| packages/backend/src/walkie/walkie.controller.ts | Catch/log async errors with context |
| packages/backend/src/store/store.service.ts | Log + rethrow store item failures |
| packages/backend/src/store/store.persistence.service.ts | Structured purchase failure logging |
| packages/backend/src/store/store.controller.ts | Structured error logging in routes |
| packages/backend/src/store/item.service.ts | Structured logging for item actions |
| packages/backend/src/shared/services/web/web.service.ts | Structured Slack API error logs |
| packages/backend/src/shared/services/slack/slack.service.ts | Structured logging for callbacks/events |
| packages/backend/src/shared/services/slack/slack.service.spec.ts | Update assertions for structured logs |
| packages/backend/src/shared/services/slack/slack.persistence.service.ts | Structured DB error logging |
| packages/backend/src/shared/services/history.persistence.service.ts | Add logger + structured persistence logs |
| packages/backend/src/shared/logger/logger.ts | JSON-line stdout logger format |
| packages/backend/src/shared/logger/error-logging.ts | New shared logError helper |
| packages/backend/src/reaction/reaction.service.ts | Structured logging for reaction persistence |
| packages/backend/src/reaction/reaction.service.spec.ts | Update assertions for structured logs |
| packages/backend/src/reaction/reaction.persistence.service.ts | Add context + throw on persistence failures |
| packages/backend/src/reaction/reaction.controller.ts | Structured controller error logging |
| packages/backend/src/quote/quote.service.ts | Structured logging for quote flow |
| packages/backend/src/quote/quote.controller.ts | Catch/log async errors with context |
| packages/backend/src/portfolio/portfolio.service.ts | Structured logging around transactions |
| packages/backend/src/portfolio/portfolio.persistence.service.ts | Structured logging in DB transaction failures |
| packages/backend/src/portfolio/portfolio.controller.ts | Add controller-level error logging |
| packages/backend/src/muzzle/muzzle.persistence.service.ts | Add logger + structured persistence logs |
| packages/backend/src/muzzle/muzzle.controller.ts | Structured controller error logging |
| packages/backend/src/mock/mock.service.ts | Add module child logger; async handler |
| packages/backend/src/mock/mock.controller.ts | Catch/log async errors with context |
| packages/backend/src/memory/memory.controller.ts | Structured logging for /memory errors |
| packages/backend/src/list/list.service.ts | Async list/remove + structured logging |
| packages/backend/src/list/list.service.spec.ts | Update tests for async + structured logs |
| packages/backend/src/list/list.persistence.service.ts | Add logger + structured DB error logs |
| packages/backend/src/list/list.controller.ts | Catch/log async errors with context |
| packages/backend/src/hook/hook.controller.ts | Structured logging on hook failures |
| packages/backend/src/event/event.service.ts | Structured logging for handler failures |
| packages/backend/src/event/event.service.spec.ts | Update assertions for structured logs |
| packages/backend/src/event/event.persistence.service.ts | Add logger + structured persistence logs |
| packages/backend/src/event/event.controller.ts | Structured logging for request handling failures |
| packages/backend/src/define/define.service.ts | Structured logging for fetch/send failures |
| packages/backend/src/define/define.controller.ts | Structured logging for request failures |
| packages/backend/src/counter/counter.service.ts | Structured logging for Slack message failures |
| packages/backend/src/counter/counter.persistence.service.ts | Structured logging for persistence steps |
| packages/backend/src/counter/counter.controller.ts | Structured controller error logging |
| packages/backend/src/confession/confession.service.ts | Structured logging for Slack send failures |
| packages/backend/src/confession/confession.service.spec.ts | Update assertions for structured logs |
| packages/backend/src/confession/confession.controller.ts | Catch/log async errors with context |
| packages/backend/src/clap/clap.service.ts | Add module child logger; async handler |
| packages/backend/src/clap/clap.controller.ts | Catch/log async errors with context |
| packages/backend/src/backfire/backfire.service.ts | Structured logging for Slack send failures |
| packages/backend/src/backfire/backfire.persistence.service.ts | Add logger + structured persistence logs |
| packages/backend/src/ai/memory/memory.persistence.service.ts | Structured logging for memory persistence |
| packages/backend/src/ai/ai.service.ts | Safer image writes; structured AI error logs |
| packages/backend/src/ai/ai.service.spec.ts | Add coverage for image-dir creation |
| packages/backend/src/ai/ai.controller.ts | Structured logging for AI endpoint failures |
| packages/backend/Dockerfile | Set IMAGE_DIR; simplify runtime artifacts |
| README.md | Add Docker log querying guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logError(this.logger, 'Slack deleteMessage returned an API error', new Error(r.error), { | ||
| channel, | ||
| ts, | ||
| user, | ||
| deleteRequest, | ||
| }); |
There was a problem hiding this comment.
deleteRequest includes the Slack bot token (token: process.env.MUZZLE_BOT_TOKEN). Logging the full request object will leak credentials into stdout / docker logs. Please redact/remove sensitive fields (e.g., token, authorization headers) before putting request payloads into structured logs.
| axios.post(encodeURI(responseUrl), response).catch((e: Error) => | ||
| logError(this.logger, 'Failed to post Slack response URL callback', e, { | ||
| responseUrl, | ||
| responseType: response.response_type, | ||
| responseText: response.text, | ||
| }), |
There was a problem hiding this comment.
responseUrl is effectively a secret (anyone with it can post a response to the slash command). Logging it in structured logs risks leaking the callback URL via docker logs. Consider removing it from logs entirely, or logging only a redacted/hash form (e.g., host + last 4 chars).
| logError(walkieLogger, 'Failed to handle /walkie request', e, { | ||
| userId: request.user_id, | ||
| channelId: request.channel_id, | ||
| responseUrl: request.response_url, | ||
| text: request.text, | ||
| }); |
There was a problem hiding this comment.
This error log includes responseUrl in the context. Slack slash command response_url values are sensitive (they allow posting callbacks) and shouldn’t be written to stdout logs. Please redact/remove responseUrl from the logged context.
| void Promise.resolve(mockService.mock(request)).catch((e) => { | ||
| logError(mockLogger, 'Failed to handle /mock request', e, { | ||
| userId: request.user_id, | ||
| responseUrl: request.response_url, |
There was a problem hiding this comment.
This error log includes responseUrl in the context. Slack slash command response_url values are sensitive (they allow posting callbacks) and shouldn’t be written to stdout logs. Please redact/remove responseUrl from the logged context.
| responseUrl: request.response_url, |
| void Promise.resolve(clapService.clap(text, user_id, response_url)).catch((e) => { | ||
| logError(clapLogger, 'Failed to handle /clap request', e, { | ||
| userId: user_id, | ||
| responseUrl: response_url, |
There was a problem hiding this comment.
This error log includes responseUrl in the context. Slack slash command response_url values are sensitive (they allow posting callbacks) and shouldn’t be written to stdout logs. Please redact/remove responseUrl from the logged context.
| responseUrl: response_url, |
| logError(this.logger, 'Failed to send ephemeral Slack message', e, { | ||
| channel, | ||
| user, | ||
| text, | ||
| postRequest, | ||
| }); |
There was a problem hiding this comment.
postRequest includes token: process.env.MUZZLE_BOT_USER_TOKEN. Logging postRequest will expose Slack credentials in logs. Please omit/redact the token field (and any other secrets) from the logged context.
| const payload: Record<string, unknown> = { | ||
| timestamp: loggedAt, | ||
| level, | ||
| module, | ||
| message: typeof message === 'string' ? message : JSON.stringify(message), | ||
| }; |
There was a problem hiding this comment.
The formatter calls JSON.stringify(...) on message (and sometimes on normalizedInfo.message). JSON.stringify can throw for circular structures and some values (e.g., BigInt), which would make logging itself throw during runtime. Consider using a safe stringify (try/catch fallback to String(message) / util.inspect) to avoid log-path crashes.
| logError(this.logger, 'Failed to send Slack message', e, { | ||
| channel, | ||
| text, | ||
| hasBlocks: !!blocks?.length, | ||
| postRequest, | ||
| errorCode: getSlackErrorCode(e), | ||
| }); |
There was a problem hiding this comment.
postRequest includes token: process.env.MUZZLE_BOT_USER_TOKEN. Logging the full request object in context will leak the token via stdout logs. Please remove/redact the token field (and consider logging only non-sensitive request attributes like channel + hasBlocks).
| .catch((e) => { | ||
| logError(this.customLogger, 'Failed to remove reaction', e, { | ||
| reaction: event.reaction, | ||
| teamId, | ||
| reactingUser: event.user, | ||
| affectedUser: event.item_user, | ||
| channelId: event.item.channel, | ||
| }); | ||
| throw e; | ||
| }); |
There was a problem hiding this comment.
removeReaction now rethrows in the .catch, but the only call site (ReactionService.handleRemovedReaction) fires-and-forgets the promise without a .catch(...). That will create an unhandled promise rejection on DB failures. Either handle/log the rejection at the call site (preferred), or keep removeReaction from rejecting (log + swallow) to preserve fire-and-forget behavior.
| channelId: channel_id, | ||
| text, | ||
| }); | ||
| res.send('Something went wrong while retrieving your definition'); |
There was a problem hiding this comment.
This handler sends a 200 response immediately, but then attempts res.send(...) inside the catch. If defineService.define(...) rejects, Express will log "Cannot set headers after they are sent" / the send will be ignored. Since the slash command is already acknowledged, avoid writing to res in the catch and instead report the error via Slack (or just log).
| res.send('Something went wrong while retrieving your definition'); |
This pull request introduces comprehensive improvements to error logging and image file handling in the backend. It standardizes structured error logging across AI and memory persistence services, ensures image files are written to a configurable and secure directory, and updates documentation to help with Docker-based log inspection. Additionally, it adds tests for image writing and updates Dockerfile configuration for runtime image storage.
Error Logging Improvements:
ai.service.ts,ai.controller.ts, andmemory.persistence.service.tswith the newlogErrorutility, providing structured logs with contextual metadata (user, team, channel, prompt, etc.) for easier debugging and traceability. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18]Image File Handling and Testing:
AIServiceto use an environment-configurable directory (IMAGE_DIR), defaulting to/tmp/mocker-images, and ensures the directory is created recursively before writing. Adds robust error handling and logging if writing fails. [1] [2]/tmp/mocker-imagesvia theIMAGE_DIRenvironment variable.writeToDiskAndReturnUrl. [1] [2]Documentation and Observability:
README.md, explaining the structured JSON logging format and providing example commands for log inspection and filtering in production environments.These changes collectively make error diagnosis easier, improve operational robustness, and clarify how to work with logs and runtime-generated images in production.