From aeefe65227877a1322224dba2d0e54da4a6e1759 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 8 Jan 2024 13:04:23 -0500 Subject: [PATCH] Fix `tracing-duration-export` compilation (#835) ## Summary I'm unable to run `puffin-cli` on `main` as the `tracing-durations-export` is marked as optional, but the crate actually depends on it to compile. Further, without `tracing-durations-export`, there are `Option` types that can't resolve to a concrete type. This PR fixes compilation with and without the feature. --- Cargo.toml | 1 + crates/puffin-cli/Cargo.toml | 2 +- crates/puffin-cli/src/logging.rs | 86 ++++++++++++++++---------------- crates/puffin-cli/src/main.rs | 17 +++++-- 4 files changed, 56 insertions(+), 50 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4e13fd341..50dc9a324 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -81,6 +81,7 @@ tokio-util = { version = "0.7.10", features = ["compat"] } toml = { version = "0.8.8" } toml_edit = { version = "0.21.0" } tracing = { version = "0.1.40" } +tracing-durations-export = { version = "0.1.0", features = ["plot"] } tracing-indicatif = { version = "0.3.6" } tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } tracing-tree = { version = "0.3.0" } diff --git a/crates/puffin-cli/Cargo.toml b/crates/puffin-cli/Cargo.toml index 9563416bd..b54da9660 100644 --- a/crates/puffin-cli/Cargo.toml +++ b/crates/puffin-cli/Cargo.toml @@ -61,12 +61,12 @@ thiserror = { workspace = true } tokio = { workspace = true } toml = { workspace = true } tracing = { workspace = true } +tracing-durations-export = { workspace = true, features = ["plot"], optional = true } tracing-subscriber = { workspace = true } tracing-tree = { workspace = true } url = { workspace = true } waitmap = { workspace = true } which = { workspace = true } -tracing-durations-export = { version = "0.1.0", features = ["plot"], optional = true } [target.'cfg(target_os = "windows")'.dependencies] mimalloc = "0.1.39" diff --git a/crates/puffin-cli/src/logging.rs b/crates/puffin-cli/src/logging.rs index 914ab187e..cab4680c6 100644 --- a/crates/puffin-cli/src/logging.rs +++ b/crates/puffin-cli/src/logging.rs @@ -1,13 +1,11 @@ -use std::env; -use std::path::PathBuf; -use std::time::Duration; - use tracing::level_filters::LevelFilter; -use tracing_durations_export::plot::PlotConfig; -use tracing_durations_export::{DurationsLayerBuilder, DurationsLayerDropGuard}; +#[cfg(feature = "tracing-durations-export")] +use tracing_durations_export::{ + plot::PlotConfig, DurationsLayer, DurationsLayerBuilder, DurationsLayerDropGuard, +}; use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; -use tracing_subscriber::EnvFilter; +use tracing_subscriber::{EnvFilter, Layer, Registry}; use tracing_tree::time::Uptime; use tracing_tree::HierarchicalLayer; @@ -26,39 +24,7 @@ pub(crate) enum Level { /// The [`Level`] is used to dictate the default filters (which can be overridden by the `RUST_LOG` /// environment variable) along with the formatting of the output. For example, [`Level::Verbose`] /// includes targets and timestamps, along with all `puffin=debug` messages by default. -pub(crate) fn setup_logging(level: Level) -> Option { - let (duration_layer, guard) = { - #[cfg(feature = "tracing-durations-export")] - if let Ok(location) = env::var("TRACING_DURATIONS_FILE") { - let location = PathBuf::from(location); - if let Some(parent) = location.parent() { - fs_err::create_dir_all(parent) - .expect("Failed to create parent of TRACING_DURATIONS_FILE"); - } - let plot_config = PlotConfig { - multi_lane: true, - min_length: Some(Duration::from_secs_f32(0.002)), - remove: Some( - ["get_cached_with_callback".to_string()] - .into_iter() - .collect(), - ), - ..PlotConfig::default() - }; - let (layer, guard) = DurationsLayerBuilder::default() - .durations_file(&location) - .plot_file(location.with_extension("svg")) - .plot_config(plot_config) - .build() - .expect("Couldn't create TRACING_DURATIONS_FILE files"); - (Some(layer), Some(guard)) - } else { - (None, None) - } - #[cfg(not(feature = "tracing-durations-export"))] - (None, None) - }; - +pub(crate) fn setup_logging(level: Level, duration: impl Layer + Send + Sync) { match level { Level::Default => { // Show nothing, but allow `RUST_LOG` to override. @@ -68,7 +34,7 @@ pub(crate) fn setup_logging(level: Level) -> Option { // Regardless of the tracing level, show messages without any adornment. tracing_subscriber::registry() - .with(duration_layer) + .with(duration) .with(filter) .with( tracing_subscriber::fmt::layer() @@ -86,7 +52,7 @@ pub(crate) fn setup_logging(level: Level) -> Option { // Regardless of the tracing level, include the uptime and target for each message. tracing_subscriber::registry() - .with(duration_layer) + .with(duration) .with(filter) .with( HierarchicalLayer::default() @@ -97,6 +63,38 @@ pub(crate) fn setup_logging(level: Level) -> Option { .init(); } } - - guard +} + +/// Setup the `TRACING_DURATIONS_FILE` environment variable to enable tracing durations. +#[cfg(feature = "tracing-durations-export")] +pub(crate) fn setup_duration() -> ( + Option>, + Option, +) { + if let Ok(location) = std::env::var("TRACING_DURATIONS_FILE") { + let location = std::path::PathBuf::from(location); + if let Some(parent) = location.parent() { + fs_err::create_dir_all(parent) + .expect("Failed to create parent of TRACING_DURATIONS_FILE"); + } + let plot_config = PlotConfig { + multi_lane: true, + min_length: Some(std::time::Duration::from_secs_f32(0.002)), + remove: Some( + ["get_cached_with_callback".to_string()] + .into_iter() + .collect(), + ), + ..PlotConfig::default() + }; + let (layer, guard) = DurationsLayerBuilder::default() + .durations_file(&location) + .plot_file(location.with_extension("svg")) + .plot_config(plot_config) + .build() + .expect("Couldn't create TRACING_DURATIONS_FILE files"); + (Some(layer), Some(guard)) + } else { + (None, None) + } } diff --git a/crates/puffin-cli/src/main.rs b/crates/puffin-cli/src/main.rs index fc104f462..ff9562ec6 100644 --- a/crates/puffin-cli/src/main.rs +++ b/crates/puffin-cli/src/main.rs @@ -415,11 +415,18 @@ async fn inner() -> Result { let cli = Cli::parse(); // Configure the `tracing` crate, which controls internal logging. - let _guard = logging::setup_logging(if cli.verbose { - logging::Level::Verbose - } else { - logging::Level::Default - }); + #[cfg(feature = "tracing-durations-export")] + let (duration_layer, _duration_guard) = logging::setup_duration(); + #[cfg(not(feature = "tracing-durations-export"))] + let duration_layer = None::; + logging::setup_logging( + if cli.verbose { + logging::Level::Verbose + } else { + logging::Level::Default + }, + duration_layer, + ); // Configure the `Printer`, which controls user-facing output in the CLI. let printer = if cli.quiet {