Skip to content

Commit

Permalink
fix: address comments
Browse files Browse the repository at this point in the history
* Add ExportError in MetricsError.
It can help unify the export error among trace, metric and logging in the future.

* Merge TraceError::Other into TraceError::Boxed.
Those two seems to have a lot in common. Created a custom wrap type for string and metric those two variants.

* Remove default implement for exporter_name.
It should have used in case like http client failure. But it could also encourage ppl not implement it at all.
  • Loading branch information
TommyCpp committed Nov 29, 2020
1 parent 7c5f135 commit 9ffbace
Show file tree
Hide file tree
Showing 21 changed files with 91 additions and 85 deletions.
7 changes: 2 additions & 5 deletions examples/actix-http/src/main.rs
Original file line number Diff line number Diff line change
@@ -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 opentelemetry::trace::TraceError;

fn init_tracer() -> Result<
(sdktrace::Tracer, opentelemetry_jaeger::Uninstall),
TraceError
> {
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")
Expand Down
7 changes: 2 additions & 5 deletions examples/actix-udp/src/main.rs
Original file line number Diff line number Diff line change
@@ -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 opentelemetry::trace::TraceError;

fn init_tracer() -> Result<
(sdktrace::Tracer, opentelemetry_jaeger::Uninstall),
TraceError
> {
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")
Expand Down
7 changes: 2 additions & 5 deletions examples/async/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -26,7 +27,6 @@ use opentelemetry::{
use std::{error::Error, io, net::SocketAddr};
use tokio::io::AsyncWriteExt;
use tokio::net::TcpStream;
use opentelemetry::trace::TraceError;

async fn connect(addr: &SocketAddr) -> io::Result<TcpStream> {
let tracer = global::tracer("connector");
Expand All @@ -53,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),
TraceError
> {
fn init_tracer() -> Result<(sdktrace::Tracer, opentelemetry_jaeger::Uninstall), TraceError> {
opentelemetry_jaeger::new_pipeline()
.with_service_name("trace-demo")
.install()
Expand Down
9 changes: 3 additions & 6 deletions examples/basic-otlp/src/main.rs
Original file line number Diff line number Diff line change
@@ -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},
Expand All @@ -10,13 +11,9 @@ use opentelemetry::{
use opentelemetry::{global, sdk::trace as sdktrace};
use std::error::Error;
use std::time::Duration;
use opentelemetry::trace::TraceError;

fn init_tracer(
) -> Result<(sdktrace::Tracer, opentelemetry_otlp::Uninstall), TraceError>
{
opentelemetry_otlp::new_pipeline()
.install()
fn init_tracer() -> Result<(sdktrace::Tracer, opentelemetry_otlp::Uninstall), TraceError> {
opentelemetry_otlp::new_pipeline().install()
}

// Skip first immediate tick from tokio, not needed for async_std.
Expand Down
7 changes: 2 additions & 5 deletions examples/basic/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -10,12 +11,8 @@ use opentelemetry::{
};
use std::error::Error;
use std::time::Duration;
use opentelemetry::trace::TraceError;

fn init_tracer() -> Result<
(sdktrace::Tracer, opentelemetry_jaeger::Uninstall),
TraceError,
> {
fn init_tracer() -> Result<(sdktrace::Tracer, opentelemetry_jaeger::Uninstall), TraceError> {
opentelemetry_jaeger::new_pipeline()
.with_service_name("trace-demo")
.with_tags(vec![
Expand Down
6 changes: 2 additions & 4 deletions examples/grpc/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 opentelemetry::trace::TraceError;

pub mod hello_world {
tonic::include_proto!("helloworld");
}

fn tracing_init(
) -> Result<(impl Tracer, opentelemetry_jaeger::Uninstall),TraceError>
{
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")
Expand Down
6 changes: 2 additions & 4 deletions examples/grpc/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ 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,
};
use std::error::Error;
use opentelemetry::trace::TraceError;

pub mod hello_world {
tonic::include_proto!("helloworld"); // The string specified here must match the proto package name.
Expand Down Expand Up @@ -37,9 +37,7 @@ impl Greeter for MyGreeter {
}
}

fn tracing_init(
) -> Result<(impl Tracer, opentelemetry_jaeger::Uninstall), TraceError>
{
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")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use opentelemetry::exporter::trace;
use opentelemetry::exporter::trace::ExportError;
use opentelemetry::exporter::{trace, ExportError};

mod v03;
mod v05;
Expand Down
9 changes: 3 additions & 6 deletions opentelemetry-jaeger/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,8 @@ use agent::AgentAsyncClientUDP;
use async_trait::async_trait;
#[cfg(any(feature = "collector_client", feature = "wasm_collector_client"))]
use collector::CollectorAsyncClientHttp;
use opentelemetry::exporter::trace::ExportError;
use opentelemetry::exporter::ExportError;
use opentelemetry::trace::TraceError;
use opentelemetry::{
exporter::trace,
global, sdk,
Expand All @@ -206,7 +207,6 @@ use std::{
time::{Duration, SystemTime},
};
use uploader::BatchUploader;
use opentelemetry::trace::TraceError;

/// Default service name if no service is configured.
const DEFAULT_SERVICE_NAME: &str = "OpenTelemetry";
Expand Down Expand Up @@ -417,10 +417,7 @@ impl PipelineBuilder {
}

/// Install a Jaeger pipeline with the recommended defaults.
pub fn install(
self,
) -> Result<(sdk::trace::Tracer, Uninstall), TraceError>
{
pub fn install(self) -> Result<(sdk::trace::Tracer, Uninstall), TraceError> {
let tracer_provider = self.build()?;
let tracer =
tracer_provider.get_tracer("opentelemetry-jaeger", Some(env!("CARGO_PKG_VERSION")));
Expand Down
2 changes: 1 addition & 1 deletion opentelemetry-otlp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ mod span;
mod transform;

pub use crate::span::{Compression, Credentials, Exporter, ExporterConfig, Protocol};
use opentelemetry::exporter::trace::ExportError;
use opentelemetry::exporter::ExportError;
use opentelemetry::trace::TraceError;

/// Create a new pipeline builder with the recommended configuration.
Expand Down
11 changes: 8 additions & 3 deletions opentelemetry-zipkin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,14 @@ mod uploader;
use async_trait::async_trait;
use http::Uri;
use model::endpoint::Endpoint;
use opentelemetry::exporter::trace::{ExportError, HttpClient};
use opentelemetry::trace::TraceError;
use opentelemetry::{exporter::trace, global, sdk, trace::TracerProvider};
use opentelemetry::{
exporter::{
trace::{self, HttpClient},
ExportError,
},
global, sdk,
trace::{TraceError, TracerProvider},
};
use std::net::SocketAddr;

/// Default Zipkin collector endpoint
Expand Down
12 changes: 11 additions & 1 deletion opentelemetry/src/api/metrics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ mod sync_instrument;
mod up_down_counter;
mod value_recorder;

use crate::exporter::ExportError;
pub use async_instrument::{AsyncRunner, BatchObserverCallback, Observation, ObserverResult};
pub use config::InstrumentConfig;
pub use counter::{BoundCounter, Counter, CounterBuilder};
Expand All @@ -38,7 +39,7 @@ pub use value_recorder::{BoundValueRecorder, ValueRecorder, ValueRecorderBuilder
pub type Result<T> = result::Result<T, MetricsError>;

/// Errors returned by the metrics API.
#[derive(Error, Debug, PartialEq)]
#[derive(Error, Debug)]
#[non_exhaustive]
pub enum MetricsError {
/// Other errors not covered by specific cases.
Expand Down Expand Up @@ -68,6 +69,15 @@ pub enum MetricsError {
/// Errors when aggregator cannot subtract
#[error("Aggregator does not subtract")]
NoSubtraction,
/// Fail to export metrics
#[error("Export metrics failed with {0}")]
ExportErr(Box<dyn ExportError>),
}

impl<T: ExportError> From<T> for MetricsError {
fn from(err: T) -> Self {
MetricsError::ExportErr(Box::new(err))
}
}

impl<T> From<PoisonError<T>> for MetricsError {
Expand Down
29 changes: 21 additions & 8 deletions opentelemetry/src/api/trace/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ pub use self::{
},
tracer::{SpanBuilder, Tracer},
};
use crate::exporter::trace::ExportError;
use crate::exporter::ExportError;
use std::time;

/// Describe the result of operations in tracing API.
Expand All @@ -149,10 +149,6 @@ pub type TraceResult<T> = Result<T, TraceError>;
#[derive(Error, Debug)]
#[non_exhaustive]
pub enum TraceError {
/// Other errors not covered by specific cases.
#[error("Trace error: {0}")]
Other(String),

/// Export failed with the error returned by the exporter
#[error("Exporting failed with {0}")]
ExportFailed(Box<dyn ExportError>),
Expand All @@ -163,7 +159,7 @@ pub enum TraceError {

/// Other errors propagated from trace SDK that weren't covered above
#[error(transparent)]
Boxed(#[from] Box<dyn std::error::Error + Send + Sync + 'static>),
Other(#[from] Box<dyn std::error::Error + Send + Sync + 'static>),
}

impl<T> From<T> for TraceError
Expand All @@ -177,12 +173,29 @@ where

impl<T> From<TrySendError<T>> for TraceError {
fn from(err: TrySendError<T>) -> Self {
TraceError::Boxed(Box::new(err.into_send_error()))
TraceError::Other(Box::new(err.into_send_error()))
}
}

impl From<Canceled> for TraceError {
fn from(err: Canceled) -> Self {
TraceError::Boxed(Box::new(err))
TraceError::Other(Box::new(err))
}
}

impl From<String> for TraceError {
fn from(err_msg: String) -> Self {
TraceError::Other(Box::new(Custom(err_msg)))
}
}

impl From<&'static str> for TraceError {
fn from(err_msg: &'static str) -> Self {
TraceError::Other(Box::new(Custom(err_msg.into())))
}
}

/// Wrap type for string
#[derive(Error, Debug)]
#[error("{0}")]
struct Custom(String);
6 changes: 6 additions & 0 deletions opentelemetry/src/exporter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,9 @@ pub mod metrics;
#[cfg(feature = "trace")]
#[cfg_attr(docsrs, doc(cfg(feature = "trace")))]
pub mod trace;

/// Marker trait for errors returned by exporters
pub trait ExportError: std::error::Error + Send + Sync + 'static {
/// The name of exporter that returned this error
fn exporter_name(&self) -> &'static str;
}
15 changes: 6 additions & 9 deletions opentelemetry/src/exporter/trace/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Trace exporters
use crate::api::trace::TraceError;
use crate::exporter::ExportError;
use crate::{
sdk,
trace::{Event, Link, SpanContext, SpanId, SpanKind, StatusCode},
Expand All @@ -22,14 +23,6 @@ pub mod stdout;
/// Describes the result of an export.
pub type ExportResult = Result<(), TraceError>;

/// Marker trait for errors returned by exporters
pub trait ExportError: std::error::Error + Send + Sync + 'static {
/// The name of exporter that returned this error
fn exporter_name(&self) -> &'static str {
"N/A"
}
}

/// `SpanExporter` defines the interface that protocol-specific exporters must
/// implement so that they can be plugged into OpenTelemetry SDK and support
/// sending of telemetry data.
Expand Down Expand Up @@ -102,7 +95,11 @@ impl std::error::Error for HttpClientError {}

#[cfg(feature = "http")]
#[cfg_attr(docsrs, doc(cfg(feature = "http")))]
impl ExportError for HttpClientError {}
impl ExportError for HttpClientError {
fn exporter_name(&self) -> &'static str {
"http client"
}
}

#[cfg(feature = "http")]
#[cfg_attr(docsrs, doc(cfg(feature = "http")))]
Expand Down
8 changes: 5 additions & 3 deletions opentelemetry/src/exporter/trace/stdout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,17 @@
//! });
//! }
//! ```
use crate::exporter::trace::ExportError;
use crate::{
exporter::trace::{ExportResult, SpanData, SpanExporter},
exporter::{
trace::{ExportResult, SpanData, SpanExporter},
ExportError,
},
global, sdk,
trace::TracerProvider,
};
use async_trait::async_trait;
use std::io::{stdout, Stdout, Write};
use std::fmt::Debug;
use std::io::{stdout, Stdout, Write};

/// Pipeline builder
#[derive(Debug)]
Expand Down

0 comments on commit 9ffbace

Please sign in to comment.