feat(mem_wal): observe flush latency through the metrics facade - #8625
Open
hamersaw wants to merge 4 commits into
Open
feat(mem_wal): observe flush latency through the metrics facade#8625hamersaw wants to merge 4 commits into
hamersaw wants to merge 4 commits into
Conversation
`WriteStats` already tracks flush counts and cumulative time, but a running total cannot be resampled into a distribution — the individual observations are gone by the time anything polls it. An embedder can compute an average and nothing else, which is exactly the wrong shape for latency: a flush pipeline is judged on its tail, not its mean. Observe each flush individually instead, through the `metrics` facade that `lance-io` already uses for object store operations. Observations route to whatever `Recorder` the embedding process installed, so this crate takes no position on the exporter and the emit sites compile away with the feature off. One family with a `kind` label rather than two: a WAL buffer flush and a memtable flush are stages of the same write pipeline and are read together, even though they differ by orders of magnitude — hence bucket bounds spanning a single object-store round trip through a multi-second dataset write. Counts and byte totals stay on `WriteStats`. They are cumulative and lose nothing to sampling, so there is no reason to route them through a recorder.
Replaces the `metrics`-facade approach from the previous commit. The problem is unchanged: `WriteStats` tracks flush counts and cumulative time, but a running total cannot be resampled into a distribution. An embedder can compute an average and nothing else, which is the wrong shape for latency — a flush pipeline is judged on its tail, not its mean. Report each flush to an optional `WalObserver` on `ShardWriterConfig`, alongside `warmer`. The consumer supplies the sink and owns the aggregation, so Lance still takes no position on the exporter, but now needs no feature flag and no process-global recorder. An injected sink rather than the facade because the consumer holds context Lance does not — the table a shard belongs to, in particular, which a process-global histogram cannot label. It also matches how consumers already reach into this config: `SsTableWarmer` and `DatasetCache` cross the same boundary the same way, while the facade has one producer in the tree and no consumer that installs a recorder. Every trait method defaults to a no-op, so adding an event later is not a breaking change for implementors. Counts and byte totals stay on `WriteStats`: they are cumulative and lose nothing to sampling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The write bench builds `ShardWriterConfig` field-by-field with no `..default()`, so adding `observer` broke every job that checks benchmarks: clippy, MSRV, build-no-lock, and the "Check benchmarks" step on mac and windows. Set it to `None` beside the sibling `warmer`. Add the test the observer commit was missing. A durable put returns only once its WAL flush landed, and the seal fence resolves only once the sealed memtable reached L0, so both callbacks have fired by the time it asserts — no sleeping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hamersaw
force-pushed
the
feature/mem-wal-flush-metrics
branch
from
August 19, 2026 01:23
acad3c0 to
baba7a3
Compare
xuanyu-z
approved these changes
Aug 19, 2026
Contributor
|
@claude review once |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
WriteStatstracks flush counts and cumulative time, but a running total cannot be resampled into a distribution — the individual observations are gone by the time anything polls it. An embedder can compute an average and nothing else, which is the wrong shape for latency: a flush pipeline is judged on its tail, not its mean.This observes each flush individually through the
metricsfacade thatlance-ioalready uses for object store operations. Observations route to whateverRecorderthe embedding process installed, so this crate takes no position on the exporter, and the emit sites compile away with the feature off.Shape
lance_mem_wal_flush_duration_seconds{kind="wal"|"memtable"}— one family with a label rather than two, because the WAL buffer flush and the memtable flush are stages of the same write pipeline and get read together. They differ by orders of magnitude, hence bucket bounds spanning a single object-store round trip through a multi-second dataset write.Counts and byte totals stay on
WriteStats: cumulative values lose nothing to sampling, so there is no reason to route them through a recorder.Notes
dataset/mem_wal/metrics.rs, mirroringlance_io::object_store::metrics(name constants, bucket bounds, adescribe_metricsan exporter calls after installing its recorder).WriteStats::record_wal_flushandrecord_memtable_flush, where the individual duration is already in hand.metricsbecomes an optional dependency of thelancecrate; the existingmetricsfeature now enables it alongsidelance-io/metrics.mem_waltests pass both ways.