OTLP browser telemetry export: follow-up improvements (KERNEL-1596)#317
Conversation
643f503 to
5e56d6a
Compare
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Export is off by default and gated on the toggle, independent of whether an endpoint is provisioned, so an always-injected relay does not export by default. Writer lifecycle moves under a runtime-toggleable controller. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Surfaces queue-overflow drops (previously log-only) plus export failures and exported counts as Prometheus counters, owned above the writer so they stay monotonic across export toggles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Detects Inspector.targetCrashed (an 'Aw, Snap!' renderer crash that leaves the browser process alive, so no service_crashed fires) and emits a page_crashed event, mapped to ERROR severity in the OTLP converter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Exercises the export path end-to-end against a real dockerized collector: severity mapping, category exclusion, promoted attributes, export counters, and a forced queue overflow proving the drop metric. Run with -tags livetest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, crash guard - Drop 'on the VM' from the public export spec descriptions. - Thread request ctx into export reconcile; log via the context logger, and drain with context.WithoutCancel so a toggle-off outlives the request. - Still emit page_crashed (and warn) if a crash fires on an untracked session. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… test Forces a real batch-queue overflow against a local hung listener so CI exercises the SDK->global-logger->counter chain every run; an SDK message rename now fails here instead of silently zeroing the metric. Removes the build-tagged docker live test (its coverage is now unit-side). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… dash - page_crashed on an untracked session now falls back to target_type "other" instead of an empty string, so the emitted event stays a valid BrowserTargetType. Adds a regression test for the untracked path. - Restore the PATCH /telemetry no-op short-circuit when the body carries neither a category block nor an export toggle. - Drop an em dash from a merge comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3286139 to
666c148
Compare
…eout - Stop OTLP export after the HTTP servers drain (in main, mirroring s2Writer) instead of concurrently inside apiService.Shutdown, so events emitted during the shutdown window are still exported. - OTLPStorageWriter.Stop now shuts the SDK provider down even when the read-loop wait exhausts ctx, using a short detached close budget, so a slow flush can't orphan the batch-export goroutines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The runtime export toggle rebuilds the OTLP writer on each enable, and the writer read from seq 0, so a disable/re-enable re-exported the whole retained ring (up to the buffer capacity), duplicating records at the collector and inflating kernel_otlp_records_exported_total. Start the OTLP writer's reader from the current stream tail instead, so only events published while export is on are forwarded. Adds NewStorageWriterAfter (S2 keeps starting from 0) and a regression test that toggles off/on and asserts no replay and no off-window export. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| }) | ||
| } | ||
| return h.Handler.Handle(ctx, r) | ||
| } |
There was a problem hiding this comment.
Drop counter wrapper loses attrs
Medium Severity
dropCountingHandler embeds slog.Handler and only overrides Handle, so promoted WithAttrs / WithGroup return the unwrapped base handler. Any logr/OTel path that names or attaches values before logging dropped log records bypasses the counter, leaving kernel_otlp_records_dropped_total stuck at 0 despite real queue overflow.
Reviewed by Cursor Bugbot for commit be7dc1d. Configure here.
| TargetId: info.targetID, | ||
| TargetType: targetType, | ||
| Url: info.url, | ||
| }) |
There was a problem hiding this comment.
Stale URL on page crash
Medium Severity
page_crashed fills url from m.sessions attach-time target info, which is never updated on navigation. Other page events use the computed nav context’s current URL. After the first navigation, crash events report the wrong page URL even though the OpenAPI text describes the URL at crash time.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit be7dc1d. Configure here.
Sayan-
left a comment
There was a problem hiding this comment.
Approve — reviewed all 11 commits (correctness + adversarial), ran the affected Go test packages, and rebuilt the headless image to re-run the live test plan end-to-end against a real dockerized OTel collector. Everything checks out.
Commit-by-commit
- Externalize batch knobs —
BTEL_OTLP_MAX_QUEUE_SIZE/EXPORT_INTERVAL/EXPORT_TIMEOUT, validated> 0, defaults match the OTel SDK so behavior is unchanged unless set. Applied only when> 0innewOTLPStorage. Good. - Per-session export toggle — export is off by default and gated on
export.otlp.enabled, independent of whether an endpoint is provisioned; writer lifecycle moves underOTLPExportController(one-shot writer rebuilt per enable). Rollback-on-failure, PATCH-preserves / PUT-replaces semantics, and the no-op short-circuit all trace correctly. - Drop/failure/exported metrics — verified the full chain against vendored SDK v0.20.0:
global.Warn("dropped log records", "dropped", d uint64)→V(1).Info→ logr→slog levelInfo-1→ theInfo-1handler threshold inmain.go.attrUint64handles all three type boxings. Counters owned above the writer so they stay monotonic across toggles. - Crash/OOM → ERROR —
service_crashed,system_oom_kill,page_crashedmap to ERROR in the converter. - Renderer-crash event —
Inspector.enableadded toenableDomains;Inspector.targetCrashed→page_crashed(categorypage). Untracked-session path falls back totarget_type: other(valid enum) rather than"", with a regression test. Category gating confirmed: the monitor publishes viatelemetrySession.Publish, which dropspageevents unless the page category is active. - Shutdown ordering (Bugbot) — OTLP stop moved to
mainafter the servers drain (mirrorss2Writer);Stop's ctx-exhausted branch closes the provider on a detached 2s budget so a hung flush can't orphan batch-export goroutines. Correct.
Tests
go test ./lib/events/ ./lib/cdpmonitor/ ./lib/metrics/ ./cmd/api/api/ ./cmd/config/ — all green. TestOTLPMetrics_CountsRealDrops forces a real batch-queue overflow through the SDK global logger, so a future SDK message rename fails CI instead of silently zeroing the metric — nice guard.
Live test plan (rebuilt headless image + real OTel collector)
Built the image and ran it against a dockerized otel-collector-contrib + an nginx target, driving the real telemetry API and real Chromium 148 over CDP (custom knobs 512 / 500ms / 30s). All PR claims reproduced:
- Boot config surfaced the knobs;
OTLP export available(not started). GET /telemetry→ 404 at boot;PUTwithexport.otlp.enabled:true→ 201 echoing it.- Real
chrome://crash→page_crashedat collector withSeverityText: ERROR/SeverityNumber: Error(17),source.event=Inspector.targetCrashed,category=page. - Converter fidelity:
network_responsecarriedhttp.request.method,url.full,http.response.status_code=Int(200); zeroscreenshot/monitorrecords reached the collector. - Toggle: with export off, a full page navigation produced zero new exported records (
exported_totalfrozen 22→22); re-enabling rebuilt the writer and resumed (22→54). /metrics:kernel_otlp_records_dropped_total 0,export_failures_total 0,records_exported_totalclimbing.
Non-blocking notes (no changes required)
reconcileExportholdsmonitorMufor up tootlpStopTimeout(5s) during a toggle-off drain; if the relay is unreachable this can briefly head-of-line-block concurrentGET/PUT/PATCH /telemetry. Latency only, not correctness — consider draining outside the lock if toggling gets frequent.BTEL_OTLP_MAX_QUEUE_SIZEis only validated> 0while the export batch size is fixed at 200; setting it below 200 won't crash but interacts oddly with the constant. Consider validating>= 200or a doc note.PUTwith an omittedexportblock disables export (defaults false) — correct PUT-replace semantics and consistent with how omitted categories turn off, but a sharp edge for consumers; PATCH correctly preserves. Worth a sentence in the openapi description.
Nice, well-tested change. LGTM.
…ce docs - Reconcile OTLP export outside monitorMu: a new exportMu serializes export start/stop and reads the committed desired state from the session, so a toggle-off drain (bounded by otlpStopTimeout) no longer head-of-line-blocks concurrent GET/PUT/PATCH /telemetry. TelemetrySession.Stop now clears the export flag so the session stays the single source of desired state. Adds a regression test proving a blocked drain doesn't stall a concurrent read. - Validate BTEL_OTLP_MAX_QUEUE_SIZE against the export batch size (must hold at least one full batch) via a shared exported constant so the two can't drift. - Document PUT-replace vs PATCH-merge semantics for the export block in the OpenAPI spec; regenerated oapi.go. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a31b83b. Configure here.
| c.cancel() | ||
| err := c.writer.Stop(ctx) | ||
| c.writer, c.cancel = nil, nil | ||
| return err |
There was a problem hiding this comment.
Disable drain exports off-window
Medium Severity
On export disable, Stop cancels the read loop then Drains the ring with no seq cutoff. Events published after the toggle-off (capture still on) are forwarded, violating the “only while export is on” invariant and inflating kernel_otlp_records_exported_total.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit a31b83b. Configure here.


