From 80c5b0df714a4ca60dc62b43452fd91eb513e786 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 24 Oct 2024 10:44:23 +1100 Subject: [PATCH] refactor(connlib): replace `LogUnwrap` with macros (#7138) Using a trait means the call-site of the log message will always be the `log_unwrap` module, despite the `#[track_caller]` annotation. That one only works for `std::panic::Location` unfortunately which `tracing` isn't using. Macros will be evaluated earlier and thus the messages will show up with the correct module name. --- rust/connlib/tunnel/src/client.rs | 38 ++++++++++++++--------- rust/logging/src/lib.rs | 4 +-- rust/logging/src/log_unwrap.rs | 48 ----------------------------- rust/logging/src/unwrap_or.rs | 50 +++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 64 deletions(-) delete mode 100644 rust/logging/src/log_unwrap.rs create mode 100644 rust/logging/src/unwrap_or.rs diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index ab564160b..63bd5ec11 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -14,7 +14,7 @@ use bimap::BiMap; use connlib_model::PublicKey; use connlib_model::{GatewayId, RelayId, ResourceId, ResourceStatus, ResourceView}; use connlib_model::{Site, SiteId}; -use firezone_logging::{anyhow_dyn_err, std_dyn_err, LogUnwrap as _}; +use firezone_logging::{anyhow_dyn_err, std_dyn_err, unwrap_or_debug, unwrap_or_warn}; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; use ip_network_table::IpNetworkTable; use ip_packet::{IpPacket, UdpSlice}; @@ -384,8 +384,10 @@ impl ClientState { dns::servfail(response.query.for_slice_ref()) }); - self.try_queue_udp_dns_response(server, source, &message) - .log_unwrap_warn("Failed to queue UDP DNS response"); + unwrap_or_warn!( + self.try_queue_udp_dns_response(server, source, &message), + "Failed to queue UDP DNS response" + ); } (dns::Transport::Tcp { source }, result) => { let message = result @@ -398,9 +400,10 @@ impl ClientState { dns::servfail(response.query.for_slice_ref()) }); - self.tcp_dns_server - .send_message(source, message) - .log_unwrap_warn("Failed to send TCP DNS response"); + unwrap_or_warn!( + self.tcp_dns_server.send_message(source, message), + "Failed to send TCP DNS response" + ); } } } @@ -966,8 +969,10 @@ impl ClientState { match self.stub_resolver.handle(message) { dns::ResolveStrategy::LocalResponse(response) => { - self.try_queue_udp_dns_response(upstream, source, &response) - .log_unwrap_debug("Failed to queue UDP DNS response"); + unwrap_or_debug!( + self.try_queue_udp_dns_response(upstream, source, &response), + "Failed to queue UDP DNS response" + ); } dns::ResolveStrategy::Recurse => { let query_id = message.header().id(); @@ -1010,9 +1015,10 @@ impl ClientState { match self.stub_resolver.handle(message.for_slice_ref()) { dns::ResolveStrategy::LocalResponse(response) => { - self.tcp_dns_server - .send_message(query.socket, response) - .log_unwrap_debug("Failed to send TCP DNS response"); + unwrap_or_debug!( + self.tcp_dns_server.send_message(query.socket, response), + "Failed to send TCP DNS response" + ); } dns::ResolveStrategy::Recurse => { let query_id = message.header().id(); @@ -1026,9 +1032,13 @@ impl ClientState { "Failed to send recursive TCP DNS quer" ); - self.tcp_dns_server - .send_message(query.socket, dns::servfail(message.for_slice_ref())) - .log_unwrap_debug("Failed to send TCP DNS response"); + unwrap_or_debug!( + self.tcp_dns_server.send_message( + query.socket, + dns::servfail(message.for_slice_ref()) + ), + "Failed to send TCP DNS response" + ); return; } }; diff --git a/rust/logging/src/lib.rs b/rust/logging/src/lib.rs index 7c650e433..07e21a1e8 100644 --- a/rust/logging/src/lib.rs +++ b/rust/logging/src/lib.rs @@ -1,7 +1,8 @@ mod dyn_err; pub mod file; mod format; -mod log_unwrap; +#[macro_use] +mod unwrap_or; use sentry_tracing::EventFilter; use tracing::{subscriber::DefaultGuard, Subscriber}; @@ -13,7 +14,6 @@ use tracing_subscriber::{ pub use dyn_err::{anyhow_dyn_err, std_dyn_err}; pub use format::Format; -pub use log_unwrap::LogUnwrap; /// Registers a global subscriber with stdout logging and `additional_layer` pub fn setup_global_subscriber(additional_layer: L) diff --git a/rust/logging/src/log_unwrap.rs b/rust/logging/src/log_unwrap.rs deleted file mode 100644 index f0396e1fb..000000000 --- a/rust/logging/src/log_unwrap.rs +++ /dev/null @@ -1,48 +0,0 @@ -use std::error::Error; - -pub trait LogUnwrap { - #[track_caller] - fn log_unwrap_warn(&self, msg: &str); - #[track_caller] - fn log_unwrap_debug(&self, msg: &str); - #[track_caller] - fn log_unwrap_trace(&self, msg: &str); -} - -impl LogUnwrap for anyhow::Result<()> { - #[track_caller] - fn log_unwrap_warn(&self, msg: &str) { - match self { - Ok(()) => {} - Err(e) => { - let error: &dyn Error = e.as_ref(); - - tracing::warn!(error, "{msg}") - } - } - } - - #[track_caller] - fn log_unwrap_debug(&self, msg: &str) { - match self { - Ok(()) => {} - Err(e) => { - let error: &dyn Error = e.as_ref(); - - tracing::debug!(error, "{msg}") - } - } - } - - #[track_caller] - fn log_unwrap_trace(&self, msg: &str) { - match self { - Ok(()) => {} - Err(e) => { - let error: &dyn Error = e.as_ref(); - - tracing::trace!(error, "{msg}") - } - } - } -} diff --git a/rust/logging/src/unwrap_or.rs b/rust/logging/src/unwrap_or.rs new file mode 100644 index 000000000..522a3220e --- /dev/null +++ b/rust/logging/src/unwrap_or.rs @@ -0,0 +1,50 @@ +#[macro_export] +macro_rules! unwrap_or_warn { + ( + $result:expr, + $($arg:tt)* + ) => { + match $result { + Ok(()) => {} + Err(e) => { + let error: &dyn ::std::error::Error = e.as_ref(); + + ::tracing::warn!(error, $($arg)*) + } + } + }; +} + +#[macro_export] +macro_rules! unwrap_or_debug { + ( + $result:expr, + $($arg:tt)* + ) => { + match $result { + Ok(()) => {} + Err(e) => { + let error: &dyn ::std::error::Error = e.as_ref(); + + ::tracing::debug!(error, $($arg)*) + } + } + }; +} + +#[macro_export] +macro_rules! unwrap_or_trace { + ( + $result:expr, + $($arg:tt)* + ) => { + match $result { + Ok(()) => {} + Err(e) => { + let error: &dyn ::std::error::Error = e.as_ref(); + + ::tracing::debug!(error, $($arg)*) + } + } + }; +}