-
Notifications
You must be signed in to change notification settings - Fork 966
Description
This was part of the original plan after #3803, but we probably have put it aside for a while:
In #4182 (comment), I've noticed that the current Process
abstraction should probably be extended to allow the existence of a tracing_subscriber
global state, so that we can handle graceful shutdown more easily.
Having noticed the clear pattern here:
Lines 44 to 53 in 52e6070
#[cfg(feature = "otel")] | |
opentelemetry::global::set_text_map_propagator( | |
opentelemetry_sdk::propagation::TraceContextPropagator::new(), | |
); | |
let (subscriber, console_filter) = rustup::cli::log::tracing_subscriber(&process); | |
tracing::subscriber::set_global_default(subscriber)?; | |
let result = run_rustup(&process, console_filter).await; | |
// We're tracing, so block until all spans are exported. | |
#[cfg(feature = "otel")] | |
opentelemetry::global::shutdown_tracer_provider(); |
... it should probably be modeled as an RAII guard.
I have also noticed that this should resolve another problem, i.e. the corresponding construct in tests is simply no longer used, as these two functions no longer have a caller:
Lines 235 to 250 in 52e6070
pub async fn before_test_async() { | |
#[cfg(feature = "otel")] | |
{ | |
opentelemetry::global::set_text_map_propagator( | |
opentelemetry_sdk::propagation::TraceContextPropagator::new(), | |
); | |
} | |
} | |
pub async fn after_test_async() { | |
#[cfg(feature = "otel")] | |
{ | |
// We're tracing, so block until all spans are exported. | |
opentelemetry::global::shutdown_tracer_provider(); | |
} | |
} |
This is most probably caused by the removal of rustup_macros
(d60f005): indeed, the tests are passing on the CI, but sometimes one might want to run tests with otel
on as well.
@djc What do you think?