Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sdk/rust/src/apis/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ impl From<rpc::ExecutorState> for ExecutorState {
}

pub fn init_logger() -> Result<(), FlameError> {
let filter = tracing_subscriber::EnvFilter::try_from_default_env()?
let filter = tracing_subscriber::EnvFilter::from_default_env()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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
  1. 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.

.add_directive("h2=error".parse()?)
.add_directive("hyper_util=error".parse()?)
.add_directive("tower=error".parse()?);
Expand Down
Loading