kvcache-ai/Mooncake

Fork 0

mirror of https://github.com/kvcache-ai/Mooncake.git synced 2026-11-05 01:17:55 +00:00

[GH-ISSUE #4546] [Bug]: [Store] HA OpLog metrics audit: gauges and counters registered but never written in production #1159

New issue

Open

opened 2026-10-29 21:23:27 +00:00 by bsakaguchi · 7 comments

bsakaguchi commented

2026-10-29 21:23:27 +00:00

Owner

Originally created by @sususama on GitHub (Oct 9, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4546

Bug Report

Summary

While reviewing #4402 (syncing ha_oplog_applied_sequence_id with recovered standby baselines), @he-yufeng flagged that set_oplog_last_sequence_id has no production caller, so ha_oplog_last_sequence_id exports 0 forever in a real deployment (comment).

I audited every gauge and counter registered in HAMetricManager (mooncake-store/src/ha_metric_manager.cpp) against all call sites in src/ on current main (db6f14bd). The pattern is broader than the one flagged metric — 8 of 17 OpLog-related set/inc methods have zero production callers.

Audit results

Gauges — always export their initial value:

Metric	Setter	Production callers	Notes
`ha_oplog_last_sequence_id`	`set_oplog_last_sequence_id`	0 (test-only)	Flagged by @he-yufeng; possibly superseded by `set_batch_record_durable_sequence`
`ha_oplog_standby_lag`	`set_oplog_standby_lag`	0 (test-only)	Lag is already computed in `HotStandbyService::GetSyncStatus()` (`primary_seq_id_ - applied_seq_id_`) but never pushed to the gauge
`ha_oplog_pending_entries`	`set_oplog_pending_entries`	0 (test-only)
`ha_pending_mutation_queue_size`	`set_pending_mutation_queue_size`	0 (test-only)

Counters — always export 0:

Metric	Incrementer	Production callers
`ha_oplog_skipped_entries_total`	`inc_oplog_skipped_entries`	0 (test-only)
`ha_oplog_gap_resolve_attempts_total` / `ha_oplog_gap_resolve_success_total`	`inc_oplog_gap_resolve_attempts` / `..._success`	0 (test-only)
`ha_oplog_etcd_write_failures` / `ha_oplog_etcd_write_retries`	`inc_oplog_etcd_write_failures` / `..._retries`	0 (test-only)
`ha_oplog_dropped_put_end`	`inc_oplog_dropped_put_end`	0 (not even exercised by tests)

For comparison, these are wired correctly: set_oplog_applied_sequence_id (oplog_applier.cpp:133), set_standby_state / inc_state_transitions / inc_oplog_watch_disconnections (hot_standby_service.cpp:38-45), set_batch_record_* and inc_batch_record_* (ordered_oplog_writer.cpp), inc_oplog_checksum_failures (oplog_applier.cpp:64), inc_oplog_applied_entries.

Two concrete examples of why this matters

ha_oplog_standby_lag — the lag value already exists internally: GetSyncStatus() computes lag_entries = primary_seq_id_ - applied_seq_id_ (hot_standby_service.cpp:487-489), and primary_seq_id_ is kept fresh by the replication loop (:695, :1144, :1232). The only missing step is pushing it to the gauge where the poll loop already mirrors applied_seq into metrics.
ha_oplog_last_sequence_id — either it is superseded by set_batch_record_durable_sequence (writer-side durable watermark, wired) and should be deleted, or it needs a writer. Same class of decision as #4399 / #4402, but on the primary side.

Proposal

For each orphan metric, decide between two options, and split the work into small, independently reviewable units:

Superseded → delete the metric, its setter, and its test-only usage (e.g. ha_oplog_last_sequence_id if ha_writer_durable_sequence / batch-record metrics already cover it).
Missing writer → add the write at the correct observation point, following #4402's placement rationale (service/writer level, not inside low-level helpers that also run on throwaway or candidate instances).

Happy to start with a per-metric disposition list and small PRs (one metric family per PR), beginning with ha_oplog_standby_lag since the lag value is already computed — or with deletions if maintainers prefer to trim the metric surface first.

Before submitting...

Ensure you searched for relevant issues and read the [documentation]

Originally created by @sususama on GitHub (Oct 9, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4546 ### Bug Report ### Summary While reviewing #4402 (syncing `ha_oplog_applied_sequence_id` with recovered standby baselines), @he-yufeng flagged that `set_oplog_last_sequence_id` has no production caller, so `ha_oplog_last_sequence_id` exports 0 forever in a real deployment ([comment](https://github.com/kvcache-ai/Mooncake/pull/4402#issuecomment-5925823897)). I audited every gauge and counter registered in `HAMetricManager` (`mooncake-store/src/ha_metric_manager.cpp`) against all call sites in `src/` on current main (`db6f14bd`). The pattern is broader than the one flagged metric — **8 of 17 OpLog-related set/inc methods have zero production callers**. ### Audit results **Gauges — always export their initial value:** | Metric | Setter | Production callers | Notes | |---|---|---|---| | `ha_oplog_last_sequence_id` | `set_oplog_last_sequence_id` | **0** (test-only) | Flagged by @he-yufeng; possibly superseded by `set_batch_record_durable_sequence` | | `ha_oplog_standby_lag` | `set_oplog_standby_lag` | **0** (test-only) | Lag is already computed in `HotStandbyService::GetSyncStatus()` (`primary_seq_id_ - applied_seq_id_`) but never pushed to the gauge | | `ha_oplog_pending_entries` | `set_oplog_pending_entries` | **0** (test-only) | | | `ha_pending_mutation_queue_size` | `set_pending_mutation_queue_size` | **0** (test-only) | | **Counters — always export 0:** | Metric | Incrementer | Production callers | Notes | |---|---|---|---| | `ha_oplog_skipped_entries_total` | `inc_oplog_skipped_entries` | **0** (test-only) | | | `ha_oplog_gap_resolve_attempts_total` / `ha_oplog_gap_resolve_success_total` | `inc_oplog_gap_resolve_attempts` / `..._success` | **0** (test-only) | | | `ha_oplog_etcd_write_failures` / `ha_oplog_etcd_write_retries` | `inc_oplog_etcd_write_failures` / `..._retries` | **0** (test-only) | | | `ha_oplog_dropped_put_end` | `inc_oplog_dropped_put_end` | **0** (not even exercised by tests) | | For comparison, these are wired correctly: `set_oplog_applied_sequence_id` (oplog_applier.cpp:133), `set_standby_state` / `inc_state_transitions` / `inc_oplog_watch_disconnections` (hot_standby_service.cpp:38-45), `set_batch_record_*` and `inc_batch_record_*` (ordered_oplog_writer.cpp), `inc_oplog_checksum_failures` (oplog_applier.cpp:64), `inc_oplog_applied_entries`. ### Two concrete examples of why this matters 1. **`ha_oplog_standby_lag`** — the lag value already exists internally: `GetSyncStatus()` computes `lag_entries = primary_seq_id_ - applied_seq_id_` (hot_standby_service.cpp:487-489), and `primary_seq_id_` is kept fresh by the replication loop (`:695`, `:1144`, `:1232`). The only missing step is pushing it to the gauge where the poll loop already mirrors `applied_seq` into metrics. 2. **`ha_oplog_last_sequence_id`** — either it is superseded by `set_batch_record_durable_sequence` (writer-side durable watermark, wired) and should be deleted, or it needs a writer. Same class of decision as #4399 / #4402, but on the primary side. ### Proposal For each orphan metric, decide between two options, and split the work into small, independently reviewable units: 1. **Superseded** → delete the metric, its setter, and its test-only usage (e.g. `ha_oplog_last_sequence_id` if `ha_writer_durable_sequence` / batch-record metrics already cover it). 2. **Missing writer** → add the write at the correct observation point, following #4402's placement rationale (service/writer level, not inside low-level helpers that also run on throwaway or candidate instances). Happy to start with a per-metric disposition list and small PRs (one metric family per PR), beginning with `ha_oplog_standby_lag` since the lag value is already computed — or with deletions if maintainers prefer to trim the metric surface first. ### Before submitting... - [ ] Ensure you searched for relevant issues and read the [documentation]

bsakaguchi added the

bug

label

2026-10-29 21:23:27 +00:00

bsakaguchi commented

2026-10-29 21:23:28 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Oct 9, 2026):

Thanks for opening this issue, @sususama!

Field	Value
Issue	#4546
GitHub user ID	`52159774`
Reporter	@sususama

A maintainer will triage this when possible. To help us respond faster, please include:

Mooncake version or commit SHA
Environment (OS, CUDA/driver, RDMA stack if relevant)
Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

@github-actions[bot] commented on GitHub (Oct 9, 2026): Thanks for opening this issue, @sususama! | Field | Value | |-------|-------| | **Issue** | #4546 | | **GitHub user ID** | `52159774` | | **Reporter** | @sususama | A maintainer will triage this when possible. To help us respond faster, please include: - Mooncake version or commit SHA - Environment (OS, CUDA/driver, RDMA stack if relevant) - Steps to reproduce and expected vs. actual behavior Useful links: [Documentation](https://kvcache-ai.github.io/Mooncake/) · [Contributing guide](https://github.com/kvcache-ai/Mooncake/blob/main/CONTRIBUTING.md) > This message was posted automatically by the issue bot.

bsakaguchi commented

2026-10-29 21:23:28 +00:00

Author

Owner

@he-yufeng commented on GitHub (Oct 9, 2026):

Spot-checked three of the eight on current main (db6f14bd): set_oplog_standby_lag, set_oplog_last_sequence_id and inc_oplog_dropped_put_end each have exactly one hit in src/, their own definition, and the lag value really is computed at hot_standby_service.cpp:487 without ever reaching the gauge. The audit holds.

Direction from my side, splitting the eight into two classes:

Wire these, they answer questions an operator actually asks during an HA incident: ha_oplog_standby_lag (the value already falls out of GetSyncStatus, the sync loop just needs to push it to the gauge), ha_oplog_pending_entries and ha_pending_mutation_queue_size (backpressure visibility), ha_oplog_etcd_write_failures / retries (etcd health), ha_oplog_gap_resolve_attempts / success and ha_oplog_skipped_entries_total (replay correctness), ha_oplog_dropped_put_end (data-loss signal).

Delete ha_oplog_last_sequence_id instead of wiring it. The writer-side durable watermark already exists as set_batch_record_durable_sequence with a real caller in ordered_oplog_writer.cpp; a second sequence gauge with a subtly different definition invites exactly the confusion that produced #4402.

Small PRs per metric family would review fastest: lag + pending, etcd, gap-resolve, dropped-put-end, plus the deletion. You did the audit, so first right of refusal is yours. Otherwise I can pick it up.

@he-yufeng commented on GitHub (Oct 9, 2026): Spot-checked three of the eight on current main (db6f14bd): set_oplog_standby_lag, set_oplog_last_sequence_id and inc_oplog_dropped_put_end each have exactly one hit in src/, their own definition, and the lag value really is computed at hot_standby_service.cpp:487 without ever reaching the gauge. The audit holds. Direction from my side, splitting the eight into two classes: Wire these, they answer questions an operator actually asks during an HA incident: ha_oplog_standby_lag (the value already falls out of GetSyncStatus, the sync loop just needs to push it to the gauge), ha_oplog_pending_entries and ha_pending_mutation_queue_size (backpressure visibility), ha_oplog_etcd_write_failures / retries (etcd health), ha_oplog_gap_resolve_attempts / success and ha_oplog_skipped_entries_total (replay correctness), ha_oplog_dropped_put_end (data-loss signal). Delete ha_oplog_last_sequence_id instead of wiring it. The writer-side durable watermark already exists as set_batch_record_durable_sequence with a real caller in ordered_oplog_writer.cpp; a second sequence gauge with a subtly different definition invites exactly the confusion that produced #4402. Small PRs per metric family would review fastest: lag + pending, etcd, gap-resolve, dropped-put-end, plus the deletion. You did the audit, so first right of refusal is yours. Otherwise I can pick it up.

bsakaguchi commented

2026-10-29 21:23:28 +00:00

Author

Owner

@sususama commented on GitHub (Oct 10, 2026):

Thanks @he-yufeng — taking this. I re-ran the audit across all 34 set_/inc_/observe_/record_ methods in ha_metric_manager.h on db6f14bd, not just the eight, and it changes three of your callings.
Agree, wiring:

ha_oplog_standby_lag — GetSyncStatus() already computes lag_entries (hot_standby_service.cpp:487); the poll loop already detects seq changes at :1294-1301, so pushing it to the gauge is a couple of lines there.
ha_oplog_skipped_entries — natural site is the IsSequenceOlder early-return at oplog_applier.cpp:72, next to the existing inc_oplog_checksum_failures().

I'd flip these from "wire" to "delete":

ha_oplog_pending_entries / ha_pending_mutation_queue_size — the writer-side queue depth they were meant to describe is already exported: ha_batch_record_committed_queue_depth (3 call sites), ha_batch_record_callback_queue_depth (2), plus
ha_writer_committed_queue_depth / ha_writer_waiting_slots from the runtime snapshot. There is no separate pending-mutation queue in the HA path to read from, so wiring them means inventing a second definition of the same number.
ha_oplog_etcd_write_retries — duplicate of ha_batch_record_retry_total (ordered_oplog_writer.cpp:567) and ha_writer_retry_count.
ha_oplog_gap_resolve_attempts / _success — no gap-resolution logic exists anywhere in src/ha (zero hits), so there is no observation point.
ha_oplog_dropped_put_end — no "dropped PutEnd" path exists; ApplyPutEnd failures already surface via the checksum and ordering checks.

One I'd like your call on: ha_oplog_etcd_write_failures. The only candidate site is the writer terminal path (ordered_oplog_writer.cpp:139-144), but what it reports is "writer terminated" (non_retryable / fenced / retry_timeout), not a single etcd write failure. Delete it, or rename it to something like ha_writer_terminal_failures?
Scope is wider than the issue states. Zero production callers is 15 of 34, not 8 — additionally observe_oplog_etcd_write_latency_us, observe_oplog_apply_latency_us, inc_oplog_batch_commits, inc_oplog_sync_batch_commits, and record_snapshot_operation. That last one looks like a real bug rather than an orphan: record_snapshot_skip is called from 17 sites but record_snapshot_operation from none, so snapshot success counts never move. I'll handle it separately.
Plan, smallest reviewable units:

delete ha_oplog_last_sequence_id + the two queue gauges + etcd_write_retries + gap-resolve pair + dropped_put_end (pure deletion, one PR)
wire ha_oplog_standby_lag
wire ha_oplog_skipped_entries
ha_oplog_etcd_write_failures — pending your answer above
follow-up issue for the remaining 7 orphans, including record_snapshot_operation

I'll start with 1 and 2.

@sususama commented on GitHub (Oct 10, 2026): Thanks @he-yufeng — taking this. I re-ran the audit across all 34 set_/inc_/observe_/record_ methods in ha_metric_manager.h on db6f14bd, not just the eight, and it changes three of your callings. Agree, wiring: - ha_oplog_standby_lag — GetSyncStatus() already computes lag_entries (hot_standby_service.cpp:487); the poll loop already detects seq changes at :1294-1301, so pushing it to the gauge is a couple of lines there. - ha_oplog_skipped_entries — natural site is the IsSequenceOlder early-return at oplog_applier.cpp:72, next to the existing inc_oplog_checksum_failures(). I'd flip these from "wire" to "delete": - ha_oplog_pending_entries / ha_pending_mutation_queue_size — the writer-side queue depth they were meant to describe is already exported: ha_batch_record_committed_queue_depth (3 call sites), ha_batch_record_callback_queue_depth (2), plus ha_writer_committed_queue_depth / ha_writer_waiting_slots from the runtime snapshot. There is no separate pending-mutation queue in the HA path to read from, so wiring them means inventing a second definition of the same number. - ha_oplog_etcd_write_retries — duplicate of ha_batch_record_retry_total (ordered_oplog_writer.cpp:567) and ha_writer_retry_count. - ha_oplog_gap_resolve_attempts / _success — no gap-resolution logic exists anywhere in src/ha (zero hits), so there is no observation point. - ha_oplog_dropped_put_end — no "dropped PutEnd" path exists; ApplyPutEnd failures already surface via the checksum and ordering checks. One I'd like your call on: ha_oplog_etcd_write_failures. The only candidate site is the writer terminal path (ordered_oplog_writer.cpp:139-144), but what it reports is "writer terminated" (non_retryable / fenced / retry_timeout), not a single etcd write failure. Delete it, or rename it to something like ha_writer_terminal_failures? Scope is wider than the issue states. Zero production callers is 15 of 34, not 8 — additionally observe_oplog_etcd_write_latency_us, observe_oplog_apply_latency_us, inc_oplog_batch_commits, inc_oplog_sync_batch_commits, and record_snapshot_operation. That last one looks like a real bug rather than an orphan: record_snapshot_skip is called from 17 sites but record_snapshot_operation from none, so snapshot success counts never move. I'll handle it separately. Plan, smallest reviewable units: 1. delete ha_oplog_last_sequence_id + the two queue gauges + etcd_write_retries + gap-resolve pair + dropped_put_end (pure deletion, one PR) 2. wire ha_oplog_standby_lag 3. wire ha_oplog_skipped_entries 4. ha_oplog_etcd_write_failures — pending your answer above 5. follow-up issue for the remaining 7 orphans, including record_snapshot_operation I'll start with 1 and 2.

bsakaguchi commented

2026-10-29 21:23:29 +00:00

Author

Owner

@Juhyun-Kim-Memphis commented on GitHub (Oct 21, 2026):

One data point for the ha_oplog_dropped_put_end disposition. #4582 deletes it because "no such logic exists in src/ha".

A dropped-PutEnd path does exist, but on the primary, in MasterService::PutEnd, not in src/ha. When the OpLog writer has no free entry (TASK_PENDING_LIMIT_EXCEEDED, -1401), PutEnd logs a warning and returns success. The record is not retried, so the standby does not get the object from this PutEnd. On current main (7e4fc11e):

github.com/kvcache-ai/Mooncake@7e4fc11ee0/mooncake-store/src/master_service.cpp (L5824-L5833)

We counted these warnings in a rig on b140aa92. It used the HA example flags from the deployment guide (oplog_batch_max_entries=1024), 3 etcd members, and a load that calls master RPCs only, 3 runs:

At 40,000 put items/s with no eviction running: about 1.0M to 1.15M of 16.4M PutEnd records (6–7%) in each run. The writer topped out near 39,000 entries/s.
At 9,300 put items/s with eviction running: 1,895 to 2,136 in the first 140 s of each run, in the same seconds as eviction cycles that had used up the 1,024 entries. That part is reported in #4818.

etcd was not slow in these runs (WAL fsync averaged 0.33 ms). No metric counted these drops; the WARNING line was the only trace. So the counter, under this name or another, has a natural increment site at that branch. It would be the data-loss signal suggested earlier in this thread. Whether PutEnd should keep returning success in this case is a separate question; we only note that the path exists.

Related: #4666 (its Appendix §B has the measurement details).

@Juhyun-Kim-Memphis commented on GitHub (Oct 21, 2026): One data point for the `ha_oplog_dropped_put_end` disposition. #4582 deletes it because "no such logic exists in `src/ha`". A dropped-PutEnd path does exist, but on the primary, in `MasterService::PutEnd`, not in `src/ha`. When the OpLog writer has no free entry (`TASK_PENDING_LIMIT_EXCEEDED`, -1401), PutEnd logs a warning and returns success. The record is not retried, so the standby does not get the object from this PutEnd. On current `main` (`7e4fc11e`): https://github.com/kvcache-ai/Mooncake/blob/7e4fc11ee0fe9553a82a408009ea2b499c76842e/mooncake-store/src/master_service.cpp#L5824-L5833 We counted these warnings in a rig on `b140aa92`. It used the HA example flags from the deployment guide (`oplog_batch_max_entries=1024`), 3 etcd members, and a load that calls master RPCs only, 3 runs: - At 40,000 put items/s with no eviction running: about 1.0M to 1.15M of 16.4M PutEnd records (6–7%) in each run. The writer topped out near 39,000 entries/s. - At 9,300 put items/s with eviction running: 1,895 to 2,136 in the first 140 s of each run, in the same seconds as eviction cycles that had used up the 1,024 entries. That part is reported in #4818. etcd was not slow in these runs (WAL fsync averaged 0.33 ms). No metric counted these drops; the WARNING line was the only trace. So the counter, under this name or another, has a natural increment site at that branch. It would be the data-loss signal suggested earlier in this thread. Whether PutEnd should keep returning success in this case is a separate question; we only note that the path exists. Related: #4666 (its Appendix §B has the measurement details).

bsakaguchi commented

2026-10-29 21:23:29 +00:00

Author

Owner

@sususama commented on GitHub (Oct 21, 2026):

Confirmed on current main — you are right, and my audit statement was too broad.

PutEnd (master_service.cpp:5853-5861 on 7540f582) calls AppendOpLogVisibleBeforeDurable and, on failure, only logs PutEnd: OpLog queue failed ... before falling through to return {}. The failure has more inputs than the queue being full: ReserveBatch returns TASK_PENDING_LIMIT_EXCEEDED when open_waiting_slots + entry_count > max_entries_per_batch (ordered_oplog_writer.cpp:284-287), and also returns the writer's terminal-state error, or UNAVAILABLE_IN_CURRENT_STATUS when the writer is no longer accepting. Nothing propagates to the caller, nothing is retried, and no metric counts it.

Two things to add to what you measured:

The same swallow exists for segment operations. PersistSegmentOpForHAOrEnqueue (master_service.cpp:13544-13550 on 7540f582) is void and only logs segment OpLog queue failed, so the silent-drop surface is wider than PUT_END.
The metric's help text never described this path. inc_oplog_dropped_put_end did have a real caller once: in a811c288, src/oplog_applier.cpp:116 discarded a PUT_END whose sequence had been skipped via skipped_sequence_ids_. ff217b8c (RFC #2756, batch-record writer) removed that mechanism together with the call site, which is why the metric became an orphan — and why "due to late arrival after skip" describes behavior that no longer exists.

Changing course on #4582

I said earlier I would keep the deletion and file a follow-up under a new name. Your measurement changes that: at 6-7% of PUT_END records lost during a plain fill, and with the failover deltas in your §G matching the skipped counts to within 0.3%, this is the data-loss signal an operator needs during an HA incident, and it should not wait on a new issue.

So #4582 now keeps ha_oplog_dropped_put_end and wires it at the branch you found, instead of retiring it:

The counter is incremented in PutEnd's if (!result) block, the only place where the loss is visible while PutEnd still returns success.
The help text is replaced, not inherited: "Total number of PUT_END records the OpLog writer rejected; PutEnd still returned success and the standby never received them." The old wording described the removed skip mechanism, which is exactly the wrong definition that produced this thread.
I kept the existing name because the event it now counts is literally a dropped PUT_END record; renaming it would silently break every dashboard and alert already keyed on it. If maintainers prefer ha_oplog_append_failed_total{op_type} instead, that is a one-line rename on top of this.
Tests: one unit test on the counter itself, one MasterServiceHATest that drives a writer to TASK_PENDING_LIMIT_EXCEEDED and asserts both that PutEnd still returns success and that the counter moves, and the serialization test now asserts the series is exported rather than retired.

I credited your numbers in the PR description and linked #4666 (Appendix §B) as the measurement source; say the word if you would rather I did not cite them directly.

Still open

The segment-op swallow is not covered by this counter, and the name is PUT_END-specific. I am filing that separately — a counter with op_type (and possibly the error code) as labels, incremented at PersistSegmentOpForHAOrEnqueue and at PutEnd, would generalize this without pretending one metric covers both. Your ReserveBatch failure taxonomy is what that issue will be built on.

The behavioural question you flagged — whether PutEnd should keep returning success when the record never reached the log — I agree is separate and more important. A counter makes the loss visible; it does not fix it, and it should not be folded into either of these.

@sususama commented on GitHub (Oct 21, 2026): Confirmed on current main — you are right, and my audit statement was too broad. `PutEnd` (`master_service.cpp:5853-5861` on `7540f582`) calls `AppendOpLogVisibleBeforeDurable` and, on failure, only logs `PutEnd: OpLog queue failed ...` before falling through to `return {}`. The failure has more inputs than the queue being full: `ReserveBatch` returns `TASK_PENDING_LIMIT_EXCEEDED` when `open_waiting_slots + entry_count > max_entries_per_batch` (`ordered_oplog_writer.cpp:284-287`), and also returns the writer's terminal-state error, or `UNAVAILABLE_IN_CURRENT_STATUS` when the writer is no longer accepting. Nothing propagates to the caller, nothing is retried, and no metric counts it. Two things to add to what you measured: - The same swallow exists for segment operations. `PersistSegmentOpForHAOrEnqueue` (`master_service.cpp:13544-13550` on `7540f582`) is `void` and only logs `segment OpLog queue failed`, so the silent-drop surface is wider than PUT_END. - The metric's help text never described this path. `inc_oplog_dropped_put_end` did have a real caller once: in `a811c288`, `src/oplog_applier.cpp:116` discarded a PUT_END whose sequence had been skipped via `skipped_sequence_ids_`. `ff217b8c` (RFC #2756, batch-record writer) removed that mechanism together with the call site, which is why the metric became an orphan — and why "due to late arrival after skip" describes behavior that no longer exists. ## Changing course on #4582 I said earlier I would keep the deletion and file a follow-up under a new name. Your measurement changes that: at 6-7% of PUT_END records lost during a plain fill, and with the failover deltas in your §G matching the skipped counts to within 0.3%, this is the data-loss signal an operator needs during an HA incident, and it should not wait on a new issue. So #4582 now keeps `ha_oplog_dropped_put_end` and wires it at the branch you found, instead of retiring it: - The counter is incremented in `PutEnd`'s `if (!result)` block, the only place where the loss is visible while `PutEnd` still returns success. - The help text is replaced, not inherited: "Total number of PUT_END records the OpLog writer rejected; PutEnd still returned success and the standby never received them." The old wording described the removed skip mechanism, which is exactly the wrong definition that produced this thread. - I kept the existing name because the event it now counts is literally a dropped PUT_END record; renaming it would silently break every dashboard and alert already keyed on it. If maintainers prefer `ha_oplog_append_failed_total{op_type}` instead, that is a one-line rename on top of this. - Tests: one unit test on the counter itself, one `MasterServiceHATest` that drives a writer to `TASK_PENDING_LIMIT_EXCEEDED` and asserts both that `PutEnd` still returns success and that the counter moves, and the serialization test now asserts the series is exported rather than retired. I credited your numbers in the PR description and linked #4666 (Appendix §B) as the measurement source; say the word if you would rather I did not cite them directly. ## Still open The segment-op swallow is not covered by this counter, and the name is PUT_END-specific. I am filing that separately — a counter with `op_type` (and possibly the error code) as labels, incremented at `PersistSegmentOpForHAOrEnqueue` and at `PutEnd`, would generalize this without pretending one metric covers both. Your `ReserveBatch` failure taxonomy is what that issue will be built on. The behavioural question you flagged — whether `PutEnd` should keep returning success when the record never reached the log — I agree is separate and more important. A counter makes the loss visible; it does not fix it, and it should not be folded into either of these.

bsakaguchi commented

2026-10-29 21:23:29 +00:00

Author

Owner

@he-yufeng commented on GitHub (Oct 21, 2026):

Citing the numbers directly is fine, and the revised wiring is the right shape: incrementing at the if (!result) branch is the one place the loss is still observable, and keeping the existing metric name avoids breaking dashboards that already key on it. The new help text matches what the counter now actually counts.

Agreed on keeping the three follow-ups separate: the segment-op swallow (PersistSegmentOpForHAOrEnqueue is void, same pattern), the generalized op_type/error-code counter, and the behavioral question of whether PutEnd should keep returning success when the record never reached the log. The last one is the one that matters for failover correctness; the counters just make it visible.

@he-yufeng commented on GitHub (Oct 21, 2026): Citing the numbers directly is fine, and the revised wiring is the right shape: incrementing at the `if (!result)` branch is the one place the loss is still observable, and keeping the existing metric name avoids breaking dashboards that already key on it. The new help text matches what the counter now actually counts. Agreed on keeping the three follow-ups separate: the segment-op swallow (PersistSegmentOpForHAOrEnqueue is void, same pattern), the generalized op_type/error-code counter, and the behavioral question of whether PutEnd should keep returning success when the record never reached the log. The last one is the one that matters for failover correctness; the counters just make it visible.

bsakaguchi commented

2026-10-29 21:23:29 +00:00

Author

Owner

@Juhyun-Kim-Memphis commented on GitHub (Oct 22, 2026):

Thanks for checking it on current main and for the history of the counter. Please do cite the numbers and #4666 Appendix §B directly; that is what they are there for. Keeping the existing name and wiring it at the if (!result) branch sounds right to us, and we agree the PutEnd return value is the separate, more important question.

@Juhyun-Kim-Memphis commented on GitHub (Oct 22, 2026): Thanks for checking it on current `main` and for the history of the counter. Please do cite the numbers and #4666 Appendix §B directly; that is what they are there for. Keeping the existing name and wiring it at the `if (!result)` branch sounds right to us, and we agree the `PutEnd` return value is the separate, more important question.