Skip to content

adapter: remove unused timestamp-difference metrics and their redundant peek work - #38614

Open
Alphadelta14 wants to merge 6 commits into
mainfrom
heather/INC-1252-remove-diff-strict-ser
Open

adapter: remove unused timestamp-difference metrics and their redundant peek work#38614
Alphadelta14 wants to merge 6 commits into
mainfrom
heather/INC-1252-remove-diff-strict-ser

Conversation

@Alphadelta14

@Alphadelta14 Alphadelta14 commented Sep 2, 2026

Copy link
Copy Markdown
Member

Motivation

While investigating why one large production environment's environmentd was
failing Prometheus scrapes, its /metrics response turned out to be roughly
55 MB across ~450k series, and the managed collector was rejecting it outright
with body size limit exceeded. That environment is a ~6x cardinality outlier
against every other environmentd in its region, so it hits the ceiling first,
but the underlying growth is not specific to it.

Auditing the response against our monitoring query registry turned up two metrics one
metric that nothing consumes:

  • mz_timestamp_difference_for_strict_serializable_ms — ~38,700 series, about
    4 MB, roughly 7% of that environment's entire scrape response.
  • mz_timestamp_difference_for_bounded_staleness_ms — same shape, negligible
    in that environment only because it barely uses bounded staleness.

No dashboard, alert, or ad-hoc query references the former, under any variant
suffix. It was introduced in 2023 and has gone unused since.
Note that the second was more recently introduced and has not gone through the motions
to be used yet.

Description

Removes both metrics and the code that fed them.

The removal is not only about response size. Both observation sites ran a
second full timestamp determination to produce their value: each one called
determine_timestamp_for again at IsolationLevel::Serializable, acquiring
temporary read holds, purely to diff against the timestamp already chosen. That
happened on every non-immediate strict-serializable peek and every
non-immediate bounded-staleness peek, in both the coordinator path
(timestamp_selection.rs) and the frontend peek path (frontend_peek.rs).
Deleting the metrics deletes that redundant work from the peek path.

Also narrows the bucket range on mz_time_to_first_row_seconds from
histogram_seconds_buckets(0.000_128, 32.0) to (0.000_512, 32.0). Across
16.1 million observations in the environment above, the le=0.000128 bucket
held zero and le=0.000256 held twelve: a client round trip does not complete
in under 512us. This drops 2 of 20 series per label combination with no
information loss and no change for any consumer. The top of the range is
deliberately left alone, because our console query-latency alert fires on a p95
above 10s and needs le=8/16/32 to interpolate across that threshold.

Note that this only slows growth of mz_time_to_first_row_seconds rather than
bounding it. Its instance_id label is unbounded, one value per cluster the
process has ever served a peek for, and the metric is a plain HistogramVec
rather than a DeleteOnDrop one, so dropped clusters are never reclaimed. In
the environment above it held 2,459 cluster ids against 433 live clusters.
Fixing that properly means either dropping the label's unbounded dimension or
reclaiming on cluster drop, which is a larger change and deliberately not in
scope here.

The generated metric catalog (doc/user/data/metrics.yml) is regenerated with
bin/gen-metrics-catalog.

Verification

No new tests. Both removed metrics were write-only, with no test coverage and
no assertions anywhere in the tree, so there is nothing to update. Verified
that no reference to either metric survives in src/, test/, or misc/, and
that cargo check -p mz-adapter --all-targets is clean with no warnings after
removing the CastLossy imports the deletions orphaned.

mz_time_to_first_row_seconds keeps its existing coverage in
src/environmentd/tests/server.rs, which asserts the metric exists and carries
an application_name label. The bucket change does not affect it.

Release notes

This release will remove the mz_timestamp_difference_for_strict_serializable_ms
metric, which was listed in the metrics appendix but not intended as a supported surface.
It will also drop two unused low-latency buckets from mz_time_to_first_row_seconds.

🤖 Generated with Claude Code

