Skip to content

Conversation

@coot
Copy link
Contributor

@coot coot commented Dec 3, 2025

The information on block-fetch is exposed in other traces in
the TraceFetchClientState tracer:

  • SendFetchRequest - when a block is requested
  • CompletedBlockFetch - when a block is fetched, includes updated
    PeerFetchInFlight information, e.g. number of blocks in flight,
    bytes in flight, etc from that peer.

See also other messages in that tracer.

@coot coot requested a review from a team as a code owner December 3, 2025 15:12
Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mgmeier mgmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't even had a discussion about this tracer, or about who might be using it.
Even though I see redundant information, there is unique info in the trace messages, and the metric based on it.

Furthermore, it can be easily shut off entirely in the Node config if not desired.

I'm going to RFC this PR until we had a proper conversation about it.

The information on block-fetch is exposed in other traces in
the `TraceFetchClientState` tracer:

* `SendFetchRequest` - when a block is requested
* `CompletedBlockFetch` - when a block is fetched, includes updated
  `PeerFetchInFlight` information, e.g. number of blocks in flight,
  bytes in flight, etc from that peer.

See also other messages in that tracer.
@mgmeier mgmeier force-pushed the coot/removed-PeerT-tracer branch from cc5422f to 103c3ff Compare December 4, 2025 10:13
@mgmeier mgmeier requested review from a team as code owners December 4, 2025 10:13
Copy link
Contributor

@mgmeier mgmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

The metric peersFromNodeKernel will disappear as a consequence of this PR; however, I believe that information can be recovered by considering the peerSelection_* bundle of metrics.

The complexity the PeerT tracer adds outweighs the little convenience it offers. I'm fine with removing it.

@mgmeier mgmeier force-pushed the coot/removed-PeerT-tracer branch from 103c3ff to d559136 Compare December 4, 2025 12:33
@mgmeier mgmeier added this pull request to the merge queue Dec 4, 2025
Merged via the queue into master with commit 101fe39 Dec 4, 2025
72 of 74 checks passed
@mgmeier mgmeier deleted the coot/removed-PeerT-tracer branch December 4, 2025 22:41
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.

4 participants