Skip to content

feat(sandbox): push daemon dependency-cache metrics over OTLP - #5339

Draft
pedrofrxncx wants to merge 3 commits into
mainfrom
feat/sandbox-daemon-otlp-deps-metrics
Draft

feat(sandbox): push daemon dependency-cache metrics over OTLP#5339
pedrofrxncx wants to merge 3 commits into
mainfrom
feat/sandbox-daemon-otlp-deps-metrics

Conversation

@pedrofrxncx

@pedrofrxncx pedrofrxncx commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Why

The golden node_modules cache runs in prod (GOLDEN_CACHE_ENABLED=1) but nothing measures it. A hit requires the pod to land on a node already warm for its repo — whether that happens is a property of fleet churn (~170 nodes/day across 3 AZs, ~80% fresh pods), not of the cache code. Without a hit rate we can't size the cross-node EFS L2 tier that would remove the same-node dependency.

The daemon couldn't tell us even if it wanted to: packages/sandbox depends on @opentelemetry/api alone — there is no MeterProvider anywhere in the package, so metrics.getMeter("link-daemon") (entry.ts) has always returned the API's no-op. Daemon metrics were never merely unscraped; they were never collected.

What

Push, not scrape. Sandbox pods are too ephemeral to be a Prometheus target — one reclaimed between two scrape windows reports nothing about the boot we care about. The daemon now owns a MeterProvider with an OTLP exporter and flushes on shutdown.

Instrument Type Labels
studio.sandbox.deps.restore Counter source, repo_hash
studio.sandbox.deps.install_ms Histogram (ms) source, repo_hash
studio.sandbox.deps.restore_ms Histogram (ms) source, repo_hash

source is l1 (reflinked the golden, install skipped) or miss (ran a full install); it grows an l2 arm when the EFS tier lands. Recorded only on completed paths — failed installs are excluded on purpose, since the ratio we want is "did the cache save a boot".

repo_hash is the cache's own credential-stripped key (16 hex chars), so a series lines up with the golden dir on disk, and the label is bounded by construction. Every attribute is a fixed key with a derived value — nothing tenant-authored leaves a pod running user code.

Ships dark

Without OTEL_EXPORTER_OTLP_ENDPOINT no provider is registered and behavior is byte-for-byte what it is today. Rollback is unsetting the env.

⚠️ Enabling this needs an egress decision — not in this PR

Sandbox egress is the netinit iptables init container, not a NetworkPolicy, and it appends:

lo ACCEPT → conntrack ACCEPT → blockCIDRs REJECT → UDP ACCEPT → TCP ACCEPT → default REJECT

