Skip to content

feat(eth/protocols/eth,p2p): add handler performance metrics #22581 - #3050

Open
gzliudan wants to merge 1 commit into
XinFinOrg:dev-upgradefrom
gzliudan:port-handler-metrics
Open

gzliudan wants to merge 1 commit into
XinFinOrg:dev-upgradefrom
gzliudan:port-handler-metrics

Conversation

@gzliudan

Copy link
Copy Markdown
Collaborator

Proposed changes

Port geth PR ethereum#22581: inbound eth packets had no serving-time signal, so slow handlers could not be correlated with the packet types that triggered them. This adds a microsecond histogram per protocol version and message code, aligned with current geth master (resetting sample, Enabled() as a function). metrics.GetOrRegisterHistogramLazy and ResettingSample were already ported here, and this fork has no snap protocol, so only the metric prefix and the eth dispatcher instrumentation are needed.

Summary

 handleMessage(backend, peer)
   msg, err := peer.rw.ReadMsg()
   size check
   defer msg.Discard()
+  if metrics.Enabled()
+    defer GetOrRegisterHistogramLazy("p2p/handle/eth/<version>/<code>")
+         .Update(time.Since(start).Microseconds())
   switch msg.Code
  • p2p.HandleHistName = "p2p/handle" supplies the prefix.
  • Sampler is metrics.ResettingSample(metrics.NewExpDecaySample(1028, 0.015)), so rarely seen packets still refresh instead of pinning a stale histogram; GetOrRegisterHistogramLazy avoids constructing a sample on every request.
  • Metric names embed xdc protocol versions (100/164/165/166) rather than upstream eth versions. No wire or state surface changes.
  • The snap handler has no counterpart in this fork, and metrics/histogram.go already carries the lazy helper, so two files change.

Ref: ethereum#22581

Evidence

  • Before: git grep HandleHistName dev-upgrade returns nothing and no p2p/handle/* metric is registered.
    After: a temporary probe with metrics enabled and one inbound message at xdc/165 code 0xff reports histogram "p2p/handle/eth/165/0xff" count=1 max=17us. The probe was removed after the run; the resetting sample resets on snapshot read, which the probe accounts for.
  • Per-commit verification (2026-11-10 16:53:05-17:01:00, log /tmp/verify-logs/port-handler-metrics-c54169f1fb-I3TWij.log): make all -> make generate (No stale files detected.) -> make tidy (No untidy module files detected.) -> clean tree -> make lint (0 issues.) -> go vet ./... -> make test, all pass at c54169f1fb.
  • go test ./eth/protocols/eth/ ./p2p/ -count=1 passes.

Merge Danger

Door: two-way

Reverting the commit removes the histograms; there is no migration, state change or wire change to unwind.

Blast Radius: Network

Only inbound eth message handling is touched. With metrics disabled the added cost is one metrics.Enabled() call per message; with metrics enabled each message registers or updates one histogram. No consensus, RPC or protocol-version behaviour changes.

Types of changes

What types of changes does your code introduce to XDC network?
Put an ✅ in the boxes that apply

  • build: Changes that affect the build system or external dependencies
  • ci: Changes to CI configuration files and scripts
  • chore: Changes that don't change source code or tests
  • docs: Documentation only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that neither fixes a bug nor adds a feature
  • revert: Revert something
  • style: Changes that do not affect the meaning of the code
  • test: Adding missing tests or correcting existing tests

Impacted Components

Which parts of the codebase does this PR touch?
Put an ✅ in the boxes that apply

  • Consensus
  • Account
  • Network
  • Geth
  • Smart Contract
  • External components
  • Not sure (Please specify below)

Checklist

Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that

  • This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
  • Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
  • Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
  • Tested the backwards compatibility.
  • Tested with XDC nodes running this version co-exist with those running the previous version.
  • Relevant documentation has been updated as part of this PR
  • N/A

Unchecked items: upstream ethereum#22581 ships no tests, and this port adds no test file. The existing eth/protocols/eth suite covers the metrics-disabled path, while the metrics-enabled path was verified with the temporary probe above. A private-network epoch run, mixed-version coexistence run and backwards-compatibility run do not apply because there is no consensus, state or wire change, and no documentation describes this metrics path.

End-to-end test plan: start a node with --metrics and a metrics sink, generate inbound eth traffic from a peer, and confirm p2p/handle/eth/<version>/<code> histograms appear with microsecond-scale latencies.

@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 4001230 and f7cba8d.

Files selected for processing: 2

  • eth/protocols/eth/handler.go
  • p2p/metrics.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@gzliudan
gzliudan force-pushed the port-handler-metrics branch 2 times, most recently from a797879 to f19ecd7 Compare November 12, 2026 12:03
…#22581

Inbound eth packets had no serving-time signal, so there was no way to correlate slow message handling with the packet types that caused it. Port geth ethereum#22581 to record a microsecond histogram per protocol version and message code.

The snap handler does not exist in this fork, and the histogram helper plus resetting sample were already ported, so only the metric name and the dispatcher instrumentation are needed.

Ref: ethereum#22581
@gzliudan
gzliudan force-pushed the port-handler-metrics branch from f19ecd7 to f7cba8d Compare November 12, 2026 23:50

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants