Revisit error handling in trace (open-telemetry#371)

Author: TommyCpp

.github/workflows/ci.yml

@@ -52,7 +52,7 @@ jobs:
         override: true
     - name: Run tests
       run: cargo --version &&
-        cargo test --verbose --manifest-path=opentelemetry/Cargo.toml --features trace,metrics,serialize,tokio,async-std,serde,http,tonic,reqwest &&
+        cargo test --verbose --manifest-path=opentelemetry/Cargo.toml --features trace,metrics,serialize,tokio,serde,http,tonic,reqwest &&
         cargo test --manifest-path=opentelemetry-jaeger/Cargo.toml &&
         cargo test --manifest-path=opentelemetry-zipkin/Cargo.toml
   meta:

CONTRIBUTING.md

@@ -93,6 +93,13 @@ patterns in the spec.
 For a deeper discussion, see:
 https://github.com/open-telemetry/opentelemetry-specification/issues/165
 
+### Error Handling
+Currently, the Opentelemetry Rust SDK has two ways to handle errors. In the situation where errors are not allowed to return. One should call global error handler to process the errors. Otherwise, one should return the errors. 
+
+The Opentelemetry Rust SDK comes with an error type `openetelemetry::Error`. For different function, one error has been defined. All error returned by trace module MUST be wrapped in `opentelemetry::trace::TraceError`. All errors returned by metrics module MUST be wrapped in `opentelemetry::metrics::MetricsError`. 
+
+For users that want to implement their own exporters. It's RECOMMENDED to wrap all errors from the exporter into a crate-level error type, and implement `ExporterError` trait.  
+
 ## Style Guide
 
 * Run `cargo clippy --all` - this will catch common mistakes and improve

examples/actix-http/src/main.rs

@@ -1,17 +1,14 @@
 use actix_service::Service;
 use actix_web::middleware::Logger;
 use actix_web::{web, App, HttpServer};
+use opentelemetry::trace::TraceError;
 use opentelemetry::{global, sdk::trace as sdktrace};
 use opentelemetry::{
     trace::{FutureExt, TraceContextExt, Tracer},
     Key,
 };
-use std::error::Error;
 
-fn init_tracer() -> Result<
-    (sdktrace::Tracer, opentelemetry_jaeger::Uninstall),
-    Box<dyn Error + Send + Sync + 'static>,
-> {
+fn init_tracer() -> Result<(sdktrace::Tracer, opentelemetry_jaeger::Uninstall), TraceError> {
     opentelemetry_jaeger::new_pipeline()
         .with_collector_endpoint("http://127.0.0.1:14268/api/traces")
         .with_service_name("trace-http-demo")

examples/actix-udp/src/main.rs

@@ -1,17 +1,14 @@
 use actix_service::Service;
 use actix_web::middleware::Logger;
 use actix_web::{web, App, HttpServer};
+use opentelemetry::trace::TraceError;
 use opentelemetry::{global, sdk::trace as sdktrace};
 use opentelemetry::{
     trace::{FutureExt, TraceContextExt, Tracer},
     Key,
 };
-use std::error::Error;
 
-fn init_tracer() -> Result<
-    (sdktrace::Tracer, opentelemetry_jaeger::Uninstall),
-    Box<dyn Error + Send + Sync + 'static>,
-> {
+fn init_tracer() -> Result<(sdktrace::Tracer, opentelemetry_jaeger::Uninstall), TraceError> {
     opentelemetry_jaeger::new_pipeline()
         .with_agent_endpoint("localhost:6831")
         .with_service_name("trace-udp-demo")

examples/async/src/main.rs

@@ -17,6 +17,7 @@
 //!     cargo run --example async_fn
 //!
 //! [`hello_world`]: https://github.com/tokio-rs/tokio/blob/132e9f1da5965530b63554d7a1c59824c3de4e30/tokio/examples/hello_world.rs
+use opentelemetry::trace::TraceError;
 use opentelemetry::{
     global,
     sdk::trace as sdktrace,
@@ -52,10 +53,7 @@ async fn run(addr: &SocketAddr) -> io::Result<usize> {
     write(&mut stream).with_context(cx).await
 }
 
-fn init_tracer() -> Result<
-    (sdktrace::Tracer, opentelemetry_jaeger::Uninstall),
-    Box<dyn Error + Send + Sync + 'static>,
-> {
+fn init_tracer() -> Result<(sdktrace::Tracer, opentelemetry_jaeger::Uninstall), TraceError> {
     opentelemetry_jaeger::new_pipeline()
         .with_service_name("trace-demo")
         .install()

examples/basic-otlp/src/main.rs

@@ -1,6 +1,7 @@
 use futures::stream::{Stream, StreamExt};
 use opentelemetry::exporter;
 use opentelemetry::sdk::metrics::PushController;
+use opentelemetry::trace::TraceError;
 use opentelemetry::{
     baggage::BaggageExt,
     metrics::{self, MetricsError, ObserverResult},
@@ -11,9 +12,7 @@ use opentelemetry::{global, sdk::trace as sdktrace};
 use std::error::Error;
 use std::time::Duration;
 
-fn init_tracer(
-) -> Result<(sdktrace::Tracer, opentelemetry_otlp::Uninstall), Box<dyn Error + Send + Sync + 'static>>
-{
+fn init_tracer() -> Result<(sdktrace::Tracer, opentelemetry_otlp::Uninstall), TraceError> {
     opentelemetry_otlp::new_pipeline().install()
 }
 

examples/basic/src/main.rs

@@ -2,6 +2,7 @@ use futures::stream::{Stream, StreamExt};
 use opentelemetry::exporter;
 use opentelemetry::global;
 use opentelemetry::sdk::{metrics::PushController, trace as sdktrace};
+use opentelemetry::trace::TraceError;
 use opentelemetry::{
     baggage::BaggageExt,
     metrics::{self, MetricsError, ObserverResult},
@@ -11,10 +12,7 @@ use opentelemetry::{
 use std::error::Error;
 use std::time::Duration;
 
-fn init_tracer() -> Result<
-    (sdktrace::Tracer, opentelemetry_jaeger::Uninstall),
-    Box<dyn Error + Send + Sync + 'static>,
-> {
+fn init_tracer() -> Result<(sdktrace::Tracer, opentelemetry_jaeger::Uninstall), TraceError> {
     opentelemetry_jaeger::new_pipeline()
         .with_service_name("trace-demo")
         .with_tags(vec![

examples/grpc/src/client.rs

@@ -2,19 +2,17 @@ use hello_world::greeter_client::GreeterClient;
 use hello_world::HelloRequest;
 use opentelemetry::global;
 use opentelemetry::sdk::propagation::TraceContextPropagator;
+use opentelemetry::trace::TraceError;
 use opentelemetry::{
     trace::{TraceContextExt, Tracer},
     Context, KeyValue,
 };
-use std::error::Error;
 
 pub mod hello_world {
     tonic::include_proto!("helloworld");
 }
 
-fn tracing_init(
-) -> Result<(impl Tracer, opentelemetry_jaeger::Uninstall), Box<dyn Error + Send + Sync + 'static>>
-{
+fn tracing_init() -> Result<(impl Tracer, opentelemetry_jaeger::Uninstall), TraceError> {
     global::set_text_map_propagator(TraceContextPropagator::new());
     opentelemetry_jaeger::new_pipeline()
         .with_service_name("grpc-client")

examples/grpc/src/server.rs

@@ -4,6 +4,7 @@ use hello_world::greeter_server::{Greeter, GreeterServer};
 use hello_world::{HelloReply, HelloRequest};
 use opentelemetry::global;
 use opentelemetry::sdk::propagation::TraceContextPropagator;
+use opentelemetry::trace::TraceError;
 use opentelemetry::{
     trace::{Span, Tracer},
     KeyValue,
@@ -36,9 +37,7 @@ impl Greeter for MyGreeter {
     }
 }
 
-fn tracing_init(
-) -> Result<(impl Tracer, opentelemetry_jaeger::Uninstall), Box<dyn Error + Send + Sync + 'static>>
-{
+fn tracing_init() -> Result<(impl Tracer, opentelemetry_jaeger::Uninstall), TraceError> {
     global::set_text_map_propagator(TraceContextPropagator::new());
     opentelemetry_jaeger::new_pipeline()
         .with_service_name("grpc-server")

opentelemetry-contrib/Cargo.toml

@@ -22,7 +22,7 @@ rustdoc-args = ["--cfg", "docsrs"]
 default = []
 base64_format = ["base64", "binary_propagator"]
 binary_propagator = []
-datadog = ["indexmap", "rmp", "async-trait"]
+datadog = ["indexmap", "rmp", "async-trait", "thiserror"]
 reqwest-blocking-client = ["reqwest/blocking", "opentelemetry/reqwest"]
 reqwest-client = ["reqwest", "opentelemetry/reqwest"]
 surf-client = ["surf", "opentelemetry/surf"]
@@ -37,6 +37,7 @@ reqwest = { version = "0.10", optional = true }
 surf = { version = "2.0", optional = true }
 http = "0.2"
 base64 = { version = "0.13", optional = true }
+thiserror = { version = "1.0", optional = true }
 
 [dev-dependencies]
 base64 = "0.13"

opentelemetry-contrib/src/trace/exporter/datadog/mod.rs

@@ -76,7 +76,7 @@
 //! use opentelemetry::exporter::trace::HttpClient;
 //! use opentelemetry_contrib::trace::exporter::datadog::{new_pipeline, ApiVersion};
 //! use async_trait::async_trait;
-//! use std::error::Error;
+//! use opentelemetry_contrib::trace::exporter::datadog::Error;
 //!
 //! // `reqwest` and `surf` are supported through features, if you prefer an
 //! // alternate http client you can add support by implementing `HttpClient` as
@@ -87,17 +87,17 @@
 //! #[async_trait]
 //! impl HttpClient for IsahcClient {
 //!   async fn send(&self, request: http::Request<Vec<u8>>) -> ExportResult {
-//!     let result = self.0.send_async(request).await?;
+//!     let result = self.0.send_async(request).await.map_err(|err| Error::Other(err.to_string()))?;
 //!
 //!     if result.status().is_success() {
 //!       Ok(())
 //!     } else {
-//!       Err(result.status().as_str().into())
+//!       Err(Error::Other(result.status().to_string()).into())
 //!     }
 //!   }
 //! }
 //!
-//! fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static>> {
+//! fn main() -> Result<(), opentelemetry::trace::TraceError> {
 //!     let (tracer, _uninstall) = new_pipeline()
 //!         .with_service_name("my_app")
 //!         .with_version(ApiVersion::Version05)
@@ -123,14 +123,14 @@ mod intern;
 mod model;
 
 pub use model::ApiVersion;
+pub use model::Error;
 
 use async_trait::async_trait;
 use http::{Method, Request, Uri};
 use opentelemetry::exporter::trace;
 use opentelemetry::exporter::trace::{HttpClient, SpanData};
+use opentelemetry::trace::TraceError;
 use opentelemetry::{global, sdk, trace::TracerProvider};
-use std::error::Error;
-use std::io;
 
 /// Default Datadog collector endpoint
 const DEFAULT_AGENT_ENDPOINT: &str = "http://127.0.0.1:8126";
@@ -211,14 +211,12 @@ impl Default for DatadogPipelineBuilder {
 
 impl DatadogPipelineBuilder {
     /// Create `ExporterConfig` struct from current `ExporterConfigBuilder`
-    pub fn install(
-        mut self,
-    ) -> Result<(sdk::trace::Tracer, Uninstall), Box<dyn Error + Send + Sync + 'static>> {
+    pub fn install(mut self) -> Result<(sdk::trace::Tracer, Uninstall), TraceError> {
         if let Some(client) = self.client {
             let endpoint = self.agent_endpoint + self.version.path();
             let exporter = DatadogExporter::new(
                 self.service_name.clone(),
-                endpoint.parse()?,
+                endpoint.parse().map_err::<Error, _>(Into::into)?,
                 self.version,
                 client,
             );
@@ -233,11 +231,7 @@ impl DatadogPipelineBuilder {
             let provider_guard = global::set_tracer_provider(provider);
             Ok((tracer, Uninstall(provider_guard)))
         } else {
-            Err(Box::new(io::Error::new(
-                io::ErrorKind::Other,
-                "http client must be set, users can enable reqwest or surf feature to use http\
-                 client implementation within create",
-            )))
+            Err(Error::NoHttpClient.into())
         }
     }
 
@@ -284,7 +278,8 @@ impl trace::SpanExporter for DatadogExporter {
             .method(Method::POST)
             .uri(self.request_url.clone())
             .header(http::header::CONTENT_TYPE, self.version.content_type())
-            .body(data)?;
+            .body(data)
+            .map_err::<Error, _>(Into::into)?;
         self.client.send(req).await
     }
 }

opentelemetry-contrib/src/trace/exporter/datadog/model/mod.rs

@@ -1,21 +1,31 @@
-use opentelemetry::exporter::trace;
-use std::fmt;
+use opentelemetry::exporter::{trace, ExportError};
 
 mod v03;
 mod v05;
 
-#[derive(Debug, Clone, Copy)]
-pub(crate) enum Error {
+/// Wrap type for errors from opentelemetry datadog exporter
+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    /// Message pack error
+    #[error("message pack error")]
     MessagePackError,
+    /// No http client founded. User should provide one or enable features
+    #[error("http client must be set, users can enable reqwest or surf feature to use http client implementation within create")]
+    NoHttpClient,
+    /// Http requests failed with following errors
+    #[error(transparent)]
+    RequestError(#[from] http::Error),
+    /// The Uri was invalid
+    #[error(transparent)]
+    InvalidUri(#[from] http::uri::InvalidUri),
+    /// Other errors
+    #[error("{0}")]
+    Other(String),
 }
 
-impl std::error::Error for Error {}
-
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            Error::MessagePackError => write!(f, "message pack error"),
-        }
+impl ExportError for Error {
+    fn exporter_name(&self) -> &'static str {
+        "datadog"
     }
 }
 

opentelemetry-jaeger/Cargo.toml

@@ -33,6 +33,7 @@ thrift = "0.13"
 tokio = { version = "0.2", features = ["udp", "sync"], optional = true }
 wasm-bindgen = { version = "0.2", optional = true }
 wasm-bindgen-futures = { version = "0.4.18", optional = true }
+thiserror = "1.0"
 
 [dependencies.web-sys]
 version = "0.3.4"