feat(daemon): support dynamically setting the logging level at runtime #105

sminez · 2024-09-13T10:37:03Z

This lets us do the following:

$ prompting-client.logging-level --filter=debug

and have the daemon update its logging filter without needing to restart the service. By default we will now log at info level.

… from the command line

…napd

olivercalder

Look pretty good, thanks for working on this!

I'm rather confused about what valid log levels are, and where log levels vs filters should be used. In some places (e.g. set_filter and log_filter), it seems like these terms are used interchangeably, which confuses me a bit more :)

What I'd really like would be to have an enum of valid log levels, ideally passed as a set of options displayed via clap. That, with a bit more clarity about level vs filter, and it looks good to me.

It seems that EnvFilter and reload, among others, are defined in external crates, so some comments or documentation about what they require would be nice too, I think. Perhaps where the filter is constructed based on level, since it seems arbitrary if one doesn't know that that's just the format which is expected by the other crate.

olivercalder · 2024-09-13T14:11:57Z

prompting-client/src/bin/set_log_level.rs

+/// Set the logging level for a running instance of the prompting client daemon
+#[derive(Debug, Parser)]
+#[clap(about, long_about = None)]
+struct Args {
+    /// The filter to use for setting the logging level
+    #[clap(short, long, value_name = "FILTER")]
+    level: String,
+}


Are there a list of supported options for the log level? Or how does the filtering work?

There's not a single list, no. The API docs here cover what is possible which really is a lot, which is why I've not restricted this to a fixed list of levels like trace, debug, info etc as an enum. That would prevent us using more granular targeting of logs in situations where we really want it.

olivercalder · 2024-09-13T14:17:55Z

prompting-client/src/daemon/server.rs

+    async fn set_logging_level(
+        &self,
+        level: Request<String>,
+    ) -> Result<Response<SetLoggingLevelResponse>, Status> {
+        let current = log_filter(&level.into_inner());
+
+        match self.reload_handle.set_filter(&current) {
+            Ok(_) => Ok(Response::new(SetLoggingLevelResponse { current })),
+            Err(e) => Err(Status::new(
+                Code::InvalidArgument,
+                format!("unable to set logging level: {e}"),
+            )),
+        }
+    }


Here level is passed into log_filter as filter, but I don't think of levels and filters as really the same thing. What can level be set to, and is it validated anywhere?

olivercalder · 2024-09-13T14:19:11Z

prompting-client/src/lib.rs

+pub const SOCKET_ENV_VAR: &str = "PROMPTING_CLIENT_SOCKET";
+pub const DEFAULT_LOG_LEVEL: &str = "info";
+
+pub fn log_filter(filter: &str) -> String {
+    format!("{filter},hyper=error,h2=error")
+}


See here.

I would prefer if there were an enum of support log levels, and if the clap arg were defined so the default is explicit.

olivercalder · 2024-09-13T14:20:55Z

prompting-client/src/protos/apparmor_prompting.rs

@@ -417,6 +423,36 @@ pub mod app_armor_prompting_client {
                );
            self.inner.unary(req, path, codec).await
        }
+        pub async fn set_logging_level(
+            &mut self,
+            request: impl tonic::IntoRequest<::prost::alloc::string::String>,


So the logging level is contained in the request?

olivercalder · 2024-09-13T14:23:42Z

prompting-client/src/daemon/server.rs

    let listener = UnixListener::bind(&socket_path).expect("to be able to bind to our socket");

    (AppArmorPromptingServer::new(service), listener)
 }

-pub struct Service<R>
+pub trait SetLogLevel: Send + Sync + 'static {
+    fn set_filter(&self, level: &str) -> crate::Result<()>;


Here as well, we define set_filter to take a level, but

olivercalder · 2024-09-13T14:24:00Z

prompting-client/src/daemon/server.rs

