refactor(metrics): phase 1 deprecation of the legacy monitor stack by warku123 · Pull Request #6988 · tronprotocol/java-tron

warku123 · 2026-09-22T03:04:47Z

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.

Fetch-block peer selection migrated off the metrics registry. The per-IP Dropwizard histogram net.latency.fetch.block.<peerIP> — written via histogramUpdateUnCheck (bypassing the metrics-enable gate), keyed per peer IP, never cleaned on disconnect — is removed from BlockMsgHandler (write site) and FetchBlockService (read site). Peer selection now ranks by a bounded, per-connection EWMA of measured request-to-block durations on PeerConnection:
- explicit unsampled state; while unsampled, reads fall back to the libp2p channel RTT (Channel.getAvgLatency()), never a placeholder observation;
- the first real fetch replaces the fallback; smoothing (fixed α = 0.1, (ewma * 9 + last) / 10, named constant with documented rationale) starts from the second sample;
- estimates are clamped to fetchBlockTimeout; shouldFetchBlock gains an explicit hard-timeout branch (unconditional switch once the wall-clock budget is exhausted) plus a saturation gate requiring a strictly better candidate;
- state lives and dies with the PeerConnection — O(1) read, bounded memory, no unbounded per-IP keys, no disconnect bookkeeping.
New metrics (Prometheus): tron:node_info is 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 when FetchBlockService arms 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 the tron:node collector surfaces as tron:node_info), matching the names used in [Feature] Remove the legacy Monitor API and non-Prometheus metrics implementation #6923. The existing unlabeled tron:block_fetch_latency_seconds histogram is unchanged.
Deprecation, no removal. service Monitor and message MetricsInfo are marked option deprecated = true in the protos; a startup WARN fires when node.metricsEnable is present, and a process-once WARN fires on the first invocation of either deprecated API (gRPC Monitor.GetStatsInfo or 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 WalletExtension staging 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 develop

Listed per principle 1 of #6921: previously a candidate whose P75 exceeded fetchBlockTimeout was 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:

Unit Tests
CI / static checks: checkstyleMain, checkstyleTest, git diff --check.

Follow up

Docs: legacy-field-to-Prometheus mapping table (including fields without an equivalent) published in the release notes and the documentation-en / documentation-zh repos before the release ships.
tron-docker metric_monitor/README.md mirror update.
Phase 2 PR in the agreed major release: remove the legacy implementation, node.metricsEnable chain (tombstone WARN when still present), proto definitions, and the Dropwizard dependency; the three counters and tron:node_info remain. 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

Backward Compatibility: Phase 1 is non-breaking — runtime behavior and response formats of both legacy endpoints (gRPC Monitor.GetStatsInfo, HTTP /monitor/getstatsinfo) are unchanged; removal only in Phase 2. Config migration is time-boxed and non-breaking during Phase 1:

Before	After
`node.metricsEnable = true`	`node.metrics.prometheus.enable = true`
(legacy port n/a)	`node.metrics.prometheus.port = 9527` (default)

The proto deprecation is the repo's first service/message-level option deprecated = true (previously only field-level); generated stubs are unchanged.
Experiment harness, configurations, and raw results: [Feature] Remove the legacy Monitor API and non-Prometheus metrics implementation #6923 (comment) (linked per reviewer request in [Feature] Remove the legacy Monitor API and non-Prometheus metrics implementation #6923).

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.

@deprecated

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

…already_known

…tests

SeriousCoding789 · 2026-10-02T08:34:23Z

    public static final String P2P_DISCONNECT = "tron:p2p_disconnect";
    public static final String INTERNAL_SERVICE_FAIL = "tron:internal_service_fail";
+    // verification counters for the bounded fetch latency estimator rollout
+    public static final String BLOCK_FETCH_ARMED = "tron:block_fetch_armed";


I suggest use a more easy to understand name like tron:block_fetch_tracked instead of tron:block_fetch_armed. Replace tron:block_fetch_secondary withtron:block_fetch_failover

Thanks for the suggestion. These counter names and their semantics were discussed and agreed in the tracking issue before implementation: the counters were requested as unlabeled, monotonically increasing series for fetch trackings armed and secondary fetches sent (#6923 (comment)), the final names tron:block_fetch_armed and tron:block_fetch_secondary were confirmed there (#6923 (comment)), and the counters are expected to be kept beyond Phase 2 (#6923 (comment)). The verification data already posted in the issue references these names as well. To keep the public metric contract consistent with that discussion, we would prefer to keep the current names; happy to revisit if the issue discussion converges on different ones.

SeriousCoding789 · 2026-10-02T08:35:08Z

+    // verification counters for the bounded fetch latency estimator rollout
+    public static final String BLOCK_FETCH_ARMED = "tron:block_fetch_armed";
+    public static final String BLOCK_FETCH_SECONDARY = "tron:block_fetch_secondary";
+    public static final String BLOCK_ALREADY_KNOWN = "tron:block_already_known";


tron:block_fetch_duplicate replace tron:block_already_known?

Then all the below initialisation and calculation logic will looks very clear. Such as

// Block fetch failover and duplicate-download counters. public static final String BLOCK_FETCH_TRACKED = "tron:block_fetch_tracked"; public static final String BLOCK_FETCH_FAILOVER = "tron:block_fetch_failover"; public static final String BLOCK_FETCH_DUPLICATE = "tron:block_fetch_duplicate"; init(MetricKeys.Counter.BLOCK_FETCH_TRACKED, "next-block fetches tracked for failover."); init(MetricKeys.Counter.BLOCK_FETCH_FAILOVER, "tracked block fetches re-requested from another peer (failover)."); init(MetricKeys.Counter.BLOCK_FETCH_DUPLICATE, "requested blocks that arrived after the same block had already been received " + "(lower bound: a copy arriving while the first is still processed is missed).");

Thanks — same as above: tron:block_already_known was named deliberately in the issue discussion. The counter records a matching outstanding adv request whose exact block id is already known before the response is processed, and it was named already-known rather than duplicate precisely because it cannot establish secondary-fetch attribution and may miss concurrent arrivals (#6923 (comment); naming confirmation: #6923 (comment)). We would like to keep the agreed name for consistency with the issue.

Thanks for the concrete sketch. The three names are pinned by the issue discussion (see the replies above), so we will keep them as-is. If you see wording in the init descriptions that could be clearer within the current names, we are happy to adopt that.

SeriousCoding789 · 2026-10-02T08:39:06Z

+
+  private volatile long fetchLatency;
+
+  private volatile boolean fetchLatencySeeded;


Please add comments to explain the name fetchLatencySeeded or rename it as hasFetchLatencySampled

Renamed to hasFetchLatencySampled in e3a8b12 (declaration, both update sites, and the fallback read; zero references to the old name remain).

SeriousCoding789 · 2026-10-02T09:10:59Z

  private volatile long blockRcvTime;

+  /**
+   * EWMA smoothing divisor for the fetch latency estimator: the previous estimate is


Please first use Exponentially Weighted Moving Average(EWMA) instead of using EWMA directly.

Done in e3a8b12 — the first occurrence now reads Exponentially Weighted Moving Average (EWMA); later uses keep the acronym.

SeriousCoding789 · 2026-10-02T09:17:44Z

+      fetchLatencySeeded = true;
+    } else {
+      fetchLatency = clampFetchLatency(
+          (fetchLatency * (EWMA_DIVISOR - 1) + latencyMillis) / EWMA_DIVISOR);


I see inside clampFetchLatency there is a StrictMathWrapper.min(CommonParameter.getInstance().fetchBlockTimeout, latency), does the latencyMillis passed here need to do that min round? to make sure the passed latencyMillis is valid.

Good question — no pre-clamp is needed at the call site. latencyMillis is the measured wall-clock request-to-block duration: BlockMsgHandler passes now - time, where both come from System.currentTimeMillis() (time is the timestamp recorded when the adv request was sent), so the value is a non-negative measured delta rather than an external input. And clampFetchLatency already bounds every stored value with max(0, min(fetchBlockTimeout, latency)) — the clamp lives inside the method precisely so both write paths (first sample and the EWMA blend) are clamped in one place. Pre-clamping the argument would be redundant.

…ments

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

warku123 added 18 commits September 8, 2026 16:54

refactor(metrics): use libp2p avg latency for fetch-block peer selection

4e749e8

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.

test(net): pin fetch-block failover semantics for unknown peer latency

20ee8a9

feat(metrics): add chain id to node info metric

aeee8c7

feat(metrics): rename node_info chain_id label to genesis_block_id

74d952e

The label value is the chain id derived from the genesis block hash, so genesis_block_id describes what it identifies more accurately.

chore(protocol): mark legacy Monitor service and MetricsInfo as depre…

56ef291

…cated Add 'option deprecated = true;' to the Monitor gRPC service and the MetricsInfo message so generated classes carry @deprecated.

chore(metrics): drop unused NET_LATENCY_FETCH_BLOCK key

10a8f2a

Its read and write points were removed with the legacy fetch-block histogram.

chore(metrics): clarify clamp comment and add missing eof newline

1f1a549

feat(metrics): add armed fetch counter and rename duplicate to block_…

2fe5584

…already_known

refactor(metrics): extract EWMA divisor constant and add convergence …

f7c9adf

…tests

fix(metrics): count requested already-known blocks by exact id

b3e8dfd

docs(metrics): register phase 1 metrics in changelog

4443724

github-actions Bot requested a review from halibobo1205 September 22, 2026 03:04

bladehan1 requested a review from SeriousCoding789 September 22, 2026 03:24

SeriousCoding789 added this to java-tron Sep 22, 2026

SeriousCoding789 added this to the GreatVoyage-v4.8.3 milestone Sep 22, 2026

SeriousCoding789 linked an issue Sep 22, 2026 that may be closed by this pull request

[Feature] Remove the legacy Monitor API and non-Prometheus metrics implementation #6923

Open

waynercheung mentioned this pull request Sep 26, 2026

[Feature] Remove the legacy Monitor API and non-Prometheus metrics implementation #6923

Open

SeriousCoding789 reviewed Oct 2, 2026

View reviewed changes

refactor(metrics): rename EWMA divisor to sliding factor and trim com…

feb287c

…ments

Navigation Menu

Repository navigation

refactor(metrics): phase 1 deprecation of the legacy monitor stack - #6988

refactor(metrics): phase 1 deprecation of the legacy monitor stack#6988
warku123 wants to merge 20 commits into
tronprotocol:release_v4.8.3from
warku123:feature/phase1-deprecate-legacy-metrics

warku123 commented Sep 22, 2026 •

edited

Loading

Uh oh!

SeriousCoding789 Oct 2, 2026

Uh oh!

warku123 Oct 5, 2026

Uh oh!

SeriousCoding789 Oct 2, 2026

Uh oh!

SeriousCoding789 Oct 2, 2026

Uh oh!

warku123 Oct 5, 2026

Uh oh!

warku123 Oct 5, 2026

Uh oh!

SeriousCoding789 Oct 2, 2026 •

edited

Loading

Uh oh!

warku123 Oct 5, 2026

Uh oh!

SeriousCoding789 Oct 2, 2026

Uh oh!

warku123 Oct 5, 2026

Uh oh!

SeriousCoding789 Oct 2, 2026

Uh oh!

warku123 Oct 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		private volatile long fetchLatency;

		private volatile boolean fetchLatencySeeded;

Conversation

warku123 commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeriousCoding789 Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

warku123 commented Sep 22, 2026 •

edited

Loading

SeriousCoding789 Oct 2, 2026 •

edited

Loading