Skip to content

Improved webhook delivery failure logging#27032

Merged
kevinansfield merged 1 commit intomainfrom
webhook-delivery-logging
Apr 1, 2026
Merged

Improved webhook delivery failure logging#27032
kevinansfield merged 1 commit intomainfrom
webhook-delivery-logging

Conversation

@kevinansfield
Copy link
Copy Markdown
Member

@kevinansfield kevinansfield commented Mar 31, 2026

ref https://linear.app/ghost/issue/ONC-1609/

  • Promoted webhook delivery failure logs from warn to error level so they surface in error-level monitoring
  • Added structured [WEBHOOK_DELIVERY_FAILURE] prefix with url, status, error_code, and message fields for easy SLO alerting
  • Added unit tests verifying the structured log output and fallback values

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 31, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3be232b8-9e04-4b1e-92f5-7c460c053488

📥 Commits

Reviewing files that changed from the base of the PR and between 501ea90 and 80ceb93.

📒 Files selected for processing (2)
  • ghost/core/core/server/services/webhooks/webhook-trigger.js
  • ghost/core/test/unit/server/services/webhooks/trigger.test.js
✅ Files skipped from review due to trivial changes (1)
  • ghost/core/test/unit/server/services/webhooks/trigger.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • ghost/core/core/server/services/webhooks/webhook-trigger.js

Walkthrough

The changes adjust webhook delivery failure logging and add unit tests. The webhook trigger now logs failures at error level with a structured [WEBHOOK_DELIVERY_FAILURE] tag and includes URL (fallback unknown), HTTP status (err.statusCode || 'none'), error code (err.code || 'unknown'), and error message, passing the original error object to the logger. Existing error handling flow (including 410 handling) is unchanged. Tests were added to assert the new logging format and fallback values when properties are absent.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Improved webhook delivery failure logging' directly summarizes the main change: enhancing webhook failure logging with higher severity level and structured fields.
Description check ✅ Passed The description is directly related to the changeset, detailing the promotion to error level, addition of structured logging tags, and unit tests covering these changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch webhook-delivery-logging

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
ghost/core/core/server/services/webhooks/webhook-trigger.js (1)

94-94: Consider preserving the error stack trace for debugging.

The structured log format is good for SLO alerting, but the current approach loses the error's stack trace which can be valuable for debugging delivery failures. Per the conventional pattern in the codebase (e.g., stripe/webhook-controller.js line 102 uses logging.error(string, err)), you can pass the error object as a second argument.

♻️ Suggested improvement to preserve stack trace
-            logging.error(`[WEBHOOK_DELIVERY_FAILURE] url=${webhook.get('target_url') || 'unknown'} status=${err.statusCode || 'none'} error_code=${err.code || 'unknown'} message=${err.message || ''}`);
+            logging.error(`[WEBHOOK_DELIVERY_FAILURE] url=${webhook.get('target_url') || 'unknown'} status=${err.statusCode || 'none'} error_code=${err.code || 'unknown'} message=${err.message || ''}`, err);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ghost/core/core/server/services/webhooks/webhook-trigger.js` at line 94, The
webhook delivery failure log currently formats key fields but drops the error
stack; update the logging.error call in webhook-trigger.js (the line that logs
`[WEBHOOK_DELIVERY_FAILURE] url=${webhook.get('target_url') ...}` and references
the err object) to pass the err object as a second argument (e.g.,
logging.error(<formatted message>, err)) so the structured message is preserved
while the error stack/metadata from err is also recorded.
🤖 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/core/server/services/webhooks/webhook-trigger.js`:
- Line 94: The webhook delivery failure log currently formats key fields but
drops the error stack; update the logging.error call in webhook-trigger.js (the
line that logs `[WEBHOOK_DELIVERY_FAILURE] url=${webhook.get('target_url') ...}`
and references the err object) to pass the err object as a second argument
(e.g., logging.error(<formatted message>, err)) so the structured message is
preserved while the error stack/metadata from err is also recorded.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 02125a61-e781-4bd8-ac01-da7a1b02b4ea

📥 Commits

Reviewing files that changed from the base of the PR and between 952a029 and 501ea90.

📒 Files selected for processing (2)
  • ghost/core/core/server/services/webhooks/webhook-trigger.js
  • ghost/core/test/unit/server/services/webhooks/trigger.test.js

Webhook delivery errors were logged at warn level with unstructured messages,
making them invisible to error-level monitoring. Promoted to error level with
a structured [WEBHOOK_DELIVERY_FAILURE] prefix for easy log filtering.
@kevinansfield kevinansfield force-pushed the webhook-delivery-logging branch from 501ea90 to 80ceb93 Compare April 1, 2026 08:40
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 1, 2026

@kevinansfield kevinansfield enabled auto-merge (squash) April 1, 2026 08:44
@kevinansfield kevinansfield merged commit 5a4041c into main Apr 1, 2026
65 of 67 checks passed
@kevinansfield kevinansfield deleted the webhook-delivery-logging branch April 1, 2026 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant