[yzhan1]

Description

Ref #2031

Follow-up to #2071 (L2->L1 promotion-on-hit, merged as da70ac9). Adds Prometheus metrics for the promotion funnel and exposes the previously-hardcoded kMaxPerHeartbeat as a config knob.

Metrics (Prometheus, master_promotion_* prefix)

Funnel — answers "is the feature firing, and where does work get lost?":

Metric	Type	Description
master_promotion_in_flight	gauge	Current in-flight tasks
master_promotion_admitted_total	counter	Tasks past all gates, enqueued
master_promotion_completed_total	counter	NotifyPromotionSuccess success
master_promotion_completed_bytes_total	counter	Bytes promoted (sum of source object_size)
master_promotion_failed_total	counter	NotifyPromotionFailure accepted (holder-reported failure)
master_promotion_cancelled_total	counter	Task erased because its prerequisite went away (object Remove/Upsert, holder client expired, or NotifyPromotionSuccess could not commit the staged replica)
master_promotion_expired_total	counter	Reaper sweep — task exceeded put_start_release_timeout_sec

Invariant: admitted = completed + failed + cancelled + expired + in_flight.

failed vs cancelled are intentionally separate so dashboards can distinguish holder-side failures (network/IO/transfer error reported via NotifyPromotionFailure) from upstream-side cancellation (the thing being promoted no longer exists).

Rejection — answers "why aren't we admitting?":

Metric	Type	Description
master_promotion_rejected_frequency_total	counter	Below promotion_admission_threshold
master_promotion_rejected_watermark_total	counter	DRAM at or above eviction high watermark
master_promotion_rejected_cap_total	counter	promotion_in_flight at promotion_queue_limit

Enough to build a working Grafana panel: rate(admitted) vs rate(completed) shows yield; rate(rejected_*) breaks down where work is dropping; in_flight / promotion_queue_limit shows saturation.

promotion_max_per_heartbeat knob

Was a compile-time constexpr kMaxPerHeartbeat = 1 in PromotionObjectHeartbeat. With the default 10s client heartbeat, this capped per-client throughput at ~6 promotions/min and made promotion_queue_limit = 50000 mostly theoretical.

Exposed as MasterServiceConfig::promotion_max_per_heartbeat (default 1, no behavior change). Wired through:

• DEFINE_uint32(promotion_max_per_heartbeat, ...) flag in master.cpp
• default_config.GetUInt32(...) parse from JSON
• MasterServiceSupervisorConfig plumbing
• MasterService constructor stores into promotion_max_per_heartbeat_, used in PromotionObjectHeartbeat
• Constructor clamps 0 → 1 (defense-in-depth — 0 would silently halt promotion delivery)
• Startup log now includes max_per_heartbeat= alongside threshold= / queue_limit=

Operators with small objects and RDMA-rich clusters can safely raise it; the new MaxPerHeartbeatKnobControlsBatchSize test exercises =3.

Module

• Transfer Engine (mooncake-transfer-engine)
• Mooncake Store (mooncake-store)
• Mooncake EP (mooncake-ep)
• Integration (mooncake-integration)
• P2P Store (mooncake-p2p-store)
• Python Wheel (mooncake-wheel)
• PyTorch Backend (mooncake-pg)
• Mooncake RL (mooncake-rl)
• CI/CD
• Docs
• Other

Type of Change

• Bug fix
• New feature
• Refactor
• Breaking change
• Documentation update
• Other

How Has This Been Tested?

promotion_on_hit_test: 33/33 pass (was 28; +5 new), 5 consecutive clean runs.

New tests:

• MetricsFunnelTracksSuccessfulPromotion — single full lifecycle bumps admitted/completed/completed_bytes correctly and returns in_flight to baseline.
• MetricsRejectionCountersIncrementOnGateMiss — verifies all three rejection counters (rejected_frequency, rejected_watermark, rejected_cap) increment when their respective branches fire. Watermark sub-case uses eviction_high_watermark_ratio = 0.0 to force the gate.
• MetricsRemoveMidPromotionCountsAsCancelled — verifies that erasing a key with an in-flight promotion (via Remove) routes the task to promotion_cancelled_total rather than promotion_failed_total.
• MaxPerHeartbeatKnobControlsBatchSize — sets knob to 3, admits 5 tasks, asserts heartbeats drain as 3 + 2 + 0.
• MaxPerHeartbeatZeroClampsToOne — pathological =0 config gets clamped to 1 by the constructor; promotion delivery is not silently disabled.

Existing tests enhanced with metric assertions:

• StalePromotionReaper — asserts promotion_expired increments.
• RemoveDuringPromotion — asserts promotion_cancelled increments.
• QueueLimitRejectsBeyondCap — asserts promotion_rejected_cap increments.
• NotifyFailureReleasesStateImmediately — asserts promotion_failed increments.
• ClientExpiryClearsPromotionTask — asserts promotion_cancelled increments.

Regression check:

• file_storage_promotion_test: 11/11 pass
• master_service_promotion_test_for_snapshot: 5/5 pass
• offload_on_evict_test: 9/9 pass

scripts/code_format.sh --check --base upstream/main (clang-format-20 in CI image): clean.

Default promotion_max_per_heartbeat = 1 means existing deployments see no behavior change; all metrics zero-emit on cold start so dashboards built against them work immediately.

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.

[gemini-code-assist]

Code Review

This pull request introduces comprehensive "Promotion-on-hit" metrics and a new configuration parameter, promotion_max_per_heartbeat, to control the number of promotion tasks returned per heartbeat. The changes include the definition, initialization, and integration of various metrics (in-flight, admitted, completed, failed, etc.) within the MasterMetricManager and MasterService. Feedback focuses on ensuring these new metrics are correctly exported for monitoring and maintaining metric invariants across all task cleanup paths, specifically by incrementing terminal counters like promotion_failed when tasks are removed without being successfully committed.

