Add in-VM OTLP telemetry export sink#308
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
archandatta
force-pushed
the
archand/kernel-1213/otel-export
branch
2 times, most recently
from
fa0efbe to
63c9a7d
Compare
Adds an optional in-VM OTLP/HTTP export sink that converts browser telemetry events into OTLP log records and forwards them to a configured endpoint, alongside the existing S2 sink. Gated on OTLP_RELAY_ENDPOINT; no behavior change when unset. - Converter maps each telemetry envelope to an OTLP log record: event type to EventName (also mirrored to kernel.event.type for backends that drop it), structured JSON body, derived severity, and kernel.* attributes. Network and console fields are promoted to semantic-convention attributes. Screenshot and monitor categories are excluded from export. - Sink mirrors S2StorageWriter: an independent ring reader feeds records through the OTel log SDK (otlploghttp + batch processor), running independently of the S2 sink. Export failures surface via a logging exporter wrapper. - Config and startup wiring, plus a dev collector config for local testing.
archandatta
force-pushed
the
archand/kernel-1213/otel-export
branch
from
63c9a7d to
5be369e
Compare
rgarcia
left a comment
rgarcia
left a comment
There was a problem hiding this comment.
reviewed — overall looks good. the converter/sink split is clean, mirroring the S2 writer lifecycle was the right call, and the e2e test decoding real OTLP protobuf off the wire is solid. two naming findings:
server/cmd/config/config.go:54-56— env var naming: consider prefixing with something browser-telemetry-specific (e.g.BTEL_*) so these aren't confused with standardOTEL_*env vars that would control kernel-images-api's own telemetry; alsoRELAYfeels overly specified since the endpoint isn't necessarily a relay (a collector in dev)server/cmd/config/config.go:60-62— nit: field names should beInstanceJWT/InstanceName/MetroName— they're platform identity envs, not OTLP-specific
Sayan-
left a comment
Sayan-
left a comment
There was a problem hiding this comment.
overall lgtm! left a few comments inline. broader review attached below.
part of me wonders if we should expose an enable/disable for exporting in the api but I don't think we should solve that in this PR
Opus Semantic Review
Contract assumption: forwarding destination coupled with vm lifecycle; exporting best effort.
A. Lifecycle & availability
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| A1 | Init failure must not crash the browser API | Optional sink shouldn't take down the VM | main.go wiring |
FAIL | main.go:145-148 os.Exit(1) on Start err. Low trigger (lazy otlploghttp.New), but real |
| A2 | Transient runtime export errors don't stop the sink | Endpoint flaps must self-heal | otlpStorage+StorageWriter |
PASS | Emit async non-blocking; processResult swallows+logs (eventsstorage.go:81); Run returns only on ctx err |
| A3 | Read loop lives whole VM lifecycle | [1] always-on requirement | StorageWriter.Run |
PASS | Exits only on reader.Read ctx cancel (ringbuffer.go:146) |
| A4 | ~Zero cost when idle | Runs on every vanilla VM | ringBuffer.Read |
PASS | Parks on <-wake (ringbuffer.go:145-149) |
B. Delivery semantics
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| B1 | Loss under pressure is safe (drops, never blocks producers or grows unbounded) | A stalled endpoint must not stall capture or OOM the VM | ring + batch queue | PASS | Best-effort by design (matches PR's deferred durability). Enqueue non-blocking, drops oldest (batch.go:305-317); ring evicts oldest. Just declare best-effort as the contract |
| B2 | Flush buffered records on graceful shutdown | Don't lose tail on SIGTERM | OTLPStorageWriter.Stop |
PASS | Stop→wait done→Drain→provider.Shutdown (main.go:333-339). SIGTERM path only |
C. Data correctness
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| C1 | Timestamps wall-clock | Suspend-skew correctness | converter | PASS | time.UnixMicro(ev.Ts); Ts wall-clock contract (event.go:79), defaulted in publishLocked |
| C2 | Per-record payload bounded | Keep under relay body cap | EventStream.Publish |
PASS | truncateIfNeeded ~1MB before ring (eventstream.go:37) |
D. Auth & security
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| D1 | Authenticated export | Relay must trust the VM | main.go headers |
PASS | Bearer KERNEL_INSTANCE_JWT (main.go:131-133) |
| D2 | Secrets not logged | Token leak | config.LogValue |
PASS | JWT + S2 token redacted (config.go:66-97) |
E. Observability & isolation
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| E1 | Export failures observable | Silent failure looks like healthy idle | loggingExporter + otel global logger |
FAIL | Export errors + ring drops → app slog (no metric). Queue-overflow drops → otel global logger, never wired (otel.SetLogger absent) → silently discarded |
| E2 | OTLP failure isolated from S2 | One sink must not degrade the other | separate writers + non-blocking emit | PASS | Independent readers/pipelines; Enqueue non-blocking → stalled endpoint can't backpressure ring or S2 |
Scorecard
- PASS (10): A2, A3, A4, B1, B2, C1, C2, D1, D2, E2
- FAIL (2): A1, E1
Blocking Summary
A1 (don't os.Exit) · E1 (surface failures in logs as aggregate or by window).
| S2Stream string `envconfig:"S2_STREAM" default:""` | ||
|
|
||
| // OTLP telemetry export. OTLPEndpoint (host[:port]) must be set to enable | ||
| // the OTLP sink, which forwards telemetry events to the metro-api relay (or |
There was a problem hiding this comment.
should drop refs to metro-api
| // otlpSeverity maps an event type to an OTLP severity. It is a deliberately | ||
| // coarse, best-effort mapping keyed off the producer's event-type string | ||
| // conventions (the literal "console_error" and the "_failed" suffix), not the | ||
| // generated event-type constants: those live in the producer package | ||
| // (cdpmonitor), which this package cannot import without a cycle. If those type | ||
| // names change, update this mapping in lockstep. |
There was a problem hiding this comment.
this seems fine as a starting point. I think it would be helpful to mux on service crash and oom kill events as well
| logger log.Logger | ||
| } | ||
|
|
||
| func newOTLPStorage(ctx context.Context, cfg OTLPConfig, log *slog.Logger) (*otlpStorage, error) { |
There was a problem hiding this comment.
I think this is fine as a starting point. I'd like for us to consider how much of these we want to control externally
| InstanceName: config.OTLPInstanceName, | ||
| Metro: config.OTLPMetro, | ||
| }, slogger) | ||
| if err := otlpWriter.Start(ctx); err != nil { |
There was a problem hiding this comment.
I think this makes sense wrt parity to s2. I'd guess ~some events will race with any intermediate relay having the information required to make routing decisions. may need to reshape depending on what we want to guarantee
Rename export env vars from OTLP_RELAY_* to BTEL_OTLP_* so they don't collide with the standard OTEL_* vars that configure the API server's own telemetry, and drop the metro-api-specific "relay" naming (dev points at a collector). Rename the platform-identity config fields to InstanceJWT / InstanceName / MetroName since they aren't OTLP-specific. Default export path is now the standard /v1/logs. Make sink startup non-fatal: an optional best-effort exporter failing to start must not take down the browser, so log and continue instead of os.Exit(1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The OTel log SDK reports records dropped under sustained backpressure via its global logger at logr V(1). That logger is a no-op until wired, and even once wired it renders just below slog Info, so a default Info handler would still swallow it. Route the SDK global logger to a handler that admits that level when export is enabled, so drops surface instead of being silently discarded (which looked identical to a healthy idle sink). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TestLoad's expected structs didn't account for the OTLPPath and OTLPServiceName defaults that Load applies, so it failed once those defaults were in place. Assert the applied defaults. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ff3f848. Configure here.
The batch processor caps records per export by count (200), not bytes. With the 1MB per-envelope publish ceiling, a full batch of large records could reach ~200MB and be rejected by the collector/relay, and the SDK drops rejected records without retry. Split each export in the loggingExporter into sub-requests under a byte budget (4MiB) so a batch of large records can't exceed the target's HTTP body limit; a single record (<=1MB) always fits in one sub-request. Adds a chunk-splitting unit test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
rgarcia
left a comment
rgarcia
left a comment
There was a problem hiding this comment.
Re-reviewed the follow-up commits: both naming findings addressed (BTEL_OTLP_* prefix, InstanceJWT/InstanceName/MetroName), sink init is now non-fatal, and export requests are byte-bounded with tests. Also verified the queue-drop logging wiring against the pinned SDK — global.Warn maps to logr V(1) → slog level −1, so the LevelInfo - 1 handler admits it. LGTM.
3 participants
Summary
BTEL_OTLP_ENDPOINT; no behavior change when unset.EventName(mirrored to akernel.event.typeattribute for backends that dropEventName), a structured JSON body, derived severity, andkernel.*attributes. High-value network/console fields are promoted to semantic-convention attributes (http.response.status_code,url.full,http.request.method). Screenshot and monitor categories are excluded.S2StorageWriter: an independent ring reader feeds records through the OTel log SDK (otlploghttp+ batch processor), running independently of S2.Config (
BTEL_OTLP_*)BTEL_OTLP_ENDPOINT(host[:port]; presence enables the sink),BTEL_OTLP_PATH(default/v1/logs),BTEL_OTLP_INSECURE,BTEL_OTLP_SERVICE_NAME(defaultkernel-browser).KERNEL_INSTANCE_JWT,INST_NAME,METRO_NAME.BTEL_OTLP_PATH=/otlp-relay/v1/logs.Review feedback addressed
OTLP_RELAY_*toBTEL_OTLP_*so they don't collide with the standardOTEL_*vars that configure the API server's own telemetry, and dropped the relay-specific naming (dev points at a collector).InstanceJWT/InstanceName/MetroName(they aren't OTLP-specific).Testing
go build ./...,go vet, and the server unit suite are green.Scope and follow-ups
BTEL_OTLP_ENDPOINTon the VM (not yet wired), gated on a resolved destination so we don't generate work with nowhere to land.Note
Medium Risk
Adds outbound telemetry with instance JWT auth and new OTel dependencies; delivery is best-effort and misconfiguration could leak volume to a customer endpoint, but failures do not stop the browser process.
Overview
Adds an optional OTLP/HTTP export path for browser telemetry, parallel to the existing S2 sink. When
BTEL_OTLP_ENDPOINTis set, the API process starts anOTLPStorageWriteron the shared event ring; unset leaves behavior unchanged.Config uses
BTEL_OTLP_*(to avoid clashing with serverOTEL_*) plus existing VM identity envs (KERNEL_INSTANCE_JWT,INST_NAME,METRO_NAME) for auth headers and OTLP resource attributes. Startup failure is non-fatal (log and continue). OTel’s global logger is wired so batch-queue drops are visible under backpressure.Conversion maps telemetry envelopes to OTLP log records: structured JSON body, coarse severity,
kernel.*metadata, promoted HTTP/console attributes, and drops screenshot/monitor categories. Export uses the OTel log SDK with HTTP batching, JWT headers, and size-based request chunking (~4 MiB) plus export-failure logging.Shutdown drains the OTLP writer after HTTP servers, mirroring S2. New unit/integration tests cover conversion, chunking, and end-to-end export to a fake receiver.
Reviewed by Cursor Bugbot for commit 23546c0. Bugbot is set up for automated code reviews on this repo. Configure here.