[Store] Report master metrics as per-second rates over time window by Lin-z-w · Pull Request #2082 · kvcache-ai/Mooncake

Lin-z-w · 2026-05-11T10:11:41Z

Description

This PR adds observability documentation for the Mooncake Store master, and refactors the periodic master metrics log to report request counters as per-second rates over the time window instead of cumulative totals.

Documentation changes:

New getting_started/observability.md page explaining the master metrics log format field-by-field and the Prometheus HTTP metrics endpoint

Code changes:

Master metrics summary now computes request counters as deltas between consecutive log outputs, formatted as xx.xx/s rates
Real-time state values (storage, key count, client count, discard staging) remain unchanged
Eviction counters now report windowed deltas instead of cumulative totals
Added AllocFail counter to surface PutStart allocation failures that trigger eviction

Module

Type of Change

How Has This Been Tested?

C++ unit tests: ./mooncake-store/tests/master_metrics_test — all tests pass
- MasterMetricsTest.SummaryUsesWindowRatesAndEvictionDeltas — verifies rate-based format, eviction deltas, and AllocFail output
- MasterMetricsTest.PutStartReplicaAllocationFailureMetric — verifies alloc failure counter
DEFAULT_KV_LEASE_TTL=500 ctest -j --output-on-failure -R master_metrics
Sphinx docs build: sphinx-build -M html source build succeeds

Checklist

I have performed a self-review of my own code.
I have formatted my own code using ./scripts/code_format.sh before submitting.
I have updated the documentation.
I have added tests to prove my changes are effective.

Document the periodic master metrics log format with field-by-field explanations, and introduce the Prometheus metrics HTTP endpoint. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Convert cumulative request counters in the periodic master metrics log to per-second rates computed as deltas between consecutive log outputs. Real-time state values (storage, keys, clients, discard staging) remain unchanged. Eviction counters now report windowed deltas. Add AllocFail counter to surface PutStart allocation failures that trigger eviction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request implements a comprehensive observability system for the Mooncake master, featuring a new documentation guide, Prometheus-compatible metrics endpoints, and enhanced periodic logging with rate-based statistics. The MasterMetricManager was updated with a snapshotting mechanism to calculate request rates and deltas over configurable intervals. Review feedback pointed out a logic error in the success rate calculations, where request and failure counters were incorrectly handled as overlapping, and noted that the EvictDiskReplica metric was missing from the summary output despite being documented.

gemini-code-assist · 2026-05-11T10:16:18Z

+       << format_rate_pair(delta(&SummaryCounters::put_starts) -
+                               delta(&SummaryCounters::put_start_fails),
+                           delta(&SummaryCounters::put_starts))


The success rate calculation (delta(put_starts) - delta(put_start_fails)) / delta(put_starts) is incorrect because put_starts and put_start_fails are mutually exclusive counters. The total requests should be delta(put_starts) + delta(put_start_fails), and the success rate should be delta(put_starts) / (delta(put_starts) + delta(put_start_fails)). This issue applies to all request counters in this section.

The put_start_requests_ counter tracks ALL PutStart requests (both successes and failures). put_start_failures_ only tracks failures. Therefore delta(put_starts) is the total in the window, and delta(put_starts) - delta(put_start_fails) gives the success count. The formula is correct.

gemini-code-assist · 2026-05-11T10:16:18Z

+    ss << "MoveRevoke="
+       << format_rate_pair(delta(&SummaryCounters::move_revokes) -
+                               delta(&SummaryCounters::move_revoke_fails),
+                           delta(&SummaryCounters::move_revokes));


The EvictDiskReplica metric is missing from the request summary section, although it is documented in the observability guide.

codecov-commenter · 2026-05-11T10:52:18Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 95.62500% with 14 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
mooncake-store/src/rpc_service.cpp	0.00%	13 Missing ⚠️
mooncake-store/src/master_metric_manager.cpp	99.60%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

