Address PR review feedback on Graylog logging

- Extract GraylogOptions into its own struct, independent of TracingOptions
- Remove .vscode/launch.json (to be added in a separate PR)
- Remove version/build additional_field calls from Graylog logger
- Switch let-else to if-let in init_graylog
- Warn when Graylog URL has no port rather than silently defaulting
- Add reconnect loop so dropped connections recover automatically
- Improve connection error messages to distinguish DNS failure from TCP errors

Author: Alexj9837

src/cli/mod.rs

@@ -29,8 +29,10 @@ pub mod client;
 pub struct Cli {
     #[clap(flatten, next_help_heading = "Logging/Debug")]
     verbose: Verbosity,
-    #[clap(flatten, next_help_heading = "Tracing and Logging")]
+    #[clap(flatten, next_help_heading = "Tracing")]
     tracing: TracingOptions,
+    #[clap(flatten, next_help_heading = "Graylog")]
+    pub graylog: GraylogOptions,
     #[clap(subcommand)]
     pub(crate) command: Command,
 }
@@ -43,12 +45,16 @@ pub struct TracingOptions {
     /// The minimum level of tracing events to send
     #[clap(long, default_value_t = Level::INFO, env = "NUMTRACKER_TRACING_LEVEL")]
     tracing_level: Level,
+}
+
+#[derive(Debug, Parser)]
+pub struct GraylogOptions {
     /// The URL of the Graylog instance
     #[clap(long = "graylog", env = "NUMTRACKER_GRAYLOG")]
-    graylog_url: Option<Url>,
+    pub graylog_url: Option<Url>,
     /// The minimum level of logging events to send
     #[clap(long, default_value_t = Level::INFO, env = "NUMTRACKER_GRAYLOG_LOG_LEVEL")]
-    logging_level: Level,
+    pub logging_level: Level,
 }
 
 #[derive(Debug, Subcommand)]
