-
Notifications
You must be signed in to change notification settings - Fork 155
Relay submissions more metrics #672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
* add more submission metrics to builder * use spawn_blocking for gzip * add submission metrics to test relay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances metrics collection for relay submissions by adding timing and compression metrics, moving blocking operations out of Tokio's runtime, and optimizing gzip compression for better performance.
- Add more detailed metrics for relay submission timing and payload sizes
- Move blocking compression operations to dedicated blocking threads
- Switch from default to fast gzip compression for relay submissions
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
crates/test-relay/src/relay.rs | Add timing metrics for measuring latency from builder to relay |
crates/test-relay/src/metrics.rs | Add new histogram metric for time-to-receive measurements |
crates/test-relay/src/main.rs | Add periodic histogram reset functionality |
crates/rbuilder/src/utils/mod.rs | Add utility function for microsecond timestamps |
crates/rbuilder/src/telemetry/metrics/mod.rs | Add new metrics for relay submission steps and payload sizes |
crates/rbuilder/src/primitives/mev_boost.rs | Update relay client interface to return submission statistics |
crates/rbuilder/src/mev_boost/submission.rs | Add sealed timestamp to bid metadata |
crates/rbuilder/src/mev_boost/mod.rs | Implement detailed timing metrics and move compression to blocking thread |
crates/rbuilder/src/live_builder/order_input/mod.rs | Move blocking orderpool operations to spawn_blocking |
crates/rbuilder/src/live_builder/block_output/relay_submit.rs | Add submission stats metrics collection |
crates/rbuilder/benches/benchmarks/mev_boost.rs | Add missing import for benchmark |
body_data = encoder | ||
.finish() | ||
.map_err(|e| SubmitBlockErr::RPCSerializationError(e.to_string()))?; | ||
body_data = spawn_blocking(move || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spawn_blocking call moves the entire body_data Vec to the blocking thread, which could be inefficient for large payloads. Consider using a streaming approach or at least moving only the encoder setup to the blocking thread while keeping the data processing async.
Copilot uses AI. Check for mistakes.
.map_err(|e| SubmitBlockErr::RPCSerializationError(e.to_string())) | ||
}) | ||
.await | ||
.map_err(|e| SubmitBlockErr::RPCSerializationError(e.to_string()))??; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The double question mark operator (??) is used to handle both the spawn_blocking error and the compression error. This creates nested error handling that could be clearer if split into separate statements or using a more explicit error handling approach.
.map_err(|e| SubmitBlockErr::RPCSerializationError(e.to_string()))??; | |
.map_err(|e| SubmitBlockErr::RPCSerializationError(e.to_string()))?; | |
body_data = join_result?; |
Copilot uses AI. Check for mistakes.
tokio::spawn(async { | ||
tokio::time::sleep(HISTOGRAM_RESET_TIME).await; | ||
reset_histogram_metrics_test_relay(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spawned task for histogram reset runs indefinitely without proper cancellation handling. Consider adding cancellation token support or at least a way to gracefully shut down this background task.
tokio::spawn(async { | |
tokio::time::sleep(HISTOGRAM_RESET_TIME).await; | |
reset_histogram_metrics_test_relay(); | |
let cancellation = global_cancellation.clone(); | |
tokio::spawn(async move { | |
tokio::select! { | |
_ = tokio::time::sleep(HISTOGRAM_RESET_TIME) => { | |
reset_histogram_metrics_test_relay(); | |
} | |
_ = cancellation.cancelled() => { | |
// Graceful shutdown: do nothing, just exit the task | |
} | |
} |
Copilot uses AI. Check for mistakes.
Ok(state) => state, | ||
Err(err) => { | ||
error!(?err, "Failed to get latest state"); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The early return inside the spawn_blocking closure prevents the rest of the cleanup logic from running when state provider fails. This could lead to incomplete orderpool maintenance. Consider using a Result return type instead of early returns.
Copilot uses AI. Check for mistakes.
π Summary
β I have completed the following steps:
make lint
make test