chore(featureflags): remove sandbox-metrics read/write kill-switch flags - #3162
Conversation
These flags were kill-switches for the ClickHouse metrics pipeline, both defaulting to true (set in 63a151a, "chore(metrics): turn feature flags on by default", #2202). This hardwires the always-on path: the orchestrator always collects sandbox metrics and the API always queries them, removing the flag declarations, the UseClickhouseMetrics field on Checks, the collection skip, the four handler short-circuits, and now-unused featureflag context and imports. No references remain. Link: #2202
PR SummaryLow Risk Overview On the API, metrics handlers no longer check the read flag or return empty payloads when disabled; they always query cluster resources or ClickHouse. Related LaunchDarkly context wiring and imports are dropped from those handlers. On the orchestrator, sandbox Checks no longer carry UseClickhouseMetrics; NewChecks takes only the sandbox, and the metrics observer no longer skips sandboxes based on the write flag. flags.go no longer declares the two bool flags. Reviewed by Cursor Bugbot for commit e019bd4. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Code Review
A nil pointer dereference can occur in the sandbox observer if it attempts to fetch metrics before the sandbox checks are fully initialized. Additionally, initializing sandbox checks after assigning the network to the sandbox exposes a race condition where concurrent observers or cleanup routines accessing the sandbox from the map can encounter a nil checks pointer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
LGTM — mechanical removal of two always-on kill-switch flags and their now-dead branches.
Extended reasoning...
Overview
This PR removes two ClickHouse sandbox metrics kill-switch feature flags (sandbox-metrics-write and sandbox-metrics-read) that have been defaulting to true since #2202. Touches 8 files: 4 API handlers lose short-circuit branches that returned empty results when the read flag was disabled; orchestrator/pkg/metrics/sandboxes.go loses the write-flag skip; Checks loses the UseClickhouseMetrics field; sandbox.go loses two MetricsWriteFlag lookups on Create/Resume; and flags.go loses the two flag declarations. Follows the exact same pattern as the recently-merged #3158 (removal of the dead EdgeProvidedSandboxMetricsFlag).
Security risks
None. This is a pure code-deletion refactor — no auth, permissions, input validation, or data-boundary changes. The removed branches only ever returned empty JSON collections when the flag was off; hardwiring them to the always-on path matches prod behavior since the flags were flipped in #2202.
Level of scrutiny
Low. Mechanical cleanup of dead kill-switch flags following a documented pattern. I grepped the tree for MetricsWriteFlag, MetricsReadFlag, UseClickhouseMetrics, and the LD flag names — zero remaining references, so the removal is complete and self-contained.
Other factors
The bug hunting system found no issues. The prior sibling PR (#3158) removing a related sandbox-metrics flag was recently merged, confirming this is an ongoing intentional cleanup. Import removals are consistent (featureflags, ldcontext, logger where only used for the removed debug log) and the NewChecks signature simplification is applied at both call sites.
These flags were kill-switches for the ClickHouse metrics pipeline, both defaulting to true (set in 63a151a, "chore(metrics): turn feature flags on by default", #2202). This hardwires the always-on path: the orchestrator always collects sandbox metrics and the API always queries them, removing the flag declarations, the UseClickhouseMetrics field on Checks, the collection skip, the four handler short-circuits, and now-unused featureflag context and imports. No references remain.
Link: #2202