Metrics reporting for tpu-client-next #5338

alexpyattaev · 2025-03-17T22:06:29Z

Problem

tpu-client-next has internal stats but they are not well-designed: memory consumption is unbounded, and no reporting into metrics is provisioned
much of the internal state for ConnectionWorkersScheduler is managed externally in callers

Summary of Changes

Internalize some of the state for ConnectionsWorkerScheduler (including the stats)
remove (now unneeded) generics from TpuClientNextClient
patch ConnectionsWorkerScheduler stats into influxdb via a small coroutine that can periodically report stats using the same tokio runtime

mergify · 2025-03-18T10:52:20Z

If this PR represents a change to the public RPC API:

Make sure it includes a complementary update to rpc-client/ (example)
Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @alexpyattaev.

KirillLykov · 2025-03-23T09:29:04Z

send-transaction-service/src/transaction_client.rs

-{
-    pub fn new(
+impl TpuClientNextClient {
+    pub fn new<T>(


I see why the structure is not needed to be generic any more: because Scheduler is stateful and hence generic can be moved just to new.

KirillLykov · 2025-03-23T09:39:38Z

tpu-client-next/src/metrics.rs

+        while !cancel.is_cancelled() {
+            interval.tick().await;
+            let view = stats.read_and_reset();
+            let connect_error = view.connect_error_cids_exhausted
+                + view.connect_error_other
+                + view.connect_error_invalid_remote_address;
+            let connection_error = view.connection_error_reset
+                + view.connection_error_cids_exhausted
+                + view.connection_error_timed_out
+                + view.connection_error_application_closed
+                + view.connection_error_transport_error
+                + view.connection_error_version_mismatch
+                + view.connection_error_locally_closed;
+            let write_error = view.write_error_stopped
+                + view.write_error_closed_stream
+                + view.write_error_connection_lost
+                + view.write_error_zero_rtt_rejected;
+
+            datapoint_info!(
+                name,
+                ("connect_error", connect_error, i64),
+                ("connection_error", connection_error, i64),
+                ("successfully_sent", view.successfully_sent, i64),
+                ("write_error", write_error, i64),
+            );
+        }


Suggested change

while !cancel.is_cancelled() {

interval.tick().await;

let view = stats.read_and_reset();

let connect_error = view.connect_error_cids_exhausted

+ view.connect_error_other

+ view.connect_error_invalid_remote_address;

let connection_error = view.connection_error_reset

+ view.connection_error_cids_exhausted

+ view.connection_error_timed_out

+ view.connection_error_application_closed

+ view.connection_error_transport_error

+ view.connection_error_version_mismatch

+ view.connection_error_locally_closed;

let write_error = view.write_error_stopped

+ view.write_error_closed_stream

+ view.write_error_connection_lost

+ view.write_error_zero_rtt_rejected;

datapoint_info!(

name,

("connect_error", connect_error, i64),

("connection_error", connection_error, i64),

("successfully_sent", view.successfully_sent, i64),

("write_error", write_error, i64),

);

}

loop {

select! {

_ = interval.tick() => {

let view = stats.read_and_reset();

let connect_error = view.connect_error_cids_exhausted

+ view.connect_error_other

+ view.connect_error_invalid_remote_address;

let connection_error = view.connection_error_reset

+ view.connection_error_cids_exhausted

+ view.connection_error_timed_out

+ view.connection_error_application_closed

+ view.connection_error_transport_error

+ view.connection_error_version_mismatch

+ view.connection_error_locally_closed;

let write_error = view.write_error_stopped

+ view.write_error_closed_stream

+ view.write_error_connection_lost

+ view.write_error_zero_rtt_rejected;

datapoint_info!(

name,

("connect_error", connect_error, i64),

("connection_error", connection_error, i64),

("successfully_sent", view.successfully_sent, i64),

("write_error", write_error, i64),

);

}

_ = cancel.cancelled() => break,

}

Otherwise, we will wait for the specified duration (3sec) until we cancel.

KirillLykov · 2025-03-23T09:47:19Z

tpu-client-next/src/metrics.rs

+    tokio_util::sync::CancellationToken,
+};
+
+impl SendTransactionStats {


What if some hypothetical client will decide to report metrics somewhere else (not influxdb)? Am I right that the idea is that in this case client will just write his own function report_to_something (because he cannot do impl for SendTransactionStats)?

KirillLykov · 2025-03-23T09:52:27Z

tpu-client-next/src/metrics.rs

@@ -0,0 +1,47 @@
+use {


Suggested change

use {

//! If `metrics` feature is activated, this module provides `report_to_influxdb`

//! method for [`SendTransactionStats`] which periodically reports transaction

//! sending statistics to InfluxDB.

use {

In this crate we use module-level comments

KirillLykov · 2025-03-23T09:54:25Z

tpu-client-next/src/metrics.rs

+};
+
+impl SendTransactionStats {
+    ///Report the statistics to influxdb in a compact form


Suggested change

///Report the statistics to influxdb in a compact form

/// Report the statistics to influxdb in a compact form.

A little bit of bike shedding here

KirillLykov · 2025-03-23T09:58:11Z

send-transaction-service/src/transaction_client.rs

-            Box::new(leader_updater.clone()),
-            receiver,
+        let scheduler = ConnectionWorkersScheduler::new(Box::new(leader_updater), receiver);
+        // leaking handle to this task, as it will run until the end of life of the process


It might be cancelled as well when, for example, validator identity is updated. So in this case metrics task will be stopped but not will be relaunched in do_update_key

KirillLykov · 2025-03-23T10:01:18Z

send-transaction-service/src/transaction_client.rs

+        // leaking handle to this task, as it will run until the end of life of the process
+        runtime_handle.spawn(scheduler.stats.clone().report_to_influxdb(
+            "send-transaction-service-TPU-client",
+            Duration::from_secs(3),


Could you move this to the some const at the top of the module?

KirillLykov · 2025-03-23T10:05:04Z

tpu-client-next/src/connection_workers_scheduler.rs

+pub struct ConnectionWorkersScheduler {
+    leader_updater: Box<dyn LeaderUpdater>,
+    transaction_receiver: TransactionReceiver,
+    pub stats: Arc<SendTransactionStats>,


I would prefer having a method for that instead of making it pub. Because as soon as we publish this crate, it will be very hard to change the implementation because this pub stats will be part of public API (in this case need to remove extra clone() in transaction_client):

pub fn stats(&self) -> Arc<SendTransactionStats> { self.stats.clone() }

KirillLykov · 2025-03-23T10:20:53Z

tpu-client-next/src/send_transaction_stats.rs

@@ -5,13 +5,8 @@ use {
    super::QuicError,
    quinn::{ConnectError, ConnectionError, WriteError},
    std::{
-        collections::HashMap,


It looks like documentation for this module is stale: we don't collect per IP statistics anylonger

KirillLykov

Overall, these changes provide quite some code simplification which I like a lot. Left some comments.

alexpyattaev force-pushed the tpu_client_next_stats branch 4 times, most recently from ac534ea to 372a50a Compare March 18, 2025 10:51

metrics reporting for tpu-client-next

2f28f72

alexpyattaev force-pushed the tpu_client_next_stats branch from 372a50a to 2f28f72 Compare March 18, 2025 11:59

alexpyattaev marked this pull request as ready for review March 18, 2025 12:47

alexpyattaev requested a review from KirillLykov March 18, 2025 12:48

KirillLykov reviewed Mar 23, 2025

View reviewed changes

KirillLykov requested changes Mar 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics reporting for tpu-client-next #5338

Metrics reporting for tpu-client-next #5338

alexpyattaev commented Mar 17, 2025 •

edited

Loading

mergify bot commented Mar 18, 2025

KirillLykov Mar 23, 2025

KirillLykov Mar 23, 2025

KirillLykov Mar 23, 2025

KirillLykov Mar 23, 2025

KirillLykov Mar 23, 2025

KirillLykov Mar 23, 2025

KirillLykov Mar 23, 2025

KirillLykov Mar 23, 2025 •

edited

Loading

KirillLykov Mar 23, 2025 •

edited

Loading

KirillLykov Mar 23, 2025

KirillLykov Mar 23, 2025 •

edited

Loading

KirillLykov Mar 23, 2025

KirillLykov left a comment

-use {
+//! If `metrics` feature is activated, this module provides `report_to_influxdb`
+//! method for [`SendTransactionStats`] which periodically reports transaction
+//! sending statistics to InfluxDB.
+use {

	///Report the statistics to influxdb in a compact form
	/// Report the statistics to influxdb in a compact form.

Metrics reporting for tpu-client-next #5338

Are you sure you want to change the base?

Metrics reporting for tpu-client-next #5338

Conversation

alexpyattaev commented Mar 17, 2025 • edited Loading

Problem

Summary of Changes

mergify bot commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirillLykov Mar 23, 2025 • edited Loading

Choose a reason for hiding this comment

KirillLykov Mar 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirillLykov Mar 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirillLykov left a comment

Choose a reason for hiding this comment

alexpyattaev commented Mar 17, 2025 •

edited

Loading

KirillLykov Mar 23, 2025 •

edited

Loading

KirillLykov Mar 23, 2025 •

edited

Loading

KirillLykov Mar 23, 2025 •

edited

Loading