Skip to content

CNS-136: expose statement logging sample rate in the operator chart - #38406

Merged
jubrad merged 7 commits into
MaterializeInc:mainfrom
jubrad:justin/cns-136-helm-chart-stop-hard-disabling-statement-logging-expose
Aug 24, 2026
Merged

CNS-136: expose statement logging sample rate in the operator chart#38406
jubrad merged 7 commits into
MaterializeInc:mainfrom
jubrad:justin/cns-136-helm-chart-stop-hard-disabling-statement-logging-expose

Conversation

@jubrad

@jubrad jubrad commented Aug 21, 2026

Copy link
Copy Markdown
Member

https://linear.app/materializeinc/issue/CNS-136/helm-chart-stop-hard-disabling-statement-logging-expose-sample-rate-as

Problem

The operator chart passed --disable-statement-logging unconditionally, so orchestratord always emitted --system-parameter-default=statement_logging_max_sample_rate=0 and query history was permanently empty in self-managed installs. There was no way to turn it back on short of forking the chart.

Solution

Replace the boolean orchestratord flag with --statement-logging-max-sample-rate=<f64> (Option<f64>, unset means no override), surfaced as the operator.args.statementLoggingMaxSampleRate chart value, defaulting to 0.99 to match Materialize Cloud. 0 still fully disables statement logging, and null inherits environmentd's default.

The sample rate does not bound what statement logging writes, statement_logging_target_data_rate does. That is exposed as operator.args.statementLoggingTargetDataRate, defaulting to unset so environmentd's 2071 B/s applies. Since environmentd already defaults to Cloud's values for the target data rate (2071) and max data credit (52428800), a default install lands on Cloud's exact configuration.

Both flags validate at parse time: the sample rate to [0, 1], which environmentd otherwise rejects by refusing to open its catalog, and the data rate to non-zero, since the token bucket refills at that rate and 0 throttles everything forever.

Testing

helm unittest misc/helm-charts/operator (43 tests) covers the default, a custom rate, 0, and unset for both values, plus a values-file fixture pinning the int64 coercion that large byte rates need. cargo clippy/test -p mz-orchestratord, bin/fmt, check-helm-docs.sh and check-copyright.sh pass. Both validators verified against the built binary. No cluster needed.