…ummary The EvictDiskReplica metric was erroneously omitted from the SummaryCounters struct and the rate-based summary output during the migration to windowed per-second rates. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…mary Add PutStartReplicaAllocationFailureMetric test to verify alloc_fail counter on replica allocation failure, and SummaryUsesWindowRatesAndEvictionDeltas test to verify per-second rate format, eviction deltas, and AllocFail output in the summary log. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Remove spurious /s suffixes from individual rate values (the "per sec" section header already indicates the unit), add missing EvictDiskReplica and Snapshots fields to the example block. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

This PR adds Mooncake Store observability documentation and refactors the master’s periodic metrics summary to report windowed per-second request rates (instead of cumulative totals), plus windowed eviction deltas. It also introduces a new PutStart allocation-failure counter intended to surface eviction-triggering allocation pressure.

Changes:

Add a new Sphinx docs page describing the master metrics log format and the admin HTTP/Prometheus endpoints.
Change MasterMetricManager’s human-readable summary to compute per-window deltas and format them as per-second rates; eviction stats become window deltas.
Add a PutStart allocation-failure counter and related unit tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
mooncake-store/tests/master_metrics_test.cpp	Adds test coverage for window-rate summaries and the new allocation-failure counter.
mooncake-store/src/rpc_service.cpp	Updates periodic master metrics logging to use the new snapshot-updating summary output.
mooncake-store/src/master_service.cpp	Increments the new alloc-failure counter on replica allocation failure paths.
mooncake-store/src/master_metric_manager.cpp	Implements snapshot-based window deltas/rates and introduces the alloc-failure counter.
mooncake-store/include/master_metric_manager.h	Adds new counter APIs and snapshot bookkeeping types/state.
docs/source/index.md	Registers the new “Observability” page in the docs TOC.
docs/source/getting_started/observability.md	Documents the master metrics log and the admin HTTP metrics endpoints.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+- **role**: HA role — `leader`, `standby`, or `candidate`
+- **state**: HA runtime state — `serving`, `starting`, `stopping`, etc.


+| `GET /metrics` | `text/plain; version=0.0.4` | All metrics in Prometheus exposition format |
+| `GET /metrics/summary` | `text/plain; version=0.0.4` | Human-readable summary (same content as the periodic log) |
+| `GET /health` | `application/json` | Health check with role, HA state, and service readiness |
+| `GET /role` | `text/plain` | Current HA role (`leader` / `standby` / `candidate`) |


+      put_start_alloc_failures_(
+          "master_put_start_alloc_fail_total",
+          "Total number of PutStart failures caused by replica allocation "
+          "failure"),


00fish0 · 2026-05-12T09:29:22Z

      put_start_failures_("master_put_start_failures_total",
                          "Total number of failed PutStart requests"),
