[TENT] Fix metrics recording zero-overhead, MLU sentinel, and parallel-vector contract#2989
Open
staryxchen wants to merge 9 commits into
Open
[TENT] Fix metrics recording zero-overhead, MLU sentinel, and parallel-vector contract#2989staryxchen wants to merge 9 commits into
staryxchen wants to merge 9 commits into
Conversation
Add metrics_recording_test.cpp with gtests driving a real TransferEngineImpl + FakeTransport and asserting on TentMetrics JSON output (delta-based, robust to singleton state). Tests covering current behavior: - CompletedTransferRecordsBytesAndLatency - FailedTransferCountsFailureNotBytes - FeasibleDeadlineRecordsMLU - HistogramJsonBucketsMatchBoundaries (regression guard) Tests covering behavior introduced by subsequent commits: - InfeasibleDeadlineRecordsSeparableCounter: a dedicated counter for infeasible deadlines so the 5.0 sentinel does not pollute the MLU histogram. - BucketsAreCompileTimeDefaults: buckets are fixed at compile time, not config-driven. Compiles to a stub test when TENT_METRICS_ENABLED=OFF. Signed-off-by: staryxchen <[email protected]>
The design doc claims compile-time metrics disable yields zero runtime overhead, but recordTaskCompletionMetrics called TentMetrics::instance() .recordReadCompleted/recordWriteFailed/recordDeadlineMLU directly (not via macros), hitting no-op stubs on every task completion when disabled. Wrap the whole recording block in #if TENT_METRICS_ENABLED so the body is empty when disabled, matching the doc claim. The inner #if around stage-latency is removed (now redundant under the outer wrap). Also drop the unused 'bytes' parameter from recordReadFailed/recordWriteFailed and the TENT_RECORD_*_FAILED macros: the implementations ignored it (failed transfers transfer no bytes), so the signature was misleading. Updated all call sites: ScopedLatencyRecorder::markFailed, transfer_engine_impl.cpp, tent_metrics_example.cpp, the recording test. Verified: - Enabled build: recording tests pass. - Disabled build: compiles cleanly, stub test passes. Signed-off-by: staryxchen <[email protected]>
When a transfer's deadline was already in the past at submit, the code recorded MLU=5.0 into the tent_deadline_mlu_permille histogram as a sentinel. A genuine MLU of 5.0 (5x over the window) is a real measurement that lands in the same +Inf bucket, so the two were indistinguishable in the histogram output -- the sentinel poisoned real data. Add a separate tent_deadline_infeasible_total counter and recordDeadlineInfeasible(); the infeasible-at-submit case now increments the counter and leaves the MLU histogram untouched. Signed-off-by: staryxchen <[email protected]>
initialize() reassigned histogram_t members mid-flight when the config supplied latency_buckets/size_buckets -- a code smell (relies on histogram_t being safely reassignable) that also produced mismatched JSON labels: histogram_boundaries_ kept the compile-time defaults while the histogram object's internal buckets changed. Most deployments never change buckets anyway. Remove the latency_buckets/size_buckets config knob entirely. Buckets are now fixed at compile time (version-controlled in kLatencyBuckets etc.), which is simpler and makes observability reproducible across deployments. Removed: MetricsConfig fields, config_loader parsing/env-vars/keys, validateConfig bucket checks, the reassignment block in initialize(), and the corresponding config_loader unit tests. Updated the recording test to assert compile-time defaults directly. Signed-off-by: staryxchen <[email protected]>
histograms_ (vector<histogram_t*>) and histogram_boundaries_ (vector<vector<double>>) were maintained as two parallel vectors whose ordering had to match by convention. getJsonMetrics() indexed them in lockstep, so a future contributor adding a histogram to one vector but not the other would silently produce mislabeled JSON or out-of-bounds reads. Collapse the two into a single vector<HistogramEntry> where each entry pairs the histogram pointer with its boundaries pointer. The ordering contract is structurally eliminated. Signed-off-by: staryxchen <[email protected]>
Validate the HTTP wiring (handlers, status codes, body content) on top of the recording assertions. Uses coro_http_client (FetchUrl helper adapted from mooncake-store/tests/master_metrics_test.cpp) to scrape the real /metrics, /metrics/json, /health endpoints started by TentMetrics. Tests: - PrometheusEndpointExposesAllCounters: GET /metrics returns 200 with tent_read_bytes_total, tent_write_bytes_total, tent_deadline_infeasible_total, tent_write_latency_us_bucket present. - JsonEndpointValid: GET /metrics/json returns 200, parseable JSON with expected counter and histogram keys. - HealthEndpointOk: GET /health returns 200 with body "OK". A retry loop absorbs the brief window between async_start() returning and the server accepting connections. Tests skip gracefully if the port bind raced (log-only mode). Signed-off-by: staryxchen <[email protected]>
Docs:
- Add tent_transport_failover_total, tent_deadline_infeasible_total,
tent_deadline_mlu_permille, tent_stage_{queue_wait,dispatch,transport}_us
to the Available Metrics table.
- Remove latency_buckets/size_buckets from config-file and env-var sections
(the knob was removed); note buckets are now compile-time.
- Document recordTransportFailover/recordDeadlineMLU/recordDeadlineInfeasible/
recordStageLatency APIs and update FAILED macros to the no-arg form.
- Clarify *_requests_total counts success+failure and *_failures_total
records no bytes.
- Correct the zero-overhead claim to mention the recordTaskCompletionMetrics
body is #if-gated.
- Note ylt omits zero-valued counters from Prometheus output.
- Update Adding New Metrics to show the HistogramEntry struct form.
CMake:
- Move option(TENT_METRICS_ENABLED ...) from tent/src/metrics/CMakeLists.txt
to tent/CMakeLists.txt so the knob is discoverable at the top level. The
target_compile_definitions stays in the metrics subdirectory. Non-breaking:
option() honors cache values.
Signed-off-by: staryxchen <[email protected]>
clang-format and cmake-format adjustments on the files modified by the metrics fixes. No semantic changes. Signed-off-by: staryxchen <[email protected]>
github-actions
Bot
added
documentation
staryxchen
marked this pull request as ready for review
July 18, 2026 09:38
staryxchen
requested review from
00fish0,
ShangmingCai,
alogfans,
chestnut-Q,
doujiang24,
dtcccc,
stmatengss and
ykwd
as code owners
July 18, 2026 09:38
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the TENT metrics collection system by removing runtime-configurable histogram buckets in favor of fixed compile-time boundaries to ensure reproducible observability. It also introduces a dedicated tent_deadline_infeasible_total counter to prevent infeasible deadlines from polluting the MLU histogram, and updates failed transfer metrics to no longer record bytes. Additionally, a comprehensive test suite (metrics_recording_test.cpp) is added to verify metrics recording and HTTP endpoints. Feedback is provided to optimize vector initialization in TentMetrics::registerMetrics() to ensure that the reserved capacity is utilized and redundant allocations are avoided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
vector::operator=(initializer_list) reallocates to fit the initializer list exactly, so the preceding reserve(8) calls were dead and the 'avoid reallocation' comment was misleading. registerMetrics() runs once per initialize() (cold path), so the reserve provided no benefit either way. Remove the two lines and the comment. Signed-off-by: staryxchen <[email protected]>
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
2 participants
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.
Description
Fixes six design / correctness issues found while reviewing the TENT metrics system (
tent/metrics/), plus a docs sync. Each change is test-driven.Correctness:
recordTaskCompletionMetricscalledTentMetrics::instance().record*directly (not via macros), so whenTENT_METRICS_ENABLED=OFFevery task completion still hit no-op stubs. The whole recording block is now#if-gated so the body is empty when disabled, matching the doc claim.MLU=5.0into thetent_deadline_mlu_permillehistogram, indistinguishable from a genuine 5x-over-window sample. Replaced with a dedicatedtent_deadline_infeasible_totalcounter.Simplicity:
bytesparameter.recordReadFailed(size_t bytes)/recordWriteFailed(size_t bytes)ignored the argument (failed transfers transfer no bytes). Dropped the parameter and updated all call sites.histograms_andhistogram_boundaries_were two vectors whose order had to match by convention; a future contributor updating one without the other would silently produce mislabeled JSON. Collapsed into a singlevector<HistogramEntry>pairing each histogram with its boundaries.initialize()reassignedhistogram_tmembers mid-flight when the config suppliedlatency_buckets/size_buckets— a code smell that also produced mismatched JSON labels. Buckets are now fixed at compile time (version-controlled), making observability reproducible. (Net -201 lines.)Docs / build:
metrics.md(added 6 missing metrics, removed the bucket config, corrected the zero-overhead claim, clarified*_requests_totalcounts success+failure).option(TENT_METRICS_ENABLED)from the leaf metrics CMakeLists totent/CMakeLists.txtfor discoverability.Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Three test layers, all green.
Test commands:
Test results:
/metrics,/metrics/json,/health) scraped viacoro_http_client; Prometheus/JSON output asserts on counters and histogram buckets.#ifgating).The new test file
metrics_recording_test.cppuses delta-based assertions (snapshot before/after) so it is robust to theTentMetricssingleton's non-resettable counter state, and drives a realTransferEngineImplwith aFakeTransportto exercise the actualrecordTaskCompletionMetricspath.Checklist
./scripts/code_format.shpre-commiton the files touched by this change and all hooks pass (ran on touched files per AGENTS.md guidance;--all-filesnot run to avoid unrelated edits)AI Assistance Disclosure
CodeBuddy (GLM-5.2) was used to implement the fixes, write the tests, and draft this PR. The human submitter has reviewed every changed line and can defend the changes end-to-end.