tldr
Follow-up improvements to the in-VM OTLP browser-telemetry exporter (KERNEL-1596), post #308. Independent, reviewable commits. Off-by-default export is preserved; nothing changes for existing sessions unless the new toggle/knobs are set.
What changed
BTEL_OTLP_MAX_QUEUE_SIZE(2048),BTEL_OTLP_EXPORT_INTERVAL(1s),BTEL_OTLP_EXPORT_TIMEOUT(30s). Defaults match the OTel SDK's, so behavior is unchanged unless set; validated as > 0.export.otlp.enabledon the telemetry API. Export is off by default and gated on the toggle, independent of whether an export destination is configured, so an always-injected relay does not generate work that gets dropped. The writer moves under a runtime-toggleable controller; toggling is best-effort (never fails a telemetry apply).kernel_otlp_records_dropped_total,kernel_otlp_export_failures_total,kernel_otlp_records_exported_total), owned above the writer so they stay monotonic across export toggles.service_crashedandsystem_oom_killnow map to ERROR in the OTLP converter (were INFO), so consumers can alert on them.Inspector.targetCrashed(an "Aw, Snap!" renderer crash that leaves the browser process alive, so noservice_crashedfires) and emit apage_crashedevent, mapped to ERROR. Categorized aspage(produced by the CDP monitor, captured only while thepagecategory is enabled).Live test results
Verified full-stack against the headless image + a real dockerized OpenTelemetry collector, driving the real telemetry API and a real Chromium over CDP:
BTEL_OTLP_*values surfaced in the boot config (otlp_max_queue_size=512 otlp_export_interval=500ms otlp_export_timeout=30s); export showsavailable, not started, at boot.GET /telemetry→ 404 at boot;PUTwithexport.otlp.enabled:true→ 201 echoing it. With export off but page/network capture on,exported_totalfroze (10 → 10) and the collector received nothing new; re-enabling resumed export (10 → 28), confirming the controller rebuilds the one-shot writer.chrome://crashproduced apage_crashedrecord at the collector withcategory=page,url=chrome://crash/, andSeverityText: ERROR.network_responsecarried promoted attributes (http.request.method,url.full,http.response.status_code=200);screenshot/monitorcategories never reached the collector./metricsservedkernel_otlp_records_dropped_total 0,kernel_otlp_export_failures_total 0,kernel_otlp_records_exported_total 28.🤖 Generated with Claude Code
Note
Medium Risk
Changes the OTLP lifecycle (off-by-default, runtime toggle, tail-only replay) and telemetry API semantics, but export remains best-effort and existing VMs are unchanged unless the new toggle is enabled.
Overview
OTLP export no longer starts at boot when an endpoint is provisioned. Export is gated on
export.otlp.enabledin the telemetry API (off by default). A runtimeOTLPExportControllerstarts/stops the sink;reconcileExportruns after PUT/PATCH under a separateexportMuso toggle-off drains do not blockGET /telemetry. Re-enabled writers read from the stream tail so the ring is not replayed.Config and observability:
BTEL_OTLP_MAX_QUEUE_SIZE,BTEL_OTLP_EXPORT_INTERVAL, andBTEL_OTLP_EXPORT_TIMEOUTtune the OTel batch processor (validated at load).OTLPMetricsplus a drop-counting slog handler feed Prometheus counters for dropped, failed, and exported records.Events: CDP
Inspector.targetCrashedemitspage_crashed(page category). OTLP severity mapsservice_crashed,system_oom_kill, andpage_crashedto ERROR.Reviewed by Cursor Bugbot for commit a31b83b. Bugbot is set up for automated code reviews on this repo. Configure here.