For the reviewer

  • The chart's enableInternalStatementLogging: true default was inert while the max rate was pinned at 0, and now becomes live. Internal statements share the single Arc<ThrottlingState> token bucket with user statements, so they do not raise total bytes above the cap, they consume the same budget. Console polling and mz_system traffic therefore compete with the user's own queries for the 2071 B/s, and throttled statements are dropped silently. Left as-is to keep this PR to one decision, but worth measuring.
  • Query history is never truncated for these five collections (database-issues#7666), so it accumulates for the lifetime of the environment. Pre-existing and identical to Cloud, which runs the same rates.
  • The new arguments change the environmentd StatefulSet spec, so existing environments flip to UpToDate=False / WaitingForApproval after the operator upgrade, and the fix only lands for them once a rollout is requested.
  • The flag rename is safe: nothing outside this chart set --disable-statement-logging. It does break a pinned-old-operator.image.tag + new-chart combination, as any generation-affecting flag change would.

🤖 Generated with Claude Code

jubrad added 2 commits August 21, 2026 13:31
The operator chart passed `--disable-statement-logging` unconditionally,
which made orchestratord set
`statement_logging_max_sample_rate=0` and left query history
permanently empty in self-managed installs.

Replace the boolean orchestratord flag with
`--statement-logging-max-sample-rate=<f64>`, surfaced as the
`operator.args.statementLoggingMaxSampleRate` chart value. It defaults
to 0.1, which keeps the sampling cost that motivated the original
opt-out bounded while making query history usable. Setting it to 0
still fully disables statement logging, and leaving it empty falls back
to environmentd's own default.
Only a null value omits the flag, an empty string renders an
argument orchestratord cannot parse. Name environmentd's own default
so the comment stands on its own.
@jubrad
jubrad requested a review from SangJunBak August 21, 2026 18:54
@jubrad
jubrad marked this pull request as ready for review August 21, 2026 18:58
@jubrad
jubrad requested a review from a team as a code owner August 21, 2026 18:58
@jubrad
jubrad requested a review from Alphadelta14 August 21, 2026 18:58
@def-

def- commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- Sample rate does not bound statement-logging storage, which is never reclaimed

misc/helm-charts/operator/values.yaml:47

Turning the default from "off" to 0.1 starts every self-managed install accumulating query-history data in persist that is never truncated, and the sample rate is the wrong lever to bound it: a byte-rate throttle already caps ingest independent of sampling, so on any moderately busy environment 0.1 and 0.99 converge to the same sustained write rate. The knob added to keep the cost bounded therefore does not bound it above a fairly low traffic threshold.

Details

Two mechanisms make this concrete.

src/storage-controller/src/collection_mgmt.rs:1188-1200StatementExecutionHistory, PreparedStatementHistory, SessionHistory, StatementLifecycleHistory and SqlText are explicitly excluded from partially_truncate_*, with an in-tree note that rows are never removed (MaterializeInc/database-issues#7666, closed, behavior unchanged). Every other append-only introspection collection here is trimmed on startup. So whatever gets logged stays forever, for the life of the environment, with no operator-facing way to reclaim it.

src/adapter/src/statement_logging.rs:709-735 — before a sampled statement is logged, its row bytes are charged against a token bucket refilled at statement_logging_target_data_rate (default 2071 B/s, burst credit 50 MiB). Sampling happens before this check, so the throttle is the binding constraint whenever the sampled traffic exceeds ~2 KB/s of row bytes, which is on the order of a few tens of statements per second at 0.99 and a few hundred at 0.1. Past that point the sample rate stops affecting steady-state volume entirely: both settings write ~2071 B/s ≈ 179 MB/day ≈ 65 GB/year into shards that are never compacted away. High query volume is precisely the case the removed --disable-statement-logging comment was guarding against.

Note also that operator.args.enableInternalStatementLogging already defaults to true in this chart (values.yaml:39). That setting has been inert because the sample rate was pinned to 0; this change makes it live, so internal-user statements land in the same never-truncated collections.

If the goal is a bounded cost rather than bounded sampling, the lever that actually caps bytes is statement_logging_target_data_rate. Exposing that (or lowering it for self-managed) gives a real ceiling on write rate; it still does not cap cumulative size, which needs truncation for these five collections. Worth deciding explicitly whether shipping this default before truncation exists is acceptable, rather than relying on the sample rate to do it.

2. LOW -- Out-of-range sample rate is rejected two layers away, at environmentd startup

src/orchestratord/src/controller/materialize/generation.rs:718

The new value is passed through the chart and orchestratord unvalidated, but environmentd constrains statement_logging_max_sample_rate to [0, 1] and treats a violating --system-parameter-default as a fatal catalog-open error. A plausible mistake such as statementLoggingMaxSampleRate: 10 (reading "fraction" as "percent") therefore surfaces as environmentd failing to boot rather than as a rejected Helm value.

Details

The chart has no values.schema.json, the template only checks kindIs "invalid", and #[clap(long)] statement_logging_max_sample_rate: Option<f64> (src/orchestratord/src/bin/orchestratord.rs:220) accepts any f64. src/adapter/src/catalog/open.rs:239-249 tolerates only VarError::UnknownParameter; the NUMERIC_BOUNDED_0_1_INCLUSIVE constraint violation returns Err and aborts startup.

Blast radius is limited: a rollout only starts when one is requested (src/orchestratord/src/controller/materialize.rs:398), so an existing instance keeps serving from the active generation and only the new generation fails to come up. A fresh install never becomes available. A range check in orchestratord at argument-parse time would reject it where the value was configured.

jubrad added 2 commits August 21, 2026 20:46
environmentd rejects a rate outside [0, 1] by refusing to open its
catalog, so a plausible typo such as 10 surfaced as a new generation
failing to boot. Reject it at argument-parse time instead.

The sample rate bounds the fraction of statements recorded, not the
size of the history: sustained write volume is capped by
statement_logging_target_data_rate, and the history is never truncated.
Say so rather than implying the rate bounds storage.
The sample rate bounds the fraction of statements recorded, not the
volume written. On busy environments the target data rate is the binding
limit, so it is the lever that actually caps how fast query history
grows. Expose it alongside the sample rate, defaulting to unset so
environmentd's own default applies.
@jubrad

jubrad commented Aug 22, 2026

Copy link
Copy Markdown
Member Author

Thanks, both findings verified against the code and both addressed.

1 (storage) — acted on, partly. Confirmed all three mechanics: the five collections are excluded from truncation (collection_mgmt.rs:1190-1202), sampling gates before the throttle (if !sample { return None } at statement_logging.rs:668 precedes the token-bucket check at :720), so the 2071 B/s default is the binding constraint above modest traffic and 0.1 vs 0.99 converge there.

The consequence for this PR is that my values.yaml comment was wrong: it implied the sample rate bounds storage. Corrected to state that it bounds the sampled fraction, that sustained volume is capped separately by statement_logging_target_data_rate, and that history is retained for the lifetime of the environment.

Taking your suggestion, statement_logging_target_data_rate is now also exposed as operator.args.statementLoggingTargetDataRate, defaulting to unset so environmentd's 2071 B/s applies. That gives operators the lever that actually caps write rate.

On shipping 0.1 before truncation exists: keeping it. Cloud runs these same never-truncated collections at 0.99, so self-managed at 0.1 is strictly cheaper than cloud, and the cumulative-growth gap (database-issues#7666) is pre-existing rather than introduced here. Enabling query history by default is the explicit goal of the parent issue. Bounded cumulative size still needs truncation, which is out of scope for this PR.

2 (range validation) — fixed. Added a value_parser range check on the flag, so the mistake is rejected where it was configured:

$ orchestratord --statement-logging-max-sample-rate=10
error: invalid value '10' for '--statement-logging-max-sample-rate <...>': sample rate must be between 0 and 1, got 10

1.5, -1, NaN and non-numeric input are rejected the same way; 0, 0.1 and 1 still pass.

On enableInternalStatementLogging: correct that this PR makes it live, and we're deliberately leaving it at true for now rather than changing a second default here. Flagged in the PR description as a reviewer gotcha.

@def-

def- commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- statementLoggingTargetDataRate >= 1000000 renders in scientific notation and crashloops the operator

misc/helm-charts/operator/templates/deployment.yaml:83

A byte-per-second knob invites values at or above 1e6, and Helm renders those unquoted from a values file in exponential form, so statementLoggingTargetDataRate: 1048576 reaches the container as --statement-logging-target-data-rate=1.048576e+07. orchestratord's clap Option<usize> rejects that at startup, so the operator Deployment goes into CrashLoopBackOff and stops reconciling every Materialize in the cluster, while helm upgrade reports success.

Details

Helm parses values files through a YAML-to-JSON round trip, so every number arrives as float64 and {{ .Values... }} prints it with Go's %v shortest-float formatting: 999999 renders as 999999, but 1000000 renders as 1e+06 (helm/helm#12195, helm/helm#11130). The value's own documentation and its sibling statement_logging_max_data_credit (50 MiB) both put the natural range for this knob well above the threshold, and the default of 2071 makes "raise it" the expected adjustment. The existing statementLoggingMaxSampleRate passthrough is not affected because it is a fraction below 1. Failure surfaces only as invalid digit found in string in the operator pod log.

-        - "--statement-logging-target-data-rate={{ .Values.operator.args.statementLoggingTargetDataRate }}"
+        - "--statement-logging-target-data-rate={{ int64 .Values.operator.args.statementLoggingTargetDataRate }}"

int64 handles both the float64 from a values file and the int64 from --set. Note that a regression test using set: will not reproduce this, since set: values are not parsed as floats. Pin it with a values: fixture file holding a value at or above 1e6 and assert the arg renders as =1048576.

Helm parses values files through YAML to JSON, so numbers arrive as
float64 and print in exponential form at or above 1e6. A byte-rate knob
invites values that large, and orchestratord's usize parser rejects
1.048576e+06, crashlooping the operator while helm upgrade reports
success. Coerce with int64, which handles both the float64 from a values
file and the int64 from --set.
@jubrad

jubrad commented Aug 22, 2026

Copy link
Copy Markdown
Member Author

Confirmed and fixed in a0d9a91. Good catch, this was a real crashloop.

Reproduced the whole chain before fixing. Values file at 1048576 rendered --statement-logging-target-data-rate=1.048576e+06, and the rebuilt binary rejects it:

error: invalid value '1.048576e+06' for '--statement-logging-target-data-rate <...>': invalid digit found in string

Threshold is exactly where you said, 999999 renders literally and 1000000 becomes 1e+06. Applied your int64 diff; all of 2071 / 999999 / 1000000 / 1048576 / 100000000 now render literally from a values file, --set still works, and unset still omits the flag.

Also confirmed your note that set: can't reproduce it (those stay int64), so the regression test uses a values: fixture at tests/values/large-target-data-rate.yaml. Mutation-checked it: dropping int64 from the template fails that test and only that test. 43 tests pass.

Two clarifications on the writeup:

  • The sample rate is safe, but not because it is a fraction below 1. statementLoggingMaxSampleRate: 0.0000001 does render exponentially, as 1e-07. It survives because Rust's f64 FromStr accepts exponent notation, where usize does not. So the immunity is in the parser, not the magnitude, which is worth knowing if that flag ever changes type.
  • int64 truncates rather than rejecting, so a nonsensical fractional byte rate like 1.5 now silently becomes 1 B/s, which would throttle statement logging almost entirely instead of failing loudly. Accepting that tradeoff rather than adding template validation, since a fractional byte count is not a plausible input, but flagging it as a known edge.

check-copyright covers .yaml, so the new fixture failed
lint-and-rustfmt.
@def-

def- commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- A non-numeric statementLoggingTargetDataRate now renders as 0, silently disabling statement logging

misc/helm-charts/operator/templates/deployment.yaml:83

sprig's int64 is cast.ToInt64, which discards the parse error and returns 0, so statementLoggingTargetDataRate: 10Mi renders --statement-logging-target-data-rate=0 instead of failing. A target data rate of 0 throttles every statement, so query history stays permanently empty with no error in the chart, the operator, or environmentd. Any value below 1 truncates to 0 the same way.

Details

Nothing downstream objects to the coerced value: the template's only guard is kindIs "invalid", the chart has no values.schema.json, and Option<usize> accepts Some(0), so the rendered manifest reads like a deliberate =0. In environmentd the bucket starts empty and refills at rate * elapsed (src/adapter/src/statement_logging.rs:405, :431), so at rate 0 checked_sub(cost) fails for every record forever. That is exactly the state this PR exists to remove, and the operator's symptom (empty query history after the upgrade) looks like the upgrade never took effect rather than like a bad value.

A suffixed quantity is a likely input here: every other byte-valued setting in this chart carries one (operator.resources.limits.memory: 512Mi, disk_limit: "1552MiB"), and this one is documented in bytes per second. Before this commit such a value reached clap and was rejected loudly. A one-line guard keeps the loud failure while keeping the float64 normalization:

{{- if kindIs "string" .Values.operator.args.statementLoggingTargetDataRate }}
{{- fail "operator.args.statementLoggingTargetDataRate must be a number of bytes per second, not a quantity string" }}
{{- end }}

@SangJunBak

Copy link
Copy Markdown
Contributor

Can we use the values we default to in Cloud?

statement_logging_default_sample_rate = 0.99
statement_logging_max_sample_rate = 0.99
statement_logging_target_data_rate = 2071
statement_logging_max_data_credit = 52428800

Motivation for these numbers: https://app.notion.com/p/materialize/Statement-log-throttling-22913f48d37b803c8074d2be609b4dca (numbers we have in cloud are a bit different, but we had to lower after a few experiments)

We recommended this to one of our larger self managed customers: https://materializeinc.slack.com/archives/C085QFLB072/p1768588261168409?thread_ts=1768587781.239839&cid=C085QFLB072

/// volume statement logging writes. Leave unset to keep environmentd's own
/// default.
#[clap(long)]
statement_logging_target_data_rate: Option<usize>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do the same input sanitation we do for statement_loggin_max_sample_rate here

Use 0.99, the rate Materialize Cloud runs at, rather than a lower
self-managed default. The sample rate does not bound what statement
logging writes, the target data rate does, so sampling less only gives
up query history completeness. environmentd's defaults for the target
data rate and max data credit already match Cloud, so neither needs an
override.

Reject a target data rate of 0. The token bucket starts empty and
refills at that rate, so 0 throttles every statement forever.
@jubrad

jubrad commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

Both addressed in ccc7e10, thanks — the Cloud numbers settle a question two review rounds had been circling.

Cloud defaults. Switched statementLoggingMaxSampleRate to 0.99. I had picked 0.1 to be conservative about the cost cited in cloud#10755, but that reasoning doesn't hold up: sampling less doesn't lower what statement logging writes, it only gives up query history completeness. The target data rate is the actual ceiling. Your numbers say the same thing, so 0.99 it is.

Worth noting the other three are already environmentd's own defaults, so they need no chart override at all:

Cloud environmentd default
statement_logging_max_sample_rate 0.99 0.99
statement_logging_default_sample_rate 0.99 0.99
statement_logging_target_data_rate 2071 2071
statement_logging_max_data_credit 52428800 50 * 1024 * 1024 = 52428800

So a default install now lands on Cloud's exact configuration. statementLoggingTargetDataRate stays exposed but defaults to unset, so it inherits 2071 rather than pinning a tuning number in the chart that your team may want to move centrally.

Input sanitation. Added a parse_data_rate validator alongside parse_sample_rate. usize already rejected negatives and non-numerics, so the meaningful check is rejecting 0: ThrottlingStateInner::tokens starts at 0 and refills by target_data_rate * elapsed, so a rate of 0 never refills and silently throttles every statement forever rather than disabling logging legibly.

$ orchestratord --statement-logging-target-data-rate=0
error: invalid value '0' for '--statement-logging-target-data-rate <...>': target data rate must be greater than 0

-1, abc and 1.5 are rejected too; 1, 2071 and 52428800 pass. One residual edge, already noted above in this thread: the template coerces with int64, so a fractional value from a values file truncates before reaching clap. 0.5 truncates to 0 and is now caught, but 1.5 truncates to 1 and is accepted as 1 B/s. Didn't add a floor since any threshold would be arbitrary, but flagging it.

Also grabbed the throttling doc for context, that's useful background I didn't have.

@SangJunBak SangJunBak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this change! 😀

@jubrad
jubrad merged commit d8ee88c into MaterializeInc:main Aug 24, 2026
88 checks passed
jubrad added a commit that referenced this pull request Aug 24, 2026
…ogging sampling is off (#38408)

https://linear.app/materializeinc/issue/CNS-138/console-distinct-empty-state-when-the-statement-logging-sample-rate-is

## Problem

Query history is backed by statement logging, which an operator can turn
off by setting the sample rate to `0`. When that happens the console
showed the generic filter-oriented "No results found." state, so the
user could not tell sampling was off from their filters being too
narrow.

## Solution

* New API module
`console/src/api/materialize/query-history/statementLoggingMaxSampleRate.ts`
reads `SHOW statement_logging_max_sample_rate`, following the existing
`maxReplicasPerCluster.ts` pattern. The effective rate is `min(session
statement_logging_sample_rate, system
statement_logging_max_sample_rate)`, so the cap alone detects a hard
opt-out.
* `QueryHistoryList` renders a distinct `SamplingDisabledState` when the
result set is empty and the cap is `0`, naming both ways to raise it:
`ALTER SYSTEM SET statement_logging_max_sample_rate`, and the
Materialize operator's Helm chart values for self-managed. The
filter-oriented state is unchanged for a non-zero rate.

The new query is gated on `enabled: isEmpty`, so it never runs on the
path that renders rows.

## Testing

Snapshot test for the compiled query, plus two cases in
`QueryHistoryList.test.tsx` covering both empty-state branches.

While adding them I found `DEFAULT_FETCH_QUERY_LIST_HANDLER` never
matched anything: the schema defaults `dateRange` to a window ending at
`new Date()`, so its separate `queryHistoryListSchema.parse(...)`
produced different filters than the `PARSED_DEFAULT_SCHEMA_VALUES` the
component renders with. The handler now reuses the same parsed value.

## Notes for the reviewer

* Rebased onto main now that #38407
([CNS-137](https://linear.app/materializeinc/issue/CNS-137)) has landed.
This branch previously carried a cherry-pick of @leedqin's #36533
un-gate; main has that change already, so the duplicate commit was
dropped in the rebase. #36533 is now redundant and can be closed.
* The Helm chart value the copy alludes to comes from #38406
([CNS-136](https://linear.app/materializeinc/issue/CNS-136)). The
message deliberately names no specific chart key, so it stays accurate
regardless of what that PR settles on.
* The copy renders in both deployment modes and mentions Helm only in a
self-managed-qualified sentence, rather than branching on
`AppConfigSwitch`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.

4 participants