Converted remaining outbox and welcome email logs to structured logs#26660
Converted remaining outbox and welcome email logs to structured logs#26660
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:
WalkthroughReplaces plain-string log messages with structured, object-based logs across member-welcome-email, outbox services, and outbox job files. New logs include a Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/services/outbox/jobs/lib/process-entries.js`:
- Around line 65-79: The check that treats falsy payloads as parse failures is
causing valid JSON falsy values to be misclassified; update processEntries logic
to introduce an explicit parseSuccess (or similar) boolean set only when
JSON.parse (or payload parsing) succeeds, then use parseSuccess === false to log
'outbox.entry.payload_parse_failed' via logging.error (referencing
handler.LOG_KEY and entry.id), otherwise proceed to the handler call and log
'outbox.entry.send_failed' on handler failure (using
handler.getLogInfo(payload)); ensure you stop relying on !payload to decide
parse failure.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ghost/core/core/server/services/member-welcome-emails/service.jsghost/core/core/server/services/outbox/handlers/member-created.jsghost/core/core/server/services/outbox/index.jsghost/core/core/server/services/outbox/jobs/lib/process-entries.jsghost/core/core/server/services/outbox/jobs/lib/process-outbox.jsghost/core/test/unit/server/services/outbox/handlers/member-created.test.jsghost/core/test/unit/server/services/outbox/index.test.jsghost/core/test/unit/server/services/outbox/jobs/lib/process-entries.test.js
ghost/core/core/server/services/outbox/jobs/lib/process-entries.js
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing documented
outbox.job.completestructured log event- Converted job completion logging from returned strings to inline structured logging with the outbox.job.complete event, matching the documented behavior and the pattern used for outbox.batch.complete.
Or push these changes by commenting:
@cursor push 0d08dfd982
Preview (0d08dfd982)
diff --git a/ghost/core/core/server/services/outbox/index.js b/ghost/core/core/server/services/outbox/index.js
--- a/ghost/core/core/server/services/outbox/index.js
+++ b/ghost/core/core/server/services/outbox/index.js
@@ -34,8 +34,7 @@
this.processing = true;
try {
- const statusMessage = await processOutbox();
- logging.info(statusMessage);
+ await processOutbox();
} catch (e) {
logging.error({
system: {
diff --git a/ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js b/ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js
--- a/ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js
+++ b/ghost/core/core/server/services/outbox/jobs/lib/process-outbox.js
@@ -88,10 +88,25 @@
const durationMs = Date.now() - jobStartMs;
if (totalProcessed + totalFailed === 0) {
- return `${OUTBOX_LOG_KEY} ${MESSAGES.NO_ENTRIES}`;
+ logging.info({
+ system: {
+ event: 'outbox.job.complete',
+ entries_processed: 0,
+ entries_failed: 0,
+ duration_ms: durationMs
+ }
+ }, `${OUTBOX_LOG_KEY} ${MESSAGES.NO_ENTRIES}`);
+ return;
}
- return `${OUTBOX_LOG_KEY} Job complete: Processed ${totalProcessed} outbox entries, ${totalFailed} failed in ${(durationMs / 1000).toFixed(2)}s`;
+ logging.info({
+ system: {
+ event: 'outbox.job.complete',
+ entries_processed: totalProcessed,
+ entries_failed: totalFailed,
+ duration_ms: durationMs
+ }
+ }, `${OUTBOX_LOG_KEY} Job complete: Processed ${totalProcessed} outbox entries, ${totalFailed} failed in ${(durationMs / 1000).toFixed(2)}s`);
}
module.exports = processOutbox;
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/outbox/index.test.js (1)
59-71: LGTM!The test correctly verifies the structured error logging behavior when
processOutboxthrows. It follows the established pattern from thestartProcessing guardtest above.Consider also asserting that the error object is captured in the log for completeness:
assert.ok(errorLog.err); assert.equal(errorLog.err.message, 'Unexpected failure');This would ensure the error details are available for debugging, but it's optional since the current assertion style is consistent with the existing test at lines 51-55.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/outbox/index.test.js` around lines 59 - 71, Add assertions to verify the captured error object is present and contains the expected message after startProcessing triggers a thrown error: after locating the log with findByEvent(logCapture.output, 'outbox.processing.error') (used already) assert that errorLog.err exists and that errorLog.err.message equals 'Unexpected failure' so the test ensures the error details from processOutboxStub rejection are logged; keep using the existing processOutboxStub and service.startProcessing setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/unit/server/services/outbox/index.test.js`:
- Around line 59-71: Add assertions to verify the captured error object is
present and contains the expected message after startProcessing triggers a
thrown error: after locating the log with findByEvent(logCapture.output,
'outbox.processing.error') (used already) assert that errorLog.err exists and
that errorLog.err.message equals 'Unexpected failure' so the test ensures the
error details from processOutboxStub rejection are logged; keep using the
existing processOutboxStub and service.startProcessing setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cd8712b-1e81-4c32-910b-ebff2eadac80
📒 Files selected for processing (3)
ghost/core/core/server/services/outbox/index.jsghost/core/core/server/services/outbox/jobs/lib/process-outbox.jsghost/core/test/unit/server/services/outbox/index.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/outbox/index.js
ref https://linear.app/ghost/issue/NY-1020 Follows up on #26599 to convert the remaining logging statements in the outbox service, process-entries, process-outbox, and member welcome email service to use structured logging with system.event fields
The !payload check could misclassify failures if JSON.parse returned a falsy value. Using a payloadParsed boolean makes the intent clearer.
Kept the existing pattern where processOutbox() returns a status message string and the caller logs it, to avoid behavioral changes beyond the structured logging conversion
Clearer than using a flag to distinguish the two failure modes
Restored original control flow structure — only the logging calls are changed to structured format, no other behavioral changes
cmraible
force-pushed
the
chris-ny-1020-remaining-structured-logs
branch
from
20a6641 to
19651ff
Compare
| entry_id: entry.id | ||
| }, | ||
| err | ||
| }, `${handler.LOG_KEY} Failed to send to ${handler.getLogInfo(payload)}: ${errorMessage}`); |
There was a problem hiding this comment.
It looks like logging.error actually automatically appends the error message here
so that means it gets duplicated - this is what i get locally:
[2026-03-06 15:14:18] ERROR [OUTBOX][MEMBER-WELCOME-EMAIL] Failed to send to faesdvf@afewsdv.com: Manual verification error Manual verification error
so maybe we don't need to append the errorMessage here (and elsewhere in outbox/welcome email log errors) ourselves? though we would have to leave the colon to make it read better, which is kinda clunky lol
ironic this pops up after i suggested last week that we shouldn't assert the error message in tests lol. though i think those tests were passing, and didn't have the duplication 🤔 so idk if it woulda caught this
There was a problem hiding this comment.
Oh, nice find! I'll update all the instances here to not append the errorMessage, and maybe change the : to . so it makes a bit more sense with or without the actual error message.
GhostLogger auto-appends err.message to the log output, so including errorMessage in the message string caused duplication. Changed trailing colons to periods so the message reads naturally with the appended error.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Now that the error message is auto-appended by GhostLogger via the err object, the manually extracted errorMessage/cleanupError variables are no longer needed for the log calls
2 participants
closes https://linear.app/ghost/issue/NY-1020/convert-outbox-logs-to-structured-logs
Summary
system.eventfieldsNew structured events
outbox.processing.erroroutbox/index.jsoutbox.entry.no_handlerprocess-entries.jsoutbox.entry.payload_parse_failedprocess-entries.jsoutbox.entry.send_failedprocess-entries.jsoutbox.entry.delete_failedprocess-entries.jsoutbox.init.failedprocess-outbox.jsoutbox.batch.completeprocess-outbox.jsoutbox.member_created.no_automated_emailmember-created.jsmember_welcome_email.sendingservice.jsNote for reviewer
The
logging.info(statusMessage)inoutbox/index.jsis left as-is —processOutbox()returns a string that the caller logs. Converting this one would require changing the return value contract. Worth discussing whether the return-for-logging pattern should be refactored in a separate PR.Test plan
process-outbox.test.jsstill passes (exercises full pipeline)ref https://linear.app/ghost/issue/NY-1020
Note
Low Risk
Logging-only changes across outbox processing and member welcome emails; low risk aside from potential differences in log output/alerting and added metadata payload size.
Overview
Converts remaining outbox processing and member welcome email log lines to structured logs with
system.eventfields, and attaches useful metadata such asmember_status,slug,event_type,entry_id, and batch timing/counts.Standardizes error/warn reporting in
outbox/index.js,process-outbox.js,process-entries.js,handlers/member-created.js, andmember-welcome-emails/service.js(including passing the caughterrobject), and expands unit tests to assert the new structured events (plus adds a newprocess-entriesunit test suite).Written by Cursor Bugbot for commit dd730e7. This will update automatically on new commits. Configure here.