It really shows! The structure of the code matches what I would expect to see for any project.
You have a huge number of imports in main.rs, which either suggests you need to break this file into smaller modules or you could get away with some minification on the list of imports. (FWIW, breaking it up is the right thing to do; try to keep main.rs as small as possible: really just handling CLI argument parsing and top-level error handling. Everything else should just be a single import from a library or one of the submodule that you call.)
I'm really not used to having all of the code be bundled into a single binary. I've always structured my bins as a tiny wrapper around a giant library. I probably got this idiom from stackoverflow originally. It is indispensible when you want to use most of the application code as a library. And integration testing requires a library, anyway.
For the minification effort, you can combine many lines. It isn't a huge improvement, but it's something. Nearly 10 lines shorter.
diff --git a/src/main.rs b/src/main.rs
index 20c6d41..7c91cf9 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -3,35 +3,24 @@ mod probe;
mod protocol;
use crate::ping::{get_server_status, ProbeStatus};
-use crate::probe::ProbingInfo;
use crate::probe::ResolutionResult::{Plain, Srv};
-use crate::TimeoutError::NegativeDuration;
-use axum::body::Body;
+use crate::{probe::ProbingInfo, TimeoutError::NegativeDuration};
use axum::extract::{Query, State};
use axum::http::header::{ToStrError, CONTENT_TYPE};
use axum::http::{HeaderMap, StatusCode};
use axum::response::{Html, IntoResponse, Response};
-use axum::routing::get;
-use axum::Router;
+use axum::{body::Body, routing::get, Router};
use hickory_resolver::TokioAsyncResolver;
-use prometheus_client::encoding::text::encode;
-use prometheus_client::metrics::family::Family;
-use prometheus_client::metrics::gauge::Gauge;
use prometheus_client::registry::Registry;
+use prometheus_client::{encoding::text::encode, metrics::family::Family, metrics::gauge::Gauge};
use std::collections::hash_map::DefaultHasher;
use std::hash::{Hash, Hasher};
-use std::net::SocketAddr;
-use std::num::ParseFloatError;
use std::sync::atomic::{AtomicU32, AtomicU64};
-use std::sync::Arc;
-use std::time::Duration;
-use tokio::net::TcpListener;
-use tokio::time::timeout;
+use std::{net::SocketAddr, num::ParseFloatError, sync::Arc, time::Duration};
+use tokio::{net::TcpListener, time::timeout};
use tower_http::trace::TraceLayer;
use tracing::{info, instrument, warn};
-use tracing_subscriber::filter::LevelFilter;
-use tracing_subscriber::layer::SubscriberExt;
-use tracing_subscriber::util::SubscriberInitExt;
+use tracing_subscriber::{filter::LevelFilter, layer::SubscriberExt, util::SubscriberInitExt};
use tracing_subscriber::{fmt, Layer};
/// The name of the request header from Prometheus, that sets the scrape timeout.
This will be handy anyway, even though you should really split this file into library modules.
You can also use -> impl IntoResponse so you don't have to do the conversion in your route's function body:
async fn handle_root() -> impl IntoResponse {
Html("mcexport - <a href=\"/probe\">Probe</a>")
}
Or even specify the exact type name, -> Html<&'static str>
Rather than #[allow(clippy::cast_sign_loss)] in protocol.rs when creating the outbound handshake packet, I would change the field to usize and deal with errors during parsing (probably with unwrap() if the value is never allowed to be negative). Parse, don’t validate
Your CI could benefit from some additional checks:
cargo doc --no-deps to catch any documentation errors.cargo machete --with-metadata to check for unused dependencies.- Add a build matrix with
stable, beta and some MSRV pinned version to catch regressions caused by changes to the compiler. This is best combined with a monthly cron schedule.
- Consider
cargo deny for license violations and security vulnerabilities (similar to dependabot).
Nothing else really stands out as unusual or missing. I'm kind of scraping the bottom of the barrel here to come up with suggestions.
It doesn't look like there are any obvious issues in any of these categories. Optimizations are really hard to pinpoint without profiling. But I would look for suspicious allocations and copying, first. For instance, AsyncReadPacket::read_packet() -> StatusResponsePacket allocates a Vec<u8>, then converts it to a String in a kind of roundabout way, by allocating a second Vec<u8> in read_string().
You can read straight from the socket into a String for StatusResponsePacket. It requires some sort of "specialization" since the return type is generic and the two InboundPacket implementers have different inner types. Another option might be serde which can deserialize from Vec<u8> straight to your final type. You could also hand-write a TryFrom<&[u8]> impl for your types to avoid the intermediate allocations and copying without overspecializing.