Rust: use tracing/tracing-subscriber for logging #18608
Conversation
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again, by re-requesting a review.
| @@ -85,7 +87,7 @@ impl<'a> Extractor<'a> { | |||
| } | |||
| translator.emit_source_file(ast); | |||
| translator.trap.commit().unwrap_or_else(|err| { | |||
| log::error!( | |||
| tracing::error!( | |||
There was a problem hiding this comment.
very minor nit: for consistency with info and warn we should probably pull in error alongside them and just have
| tracing::error!( | |
| error!( |
here
| @@ -180,11 +182,19 @@ fn cwd() -> anyhow::Result<AbsPathBuf> { | |||
|
|
|||
| fn main() -> anyhow::Result<()> { | |||
| let start = Instant::now(); | |||
| let flame_layer = if let Ok(path) = std::env::var("CODEQL_EXTRACTOR_RUST_OPTION_FLAME_LOG") { | |||
There was a problem hiding this comment.
I would prefer this to go through the same mechanism we have for other configuration knobs, rather than a direct access to the env here:
- adding the corresponding option to
codeql-extractor.ymlwith docs and all - adding
flame_logto theConfigstructure - configuring tracing after extracting said configuration
This would make it work with the whole extractor option machinery, and from the code perspective I like the invariant that all configuration goes through the Config structure, and we don't have to go hunt for other env accesses across the code.
| @@ -51,7 +51,7 @@ impl<'a> RustAnalyzer<'a> { | |||
| project: &ProjectManifest, | |||
| config: &CargoConfig, | |||
| ) -> Option<(RootDatabase, Vfs)> { | |||
| let progress = |t| (log::trace!("progress: {}", t)); | |||
| let progress = |t| (tracing::trace!("progress: {}", t)); | |||
There was a problem hiding this comment.
very minor nit: again we can drop the tracing namespace here provided we also pull trace in the use above
| @@ -62,7 +62,7 @@ impl<'a> RustAnalyzer<'a> { | |||
| match load_workspace_at(manifest.as_ref(), config, &load_config, &progress) { | |||
| Ok((db, vfs, _macro_server)) => Some((db, vfs)), | |||
| Err(err) => { | |||
| log::error!("failed to load workspace for {}: {}", manifest, err); | |||
| tracing::error!("failed to load workspace for {}: {}", manifest, err); | |||
There was a problem hiding this comment.
here we can directly drop tracing:: as far as I can see (we already pull in error above)
| match severity { | ||
| DiagnosticSeverity::Debug => tracing::debug!( | ||
| "{}:{}:{}: {}", | ||
| self.path, | ||
| start.line + 1, | ||
| start.col + 1, | ||
| &full_message | ||
| ), | ||
| DiagnosticSeverity::Info => tracing::info!( | ||
| "{}:{}:{}: {}", | ||
| self.path, | ||
| start.line + 1, | ||
| start.col + 1, | ||
| &full_message | ||
| ), | ||
| DiagnosticSeverity::Warning => tracing::warn!( | ||
| "{}:{}:{}: {}", | ||
| self.path, | ||
| start.line + 1, | ||
| start.col + 1, | ||
| &full_message | ||
| ), | ||
| DiagnosticSeverity::Error => tracing::error!( | ||
| "{}:{}:{}: {}", | ||
| self.path, | ||
| start.line + 1, | ||
| start.col + 1, | ||
| &full_message | ||
| ), |
There was a problem hiding this comment.
you can avoid all this duplication by using tracing::Level and replacing the old log:log! with tracing:event!
There was a problem hiding this comment.
I tried, but that does not seem to be possible. See also: tokio-rs/tracing#2730
| /// Create a `Subscriber` configured with the tracing level based on the environment variables | ||
| /// `RUST_LOG` and `CODEQL_VERBOSITY` (prioritized in that order), falling back to `warn` if neither is set. | ||
| pub fn default_subscriber_with_level( | ||
| language: &str, | ||
| ) -> Filtered< | ||
| tracing_subscriber::fmt::Layer< | ||
| tracing_subscriber::Registry, | ||
| DefaultFields, | ||
| Format<tracing_subscriber::fmt::format::Full, ()>, | ||
| >, | ||
| EnvFilter, | ||
| tracing_subscriber::Registry, | ||
| > { |
There was a problem hiding this comment.
hmm, this bypasses our own current numerical env we use for this CODEQL_RUST_EXTRACTOR_VERBOSE or CODEQL_RUST_EXTRACTOR_OPTION_VERBOSE, which I would prefer to keep (AFAIK CODEQL_VERBOSITY is not a generic codeql cli setting, so actually I think the env extractor namespacing should be used, although admittedly verbosity is still not enrolled in our codeql-extractor.yml file).
There was a problem hiding this comment.
Yes, this is the same behaviour as the "tree-sitter" based extractors use.
There was a problem hiding this comment.
I'm ok with moving from the integer verbose level input to the verbosity string input, but I would still prefer for it to go through the rest of the Config/extractor option machinery for Rust. Could you maybe:
- have a
codeql_verbosity: Option<&str>or similar type parameter here, instead of taking it directly from the env - in the
set_tracing_levelfunction, passstd::env("CODEQL_VERBOSITY")so we preserve the same behaviour for other extractor - in rust, add a
verbositystring extractor option - in rust, replace
verbose: u8withverbosity: Option<String>in theConfigstructure, and pass that todefault_subscriber_with_level - that would cover
CODEQL_EXTRACTOR_RUST_VERBOSITYorCODEQL_EXTRACTOR_RUST_OPTION_VERBOSITY(including passing it via-overbosity). If we also wantCODEQL_VERBOSITY, that can be added to the config extraction (maybe it is already?)
This PR changes the logging system in the Rust codebase to use the
tracingandtracing-subscribercrates and configures them the same way as the Ruby extractor. In addition it adds the possibility to collect flame graph data by ssettingCODEQL_EXTRACTOR_RUST_OPTION_FLAME_LOGto a file path. The collected data can be visualised as a flame graph or chart using theinfernotool .