Alphadelta14 and others added 3 commits September 1, 2026 20:01
The sibling of mz_timestamp_difference_for_strict_serializable_ms, removed
for the same reasons. Nothing queries it, and observing it required a second
full determine_timestamp_for call, at IsolationLevel::Serializable, on every
non-immediate bounded-staleness peek.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Alphadelta14
Alphadelta14 requested review from a team as code owners September 2, 2026 00:27
@def-

def- commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- bounded-staleness metric is broken, not legacy, and removing it drops the only staleness instrument for a live isolation level

src/adapter/src/metrics.rs:111

The premise behind the removal ("introduced in 2023 and unused since", "same shape") does not hold for mz_timestamp_difference_for_bounded_staleness_ms. It was added on 2026-06-11 with the bounded staleness isolation level itself (#36328), it emits zero series rather than a few, and it never ran the redundant second determination that the description credits this PR with deleting, because its gate can never be true. It is not unused by choice, it is broken, and this deletes the code a fix would target.

Details

Both observation sites were gated on !det.respond_immediately(), copied from the adjacent strict-serializable block. respond_immediately() is !self.upper.less_equal(chosen_ts) (src/adapter/src/coord/timestamp_selection.rs:759). For a bounded-staleness query with an oracle anchor, determine_timestamp_via_constraints pushes largest_not_in_advance_of_upper as a hard upper constraint (timestamp_selection.rs:368) and the FreshestAvailable candidate is advanced no further than that bound, so the chosen timestamp is always strictly below the upper and respond_immediately() is always true. Only AS OF under bounded staleness can reach the block, and there both determinations pick the user's T, so the observation is 0. The never-records defect itself is already tracked in Linear CPU-114.

Two consequences worth reflecting in the change: all of the measured cardinality and all of the removed redundant peek work come from the strict-serializable metric alone; and doc/developer/design/20260429_bounded_staleness_isolation.md:213 cites this metric as the signal for one of that design's open questions (when the per-query oracle round trip becomes worth caching), which this PR leaves stale with no instrument behind it. Either fix the gate (observe whenever the level is bounded staleness and a timestamp was chosen) and keep the metric, or drop it deliberately with the bounded-staleness owner and update that open question.

2. LOW -- bucket comment misstates what mz_time_to_first_row_seconds measures

src/adapter/src/metrics.rs:170

The new comment justifies the 512us floor with "a full client round trip never completes faster than a few hundred microseconds", but no client round trip is inside this measurement. The empirical bucket counts still support the change; the reason recorded next to it does not, and it is the reason the next person tuning these buckets will read.

Details

execute_started is taken inside SessionClient::execute (src/adapter/src/client.rs:816), after the portal is bound, and the histogram observes execute_started.elapsed() when the first row appears (client.rs:2227). That is a server-side execute-to-first-row span. The population that can land below 512us is exactly constant-folded responses that never touch a cluster and fast-path peeks sequenced in the session task, which is what the frontend peek work in this crate is trying to make more common. Restating the justification in terms of the observed distribution would keep it true as that latency moves.

Copilot AI 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.

🟡 Changes recommended

The description incorrectly claims the retained bounded-staleness metric and its redundant work are also removed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Removes an unused strict-serializable timestamp metric and its redundant timestamp-selection work, reducing scrape size and peek overhead.

Changes:

  • Removes the strict-serializable timestamp-difference metric.
  • Eliminates duplicate timestamp determination in both peek paths.
  • Drops two low-latency histogram buckets.
File summaries
File Description
src/adapter/src/metrics.rs Removes metric registration and narrows histogram buckets.
src/adapter/src/frontend_peek.rs Removes redundant frontend timestamp determination.
src/adapter/src/coord/timestamp_selection.rs Removes redundant coordinator timestamp determination.
doc/user/data/metrics.yml Removes generated metric catalog entries.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/adapter/src/metrics.rs Outdated
buckets: histogram_seconds_buckets(0.000_128, 32.0)
// NOTE: This bucket is slightly reduced since measures sub <512us are few and far between.
// This has a high impact on cardinality otherwise (and is slightly leaky)
buckets: histogram_seconds_buckets(0.000_512, 32.0)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@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.

LGTM

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