+      put_start_alloc_failures_(
+          "master_put_start_alloc_fail_total",


Should we consider this suggestion?

Adopted the suggestion — renamed to master_put_start_alloc_failures_total to match the *_failures_total convention used by other failure counters.

+    ss << "QueryTask=(Req="
+       << format_rate_pair(delta(&SummaryCounters::query_tasks) -
+                               delta(&SummaryCounters::query_task_fails),
+                           delta(&SummaryCounters::query_tasks))
+       << "), ";
+    ss << "FetchTasks=(Req="
+       << format_rate_pair(delta(&SummaryCounters::fetch_tasks) -
+                               delta(&SummaryCounters::fetch_task_fails),
+                           delta(&SummaryCounters::fetch_tasks))
+       << "), ";
    ss << "MarkTaskToComplete= (Req="


    ss << "MarkTaskToComplete= (Req="
-       << mark_task_to_complete_requests_.value() -
-              mark_task_to_complete_failures_.value()
-       << "/" << mark_task_to_complete_requests_.value() << "), ";
+       << format_rate_pair(
+              delta(&SummaryCounters::mark_task_to_complete_requests) -
+                  delta(&SummaryCounters::mark_task_to_complete_fails),
+              delta(&SummaryCounters::mark_task_to_complete_requests))


00fish0 · 2026-05-12T09:28:42Z

-                          << ha::MasterRuntimeStateToString(snapshot.state)
-                          << ", summary=" << BuildMetricsSummaryText();
+                std::ostringstream log_stream;
+                log_stream << "Master Admin Metrics: role="


The log line currently reads role=X, state=Y, summary=role=X, state=Y, service_ready=... — role/state appear twice (the outer pair is this log line's legacy prefix; the inner pair is the start of the summary payload, which is also what /metrics/summary serves).

The duplication predates this PR, but since we're refactoring this spot anyway it might be worth dropping the outer pair — note that's a log-format change, and the observability.md example added in this PR would need updating to match.

Removed the duplicate role/state/service_ready from the summary= prefix in the periodic log. The outer role= and state= at the log header level are sufficient — the inner copy was redundant since the log already had those fields at the top level. BuildMetricsSummaryText() (used by /metrics/summary) is unchanged and keeps its self-contained format.

00fish0 · 2026-05-12T09:29:22Z

      put_start_failures_("master_put_start_failures_total",
                          "Total number of failed PutStart requests"),
+      put_start_alloc_failures_(
+          "master_put_start_alloc_fail_total",


Should we consider this suggestion?

00fish0 · 2026-05-12T09:37:11Z

@@ -242,11 +242,31 @@ bool MasterAdminServer::Start() {
        metric_report_thread_ = std::thread([this]() {


enable_metric_reporting=false leaves /metrics/summary rates stuck at 0.00, and the doc claims something the code doesn't do:

A couple of related issues around the enable_metric_reporting flag:

The flag only gates the periodic log thread, not the HTTP server. In MasterAdminServer::Start(), InitHttpServer() + http_server_.async_start() run unconditionally; only metric_report_thread_ is behind if (enable_metric_reporting_). And master.cpp calls admin_server.Start() unconditionally too. So with enable_metric_reporting=false, /metrics, /metrics/summary, /health, /role, /ha_status are all still served — only the every-10s log line goes away. But observability.md says:

Set enable_metric_reporting to false to disable both the periodic log and the HTTP metrics server.

That's inaccurate — the HTTP server stays up. The doc should be corrected (or the code changed to actually gate the server, but that's a bigger behavior change).

This PR makes /metrics/summary's rates depend on the periodic log thread running. Since the switch to window rates, get_summary_string() computes (current - snapshot) / elapsed, and summary_snapshot_ is only ever advanced by get_summary_string_and_update_snapshot(), which is called exclusively from the periodic log thread. So when enable_metric_reporting=false: the thread never runs → the snapshot is never initialized → has_previous_summary is always false → every request rate in /metrics/summary is 0.00 regardless of actual traffic. (/metrics is unaffected — it serves the raw cumulative counters.) Before this PR get_summary_string() returned cumulative values, which were meaningful regardless of the flag, so this is a new implicit coupling.

Good catch. The documentation has been corrected — enable_metric_reporting only gates the periodic log thread, not the HTTP server.

On the snapshot dependency: simply making get_summary_string() always advance the snapshot would cause rate inaccuracy — if Prometheus scrapes /metrics/summary at t=3s between two log outputs, it would reset the timestamp, and the next log output (at t=10s) would report rates over only 7s instead of 10s. The correct fix needs a minimum-interval guard or an independent snapshot timer, which is more involved than it appears. We'll address this in a follow-up PR. The /metrics endpoint is unaffected since it serves raw cumulative counters.

…, log dedup - Add AllocFail counter to Prometheus serialization and rename to master_put_start_alloc_failures_total for naming consistency - Standardize task operation separators (QueryTask/FetchTasks/ MarkTaskToComplete use ':' instead of '=') and remove stray space and trailing comma in MarkTaskToComplete output - Remove duplicate role/state prefix from periodic log line (outer role/state already present, summary= prefix was redundant) - Fix role and /role endpoint documentation (only leader/standby, candidate is a state not a role) - Correct enable_metric_reporting docs (only gates log thread, HTTP endpoints always available) - Update doc example to match actual log output Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

00fish0

This is great work — very useful! The shift from cumulative counters to windowed per-second rates makes the periodic log much easier to read at a glance, and the observability page is a nice addition.

One small nit on the PR title: right now it reads as a docs-only change, but the more impactful piece is actually the refactor that makes the master metrics log report request counters as per-second rates over the time window instead of cumulative totals. Could we update the title to reflect that? Something like [Store] Report master metrics as per-second rates over time window with the docs mentioned as a secondary item would set reviewers' expectations better.

…vcache-ai#2082) Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: fatSheep <tzh2005t@163.com>

Lin-z-w and others added 2 commits May 11, 2026 16:07

[Doc] Add observability page for Mooncake Store master metrics

58128f4

Document the periodic master metrics log format with field-by-field explanations, and introduce the Prometheus metrics HTTP endpoint. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Lin-z-w requested review from ShangmingCai, XucSh, YiXR, stmatengss and ykwd as code owners May 11, 2026 10:11

github-actions Bot added run-ci Store labels May 11, 2026

gemini-code-assist Bot reviewed May 11, 2026

View reviewed changes

Lin-z-w changed the title ~~Docs/add observability page~~ [Store] Docs/add observability page May 12, 2026

Lin-z-w and others added 2 commits May 12, 2026 10:34

stmatengss requested a review from Copilot May 12, 2026 08:27

Copilot started reviewing on behalf of stmatengss May 12, 2026 08:28 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

00fish0 reviewed May 12, 2026

View reviewed changes

Lin-z-w and others added 2 commits May 12, 2026 20:10

doc: AllocFail for both put and upsert

885678c

00fish0 approved these changes May 14, 2026

View reviewed changes

00fish0 added the run-e2e-ci label May 15, 2026

github-actions Bot removed the run-e2e-ci label May 15, 2026

Lin-z-w changed the title ~~[Store] Docs/add observability page~~ [Store] Report master metrics as per-second rates over time window May 19, 2026

ykwd merged commit 4615104 into kvcache-ai:main May 19, 2026
27 of 41 checks passed

Lin-z-w deleted the docs/add-observability-page branch May 25, 2026 09:30

A-Liuhao pushed a commit to A-Liuhao/Mooncake that referenced this pull request Jun 25, 2026

[Store] Report master metrics as per-second rates over time window (k…

cc5bd58

…vcache-ai#2082) Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: fatSheep <tzh2005t@163.com>

		- role: HA role — `leader`, `standby`, or `candidate`
		- state: HA runtime state — `serving`, `starting`, `stopping`, etc.

		@@ -242,11 +242,31 @@ bool MasterAdminServer::Start() {
		metric_report_thread_ = std::thread([this]() {

Uh oh!

Conversation

Lin-z-w commented May 11, 2026

Description

Module

Type of Change

How Has This Been Tested?

Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 11, 2026

Choose a reason for hiding this comment

Uh oh!

Lin-z-w May 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 11, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

00fish0 May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Lin-z-w May 12, 2026

Choose a reason for hiding this comment

Uh oh!

00fish0 May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lin-z-w May 12, 2026

Choose a reason for hiding this comment

Uh oh!

00fish0 May 12, 2026

Choose a reason for hiding this comment

Uh oh!

00fish0 May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lin-z-w May 12, 2026

Choose a reason for hiding this comment

Uh oh!

00fish0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented May 11, 2026 •

edited

Loading

00fish0 May 12, 2026 •

edited

Loading

00fish0 May 12, 2026 •

edited

Loading