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.
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.
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.