[sdk/rust] Fix init_logger failing when RUST_LOG is not set#343
Conversation
Use from_default_env() instead of try_from_default_env() to provide a default filter when RUST_LOG environment variable is missing, rather than returning an error.
Summary of ChangesHello @k82cn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of CLI tools by preventing crashes related to missing environment variables during logging initialization. By adjusting the logging configuration to gracefully handle the absence of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a crash in init_logger that occurred when the RUST_LOG environment variable was not set. The change from try_from_default_env to from_default_env is appropriate. However, I've raised a more significant architectural concern regarding the init_logger function. As a public function within an SDK, it should not initialize a global logger, as this can lead to conflicts and panics in applications that use this SDK and have their own logging configuration. I have recommended refactoring this function to provide logging components (like a tracing::Layer) that the consuming application can integrate into its own logging setup, making the SDK more robust and idiomatic for a library.
|
|
||
| pub fn init_logger() -> Result<(), FlameError> { | ||
| let filter = tracing_subscriber::EnvFilter::try_from_default_env()? | ||
| let filter = tracing_subscriber::EnvFilter::from_default_env() |
There was a problem hiding this comment.
While this change correctly fixes the crash when RUST_LOG is not set, the init_logger function itself has a design issue that's common for libraries. It initializes a global logger by calling .init() (on line 231), which can conflict with an application's own logging setup and cause a panic. A library should typically not initialize a global logger.
Instead of initializing the logger directly, consider refactoring init_logger to return a tracing::Layer and a filter. The application using the SDK can then compose these with its own logging configuration. This makes the SDK more flexible and prevents runtime panics in applications that also configure logging.
For example:
use tracing_subscriber::{EnvFilter, Layer, layer::SubscriberExt};
use tracing::Subscriber;
// This function would return a layer that can be composed by the application.
pub fn flame_layer<S>() -> impl Layer<S>
where
S: Subscriber + for<'a> tracing_subscriber::registry::LookupSpan<'a>,
{
let time_format = LocalTime::new(format_description!(
"[hour repr:24]:[minute]:[second]::[subsecond digits:3]"
));
tracing_subscriber::fmt::layer()
.with_timer(time_format)
.with_target(true)
.with_ansi(false)
}
// This function would return the filter.
pub fn flame_filter() -> Result<EnvFilter, FlameError> {
Ok(tracing_subscriber::EnvFilter::from_default_env()
.add_directive("h2=error".parse()?)
.add_directive("hyper_util=error".parse()?)
.add_directive("tower=error".parse()?))
}The application using the SDK would then be responsible for initializing the logger:
// In application code (e.g., main.rs)
use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt};
fn main() -> Result<(), flame::apis::FlameError> {
let filter = flame::apis::flame_filter()?;
let layer = flame::apis::flame_layer();
tracing_subscriber::registry()
.with(filter)
.with(layer)
.init();
// ... rest of the app
Ok(())
}This change would make the SDK more robust and easier to integrate into other applications.
References
- In a library, avoid modifying the global logging configuration for dependencies. This can cause conflicts with the application's own logging setup. Instead, document how users can configure logging or provide an opt-in utility (like a context manager) for temporary log level adjustments.
1 participant
Summary
flmpingand other CLI tools failing withInvalidConfig("environment variable not found")whenRUST_LOGis not setfrom_default_env()instead oftry_from_default_env()to provide a default filter whenRUST_LOGenvironment variable is missingRoot Cause
EnvFilter::try_from_default_env()returnsFromEnvErrorwhenRUST_LOGis not set, which was being converted toInvalidConfigerror via theFrom<FromEnvError>impl, causing confusing error messages.Testing