[codecov-commenter]

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

Codecov Report

❌ Patch coverage is 93.01205% with 29 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
mooncake-store/tests/promotion_on_hit_test.cpp	96.32%	11 Missing ⚠️
mooncake-store/src/master_metric_manager.cpp	87.80%	10 Missing ⚠️
mooncake-store/src/master.cpp	0.00%	5 Missing ⚠️
mooncake-store/include/master_config.h	50.00%	2 Missing ⚠️
mooncake-store/src/master_service.cpp	95.65%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[yzhan1]

@ykwd @LujhCoconut PTAL for adding metrics. Thanks

[Copilot]

Pull request overview

This PR is a follow-up enhancement to the existing L2→L1 “promotion-on-hit” feature in mooncake-store, adding Prometheus observability metrics for the promotion lifecycle and exposing the previously hardcoded per-heartbeat batching limit as a configurable knob (promotion_max_per_heartbeat).

Changes:

• Add a set of master_promotion_* Prometheus metrics to track the promotion funnel (in-flight/admitted/completed/bytes/failed/expired/cancelled) and rejection reasons (frequency/watermark/cap).
• Expose promotion_max_per_heartbeat as a gflag + JSON-configurable field, plumb it through config structures, and apply it in PromotionObjectHeartbeat (with a defensive clamp of 0 -> 1).
• Extend unit tests to validate key metric increments and validate the new heartbeat batch-size knob behavior.

Reviewed changes

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

Show a summary per file

File	Description
mooncake-store/tests/promotion_on_hit_test.cpp	Adds assertions for new promotion metrics and adds tests for the max-per-heartbeat batching knob.
mooncake-store/src/master.cpp	Introduces --promotion_max_per_heartbeat flag and wires it into config parsing/override logic.
mooncake-store/src/master_service.cpp	Implements metric increments across promotion lifecycle + uses promotion_max_per_heartbeat_ to bound heartbeat batches.
mooncake-store/src/master_metric_manager.cpp	Defines/serializes new promotion-related Prometheus metrics and includes them in the summary string.
mooncake-store/include/master_service.h	Tracks the new promotion_max_per_heartbeat_ field and updates cancellation paths to emit metrics.
mooncake-store/include/master_metric_manager.h	Declares promotion metric helpers/getters and adds member fields for the new metrics.
mooncake-store/include/master_config.h	Adds promotion_max_per_heartbeat to relevant config structs and propagation helpers.

---

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

[LujhCoconut]

Sorry for the late review on your PR.
I have a concern — this looks like a pre-existing issue.

RemoveAll() and BatchRemove() also erase ObjectMetadata entries but don't call ErasePromotionTaskIfPresent. The helper's own docstring says "Call from any path that erases an ObjectMetadata entry", but these two paths were missed. This is a pre-existing bug, but since this PR's scope is promotion-task lifecycle instrumentation, it should be fixed here.

Suggested fix — add one line before each shard->metadata.erase(it):

// master_service.cpp:2440 (RemoveAll):                                                                                
ErasePromotionTaskIfPresent(shard, it->first);
it = shard->metadata.erase(it); 

//  master_service.cpp:2496 (BatchRemove, stale-handle path):                                                           
ErasePromotionTaskIfPresent(shard, key);                                                                            
shard->metadata.erase(it);  

// master_service.cpp:2532 (BatchRemove, normal path):                                                                 
ErasePromotionTaskIfPresent(shard, key);                                                                            
shard->metadata.erase(it); 

Without this, RemoveAll / BatchRemove orphan the promotion_tasks entry, leaking promotion_in_flight_ for up to put_start_release_timeout_sec_ until the reaper sweeps.

[LujhCoconut]

[Code Smell] this block inlines the same logic as ErasePromotionTaskIfPresent. Consider replacing with the helper call to keep a single source of truth:

ErasePromotionTaskIfPresent(shard, it->first);
it = shard->metadata.erase(it);               

[yzhan1]

Thanks for the comments! PTAL at the fixes

[LujhCoconut]

Thanks for your patience. After addressing the comments above, please resolve the merge conflicts before merging.

[LujhCoconut]

Thank you for the PR. I've noticed the intermittent CI build failures caused by promotion_on_hit_test.cpp.

Errors while running CTest
The following tests FAILED:
	 18 - promotion_on_hit_test (Failed)
Error: Process completed with exit code 8.

This seems like an easily fixable timing sensitivity。

The StalePromotionReaper test sets put_start_release_timeout_sec=1 and sleeps for 2s waiting for the reaper to expire the task, but the reaper cycle interval is also exactly 1s, and the first cycle is delayed by 10ms due to the strict >
comparison (now - last_discard_time > 1s evaluates to 1s > 1s = false). Under CI load, slight timing drift between task creation and reaper firing can leave the task un-expired when the sleep ends, so the dedup gate still blocks re-enqueue and the pending->size() assertion fails.

A straightforward fix may be like this:

// Set put_start_release_timeout_sec to a value at least 2x smaller than the sleep duration, giving the reaper a comfortable margin:                                                                                                   
                                                                                                                        
config.put_start_release_timeout_sec = 1;                                                                             
// ...                                                                                                                
std::this_thread::sleep_for(std::chrono::seconds(3));  // 3s >> 1s timeout 

Would you be open to fixing this pre-existing timing sensitivity? Either way, it won't block the merge of this PR.

[Copilot]

Pull request overview

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

[yzhan1]

@LujhCoconut PTAL. Should've addressed all your comments

[LujhCoconut]

LGTM. Thanks for this PR.