feat: add OpenTelemetry error reporting - #13
Conversation
77d999b to
9092b14
Compare
9092b14 to
d0e977b
Compare
Keep both renderLandingPage and captureError in worker entrypoint.
murderteeth
left a comment
There was a problem hiding this comment.
Summary
Adds vendor-neutral OpenTelemetry error reporting to the Cloudflare Worker. Since the OTel Node SDK can't run on Workers, src/observability.ts hand-builds an OTLP/HTTP JSON log record and POSTs it via ctx.waitUntil (off the latency path). Fires on unexpected errors and on ApiError with status ≥ 500; no-op until OTEL_EXPORTER_OTLP_ENDPOINT (or _LOGS_ENDPOINT) is set. Verified locally: typecheck passes, 43/43 tests pass, wrangler deploy --dry-run bundles cleanly. Payload carries only error.name/message/stack — no request data or secrets.
Dependencies
- package.json unchanged — but
bun.lockgained an orphan@opentelemetry/api@1.9.1entry (see Issues). The package itself is benign (official OTel org, no install scripts), but nothing imports it.
Issues
- bun.lock:171 — Orphan
@opentelemetry/api@1.9.1lockfile entry with nopackage.jsoncounterpart; residue from the abandoned SDK approach, and it does get installed (resolves vitest's optional peer).- Change: revert the
bun.lockhunk so the lockfile matches the manifest. - Done when:
bun install --frozen-lockfilesucceeds and@opentelemetry/apino longer appears inbun.lockornode_modules.
- Change: revert the
Suggestions
- src/index.ts:92 / src/edge-cache.ts:79 —
ctx.waitUntil(edgeCache().put(...))has no.catch, so background cache-write failures become unhandled rejections that never reach the OTLP backend. This PR is the natural place to route them throughcaptureError. - test/observability.test.ts:59 — the "reports rejected exports" test stubs a resolved 401; the actual fetch-rejection
.catchbranch (src/observability.ts:71) is untested, and the documented no-op path (endpoint unset → zero fetches) has no test either. - src/observability.ts:39 —
String(Date.now() * 1_000_000)exceedsMAX_SAFE_INTEGER;String(BigInt(Date.now()) * 1_000_000n)is exact. - src/observability.ts:16 —
parseHeaderssplits on,without percent-decoding, diverging from the OTLP env-var spec; a header value containing a comma would be split into a bogus second header.
Verdict
APPROVE
How This Was Reviewed
This review was conducted using the review-pr skill.
|
There was a problem hiding this comment.
Summary
Re-review of 8668b12 (delta: one commit since ee2411a). Prior review items verified: orphan @opentelemetry/api lockfile entry removed (frozen install clean), edge-cache waitUntil failures now routed through captureError, real fetch-rejection and no-op tests added without weakening the old ones, timeUnixNano now exact via BigInt. Typecheck, tests (46/46), and wrangler deploy --dry-run all pass. One new blocking defect introduced by the parseHeaders rewrite.
Issues
- src/observability.ts:22 — The new
decodeURIComponentcalls inparseHeadersare unguarded, so a malformedOTEL_EXPORTER_OTLP_HEADERSvalue containing a raw un-encoded%(e.g.authorization=Api-Key 50%off) throwsURIErrorsynchronously insidecaptureError. Verified by execution. At src/index.ts:111/123 the throw escapes the catch block, turning every 5xx into a Cloudflare 1101 worker exception instead of the JSON error response; at src/edge-cache.ts:88 it rejects thewaitUntilpromise, re-introducing the uncaught background rejection this revision just fixed.- Change: wrap the two decodes in a try/catch inside
parseHeadersonly — fall back to the raw (undecoded) key/value or skip the malformed pair. - Keep: the comma-split + percent-decode behavior for well-formed values must remain (a
%2Cin a value still decodes to,); the existing observability tests must still pass. - Done when: a test sets
OTEL_EXPORTER_OTLP_HEADERSto a value with a raw%and assertscaptureErrordoes not throw, alongside the existing decode tests passing.
- Change: wrap the two decodes in a try/catch inside
(This is a new regression introduced when applying previous review feedback. Lets prioritize a spec for automated pr reviews and cite this as a reference case.)
Verdict
REQUEST_CHANGES
How This Was Reviewed
This review was conducted using the review-pr skill.
There was a problem hiding this comment.
Previous round's item is resolved — decodeHeaderPart guards the decode, %2C still decodes, raw-% test added. Branch on its own is green (typecheck clean, 47/47).
Issues
- Resolve conflicts with
main
Verdict
REQUEST_CHANGES
How This Was Reviewed
This review was conducted using the review-pr skill.
murderteeth
left a comment
There was a problem hiding this comment.
Summary
PR #13 adds vendor-neutral OTLP/HTTP error reporting for Worker failures. The latest revision resolves the previous malformed-header issue and cleanly incorporates current main. No blocking issues found.
Issues
None.
Suggestions
None.
Verdict
APPROVE
How This Was Reviewed
- Reviewed the complete diff against current
main - Verified prior review findings and conflict resolution
- Confirmed no new dependencies
bun run typecheckpassedbun run testpassed: 101/101 testswrangler deploy --dry-runpassed- GitHub CI checks are green
- Visual verification was not applicable because there are no UI changes
This review was conducted using the review-pr skill.
|
on hold until doppler - so we can get better deployments |
Summary
Report Worker errors as OpenTelemetry log records over OTLP/HTTP. Cloudflare Workers can't run the OpenTelemetry Node SDK (its OTLP exporter is Node-coupled), so the OTLP/HTTP log payload is built and sent directly via
fetch— still standard OTLP. Vendor-neutral: pointOTEL_EXPORTER_OTLP_ENDPOINTorOTEL_EXPORTER_OTLP_LOGS_ENDPOINT(and the matchingOTEL_EXPORTER_OTLP_*_HEADERSfor auth) at any OTLP backend. No-op until configured.How to review
src/observability.ts:captureError(ctx, env, error)POSTs an OTLPLogsDatapayload (ERROR severity,exception.*attributes). Signal-specific logs endpoints are used as-is; the generic endpoint gets/v1/logsappended. Headers are percent-decoded per the OTLP env-var spec.timeUnixNanoisBigInt(Date.now()) * 1_000_000n. Usesctx.waitUntilso the export completes after the response is returned.src/index.ts: unexpected-error andApiErrorwith status ≥ 500 callcaptureError.OTEL_*fields are on theEnvtype.src/edge-cache.ts: backgroundcaches.default.putfailures are routed throughcaptureError.Test plan
OTEL_EXPORTER_OTLP_ENDPOINT(+ headers), trigger the 500 branch, confirm the log record reaches the backend.bun run testandbun run typecheck.Risk / impact
Low. With the OTLP endpoint unset (the default), reporting is a no-op and behavior is unchanged. No funds, auth, or migration paths touched.