Expose proxy errors as first-class browser telemetry events - #331
Conversation
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 30fe6f1. Configure here.
|
Reviewed with a focus on correctness. Leaving this as a comment rather than a formal review. Needs changing1. The
Three more documented values aren't reachable as header values:
Detection still fires either way since the value is passed through verbatim, but the spec is the published contract and it's already mirrored downstream, so a consumer matching the documented list silently drops a large share of real proxy errors. 2.
3.
4. The event is only deliverable to sessions that opted into the full network stream.
Worth a look5. The header is origin-controlled and nothing checks provenance. Any site can serve a 502 carrying 6. 7. No dedup or rate limit. One event per failed request means volume tracks the outage, into a fixed-capacity ring, at the moment the pipeline is most contended. Checked outRegenerated One thing I couldn't confirm: that bumped HTTPS responses surface the header normally through Minor
|
|
Second pass. Vocabulary, OTLP severity,
Smaller: the limiter keys on Withdrawing my Last thing, I'd stop calling this anti-spoofing. |
110a2bc to
8556738
Compare
Sayan-
left a comment
There was a problem hiding this comment.
Ask: pls update the pr desc since it's stale.
Final pass, focused on correctness and parity.
Follow-ups (not blocking)
Rate limiter tuning. 1s per sessionID + code means at most one URL per code per
second. In a 6h prod sample 54% of branded errors shared a (second, code) bucket. The
schema and README promise per-URL attribution, and the unsampled aggregate already exists
server-side, so per-URL detail is the whole point of this event. Worth tightening the
interval or adding resource_type to the key, and documenting the sampling either way.
Severity. proxy_error maps to OTLP ERROR next to service_crashed, while
network_loading_failed is WARN. In practice ~99% of branded proxy errors are subresources
(blocked scripts, images, fetches). Worth revisiting to WARN, or ERROR only for Document.
WebSocket handshakes. The producer's comment says the header exists for WebSocket and
subresource requests. Subresources are covered; WebSockets aren't (dispatchEvent has no
Network.webSocket* case). Either handle it or call it a documented non-goal.
Smaller:
- Unknown codes drop silently with no log, inside
proxyErrorRateLimited. Worth a log line
and a comment pinning the enum to the producer. NoteTestProxyErrorRateLimituses a nil
logger so adding one panics it. codeisn't promoted to an OTLP attribute.promotedAttributesdoes this for console
levelfor the same reason; no collision.proxyLastEmitcomment says entries stay bounded, but they aren't pruned on detach.- Spec says
statusis 502; the gate is>= 500. - README got
request_idbut notframe_id,loader_id,nav_seq, and the network field
table has noproxy_errorrow. - Case-insensitivity test uses
source_auth_error, which nothing emits andValid()drops.
No test covers the< 500gate, unknown codes through the handler, or the untracked
nav_seqfallback. The two handler subtests only pass because they use different codes. - Stray blank line at
handlers.go:666. - Origin-supplied
X-Kernel-Proxy-Errorisn't stripped by the producer, so any site can
forge an event. This PR's own e2e test demonstrates it. Fix belongs on the producer side.
|
thanks for the pass. on the follow-ups — quick status of what was decided / left: deferred to the producer (not in this PR)
documented non-goals
decisions
everything else from the final pass (otlp |

Summary
The metro egress host-proxy serves branded 5xx error pages carrying an
X-Kernel-Proxy-Errorheader with a typed code when a proxy-layer failure occurs. This change makes those failures first-class in browser telemetry from the image side: the CDP collector now emits a dedicated low-volumeproxy_errorevent carrying the code, instead of burying them in the rawnetwork_responsestream.What changed
openapi.yaml+ regeneratedoapi.go/category_gen.go: newBrowserProxyErrorEvent/BrowserProxyErrorEventDataschema (proxy_error, categorynetwork) registered in theKnownBrowserTelemetryEventdiscriminated union.codeis a typed enum mirroring the proxy's wire values (destination_blocked,provider_blacklisted,provider_unreachable,proxy_unavailable,upstream_timeout,upstream_dns_failure,upstream_connect_failed);statusis a requiredint(502).cdpmonitor/handlers.go: onNetwork.responseReceived, when a 5xx response carriesX-Kernel-Proxy-Error, emitproxy_errorwith the header value ascode, plusstatus,url,method,request_id,nav_seq, and target/frame context. Header lookup is case-insensitive; origin 5xx pass-through is untouched (no masking). Requests in flight at CDP attach get their context from the CDP params.codeis validated against the generated enum before use, so unknown header values are dropped and the rate-limit map stays bounded; emission is deduplicated per (session, code) with a 1s min interval so volume tracks an outage without flooding the ring.lib/events/otlpconvert.go:proxy_errormaps to ERROR severity only for the top-level Document and WARN for subresources;statuspromotes tohttp.response.status_code.Notes / trade-offs
codeis always the real header value. No derived/synthetic codes.networktelemetry category (CDP-derived and opt-in). Its value is per-session/per-URL attribution for sessions already capturing the network stream — not a default-on alerting signal (proxy failures are only observable while the CDP collector runs).oapi.godiff is large because it is regenerated (embedded spec + union accessors).Related
BrowserTelemetryEventunion + Stainless SDK models so consumers can type againstproxy_error.Tests
go test ./lib/cdpmonitor/ ./lib/events/— green (unit + real-Chromium e2e).Note
Medium Risk
Adds new telemetry on the hot CDP response path (gated to 502 + header) and changes OTLP alerting semantics for document vs subresource proxy failures; scope is bounded by enum validation and sampling.
Overview
Introduces
proxy_erroras a first-class network telemetry event when the CDP monitor classifies a 502 response that carriesX-Kernel-Proxy-Error, surfacing metro egress proxy failures with a typedcodeinstead of only genericnetwork_responsetraffic.Schema & pipeline: OpenAPI adds
BrowserProxyErrorEvent(enum codes aligned with metro), regeneratedoapi.go/category_gen.go, and union wiring.cdpmonitordetects the header onNetwork.responseReceived(502-only gate), fills request/nav context from pending state or CDP params, validates codes against the enum, and rate-limits to at most one emit per session+code+resource type per second.OTLP: Promotes
codetokernel.proxy_error_code;proxy_errorseverity is ERROR forDocumentand WARN for subresources.Unit tests, a Chromium e2e, and README taxonomy updates cover the new path. Events remain opt-in via the network CDP collector category.
Reviewed by Cursor Bugbot for commit ed5697e. Bugbot is set up for automated code reviews on this repo. Configure here.