feat: add structured winston logging to cloud functionsComp log#366
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Winston-based structured logger and instruments the sendDailyDigest cloud function: logs function start (with input), the no-events early return, notification dispatch (message count), chunk send errors via logError, and final completion; also adds winston deps. ChangesDaily Digest Logging
Sequence DiagramsequenceDiagram
participant Client
participant sendDailyDigest
participant logger
participant Expo
Client->>sendDailyDigest: invoke callable with input
sendDailyDigest->>logger: logEntry("dailyDigest/sendDailyDigest started", input)
sendDailyDigest->>sendDailyDigest: compute events and count
alt no events
sendDailyDigest->>logger: logEntry("no events today")
sendDailyDigest-->>Client: return success
else events present
sendDailyDigest->>logger: logEntry("broadcasting", { count })
loop per chunk
sendDailyDigest->>Expo: sendPushNotifications(chunk)
Expo-->>sendDailyDigest: response / error
sendDailyDigest->>logger: logError(on chunk send failure)
end
sendDailyDigest->>logger: logEntry("completed", { count, processed? })
sendDailyDigest-->>Client: return success with counts
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloud-functions/src/dailyDigest.ts`:
- Line 5: The file logger.ts is empty so logEntry/logError have no
implementation and dailyDigest.ts still uses console.error in the push-send
failure path; implement and export structured logging functions in
cloud-functions/src/logger.ts (export logEntry and logError with signatures that
accept message, context object, and optional Error), then update
cloud-functions/src/dailyDigest.ts to replace the console.error(...) call in the
push-send failure block with logError(errorMessage, { event:
'dailyDigest.pushSend', payload: pushPayload, response: sendResponse }, err) (or
similar) so errors are emitted in structured JSON; ensure imports remain {
logEntry, logError } and that logError includes both the error object and
contextual fields for observability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 36423a42-6184-49a1-bad2-32435ead3fac
📒 Files selected for processing (2)
cloud-functions/src/dailyDigest.tscloud-functions/src/logger.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloud-functions/src/logger.ts`:
- Around line 43-61: The logError function assumes error is an Error and reads
error.stack directly; defend it so logging never throws: inside logError
validate the runtime value (use instanceof Error or typeof checks) and derive
safe values for stack and error message—e.g., if error is an Error use
error.stack and error.message; if it's a string use that string as message and
null stack; if it's an object JSON.stringify it for message and stack=null; if
null/undefined set message to a default like "unknown error" and stack to
null—then pass those safe values into logger.error along with existing fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff72ed3f-eff5-4fba-88fe-87b6fb10e916
📒 Files selected for processing (2)
cloud-functions/src/dailyDigest.tscloud-functions/src/logger.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- cloud-functions/src/dailyDigest.ts
|
@shreyjeetdas handle the coderabbity suggestion |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cloud-functions/src/dailyDigest.ts (1)
3-4:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the named
Expoexport here.Line 3 uses a default import, but the upstream
expo-server-sdkexamples useExpoas a named export for ESM and{ Expo }destructuring for CommonJS. This import is brittle under stricter TS/module settings and can fail at type-check or runtime. (github.com)Suggested fix
-import Expo from 'expo-server-sdk'; +import { Expo } from 'expo-server-sdk';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/dailyDigest.ts` around lines 3 - 4, The import uses a default import for Expo which can break under strict module/TS settings; replace the default import with the named export (use the named Expo import from 'expo-server-sdk') so that the existing instantiation (const expo = new Expo()) uses the correct named symbol; update the import statement accordingly to import { Expo } and leave the rest of the code unchanged.
🧹 Nitpick comments (1)
cloud-functions/package.json (1)
45-45: ⚡ Quick winRemove the deprecated
@types/winstonstub.Line 45 adds a package that Winston no longer needs: the
@types/winstonpackage is deprecated, and thewinstonpackage already ships its own TypeScript declarations. Keeping both increases the chance of type-version skew between the runtime and the editor/compiler. (npmjs.com)Suggested fix
- "`@types/winston`": "^2.4.4",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/package.json` at line 45, Remove the deprecated dependency entry "`@types/winston`" from package.json; Winston ships its own TypeScript types so delete the "`@types/winston`" line from the dependencies/devDependencies, run npm/yarn install to update lockfile, and commit the updated package.json and lockfile so the project no longer references the deprecated stub.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloud-functions/package.json`:
- Around line 56-57: The package.json file has a missing comma before the
"private" property which makes the JSON invalid; open the cloud-functions
package.json, locate the trailing brace before the "private" key and insert the
missing comma so the object entries are properly separated (ensure the property
name "private" remains unchanged and the file parses as valid JSON so npm ci
succeeds).
In `@cloud-functions/src/dailyDigest.ts`:
- Around line 34-39: Remove all Git conflict markers (<<<<<<<, =======, >>>>>>>)
and merge the two conflicting branches by combining structured logging and
pagination/response fields: inside the dailyDigest function keep the
logEntry('dailyDigest', ...) call and return the expanded structured result {
success: true, message: "No events today.", count: 0, processed: 0 } so both
behaviors remain; repeat this approach for the other conflict blocks (around the
pagination logic in the middle of the file and the structured logging at the
end) by merging pagination handling with the existing logging/response shape
rather than choosing one side.
---
Outside diff comments:
In `@cloud-functions/src/dailyDigest.ts`:
- Around line 3-4: The import uses a default import for Expo which can break
under strict module/TS settings; replace the default import with the named
export (use the named Expo import from 'expo-server-sdk') so that the existing
instantiation (const expo = new Expo()) uses the correct named symbol; update
the import statement accordingly to import { Expo } and leave the rest of the
code unchanged.
---
Nitpick comments:
In `@cloud-functions/package.json`:
- Line 45: Remove the deprecated dependency entry "`@types/winston`" from
package.json; Winston ships its own TypeScript types so delete the
"`@types/winston`" line from the dependencies/devDependencies, run npm/yarn
install to update lockfile, and commit the updated package.json and lockfile so
the project no longer references the deprecated stub.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8be1ea05-3c8f-4693-af3f-57afab149f69
📒 Files selected for processing (2)
cloud-functions/package.jsoncloud-functions/src/dailyDigest.ts
|
@shreyjeetdas check the coderabbit sugesstion for npm ci failing |
|
This is already handled in the current implementation — logError accepts error: unknown and covers all cases: Error instances (extracts message and stack), strings, objects (JSON stringified), and null/undefined (defaults to 'unknown error'). No changes needed. |
|
@shreyjeetdas you missed soem dependecies |
|
I've added winston as a structured logging dependency in package.json. The package-lock.json needs to be updated by running npm install in the cloud-functions directory. Since I don't have Node.js installed locally, could you update the lock file or let me know the preferred way to handle new dependencies in this project? |
|
wait till evening i will accept the pr and update the lockfile from my side |
|
@roshankumar0036singh Hi, the PR is still not merged! |
|
@shreyjeetdas sorry about that missed it can you solve the conlfict |
|
|
Hi @roshankumar0036singh, I've resolved the merge conflict and force-pushed the updated branch. The changes are now based on the latest upstream version. |
roshankumar0036singh
merged commit 5db1e22
into
roshankumar0036singh:main
2 participants
Screenshot (when helpful)
N/A — backend logging implementation, no UI involved.
What this PR does
The cloud functions had no structured logging, making it impossible
to trace requests, debug issues, or monitor function behaviour in
production.
This PR adds a dedicated
logger.tsmodule usingwinstonthatproduces structured JSON logs in the format:
{ timestamp, level, service, eventId, userId, message, context, stack }
The logger exposes two functions:
logEntry()— logs function entry/exit points with input/outputlogError()— logs errors with full stack tracedailyDigest.tshas been updated to use the new logger at all keypoints: function start, no-events early return, broadcast start,
push notification sending, error handling, and function completion.
Type of change
Related issue
Fixes #327
More screenshots (optional)
N/A
Checklist
Summary by CodeRabbit
Refactor
Chores
Stability