gateway-otlp is ClusterIP 172.20.146.131, inside blockCIDRs' 172.16.0.0/12rejected at step 3, before any port ACCEPT at step 5. So adding 4317 to netinit.allowedTCPPorts, however high the port, changes nothing. Options, in preference order:

  1. Point at gateway-otlp-ingest (LoadBalancer, 443:32184/TCP). Public ELB address clears the CIDR REJECT and lands on the already-allowed 443. Zero netinit change, zero new hole into the cluster, TLS by default. Needs confirmation of what auth that listener expects (the sibling 8443 suggests a separate authed path).
  2. A destination-scoped ACCEPT inserted before the block rules — new netinit.allowedDestinations knob. Needs the ClusterIP pinned in values (Helm can't resolve DNS), so it's cluster-specific and breaks if the Service is recreated.
  3. Widening blockCIDRs by port — rejected, that would let user code reach any in-cluster service on that port.

Shutdown ordering is load-bearing

shutdown() already commits and pushes the user's work, bounded at 30s inside a 30s grace period — that push is the only durable copy of their work. So the flush is dispatched before the git sync (runs concurrently with it) and awaited only after, bounded at 3s. Awaiting it up front would put telemetry ahead of user data; not awaiting at all would let the trailing process.exit(0) cut the export mid-flight. OTLP acks 202 without waiting on ingestion, so a healthy collector answers in well under a second.

Notes

  • Exporter is HTTP/protobuf — binary and compressed as asked, without pulling @grpc/grpc-js into the bundle. Swapping to gRPC is a one-line change; both are already in the lockfile. Daemon bundle grows 3.13 MB → 4.09 MB (protobufjs), read once per pod boot from a local image layer.
  • Instruments are resolved per call rather than cached. The metrics API reads the global provider at call time and never proxies a later registration, so a cached instrument built before initTelemetry() would record into the void forever, silently. This runs about once per pod boot — nothing to optimize, and the trap disappears instead of needing a comment.
  • repoHash is now exported from golden-cache.ts (was module-private).

Testing

  • New install-metrics.test.ts: registers a real SDK MeterProvider + InMemoryMetricExporter (no mocks) and reads the points back — asserts both outcomes are counted, that a miss times the install while a hit times the restore (crossing these would make L1 look as expensive as a cold install), and that clone-URL credentials never reach the collector.
  • bun test packages/sandbox/daemon/setup/ → 87 pass; daemon.e2e.test.ts → 103 pass.
  • bun run fmt, bun run lint (0 errors), knip clean.
  • bun run --cwd=packages/sandbox check reports one pre-existing error in packages/shared/src/sdk/types/connection.ts (zod _zod.version.minor 3 vs 4) — reproduced on the branch base, unrelated to this change.

Not yet validated on a real pod — that needs the chart PR and the egress decision above. Follows the repo norm of confirming a live increment before calling it done.


Summary by cubic

Adds OTLP push metrics to the sandbox daemon to measure the golden node_modules cache hit rate and the time cost of installs vs. restores. Ships dark; behavior is unchanged unless OTEL_EXPORTER_OTLP_ENDPOINT is set.

  • New Features

    • In-process MeterProvider with an OTLP proto exporter, initialized at startup; instruments are created lazily to avoid no-op meters.
    • Metrics: studio.sandbox.deps.restore (Counter), studio.sandbox.deps.install_ms and studio.sandbox.deps.restore_ms (Histograms), labeled by source (l1|miss) and repo_hash (credential-stripped, 16 hex). Histograms use explicit buckets (install: 1s–5min; restore: 50ms–30s). Durations time the whole dependency step.
    • Flush on shutdown after the user’s push, capped at 3s.
  • Dependencies

    • Pin zod to 4.3.6 across the workspace to ensure a single copy.

Written for commit 80d5a97. Summary will update on new commits.

Review in cubic

The golden node_modules cache runs in prod but nothing measures it: a hit
requires the pod to land on a node already warm for its repo, and whether
that happens is a property of fleet churn, not of the cache code. We have
no idea what the hit rate is, so we can't size the cross-node (EFS L2)
work that would remove the same-node dependency.

The daemon couldn't tell us even if it wanted to. `packages/sandbox`
depends on `@opentelemetry/api` alone — there is no MeterProvider
anywhere, so `getMeter("link-daemon")` has always been the API's no-op.
Daemon metrics aren't unscraped; they were never collected.

Push rather than scrape: sandbox pods are too ephemeral to be a Prometheus
target, and a pod reclaimed between two scrape windows would report
nothing about the boot we care about. So the daemon owns a MeterProvider
with an OTLP exporter and flushes on shutdown.

Adds `studio.sandbox.deps.restore{source,repo_hash}`,
`.install_ms` and `.restore_ms`, recorded on the two completed install
paths (golden hit / full install). `source` will grow an `l2` arm when the
EFS tier lands.

Ships dark: without OTEL_EXPORTER_OTLP_ENDPOINT no provider is registered
and behavior is byte-for-byte what it is today. Enabling is a chart change
that also has to solve egress — netinit's iptables REJECTs the RFC1918
blockCIDRs before the port ACCEPTs, so the collector's ClusterIP is
unreachable no matter which port is allowed. That lands separately.

Shutdown ordering is load-bearing: the flush is dispatched before the git
sync so it overlaps with it, and awaited only after, bounded at 3s.
Awaiting it up front would put telemetry ahead of the user's work inside a
30s grace period; not awaiting at all would let process.exit() cut the
export mid-flight.
Three real defects from review:

- Histogram buckets. Neither instrument set explicitBucketBoundaries, so
  the SDK's defaults applied — they top out at 10s while installs run for
  tens of seconds, putting every real point in the +Inf overflow bucket
  and making p50 equal p95. That quantile is the entire reason the
  instrument exists. Boundaries now span 1s-5min for install and 50ms-30s
  for restore, following the advice pattern already used in
  apps/api/src/observability/middleware.ts. The test now asserts the
  buckets bracket the value and that overflow is empty.

- The shutdown comment claimed the flush ran concurrently with the git
  sync. It cannot: publish() is synchronous (spawnSync via gitSync), so it
  blocks the loop and the exporter's request can make no progress
  alongside it. The split dispatch bought nothing, so it's gone — one
  awaited flush after the push, which has the same behavior and nothing to
  explain.

- The test registered a global MeterProvider and shut it down without
  metrics.disable(). Bun shares a process across test files, so every file
  ordered after it would see a dead provider as the global and silently
  record nothing. Now torn down in a finally.

Also times the whole dependency step rather than only the install: a
failed golden probe is real boot latency, and on a miss the cost L2 would
replace is probe + install.
@pedrofrxncx
pedrofrxncx force-pushed the feat/sandbox-daemon-otlp-deps-metrics branch from bdfb4d6 to 6457c06 Compare July 28, 2026 16:38
@pedrofrxncx
pedrofrxncx marked this pull request as draft July 28, 2026 16:40
@pedrofrxncx

Copy link
Copy Markdown
Collaborator Author

All four confirmed. Verified each before acting, and you're right on the ordering question too — moving to draft.

1. Ship the log line — agreed, that's #5346

Verified the constraint you cite is exactly where you said (dep-metrics.ts:105-112), and the bootId argument is the part I hadn't thought through: stepInstall does re-run within a boot, so a counter aggregated across pods silently conflates "one pod installed three times" with "three pods" — and bootId can't fix it as an attribute without unbounded cardinality. That's a correctness problem with the metric shape, not just a delivery-channel preference.

#5346 ships it: one ~100-byte line per completed step, next to emitInstalledDeps, with a pure-builder test asserting the byte budget and that a ghs_ token survives nowhere in the output. No egress decision, no chart PR, no new deps.

Your framing is the right one and it inverts what I did: the log line answers P0's question, and OTLP push from sandbox pods is a separate platform capability that should start with the egress decision rather than end by deferring it. So this PR is now draft, blocked on that decision, and no longer carries P0's urgency. Worth noting it isn't purely speculative — push telemetry was an explicit ask from @Nicacio (ephemeral pods lose scrape windows; SIGTERM flush; willing to open egress for it) — but that's a platform conversation, not this one, and it shouldn't have been the thing standing between us and a hit rate.

I've fixed the other three rather than leaving a draft with known bugs.

2. Bucket boundaries — fixed

Confirmed: no advice, so the SDK defaults (topping out at 10s) applied to a value my own test sets to 42s. Every real point in +Inf, p50 == p95. Followed the pattern at apps/api/src/observability/middleware.ts:36-44:

  • install_ms: 1s → 5min
  • restore_ms: 50ms → 30s (so a healthy ~200ms reflink degrading to 2s is visible, as you note)

The test now asserts the boundaries bracket the recorded value and that the overflow bucket is empty — otherwise this regresses silently the moment someone edits the list.

3. "Runs concurrently" — false, comment and split both removed

Confirmed: publish() is export function publish( (routes/git.ts:638) → gitSync()spawnSync (git/git-sync.ts:20). Synchronous, blocks the loop, the exporter cannot progress alongside it. The split dispatch bought exactly zero and the 6-line comment justifying it described a benefit that doesn't exist. Now one await flushTelemetry() before process.exit(0), with the comment saying why it can't start earlier instead of claiming it does.

4. Global provider leak — fixed

await provider.shutdown() + metrics.disable() in a finally. Same class of cross-file global-state leak that already caught me once on this branch (an instrument memo cached the no-op provider), so it should have been the first thing I checked.

Minor

Also fixed: the timing now covers the whole dependency step from before the golden probe, so a failed probe's cost stops being invisible.

Left as-is: the fingerprint-skip and no-package-manager early returns stay uninstrumented. Agreed the denominator is "dependency steps that finished", not "installs attempted" — #5346 documents that explicitly rather than papering over it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant