Repository navigation
Conversation
Replace the legacy Dropwizard per-peer histogram P75 with Channel.getAvgLatency() for fetch-block peer selection, and remove the per-peer histogram write in BlockMsgHandler.
…and fetch-block peer selection Add tests to satisfy the changed-line coverage gate (>60%) that failed in the fork validation CI: - PrometheusApiServiceTest: testNodeInfoMetric verifies the tron:node_info Info collector is registered and exposes the node version; testNodeInfoUnknownKey exercises the null-guard branch in MetricsInfo.set; testApplyBlockDupWitness and testApplyBlockWithTxs cover the migrated MetricsService.applyBlock Prometheus-only path (dup-witness MINER counter and TXS counter). - FetchBlockServiceTest: testSelectLowestLatencyPeer verifies that fetchBlockProcess selects the idle peer with the lowest channel avg latency (the migrated replacement for the legacy per-IP histogram P75) and dispatches a FetchInvDataMessage; testSwitchOnOldPeerTimeout covers the fast-switch branch when the old peer exceeds fetchBlockTimeout.
…timator Replace the raw channel average latency used by fetch-block peer selection with a bounded per-connection estimator: - PeerConnection tracks a volatile fetchLatency EWMA (alpha = 0.1), seeded from the channel average latency on the first sample and clamped to [0, fetchBlockTimeout] to resist outliers - BlockMsgHandler feeds measured fetch durations into the estimator - FetchBlockService reads the estimator; the wall-clock hard timeout switches peers unconditionally while the latency-saturation gate requires a strictly better candidate to avoid 500v500 flapping Fetch latency stays observable via the unlabeled Prometheus histogram.
Add tron:node_info{version="..."} so the node version that the legacy
Monitor API used to report is still observable through prometheus.
Node IP is intentionally not added; the prometheus instance label
already identifies the source node.
…zation The first real fetch latency now directly initializes the estimator (isomorphic to RFC 6298 SRTT initialization) instead of being blended with the channel average latency. The channel latency is demoted to a read-only fallback for the unsampled state via getFetchLatency(), and never enters the sample sequence. EWMA alpha=0.1 applies from the second sample onward; clamp keeps math-check compliance.
…fallback Unsampled peers now read their channel avgLatency as a fallback instead of 0, so the both-unsampled quadrant flips from suppressing failover to allowing it (candidate 0 < (200 - 0) * 0.5). The first-sample test now asserts direct replacement with clamp instead of channel blending, and the isolation test asserts the fresh connection's channel fallback.
The label value is the chain id derived from the genesis block hash, so genesis_block_id describes what it identifies more accurately.
…cated Add 'option deprecated = true;' to the Monitor gRPC service and the MetricsInfo message so generated classes carry @deprecated.
Log a process-level warning once when the deprecated legacy metrics stack is used: node startup with node.metricsEnable, HTTP /monitor/getstatsinfo, and rpc Monitor.GetStatsInfo. The servlet and rpc warnings use independent once-flags.
…cates Register two label-free prometheus counters: tron:block_fetch_secondary increments when the estimator-driven failover issues a secondary fetch; tron:block_duplicate increments when an adv block below head (already processed) is received.
Its read and write points were removed with the legacy fetch-block histogram.
Address PR tronprotocol#6988 review comments: - rename boolean field fetchLatencySeeded -> hasFetchLatencySampled (declaration and all reads/writes in updateFetchLatency/getFetchLatency) - spell out "Exponentially Weighted Moving Average (EWMA)" at its first occurrence in PeerConnection.java; later occurrences stay as EWMA
…hase1-deprecate-legacy-metrics
| * | ||
| * @param latencyMillis measured fetch latency in milliseconds | ||
| */ | ||
| public void updateFetchLatency(long latencyMillis) { |
There was a problem hiding this comment.
[DISCUSS] A single transient outlier can saturate a warmed estimate
The sample is blended before clamping, so one slow response can push a warmed estimate straight to the clamp. #6923 measured this (oldMs=6, sampleMs=2003, newMs=200, #issuecomment-5996678986), but only as responsiveness under sustained degradation. The transient case wasn't covered there: one slow sample, then the peer is healthy again.
Example with timeout = 500 and a warmed estimate of 100 ms: any single sample ≥ 4100 ms saturates. Samples can be up to ADV_TIME_OUT = 20 s before PeerStatusCheck disconnects. Decaying back below 250 ms then takes ~9 healthy samples. During that time:
- if this peer is the old peer, the saturation gate dispatches a secondary on the next 50 ms tick;
- if it is a candidate, it ranks last.
The old P75 over a ~1028-sample decaying reservoir was essentially insensitive to a single outlier.
This costs at most some extra secondaries and duplicate downloads, not correctness, so it may well be acceptable. If not, a one-line change caps any single sample's influence at timeout / 10:
fetchLatency = clampFetchLatency(
(fetchLatency * (FETCH_LATENCY_EWMA_FACTOR - 1) + clampFetchLatency(latencyMillis))
/ FETCH_LATENCY_EWMA_FACTOR);
Either way, it would help to state the choice in the Javadoc next to the α rationale.
There was a problem hiding this comment.
Thanks for looking at this — you're right that the transient-outlier case is not covered by the #6923 runs, and the arithmetic checks out: with the blend applied before the clamp, a single sample of ≥ ~10× a healthy estimate moves the estimate straight to the clamp, and at α = 0.1 it takes about nine healthy samples to decay back (≈27 s at the 3 s block cadence).
We kept blend-then-clamp deliberately. Clamping the sample instead of the estimate caps each sample's contribution, so under sustained degradation the estimate would approach the clamp only geometrically — roughly 20+ samples versus the measured 1–3 — which directly weakens the saturation-gate switching that motivated this change, and the convergence behavior pinned in #6923. The transient cost is bounded as you note: while the estimate is saturated, the worst case is one secondary dispatch per 50 ms worker tick, every response still feeds the estimator a real measured sample, hard timeout remains the unconditional backstop, and the ranking self-heals within ~9 samples.
So we prefer keeping the current semantics; the choice, its rationale, and the transient-outlier cost are now stated in the estimator javadoc (cb7dd42). Happy to revisit sample-side clamping if you still prefer it.
…hase1-deprecate-legacy-metrics
4 participants
What does this PR do?
Implements #6923 (item 7 of #6921) — Phase 1 of the two-release retirement of the legacy Monitor metrics stack: migrate its only functional consumer, deprecate the legacy APIs, and add the replacement observability, while keeping both legacy endpoints fully functional until Phase 2. This is Phase 1 of #6923; the issue stays open until the Phase 2 removal PR.
net.latency.fetch.block.<peerIP>— written viahistogramUpdateUnCheck(bypassing the metrics-enable gate), keyed per peer IP, never cleaned on disconnect — is removed fromBlockMsgHandler(write site) andFetchBlockService(read site). Peer selection now ranks by a bounded, per-connection EWMA of measured request-to-block durations onPeerConnection:Channel.getAvgLatency()), never a placeholder observation;α = 0.1,(ewma * 9 + last) / 10, named constant with documented rationale) starts from the second sample;fetchBlockTimeout;shouldFetchBlockgains an explicit hard-timeout branch (unconditional switch once the wall-clock budget is exhausted) plus a saturation gate requiring a strictly better candidate;PeerConnection— O(1) read, bounded memory, no unbounded per-IP keys, no disconnect bookkeeping.tron:node_infois an Info metric with two labels; the three fetch counters are unlabeled.tron:node_info{version, genesis_block_id}(Info) — node version plus the full genesis block hash as the canonical chain identifier;tron:block_fetch_armed— incremented whenFetchBlockServicearms a fetch tracking;tron:block_fetch_secondary— incremented when a secondary fetch is sent;tron:block_already_known— incremented for a matching outstanding adv request whose exact block ID is already known before processing that response (best-effort; concurrent arrivals may be missed; does not establish secondary-fetch attribution).The three fetch counters describe fetch behavior that remains after the legacy stack is removed and are retained long-term; normalized rates are derived in PromQL. In code the counters are named
tron:block_fetch_armed/tron:block_fetch_secondary/tron:block_already_known; the Prometheus exposition appends_total(and thetron:nodecollector surfaces astron:node_info), matching the names used in [Feature] Remove the legacy Monitor API and non-Prometheus metrics implementation #6923. The existing unlabeledtron:block_fetch_latency_secondshistogram is unchanged.service Monitorandmessage MetricsInfoare markedoption deprecated = truein the protos; a startup WARN fires whennode.metricsEnableis present, and a process-once WARN fires on the first invocation of either deprecated API (gRPCMonitor.GetStatsInfoor HTTP/monitor/getstatsinfo— the latter is not gated by the config key). Both APIs keep serving exactly as before.Why are these changes required?
Per #6921, Prometheus is the single supported monitoring backend and public APIs get a one-release deprecation window before removal. The per-IP histogram is functional scheduling state, not observability: its read must be migrated before the legacy registry can be deleted, so Phase 2 can be a pure deletion following the
WalletExtensionstaging precedent (#6975). The old signal also has real defects the EWMA fixes: a candidate that has never served a fetch reads P75 = 0.0 from the auto-created empty histogram and is always ranked fastest, making the comparison branch history-dependent; samples from different fetch paths are mixed; and the metric family is unbounded.Behaviour differences vs
developListed per principle 1 of #6921: previously a candidate whose P75 exceeded
fetchBlockTimeoutwas filtered out; with the clamped EWMA a saturated candidate stays eligible and may receive the secondary request at the hard timeout when no better candidate exists — a deliberate liveness improvement, documented and tested.This PR has been tested by:
checkstyleMain,checkstyleTest,git diff --check.Follow up
metric_monitor/README.mdmirror update.node.metricsEnablechain (tombstone WARN when still present), proto definitions, and the Dropwizard dependency; the three counters andtron:node_inforemain. Target major will be recorded in Tracking: code refactor and cleanup #6921 and cross-referenced in [Feature] Remove the legacy Monitor API and non-Prometheus metrics implementation #6923.Extra details
Monitor.GetStatsInfo, HTTP/monitor/getstatsinfo) are unchanged; removal only in Phase 2. Config migration is time-boxed and non-breaking during Phase 1:node.metricsEnable = truenode.metrics.prometheus.enable = truenode.metrics.prometheus.port = 9527(default)option deprecated = true(previously only field-level); generated stubs are unchanged.