add webhook debugging logs #924
Conversation
WalkthroughAdds structured logging and per-webhook idempotency job IDs when enqueuing transaction webhooks; enriches webhook worker logs with transactionId, hmacMode, response details, and changes 5xx handling to escalate and rethrow to trigger retries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Producer as Enqueue code
participant Queue as Job Queue
participant Worker as Webhook Worker
Producer->>Producer: compute idempotencyKey per-webhook\nlog pre-enqueue (eventType, queueId, count)
Producer->>Queue: enqueue job (jobId = idempotencyKey, payload)
Producer->>Producer: log post-enqueue (destination, webhookId, idempotencyKey)
Queue->>Worker: deliver job (payload, jobId)
Worker->>Worker: extract transactionId from data.queueId\nlog attempt (transactionId,destination,eventType,webhookId,hmacMode)
Worker->>External: send HTTP webhook
alt success (2xx)
External-->>Worker: response (status, body)
Worker->>Worker: log info with responseCode, truncated body
else client/other non-5xx
External-->>Worker: response
Worker->>Worker: log warn with responseCode, truncated body
else server error (5xx)
External-->>Worker: response
Worker->>Worker: log error with responseCode, truncated body, queueId
Worker->>Worker: throw to trigger retry
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
src/worker/queues/send-webhook-queue.ts (1)
165-177: Consider log verbosity for high webhook counts.While the per-webhook logging is valuable for debugging, it may become verbose if many webhooks are configured for a transaction event. Consider using a debug log level or batching the log output if verbosity becomes an issue in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/worker/queues/send-webhook-queue.ts(2 hunks)src/worker/tasks/send-webhook-worker.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/worker/queues/send-webhook-queue.ts (1)
src/shared/utils/logger.ts (1)
logger(89-116)
src/worker/tasks/send-webhook-worker.ts (2)
src/shared/utils/env.ts (1)
env(23-188)src/shared/utils/logger.ts (1)
logger(89-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: lint
🔇 Additional comments (5)
src/worker/queues/send-webhook-queue.ts (2)
16-16: LGTM!The logger import is correctly added to support the new debugging logs.
141-163: Good idempotency implementation.The introduction of an idempotency key based on webhook URL, event type, and queue ID ensures that duplicate webhook deliveries are prevented at the job queue level. Using it as the
jobIdallows BullMQ to handle deduplication automatically.src/worker/tasks/send-webhook-worker.ts (3)
31-35: LGTM!The transactionId extraction is safe and correctly typed as potentially undefined.
85-95: LGTM!The warning log for missing transactions provides valuable debugging context and is correctly scoped within the transaction event handler.
146-158: LGTM!The enhanced 5xx error logging with additional context fields and error-level severity is appropriate for debugging retry scenarios.
| // Log webhook attempt with HMAC info | ||
| const hmacMode = env.ENABLE_CUSTOM_HMAC_AUTH ? "custom" : "standard"; | ||
| logger({ | ||
| service: "worker", | ||
| level: "info", | ||
| message: `[Webhook] Attempting to send webhook for transaction ${transactionId} at destination ${webhook.url}`, | ||
| queueId: transactionId, | ||
| data: { | ||
| eventType: data.type, | ||
| destination: webhook.url, | ||
| webhookId: webhook.id, | ||
| hmacMode, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Handle undefined transactionId in log messages.
For non-transaction webhook types (e.g., BACKEND_WALLET_BALANCE, WALLET_SUBSCRIPTION), transactionId will be undefined, resulting in log messages like "for transaction undefined". Consider using conditional formatting to improve log clarity.
Apply this diff to improve the log message:
- message: `[Webhook] Attempting to send webhook for transaction ${transactionId} at destination ${webhook.url}`,
+ message: transactionId
+ ? `[Webhook] Attempting to send webhook for transaction ${transactionId} at destination ${webhook.url}`
+ : `[Webhook] Attempting to send webhook at destination ${webhook.url}`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Log webhook attempt with HMAC info | |
| const hmacMode = env.ENABLE_CUSTOM_HMAC_AUTH ? "custom" : "standard"; | |
| logger({ | |
| service: "worker", | |
| level: "info", | |
| message: `[Webhook] Attempting to send webhook for transaction ${transactionId} at destination ${webhook.url}`, | |
| queueId: transactionId, | |
| data: { | |
| eventType: data.type, | |
| destination: webhook.url, | |
| webhookId: webhook.id, | |
| hmacMode, | |
| }, | |
| }); | |
| // Log webhook attempt with HMAC info | |
| const hmacMode = env.ENABLE_CUSTOM_HMAC_AUTH ? "custom" : "standard"; | |
| logger({ | |
| service: "worker", | |
| level: "info", | |
| message: transactionId | |
| ? `[Webhook] Attempting to send webhook for transaction ${transactionId} at destination ${webhook.url}` | |
| : `[Webhook] Attempting to send webhook at destination ${webhook.url}`, | |
| queueId: transactionId, | |
| data: { | |
| eventType: data.type, | |
| destination: webhook.url, | |
| webhookId: webhook.id, | |
| hmacMode, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In src/worker/tasks/send-webhook-worker.ts around lines 37 to 50, the log
includes "for transaction ${transactionId}" but transactionId can be undefined
for non-transaction webhook types; change the log to conditionally include the
transaction id only when present (e.g., "for transaction <id>" or otherwise use
the event type or a generic label like "non-transaction event"), and ensure
queueId/data fields reflect the same (omit queueId or set to a fallback such as
data.type) so logs are clear and don't show "undefined".
| // Log the response | ||
| if (resp) { | ||
| const logLevel = resp.ok ? "info" : resp.status >= 500 ? "error" : "warn"; | ||
| logger({ | ||
| service: "worker", | ||
| level: logLevel, | ||
| message: `[Webhook] Webhook response received: ${resp.status} for transaction ${transactionId} at destination ${webhook.url}`, | ||
| queueId: transactionId, | ||
| data: { | ||
| eventType: data.type, | ||
| destination: webhook.url, | ||
| webhookId: webhook.id, | ||
| responseCode: resp.status, | ||
| responseOk: resp.ok, | ||
| hmacMode, | ||
| responseBody: resp.body.substring(0, 200), // Truncate response body to first 200 chars | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Guard against non-string response body.
Line 135 calls resp.body.substring(0, 200) without verifying that body is a string. If the webhook response body is an object, null, or undefined, this will throw a runtime error.
Apply this diff to safely handle the response body:
- responseBody: resp.body.substring(0, 200), // Truncate response body to first 200 chars
+ responseBody: typeof resp.body === 'string'
+ ? resp.body.substring(0, 200)
+ : JSON.stringify(resp.body).substring(0, 200), // Truncate response body to first 200 chars📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Log the response | |
| if (resp) { | |
| const logLevel = resp.ok ? "info" : resp.status >= 500 ? "error" : "warn"; | |
| logger({ | |
| service: "worker", | |
| level: logLevel, | |
| message: `[Webhook] Webhook response received: ${resp.status} for transaction ${transactionId} at destination ${webhook.url}`, | |
| queueId: transactionId, | |
| data: { | |
| eventType: data.type, | |
| destination: webhook.url, | |
| webhookId: webhook.id, | |
| responseCode: resp.status, | |
| responseOk: resp.ok, | |
| hmacMode, | |
| responseBody: resp.body.substring(0, 200), // Truncate response body to first 200 chars | |
| }, | |
| }); | |
| } | |
| // Log the response | |
| if (resp) { | |
| const logLevel = resp.ok ? "info" : resp.status >= 500 ? "error" : "warn"; | |
| logger({ | |
| service: "worker", | |
| level: logLevel, | |
| message: `[Webhook] Webhook response received: ${resp.status} for transaction ${transactionId} at destination ${webhook.url}`, | |
| queueId: transactionId, | |
| data: { | |
| eventType: data.type, | |
| destination: webhook.url, | |
| webhookId: webhook.id, | |
| responseCode: resp.status, | |
| responseOk: resp.ok, | |
| hmacMode, | |
| responseBody: typeof resp.body === 'string' | |
| ? resp.body.substring(0, 200) | |
| : JSON.stringify(resp.body).substring(0, 200), // Truncate response body to first 200 chars | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/worker/tasks/send-webhook-worker.ts around lines 120-138, resp.body is
assumed to be a string and resp.body.substring(0, 200) can throw if body is
null/undefined or an object; update the logging to safely handle non-string
bodies by normalizing the body before truncation: if body is a string use it, if
it's an object stringify it (catching possible errors), and if null/undefined
use an empty string, then truncate the normalized string to 200 chars for
responseBody in the logger; ensure this normalization is done inline or via a
small helper and avoid calling substring on non-strings.
| logger({ | ||
| service: "worker", | ||
| level: logLevel, | ||
| message: `[Webhook] Webhook response received: ${resp.status} for transaction ${transactionId} at destination ${webhook.url}`, |
There was a problem hiding this comment.
Handle undefined transactionId in log messages.
Similar to the earlier log, this message will show "for transaction undefined" for non-transaction webhooks.
Apply this diff:
- message: `[Webhook] Webhook response received: ${resp.status} for transaction ${transactionId} at destination ${webhook.url}`,
+ message: transactionId
+ ? `[Webhook] Webhook response received: ${resp.status} for transaction ${transactionId} at destination ${webhook.url}`
+ : `[Webhook] Webhook response received: ${resp.status} at destination ${webhook.url}`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message: `[Webhook] Webhook response received: ${resp.status} for transaction ${transactionId} at destination ${webhook.url}`, | |
| message: transactionId | |
| ? `[Webhook] Webhook response received: ${resp.status} for transaction ${transactionId} at destination ${webhook.url}` | |
| : `[Webhook] Webhook response received: ${resp.status} at destination ${webhook.url}`, |
🤖 Prompt for AI Agents
In src/worker/tasks/send-webhook-worker.ts around line 126, the log message
interpolates transactionId directly and can print "for transaction undefined"
for non-transaction webhooks; change the message construction to conditionally
include the "for transaction <id>" segment only when transactionId is defined
(or use a safe fallback like 'N/A') so logs don't show "undefined" — e.g. build
the message string with a ternary or template fragment that appends `for
transaction ${transactionId}` only when transactionId is truthy.
d4mr
force-pushed
the
pb/webhook-debugging-logs
branch
from
54cd1cc to
3b83955
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/worker/queues/send-webhook-queue.ts (2)
141-150: Good observability enhancement.The summary log provides a clear high-level view before processing webhooks. The structured data with
eventTypeandwebhookCountwill be valuable for debugging.Minor suggestion: The message could be more concise: "Enqueuing transaction webhooks to queue for transaction" → "Enqueuing webhooks for transaction" (the redundancy of "transaction" and "queue" could be reduced).
133-178: Consider adding similar logging to other webhook types.Transaction webhooks now have comprehensive logging, but the other webhook enqueueing methods (
_enqueueContractSubscriptionWebhook,_enqueueBackendWalletBalanceWebhook,_enqueueWalletSubscriptionWebhook) lack similar observability.For consistent debugging across all webhook types, consider adding similar structured logging to these methods as well. This could be done in a follow-up PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/worker/queues/send-webhook-queue.ts(2 hunks)src/worker/tasks/send-webhook-worker.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/worker/tasks/send-webhook-worker.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/worker/queues/send-webhook-queue.ts (1)
src/shared/utils/logger.ts (1)
logger(89-116)
🔇 Additional comments (3)
src/worker/queues/send-webhook-queue.ts (3)
16-16: LGTM!The logger import is correctly placed and necessary for the new debugging functionality.
155-163: Good functional improvement: idempotency enforcement.Extracting the idempotency key to a variable improves readability, and using it as
jobIdensures BullMQ will deduplicate jobs automatically. This prevents duplicate webhook deliveries for the same webhook/event/transaction combination.Note: This is a functional change (enforcing deduplication via BullMQ's job ID mechanism), not just a logging addition. The behavior change is positive and improves reliability.
165-177: Detailed per-webhook tracking added.The per-webhook log entries provide excellent granularity for debugging webhook delivery issues. The structured data includes all relevant identifiers for tracking individual webhook jobs.
Note: If a transaction triggers many webhooks, this will generate multiple log entries per transaction. This verbosity is appropriate for debugging but can be controlled via the
LOG_SERVICESenvironment variable.
PR-Codex overview
This PR enhances the logging functionality in the
SendWebhookQueueandsend-webhook-workermodules. It adds detailed logs for enqueuing webhooks, sending attempts, and responses, including transaction IDs and HMAC information for better traceability and debugging.Detailed summary
loggerimport tosend-webhook-queue.ts.Summary by CodeRabbit