+    L: From<EnvFilter> + Send + Sync + 'static,
+    S: 'static,
+{
+    fn set_filter(&self, filter: &str) -> crate::Result<()> {


then set_filter is implemented to take a filter.

olivercalder · 2024-09-13T14:29:52Z

prompting-client/src/daemon/server.rs

+        let f = filter
+            .parse::<EnvFilter>()
+            .map_err(|_| Error::UnableToUpdateLogLevel {
+                reason: format!("{filter:?} is not a valid logging filter"),
+            })?;


It looks like EnvFilter is defined in another crate (not in this repo) so I definitely think there should be some documentation (both in the code and in the CLI args) about what valid log levels are.

olivercalder · 2024-09-13T14:33:18Z

prompting-client/src/daemon/server.rs

+                reason: format!("{filter:?} is not a valid logging filter"),
+            })?;
+
+        self.reload(f).map_err(|e| Error::UnableToUpdateLogLevel {


Reload also isn't defined in this repo either? Seems like some tonic magic or something

it's a feature of tracing_subscriber, yeah. All of this is just a way of plumbing a valid logging filter through to this call to reload

matthew-hagemann

🎉

sminez · 2024-09-13T14:51:25Z

@olivercalder I see what you mean about being inconsistent around levels vs filters. I'll update things to consistently use the term filter everywhere but documenting the full functionality of what is supported here (or restricting things to a subset) is probably not what we want to go for. I'll update the clap docs so that they cover the common use cases of just setting a logging level for the daemon code itself. The issue is that a very large number of possible filters are possible and I don't really want to link to Rust API documentation in the CLI.

olivercalder

Looks great, thanks for the changes! Some filenames still use "log level" terminology, and could be updated to use "filter" instead, but not a huge deal.

olivercalder · 2024-09-13T16:13:21Z

prompting-client/src/bin/set_log_level.rs

I think the filename could be updated to match as well

olivercalder · 2024-09-13T16:13:25Z

prompting-client/src/cli_actions/log_level.rs

olivercalder · 2024-09-13T16:13:54Z

prompting-client/src/bin/set_log_level.rs

+//! This is a helper command for setting our logging filter within the daemon while it is running.
+//! Doing this involves having to send a GRPC message to the daemon which inolves more plumbing
+//! than we'd ideally like but it allows us to update the filter without having to restart the
+//! daemon.
+//!
+//! The docs on the command itself cover how to set simple logging levels but the tracing framework
+//! we are using actually supports a rich syntax for how these filters are written. As and when we
+//! need to use this in anger, the docs can be found here:
+//!   https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html


Thanks, this is great!

olivercalder · 2024-09-13T16:14:24Z

prompting-client/src/bin/set_log_level.rs

+/// Set the logging level for a running instance of the prompting client daemon.
+///
+/// Simple usage of this command involves specifying a level based filter for what logs are written
+/// to the system journal by the prompting-client daemon. The supported levels are (in order of
+/// least to most verbose):
+///
+///   - error
+///   - warn
+///   - info
+///   - debug
+///   - trace
+///
+/// The default level is "info".


sminez added 2 commits September 13, 2024 09:16

feat(daemon): support dynamically setting the logging level at runtime

1d94f2c

feat(daemon): support dynamically setting the logging level at rutime…

b99706b

… from the command line

sminez requested review from olivercalder, matthew-hagemann and d-loose September 13, 2024 10:37

fix(integration tests): updating to use the new error messages from s…

f0f1ee6

…napd

olivercalder requested changes Sep 13, 2024

View reviewed changes

matthew-hagemann previously approved these changes Sep 13, 2024

View reviewed changes

chore: addressing review feedback

2fd260f

sminez dismissed matthew-hagemann’s stale review via 2fd260f September 13, 2024 15:54

sminez requested review from olivercalder and matthew-hagemann September 13, 2024 15:55

olivercalder approved these changes Sep 13, 2024

View reviewed changes

sminez merged commit abed3ac into main Sep 13, 2024
9 checks passed

sminez deleted the set-logging-level branch September 13, 2024 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(daemon): support dynamically setting the logging level at runtime #105

feat(daemon): support dynamically setting the logging level at runtime #105

sminez commented Sep 13, 2024 •

edited

Loading

olivercalder left a comment

olivercalder Sep 13, 2024

sminez Sep 13, 2024

olivercalder Sep 13, 2024

olivercalder Sep 13, 2024

olivercalder Sep 13, 2024

olivercalder Sep 13, 2024

olivercalder Sep 13, 2024

olivercalder Sep 13, 2024

olivercalder Sep 13, 2024

sminez Sep 13, 2024

matthew-hagemann left a comment

sminez commented Sep 13, 2024

olivercalder left a comment

olivercalder Sep 13, 2024

olivercalder Sep 13, 2024

olivercalder Sep 13, 2024

olivercalder Sep 13, 2024

feat(daemon): support dynamically setting the logging level at runtime #105

feat(daemon): support dynamically setting the logging level at runtime #105

Conversation

sminez commented Sep 13, 2024 • edited Loading

olivercalder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-hagemann left a comment

Choose a reason for hiding this comment

sminez commented Sep 13, 2024

olivercalder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sminez commented Sep 13, 2024 •

edited

Loading