@@ -163,15 +169,9 @@ impl TracingOptions {
     pub(crate) fn tracing_url(&self) -> Option<Url> {
         self.tracing_url.clone()
     }
-        pub(crate) fn level(&self) -> Level {
+    pub(crate) fn level(&self) -> Level {
         self.tracing_level
     }
-    pub(crate) fn graylog_url(&self) -> Option<Url> {
-        self.graylog_url.clone()
-    }
-    pub(crate) fn logging_level(&self) -> Level {
-        self.logging_level
-    }
 }
 
 #[cfg(test)]
@@ -351,10 +351,10 @@ mod tests {
         let cli = Cli::try_parse_from([APP, "--graylog", "tcp://graylog.example.com:12201", "serve"])
             .unwrap();
         assert_eq!(
-            cli.tracing().graylog_url(),
+            cli.graylog.graylog_url,
             Some("tcp://graylog.example.com:12201".parse().unwrap())
         );
-        assert_eq!(cli.tracing().logging_level(), Level::INFO);
+        assert_eq!(cli.graylog.logging_level, Level::INFO);
 
         let cli = Cli::try_parse_from([
             APP,
@@ -366,10 +366,10 @@ mod tests {
         ])
         .unwrap();
         assert_eq!(
-            cli.tracing().graylog_url(),
+            cli.graylog.graylog_url,
             Some("tcp://graylog.example.com:12201".parse().unwrap())
         );
-        assert_eq!(cli.tracing().logging_level(), Level::WARN);
+        assert_eq!(cli.graylog.logging_level, Level::WARN);
     }
 
     #[test]

src/logging.rs

@@ -29,8 +29,7 @@ use tracing_subscriber::util::SubscriberInitExt as _;
 use tracing_subscriber::{EnvFilter, Layer};
 use url::Url;
 
-use crate::build_info::GIT_COMMIT_HASH;
-use crate::cli::TracingOptions;
+use crate::cli::{GraylogOptions, TracingOptions};
 
 #[derive(Debug, thiserror::Error)]
 pub enum LoggingError {
@@ -87,57 +86,62 @@ where
     }
 }
 
-fn init_graylog<S>(endpoint: Option<Url>,level: Level,) -> Result<Option<impl Layer<S>>, LoggingError>
+fn init_graylog<S>(opts: &GraylogOptions) -> Result<Option<impl Layer<S>>, LoggingError>
 where
     S: Subscriber + for<'s> LookupSpan<'s>,
 {
-    let Some(endpoint) = endpoint else {
-        return Ok(None);
-    };
-
-    let address = format!(
-        "{}:{}",
-        endpoint.host_str().unwrap_or("localhost"),
-        endpoint.port().unwrap_or(12201),
-    );
-
-    match Logger::builder()
-        .additional_field("version", env!("CARGO_PKG_VERSION"))
-        .additional_field("build", GIT_COMMIT_HASH.unwrap_or("unknown"))
-        .connect_tcp(address)
-    {
-        Ok((logger, mut handle)) => {
-            tokio::spawn(async move {
-                let errors = handle.connect().await;
-                if errors.0.is_empty() {
-                    error!("Failed to connect to graylog - address lookup failed");
-                } else {
-                    warn!(
-                        "Connection to graylog {:?} closed: {:?}",
-                        handle.address(),
-                        errors
-                    );
-                }
-            });
-            Ok(Some(
-                logger
-                    .with_filter(LevelFilter::from_level(level))
-                    .with_filter(FilterFn::new(|m| {
-                        m.target().starts_with(env!("CARGO_PKG_NAME"))
-                    })),
-            ))
-        }
-        Err(e) => {
-            eprintln!("Couldn't create graylog logger: {e}");
-            Ok(None)
+    if let Some(endpoint) = &opts.graylog_url {
+        let level = opts.logging_level;
+        let port = endpoint.port().unwrap_or_else(|| {
+            warn!("No port specified for Graylog endpoint, using default 12201");
+            12201
+        });
+        let address = format!(
+            "{}:{}",
+            endpoint.host_str().unwrap_or("localhost"),
+            port,
+        );
+
+        match Logger::builder().connect_tcp(address) {
+            Ok((logger, mut handle)) => {
+                tokio::spawn(async move {
+                    loop {
+                        let errors = handle.connect().await;
+                        if errors.0.is_empty() {
+                            error!(
+                                "Graylog DNS lookup failed for {:?} - no addresses resolved",
+                                handle.address()
+                            );
+                            break;
+                        } else {
+                            for (addr, err) in &errors.0 {
+                                warn!("Graylog connection to {addr} failed: {err} - reconnecting");
+                            }
+                        }
+                    }
+                });
+                Ok(Some(
+                    logger
+                        .with_filter(LevelFilter::from_level(level))
+                        .with_filter(FilterFn::new(|m| {
+                            m.target().starts_with(env!("CARGO_PKG_NAME"))
+                        })),
+                ))
+            }
+            Err(e) => {
+                eprintln!("Couldn't create graylog logger: {e}");
+                Ok(None)
+            }
         }
+    } else {
+        Ok(None)
     }
 }
 
-pub fn init(logging: Option<Level>, tracing: &TracingOptions) -> Result<(), LoggingError> {
+pub fn init(logging: Option<Level>, tracing: &TracingOptions, graylog: &GraylogOptions) -> Result<(), LoggingError> {
     let log_layer = init_stdout(logging);
     let trace_layer = init_tracing(tracing.tracing_url(), tracing.level())?;
-    let graylog_layer = init_graylog(tracing.graylog_url(), tracing.logging_level())?;
+    let graylog_layer = init_graylog(graylog)?;
     // Whatever level is set for logging/tracing, ignore the noise from the low-level libraries
     let filter = EnvFilter::new("trace") // let everything through
         .add_directive("h2=info".parse().expect("Static string is valid")) // except http,
@@ -161,23 +165,24 @@ mod tests {
     use tracing_subscriber::Registry;
 
     use super::init_graylog;
+    use crate::cli::GraylogOptions;
 
     #[test]
     fn no_graylog_endpoint_returns_none() {
-        // No URL configured means no layer should be added
-        let result = init_graylog::<Registry>(None, Level::INFO);
+        let opts = GraylogOptions { graylog_url: None, logging_level: Level::INFO };
+        let result = init_graylog::<Registry>(&opts);
         assert!(matches!(result, Ok(None)));
     }
 
     #[tokio::test]
     async fn graylog_with_endpoint_returns_layer() {
-        // Bind a random local port to accept the TCP connection
         let listener = TcpListener::bind("127.0.0.1:0").expect("bind local port");
         let port = listener.local_addr().expect("local addr").port();
-        let url = format!("tcp://127.0.0.1:{port}")
-            .parse()
-            .expect("valid url");
-        let result = init_graylog::<Registry>(Some(url), Level::INFO);
+        let opts = GraylogOptions {
+            graylog_url: Some(format!("tcp://127.0.0.1:{port}").parse().expect("valid url")),
+            logging_level: Level::INFO,
+        };
+        let result = init_graylog::<Registry>(&opts);
         assert!(matches!(result, Ok(Some(_))));
     }
 }

src/main.rs

@@ -30,7 +30,7 @@ mod template;
 #[tokio::main]
 async fn main() -> Result<(), Box<dyn Error>> {
     let args = Cli::init();
-    let _ = logging::init(args.log_level(), args.tracing());
+    let _ = logging::init(args.log_level(), args.tracing(), &args.graylog);
     match args.command {
         Command::Serve(opts) => graphql::serve_graphql(opts).await,
         #[cfg(not(feature = "client"))]