feat(retry): add structured retry-event logging (ADA-736) - #263
feat(retry): add structured retry-event logging (ADA-736)#263bcamarneiro wants to merge 1 commit into
Conversation
Emit machine-parseable retry_attempt and retry_exhausted events from fetchWithRetry so monitoring dashboards and error-reporting tools can surface retry storms, rate-limiting patterns, and backoff efficacy without grepping ad-hoc console lines. - New retryLogging.ts: StructuredRetryEvent type + logStructuredRetry() - retryClient.ts: emit events for retryable-status and network-error retries, plus exhausted-attempts terminal events - URL scrubbing: never logs tokens/auth embedded in URLs
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Review (ADA-736) — Verdict: CONCERNSReal, in-scope change: structured
Minor (non-blocking): the Reviewed by Hermes Agent. |
Review verdict: concerns (ADA-736)Real, in-scope change: structured retry-event logging (retryLogging.ts + retryClient.ts integration) is additive, typed, well-documented, and preserves existing retry/backoff/abort semantics — no regression risk found in the retry loop itself. Concerns
None of these block merge outright; all are cheap to fix. |
There was a problem hiding this comment.
🟡 Not ready to approve
The structured logging path has a correctness gap for URL inputs (logged as "unknown") and the new URL-safety contract is currently documented in a potentially misleading way.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds structured, machine-parseable retry telemetry to fetchWithRetry so retry behavior can be monitored (attempts vs exhaustion) without relying on ad-hoc console text.
Changes:
- Introduces
StructuredRetryEvent+logStructuredRetry()forretry_attempt/retry_exhaustedconsole events. - Integrates structured event emission into
fetchWithRetryfor retryable HTTP statuses and network errors. - Adds URL “scrubbing” helper to avoid logging full origins in retry events.
File summaries
| File | Description |
|---|---|
| frontend/services/retryLogging.ts | Adds the structured event type and logger used for retry attempt/exhaustion events. |
| frontend/services/retryClient.ts | Emits structured retry events from the retry loop and adds helpers to produce a “safe” URL for logs. |
Review details
Suppressed comments (1)
frontend/services/retryClient.ts:358
- Structured retry logging is newly introduced here, but the existing
retryClientunit tests don’t assert that the correct structured payload is emitted (or that URLs are scrubbed). Consider spying onconsole.warn/console.errorin tests to both validate the emittedStructuredRetryEventand avoid noisy stderr output during test runs.
const delayMs = calculateBackoffDelayMs(attempt, cfg, retryAfter);
logStructuredRetry(
buildRetryEvent('retry_attempt', input, attempt, cfg.maxRetries, delayMs, res.status),
);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| /** Extract a safe (path+query) URL from a RequestInfo for log events. */ | ||
| function safeUrl(input: RequestInfo): string { | ||
| if (typeof input === 'string') return sanitiseUrl(input); | ||
| if (input instanceof Request) return sanitiseUrl(input.url); | ||
| return 'unknown'; | ||
| } |
| /** | ||
| * The request URL (path + query only — never includes credentials in the | ||
| * query string, which some APIs require). Redact if needed before calling. | ||
| */ |
What
Emits machine-parseable
retry_attemptandretry_exhaustedevents fromfetchWithRetryso monitoring dashboards and error-reporting tools can surface retry storms, rate-limiting patterns, and backoff efficacy without grepping ad-hoc console lines.Changes
frontend/services/retryLogging.ts—StructuredRetryEventtype withevent,url,attempt,maxRetries,delayMs,status, anderrorfields, pluslogStructuredRetry()that usesconsole.warn(retry attempts) andconsole.error(exhausted retries) so events are collected by frontend error reporters.frontend/services/retryClient.ts— integrated structured logging into the retry loop:retry_attemptevents on each retryable status or network error,retry_exhaustedwhen all attempts are consumed. URL scrubbing strips origin to avoid logging embedded auth tokens.Verification