Skip to content

feat(sandbox-env): opt-in daemon OTLP metrics endpoint - #5340

Open
pedrofrxncx wants to merge 1 commit into
mainfrom
feat/sandbox-chart-daemon-otlp-endpoint
Open

feat(sandbox-env): opt-in daemon OTLP metrics endpoint#5340
pedrofrxncx wants to merge 1 commit into
mainfrom
feat/sandbox-chart-daemon-otlp-endpoint

Conversation

@pedrofrxncx

@pedrofrxncx pedrofrxncx commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Chart side of #5339. That PR gives the daemon a MeterProvider that stays dormant without OTEL_EXPORTER_OTLP_ENDPOINT; this one lets an operator set it.

telemetry:
  otlpEndpoint: ""              # empty = off (default, today's behavior)
  serviceName: studio-sandbox-daemon

Renders nothing when empty; injects both env vars when set. Verified with helm template both ways, helm lint clean.

Endpoint-agnostic on purpose — the actual URL belongs in the per-env values in deco-apps-cd, not in the chart. That's also why this isn't blocked on deciding which collector address to use: the chart change is identical for every option.

The constraint that makes this non-obvious

values.yaml now documents it, because getting it wrong fails silently — every export is dropped and the dashboard just stays empty. 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

The CIDR REJECT fires before the port ACCEPTs. A collector on an in-cluster ClusterIP lands in RFC1918, i.e. inside blockCIDRs, so it is unreachable no matter which port is added to allowedTCPPortsgateway-otlp is 172.20.146.131, inside 172.16.0.0/12. Reaching a ClusterIP would need a destination-scoped ACCEPT inserted ahead of the blockCIDRs loop, which this PR does not add.

An address that already clears the rule needs no netinit change: gateway-otlp-ingest is a LoadBalancer on 443:32184/TCP, and 443 is already in allowedTCPPorts. That's the recommended value, pending confirmation of what auth that listener expects.

Also noted in values: enabling this opens a path from pods running untrusted user code to the collector, so a compromised sandbox can emit arbitrary series. The daemon only ever sends fixed instrument names with derived attribute values.

Notes


Summary by cubic

Adds an opt-in OTLP metrics endpoint for the sandbox daemon. Default behavior stays the same until you set telemetry.otlpEndpoint.

  • New Features

    • Injects OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_SERVICE_NAME when telemetry.otlpEndpoint is set (chart v0.9.34; README/values updated).
  • Migration

    • To enable, set telemetry.otlpEndpoint to a collector address reachable past netinit. In-cluster ClusterIP is blocked by RFC1918 REJECT; use the collector’s LoadBalancer on 443 or adjust netinit. Enabling allows sandbox pods to reach your collector.

Written for commit 909f30c. Summary will update on new commits.

Review in cubic

Wires the chart side of the daemon's MeterProvider: when
`telemetry.otlpEndpoint` is set, inject OTEL_EXPORTER_OTLP_ENDPOINT and
OTEL_SERVICE_NAME into the sandbox container. Empty by default, which is
what the daemon already treats as "register no provider" — so this is
additive and inert until an operator opts in.

Endpoint-agnostic on purpose: the value belongs in the per-env values, not
here. But picking one is not free, so values.yaml carries the constraint
that will otherwise cost someone an afternoon — netinit's iptables REJECTs
the RFC1918 blockCIDRs BEFORE the port ACCEPTs, so a collector on an
in-cluster ClusterIP is unreachable no matter which port is added to
allowedTCPPorts, and every export is dropped silently. An address that
clears that rule (e.g. the collector's LoadBalancer on the already-allowed
443) needs no netinit change at all.

Also notes the trust consequence: this opens a path from pods running
untrusted user code to the collector.
@pedrofrxncx

Copy link
Copy Markdown
Collaborator Author

⚠️ This chart change cannot reach prod as-is — and it surfaced a live bug

Chasing the grace-period number for the sibling PR turned up chart drift that's worth handling before either merges.

Prod pins sandbox-env targetRevision: 0.9.5 (deco-apps-cd/values.yaml:110). The tree is at 0.9.33 (0.9.34 with this PR). So this PR ships a values knob prod won't see until someone bumps targetRevision — that bump is a 29-version delta including a default image-tag change, so it wants its own review rather than riding along here.

The live bug it exposed. #4561 (fix(sandbox): make the SIGTERM git-sync survive to completion, 2026-07-14) raised terminationGracePeriodSeconds 30 → 90 in values.yaml but did not bump Chart.yaml — it stayed at 0.9.5. The publish workflow skips existing versions (the 0.9.2 changelog entry documents this exact trap happening before), so that change was never published. Verified end to end:

  • git show 30ac6962e -- Chart.yaml → no version change
  • prod pins 0.9.5
  • a live prod pod today: terminationGracePeriodSeconds: 30

So prod sandbox pods run a 30s grace period while the shutdown git push alone self-limits at 30s (DEFAULT_GIT_TIMEOUT_MS). That is precisely the "too tight, drops unsynced work" condition #4561 was written to fix, and it has been live the whole time. On spot reclaim / rollout / idle eviction, the push can be SIGKILLed mid-flight and the pod's /app emptyDir is the only other copy.

I'm not touching deco-apps-cd here — flagging it because it's user-data-loss shaped and it needs a decision on how far to jump.

Relevance to #5339: the flush there is dispatched before the git sync and awaited after, bounded at 3s, so it only consumes budget on a path that already finished the push. It doesn't make this worse. But it was sized against a grace period I'd assumed was 90.

@pedrofrxncx

Copy link
Copy Markdown
Collaborator Author

Retargeting: the metrics this chart knob enables moved to a log line in #5346, which needs no endpoint and no egress decision. This PR now only matters if we want OTLP push from sandbox pods as a platform capability — see the discussion on #5339, which is in draft pending that. Holding here rather than merging a knob nothing sets.

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