Skip to content

adapter: add a per-metric denylist for curated metric sinks (SQL-675) - #38858

Open
mtabebe wants to merge 1 commit into
MaterializeInc:mainfrom
mtabebe:ma/sql-675-per-metric-filter-of-running
Open

mtabebe wants to merge 1 commit into
MaterializeInc:mainfrom
mtabebe:ma/sql-675-per-metric-filter-of-running

Conversation

@mtabebe

@mtabebe mtabebe commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Problem:

enable_metric_sink is all-or-nothing and only gates future installs, so a single misbehaving curated series cannot be silenced in the field. The sinks an operator wants off mid-incident are the ones already running on existing replicas, which the flag leaves up until replica drop or an envd restart.

Solution:

Add a disabled_metric_sinks system var holding a comma-separated list of curated definition names. install_metric_sinks skips a denied definition, and reconcile_metric_sinks converges the installed set whenever the var changes: a newly denied definition is dropped wherever it runs, a no-longer-denied one is installed on every replica. The denylist subtracts from enable_metric_sink rather than overriding it, so with that flag off nothing installs regardless.

Testing:

  • A unit test for parsing: padding, empty entries, a trailing comma, an unknown name, and exact matching otherwise.
  • metric-sink.td covers the cases: teardown of a running sink while the other survives, re-install on clear, a replica created under the ban never installing it, and an unknown name tearing nothing down.

Problem:

`enable_metric_sink` is all-or-nothing and only gates future installs, so a
single misbehaving curated series cannot be silenced in the field. The sinks
an operator wants off mid-incident are the ones already running on existing
replicas, which the flag leaves up until replica drop or an envd restart.

Solution:

Add a `disabled_metric_sinks` system var holding a comma-separated list of
curated definition names. `install_metric_sinks` skips a denied definition, and
`reconcile_metric_sinks` converges the installed set whenever the var changes:
a newly denied definition is dropped wherever it runs, a no-longer-denied one
is installed on every replica. The denylist subtracts from `enable_metric_sink`
rather than overriding it, so with that flag off nothing installs regardless.

Testing:
- A unit test for parsing: padding, empty entries, a trailing comma, an
  unknown name, and exact matching otherwise.
- metric-sink.td covers the cases: teardown of a running sink while the
  other survives, re-install on clear, a replica created under the ban never
  installing it, and an unknown name tearing nothing down.
@mtabebe
mtabebe requested a review from antiguru September 14, 2026 23:07
@mtabebe
mtabebe marked this pull request as ready for review September 14, 2026 23:07
@mtabebe
mtabebe requested review from a team as code owners September 14, 2026 23:07
@def-

def- commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- disabled_metric_sinks doubles as a retroactive fleet-wide install of the curated sinks

src/adapter/src/coord/metric_sink.rs:252

reconcile_metric_sinks reinstalls every non-denied definition on every replica in the environment, not just the ones it tore down, so changing disabled_metric_sinks also creates curated sink dataflows on long-lived replicas that never had them. An operator denying one definition to shed load mid-incident thereby adds the other definition's dataflow, with its arrangements, to every replica of every customer cluster in the environment.

Details

install_metric_sinks has only two other call sites: bootstrap_metric_sinks and handle_create_cluster_replica (src/adapter/src/coord/catalog_implications.rs:1745). enable_metric_sink defaults to false (src/sql/src/session/vars/definitions.rs:2243-2249) and nothing re-installs when it flips, so in any environment where the flag was turned on while replicas were already running, those replicas carry no curated sinks until envd restarts. The module doc records the asymmetry for the off direction at metric_sink.rs:194-197; the on direction has the same gap. The unconditional all_cluster_replicas() loop closes that gap as a side effect of the first denylist change, which is not what the lever advertises.

The cost is the one the module doc itself calls out at metric_sink.rs:18-21: each definition is a dataflow with arrangements on customer compute, charged to that customer's cluster. And the trigger is wider than a deliberate deny — reconcile_metric_sinks also fires on Op::ResetAllSystemConfiguration and on the LaunchDarkly sync pushing the var through set_system_vars (ddl.rs:491, ddl.rs:509).

Suggested fix: hold the previous denylist on the Coordinator and restrict the reinstall loop to definitions that went denied → not-denied, so a change that only adds names never installs anything. That also makes the common case (deny during an incident) a pure teardown.

2. LOW -- a denylist entry that matches no definition is reported only at debug!, which is off by default

src/adapter/src/coord/metric_sink.rs:235

The only signal that a disabled_metric_sinks entry matched no curated definition is a debug! line, and log_filter defaults to info (src/sql/src/session/vars/definitions.rs:899-904), so it reaches nobody in a normal deployment. A misspelled or mis-cased name leaves ALTER SYSTEM SET returning success, SHOW disabled_metric_sinks echoing the typo back, and the sink still running, with no indication the lever did not fire.

Details

metric_sink_denied (metric_sink.rs:566) matches exactly and case-sensitively — the new unit test pins !denied("A", "a") — and the var carries no domain constraint, so any string is accepted. Since tolerating unknown entries is deliberate (so a rolled-back definition needs no lockstep edit), the diagnostic is the whole safety net for a typo; warn! would make it one.

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.

2 participants