[TransferEngine] Add standard Prometheus metrics to Classic TE(1/2)#2958
[TransferEngine] Add standard Prometheus metrics to Classic TE(1/2)#2958xiangui33423 wants to merge 2 commits into
Conversation
Add a TransferEngineMetrics singleton exporting standard Prometheus-style metrics for the Classic Transfer Engine, mirroring the TENT metrics naming and behavior: - transfer_requests_total, transfer_bytes_total, transfer_failures_total (counters) - transfer_latency_seconds, transfer_size_bytes (histograms) - inflight_transfers (gauge) Metrics are wired into the existing WITH_METRICS submit/completion anchors so each transfer is recorded exactly once (recorded at submit; exactly one completion/failure recorded when a task first reaches a terminal state, guarded by the existing start_time reset). Repeated getTransferStatus polling does not double count. An optional coro_http server exposes the same endpoints as TENT (/metrics, /metrics/summary, /metrics/json, /health), started only when MC_TE_METRIC_HTTP_PORT is set. The existing periodic logging thread is preserved unchanged; metrics are additive. Tests cover successful transfer, failed transfer, latency/bytes recording, inflight returning to zero, and duplicate-completion dedup through the real TransferEngineImpl::getTransferStatus path. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request introduces standard Prometheus-style metrics collection and an optional HTTP server to export them in the Classic Transfer Engine. It adds the TransferEngineMetrics class to track transfer requests, bytes, failures, latency, and size, along with corresponding documentation and unit tests. The review feedback highlights two critical issues: first, the configured bind address (config.http_host) is ignored when constructing the HTTP server, causing it to always bind to 0.0.0.0; second, a race condition exists because task.start_time is set and recordSubmitted() is called after submitTransfer() returns, which can lead to incorrect latency measurements or missed completion/failure metrics if the transfer completes asynchronously before those lines execute.
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.
| http_server_ = std::make_unique<coro_http_server>( | ||
| config.http_server_threads, config.http_port); |
There was a problem hiding this comment.
The config.http_host parameter is loaded from the environment variable MC_TE_METRIC_HTTP_HOST but is completely ignored when constructing the coro_http_server. This causes the server to always bind to the default address 0.0.0.0 (all interfaces), even if the user explicitly configured a local/private IP (like 127.0.0.1) for security reasons.\n\nPass config.http_host as the third argument to the coro_http_server constructor to respect the configured bind address.
http_server_ = std::make_unique<coro_http_server>(\n config.http_server_threads, config.http_port, config.http_host);| for (auto& task : batch.task_list) { | ||
| if (task.start_time.time_since_epoch().count() == 0) { | ||
| task.start_time = now; | ||
| TransferEngineMetrics::instance().recordSubmitted(); | ||
| } |
There was a problem hiding this comment.
Race Condition in Metrics Recording\n\nThere is a critical race condition between task submission and status polling. Currently, task.start_time is set and recordSubmitted() is called after multi_transports_->submitTransfer() returns.\n\nBecause the transport processes transfers asynchronously, a fast transfer can complete (or fail) and be polled via getTransferStatus() before the submission thread executes lines 132-136.\n\nWhen this happens:\n1. getTransferStatus() sees task.start_time as 0 and skips recording completion/failure metrics.\n2. The submission thread then sets task.start_time to now and calls recordSubmitted().\n3. On a subsequent poll, getTransferStatus() sees task.start_time as non-zero and records the completion/failure, but with incorrect latency (measured from the submission thread's post-facto timestamp to the second poll time).\n4. If the user only polls once (since the first poll already returned a terminal status), task.start_time remains non-zero forever, and the completion/failure is never recorded, leading to a permanent leak in the inflight_transfers gauge.\n\nRecommended Fix:\nInitialize task.start_time and call recordSubmitted() before calling multi_transports_->submitTransfer(). If the submission fails (i.e., s.ok() is false), you can roll back the metrics by resetting task.start_time and calling recordFailed() to keep the inflight gauge balanced.\n\nThis issue also applies to submitTransferWithNotify, mp_submitTransfer, and mp_submitTransferWithNotify.
Fixes two issues raised in review: 1. HTTP server ignored the configured bind address. Pass config.http_host as the third coro_http_server constructor argument so MC_TE_METRIC_HTTP_HOST is respected instead of always binding 0.0.0.0. 2. Race between submission and status polling. Previously start_time was stamped and recordSubmitted() called in TransferEngineImpl *after* submitTransfer() returned, so a fast asynchronous completion could be polled before the task was counted -- dropping the completion/failure record and leaking the inflight_transfers gauge. Move the stamping + recordSubmitted() into MultiTransport::submitTransfer / mp_submitTransfer, before each task is posted to its transport, via new markTaskSubmitted()/rollbackTaskSubmission() helpers. On submitTransferTask failure the submission is rolled back (recorded as a failure) so the request/inflight counters stay balanced. This also removes the four duplicated recording blocks from the impl header. Recording is gated on TransferEngineMetrics::isInitialized() (the process-wide MC_TE_METRIC switch), matching the pre-existing logging behavior. Adds tests: single-poll on an already-terminal task is recorded (no inflight leak) and submission rollback balances the inflight gauge. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
This CI failure isn't related to my changes. The build stopped in mooncake-common (default_config.cpp, environ.cpp), which this branch doesn't touch. The root cause is sccache failing to reach its GitHub Actions cache backend — a transient DNS error (failed to lookup address information: Try again) on the Azure blob storage host. sccache aborted before the compiler ever ran. Could someone re-run the job? These usually clear on retry. |
|
@xiangui33423 Thanks for your contribution! Could you share some screenshots of the metric output? |
staryxchen
left a comment
There was a problem hiding this comment.
Can we reuse Tent's existing monitoring base code?
Response to Review CommentsMetrics Output ScreenshotsHere are the three output formats exposed by the Classic TE metrics: 1. Prometheus Format (
|
staryxchen
left a comment
There was a problem hiding this comment.
The output shared above does not match what this branch produces when run end-to-end — the actual /metrics, /metrics/json, and /metrics/summary output differs materially from what's pasted here. I'm not going to itemize the discrepancies; please run the PR yourself end-to-end and attach a real terminal capture (screenshots or raw curl output) of those three endpoints. I'd like to see evidence of a genuine end-to-end run before continuing the review.
Description
Adds standard Prometheus-style metrics to the Classic Transfer Engine,
closing the observability gap with TENT. This is the first, intentionally
small PR for #2713 — it does not unify the Classic TE and TENT metrics
implementations or refactor the architecture; that work is deferred to PR2.
Before this change, the Classic TE
WITH_METRICSpath was log-only: itaccumulated a bytes counter and a latency histogram and periodically printed a
throughput/latency line (gated on
MC_TE_METRIC). There were nostandard-named counters, no inflight gauge, no failure/request counters, and no
HTTP export.
What this PR adds
A new
TransferEngineMetricssingleton(
mooncake-transfer-engine/include/transfer_engine_metrics.h+src/transfer_engine_metrics.cpp) that mirrors TENT's metric naming andbehavior:
transfer_requests_totaltransfer_bytes_totaltransfer_failures_totaltransfer_latency_secondstransfer_size_bytesinflight_transfersDesign highlights
TransferEngineImplanchors:recordSubmitted()fires once per task whenstart_timeis first set atsubmit; the terminal branch of
getTransferStatus()records exactly onerecordCompleted()/recordFailed()and immediately resetsstart_time, sorepeated status polling never double-counts. FAILED/CANCELED/TIMEOUT all map
to a failure and clear the inflight slot.
TENT (
/metrics,/metrics/summary,/metrics/json,/health) and onlystarts when
MC_TE_METRIC_HTTP_PORTis set, so default behavior isunchanged.
transfer_enginealready linksyalantinglibs, so no newdependency is introduced.
additive.
WITH_METRICScompile flag (fully compiled outotherwise) and a runtime
setEnabled(false)switch that short-circuits aftera single atomic load. Initialization is idempotent across engine instances.
_secondsunit is kept and always emitted (ylt stores a histogram_suminan int64 gauge and suppresses serialization for sub-second sums).
New environment variables (documented in the Transfer Engine design docs):
MC_TE_METRIC— enable metrics collection (existing switch, now also drivesthe standard metrics)
MC_TE_METRIC_HTTP_PORT— HTTP export port (0/unset = disabled)MC_TE_METRIC_HTTP_HOST— bind address (default0.0.0.0)MC_TE_METRIC_HTTP_THREADS— HTTP worker threads (default 1)Relates to #2713.
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?
Added
transfer_engine_metrics_test(7 tests). It verifies the requiredbehaviors both through the
TransferEngineMetricsAPI directly and end-to-endthrough the real
TransferEngineImpl::getTransferStatuspath with a faketransport:
FAILED variants)
transport_uint_teststill passes (no regression).Test commands:
Test results:
/metrics,/metrics/json, and/metrics/summaryoutput is well-formed Prometheus/JSON text)Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks passRemaining work for PR2
scope here).
TENT_METRICS_*vs. the Classic TE
MC_TE_METRIC_*env vars).