rust(feat): Add test server to CLI #450
Conversation
rust/crates/sift_cli/build.rs
Outdated
| @@ -0,0 +1,21 @@ | |||
| /// Build descriptor's so that the Black Hole gRPC server can | |||
| /// stand up the reflection service. | |||
| fn main() -> Result<(), Box<dyn std::error::Error>> { | |||
There was a problem hiding this comment.
There shouldn't be a need for you to compile these protos since they're already compiled and available in the sift_rs crate which you can add as a dependency like here:
sift/rust/crates/sift_cli/Cargo.toml
Line 28 in 737efa8
There was a problem hiding this comment.
This is just compiling the descriptors since we need that for the reflection service. We could either move this to sift_rs and create this when generating the protos, or keep it in the build step for sift_cli.
There was a problem hiding this comment.
I updated the build to generate these, we're pulling it from sift_rs now: 0861724
| use crate::cmd::test_server::metrics_streaming_client::Metrics; | ||
|
|
||
| pub async fn run(ctx: Context, args: TestServerArgs) -> Result<ExitCode> { | ||
| let local_address = args.local_address.unwrap_or("127.0.0.1:50051".into()); |
There was a problem hiding this comment.
I would prefer 0.0.0.0 since it's more user-friendly. Allows it to be reached inside of a docker network for example. Also as a note of good practice:
local_address = args.local_address.unwrap_or_else(|| "127.0.0.1:50051".to_string())for lazy evaluation of the fallback rather than immediate
|
|
||
| pub async fn run(ctx: Context, args: TestServerArgs) -> Result<ExitCode> { | ||
| let local_address = args.local_address.unwrap_or("127.0.0.1:50051".into()); | ||
| let addr = local_address.parse()?; |
There was a problem hiding this comment.
Generally helpful to include contextual information with errors for better traceability:
use anyhow::{Result, Context};
let addr = local_address.parse().context("foobar")?;There was a problem hiding this comment.
Same for all other places.
| // Initialize streaming client. | ||
| let mut streaming_client = | ||
| MetricsStreamingClient::build(ctx, &args.stream_metrics, &args.metrics_asset_name)?; | ||
| if streaming_client.is_some() { |
There was a problem hiding this comment.
Prefer:
if let Some(client) = streaming_client.as_mut() {
client.initialize.await.context("failed to initialize client").await?;
}|
|
||
| // Start task to ingest metrics to Sift. | ||
| let ingest_metrics_task = tokio::spawn(async move { | ||
| if streaming_client.is_none() { |
There was a problem hiding this comment.
Prefer:
let Some(mut client) = streaming_client else {
return;
};| return; | ||
| } | ||
|
|
||
| let mut client = streaming_client.unwrap(); |
There was a problem hiding this comment.
see:
let Some(mut client) = streaming_client else {
return;
};
solidiquis
left a comment
solidiquis
left a comment
There was a problem hiding this comment.
Definitely run: cargo fmt and cargo clippy. The latter will give you a lot of good feedback.
| last_total_num_messages = current_total_num_messages; | ||
|
|
||
| // Clear terminal and print metrics. | ||
| stdout() |
There was a problem hiding this comment.
Calling stdout() repeatedly is bad for performance because it will repeatedly get a handle and lock stdout. See this method to acquire the handle/lock once: https://doc.rust-lang.org/std/io/struct.Stdout.html#method.lock
| #[derive(Default)] | ||
| pub struct TestServer { | ||
| /// Total number of streams created. | ||
| total_num_streams: AtomicU32, |
There was a problem hiding this comment.
Do these need to be atomics?
There was a problem hiding this comment.
They're modified in the the grpc handlers, so they need to be some type of synchronized struct.
| #[tonic::async_trait] | ||
| impl PingService for TestServer { | ||
| async fn ping(&self, _request: Request<PingRequest>) -> Result<Response<PingResponse>, Status> { | ||
| let resp = PingResponse { |
There was a problem hiding this comment.
You might be able to just do PingResponse::default().
marc-sift
force-pushed
the
test-server-cli
branch
from
7680510 to
0861724
Compare
marc-sift
force-pushed
the
test-server-cli
branch
from
0861724 to
b14b74e
Compare
No description provided.