[Bug]: Global metrics provider gets shutdown when local provider is dropped about opentelemetry-rust HOT 4 CLOSED

That looks like an acceptable solution.

• The function opentelemetry_otlp::new_pipeline() call chain also sets the global static provider. Should it maintain this behavior, or should it be adjusted to simply create and return the provider/meter without modifying the static provider? This will get rid of cloning.

I think it would be preferable to remove the implicit call to global::set_meter_provider() and make it mandatory and explicit to the caller. I got very confused the first time I used the OTLP exporter.

• Should the convenience interface opentelemetry_otlp::new_pipeline() be removed altogether, and let the application use the standard way of creating the MeterProvider and Meters?

The only other reference I have is from the opentelemetry-stdout exporter and if we could manage to align the required setup for both, then that would be helpful.

from opentelemetry-rust.

Apart from this, separate from this issue, we also need to discuss -

The function opentelemetry_otlp::new_pipeline() call chain also sets the global static provider. Should it maintain this behavior, or should it be adjusted to simply create and return the provider/meter without modifying the static provider? This will get rid of cloning.

Should the convenience interface opentelemetry_otlp::new_pipeline() be removed altogether, and let the application use the standard way of creating the MeterProvider and Meters?

Yes to both! Lets continue discussing in #1592

from opentelemetry-rust.

Here's the output of the collector when running the demo basic-otlp as is: collector-output-broken.log. Notably see that there are no metrics being collected.

Here's a workaround patch that keeps the provider created in the build() function in scope and passes it to the global metrics provider:

diff --git i/opentelemetry-otlp/examples/basic-otlp/src/main.rs w/opentelemetry-otlp/examples/basic-otlp/src/main.rs
index 4f4320ca..ff64cfe3 100644
--- i/opentelemetry-otlp/examples/basic-otlp/src/main.rs
+++ w/opentelemetry-otlp/examples/basic-otlp/src/main.rs
@@ -49,7 +49,10 @@ fn init_metrics() -> Result<(), MetricsError> {
         )]))
         .build();
     match provider {
-        Ok(_provider) => Ok(()),
+        Ok(provider) => {
+            global::set_meter_provider(provider);
+            Ok(())
+        }
         Err(err) => Err(err),
     }
 }
diff --git i/opentelemetry-otlp/src/metric.rs w/opentelemetry-otlp/src/metric.rs
index 75e968b0..5c12f5d3 100644
--- i/opentelemetry-otlp/src/metric.rs
+++ w/opentelemetry-otlp/src/metric.rs
@@ -241,8 +241,6 @@ where
 
         let provider = provider.build();
 
-        global::set_meter_provider(provider.clone());
-
         Ok(provider)
     }
 }

With this patch applied, the collector log looks as expected and metrics are shown: collector-output-with-patch.log

This is all against current master (d5392dc).

I think the intended behaviour is to call shutdown() only when the last reference to SdkMeterProvider is dropped. To my knowledge this is not possible to do in Rust in a thread-safe manner. Instead Drop should not be implemented for SdkMeterProvider, but instead for Pipeline (or the relevant object that requires proper shutdown semantics, as fas as I can see only the reader is involved so far) so that multiple references to the same SdkMeterProvider can be hold and dropped without side-effects.

from opentelemetry-rust.

@bwalk-at-ibm Your analysis is correct. We shouldn't have Drop implemented for SdkMeterProvider.
One of the option could be to replace the existing code:

#[derive(Clone, Debug)]
pub struct SdkMeterProvider {
    pipes: Arc<Pipelines>,
    meters: Arc<Mutex<HashMap<Scope, Arc<SdkMeter>>>>,
    is_shutdown: Arc<AtomicBool>,
}
impl Drop for SdkMeterProvider {
    fn drop(&mut self) {
        if let Err(err) = self.shutdown() {
            global::handle_error(err);
        }
    }
}

with

#[derive(Clone, Debug)]
pub struct SdkMeterProvider {
    inner: Arc<SdkMeterProviderInner>,
}

pub struct SdkMeterProviderInner {
    pipes: Pipelines,
    meters: Mutex<HashMap<Scope, Arc<SdkMeter>>>,
    is_shutdown: AtomicBool,
}

impl Drop for SdkMeterProviderInner {
    fn drop(&mut self) {
        if let Err(err) = self.shutdown() {
            global::handle_error(err);
        }
    }
}

This will ensure that the shutdown is only invoked when all the references to SdkMeterProvider are dropped.

Apart from this, separate from this issue, we also need to discuss -

• The function opentelemetry_otlp::new_pipeline() call chain also sets the global static provider. Should it maintain this behavior, or should it be adjusted to simply create and return the provider/meter without modifying the static provider? This will get rid of cloning.

• Should the convenience interface opentelemetry_otlp::new_pipeline() be removed altogether, and let the application use the standard way of creating the MeterProvider and Meters?

from opentelemetry-rust.