Add Graylog logging configuration #252 by Alexj9837 · Pull Request #264 · DiamondLightSource/numtracker

Alexj9837 · 2026-03-11T13:12:53Z

#252

Summary

Add tracing-gelf layer to ship structured logs to a Graylog instance via GELF TCP
Expose --graylog <URL> and --logging-level <LEVEL> CLI flags (also via NUMTRACKER_GRAYLOG / NUMTRACKER_GRAYLOG_LOG_LEVEL env vars)
Add Graylog env var injection to Helm chart with numtracker.graylog.enabled/host/level values

src/cli/mod.rs

.vscode/launch.json

ZohebShaikh

Looks good only few minor things

src/logging.rs

ZohebShaikh · 2026-03-17T13:21:28Z

src/logging.rs

+                if errors.0.is_empty() {
+                    error!("Failed to connect to graylog - address lookup failed");
+                } else {
+                    warn!(
+                        "Connection to graylog {:?} closed: {:?}",
+                        handle.address(),
+                        errors
+                    );


errors.0 give Vec<SocketAddr,Error> docs

If there are errors you get a warning which I find difficult to understand
This is just a question to understand the code.

I don't know will it be very difficult to write unit test to check all the error cases

The gelf library is odd. There is a bit more explanation in the amygdala version here.

I'm not sure if the warn should also be an error but the no errors == error case caught me out as well so it might be worth including a similar comment here.

- 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

codecov · 2026-03-30T13:59:52Z

Codecov Report

❌ Patch coverage is 78.57143% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.53%. Comparing base (4d5553c) to head (9092648).

Files with missing lines	Patch %	Lines
src/logging.rs	68.25%	20 Missing ⚠️
src/main.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
+ Coverage   76.35%   76.53%   +0.18%     
==========================================
  Files          13       13              
  Lines        1937     2033      +96     
==========================================
+ Hits         1479     1556      +77     
- Misses        458      477      +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tpoliaw

Looks pretty good, thanks for looking at it. A couple of changes could make the error handling a bit cleaner and keep it more consistent with what is already there though.

tpoliaw · 2026-03-30T14:37:17Z

src/cli/mod.rs

+    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")]
+    pub logging_level: Level,


Could this be graylog_level to mirror the tracing_level in TracingOptions

Yeah that makes sense, I was in an isolated view of being inside "GraylogOptions" so logging level, but graylog_level gives more context

tpoliaw · 2026-03-30T14:38:54Z

src/cli/mod.rs

+    pub logging_level: Level,
+}
+
+impl GraylogOptions {


Could this impl be moved down to below the impl TracingOptions block to keep it more consistent?

moved this down.

tpoliaw · 2026-03-30T16:22:02Z

src/cli/mod.rs

+    /// Returns the `host:port` address string for connecting to Graylog, or `None` if no URL is
+    /// configured. Returns an error if the URL has no port — a missing port is always a
+    /// misconfiguration and should not be silently defaulted.
+    pub fn address(&self) -> Result<Option<String>, String> {


This is mainly validation stuff so might be better served as a clap value_parser to generate better error messages and let the rest of the code assume things were valid.

On the other hand we're also ignoring the scheme and always using tcp in the logging init method.
Assuming we're sticking with tcp and the gelf library expects a string, I don't think we need to work with a URL here at all - accepting host and port might be clearer. It could be surprising to pass udp://graylog.example.com:1234 and for it to still use tcp without warning.

tpoliaw · 2026-03-30T16:24:13Z

src/main.rs

 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);


We should probably handle errors here. Currently if any log setup fails you get no output and debugging why is a pain.

tpoliaw · 2026-03-30T16:25:07Z

Cargo.toml

 openidconnect = { version = "4.0.0", optional = true }
 toml = { version = "1.0.0", optional = true }
+tracing-gelf = "0.9.0"
+thiserror = "2.0.18"


Bringing in thiserror for a single error type might be an overkill. Implementing the error manually is not too much boilerplate.

removing thiserror.

tpoliaw · 2026-03-30T16:28:47Z

.gitignore

 trackers/
+
+# Local IDE settings
+.vscode/settings.json


Shouldn't be part of this PR - it's a good change but can go in at the same time as the launch.json, maybe as .vscode/ and then manually add the specific files.

removing it.

tpoliaw · 2026-03-30T16:45:42Z

src/logging.rs

+                if errors.0.is_empty() {
+                    error!("Failed to connect to graylog - address lookup failed");
+                } else {
+                    warn!(
+                        "Connection to graylog {:?} closed: {:?}",
+                        handle.address(),
+                        errors
+                    );


The gelf library is odd. There is a bit more explanation in the amygdala version here.

I'm not sure if the warn should also be an error but the no errors == error case caught me out as well so it might be worth including a similar comment here.

tpoliaw · 2026-03-30T16:51:29Z

src/logging.rs

+    #[error("Exporter build error: {0}")]
+    Exporter(#[from] ExporterBuildError),
+    #[error("Graylog error: {0}")]
+    Graylog(#[from] std::io::Error),


Is the Graylog variant ever used?

Alexj9837 marked this pull request as ready for review March 16, 2026 15:59

ZohebShaikh reviewed Mar 17, 2026

View reviewed changes

src/cli/mod.rs Outdated Show resolved Hide resolved

ZohebShaikh reviewed Mar 17, 2026

View reviewed changes

.vscode/launch.json Outdated Show resolved Hide resolved

ZohebShaikh reviewed Mar 17, 2026

View reviewed changes

src/logging.rs Outdated Show resolved Hide resolved

src/logging.rs Outdated Show resolved Hide resolved

src/logging.rs Outdated Show resolved Hide resolved

ZohebShaikh reviewed Mar 17, 2026

View reviewed changes

Alexj9837 force-pushed the numtracker_graylog branch from bda4336 to 831a990 Compare March 18, 2026 15:44

Alexj9837 added 8 commits March 30, 2026 14:47

Add Graylog GELF logging layer

d4cd088

Add Graylog configuration to Helm chart

3a5641f

Document Graylog logging configuration

f0bb43a

Add VSCode debug launch config

ff2a5fe

changed to align with tracing

700688b

removing launch.json

27aa07c

Regenerate Cargo.lock after rebase onto main

0a86a82

Alexj9837 force-pushed the numtracker_graylog branch from 831a990 to 0a86a82 Compare March 30, 2026 13:56

Alexj9837 added 2 commits March 30, 2026 15:02

Fix clippy: use is_err() instead of matches!(result, Err(_))

9c231c4

Fix nightly rustfmt formatting

9092648

tpoliaw requested changes Mar 30, 2026

View reviewed changes

Conversation

Alexj9837 commented Mar 11, 2026

Summary

Uh oh!

Uh oh!

Uh oh!

ZohebShaikh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZohebShaikh Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tpoliaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ZohebShaikh Mar 17, 2026 •

edited

Loading

codecov bot commented Mar 30, 2026 •

edited

Loading