|
| 1 | +# Error handling patterns in public API interfaces |
| 2 | +## Date |
| 3 | +27 Feb 2025 |
| 4 | + |
| 5 | +## Summary |
| 6 | + |
| 7 | +This ADR describes the general pattern we will follow when modelling errors in public API interfaces - that is, APIs that are exposed to users of the project's published crates. It summarises the discussion and final option from [#2571](https://github.com/open-telemetry/opentelemetry-rust/issues/2571); for more context check out that issue. |
| 8 | + |
| 9 | +We will focus on the exporter traits in this example, but the outcome should be applied to _all_ public traits and their fallible operations. |
| 10 | + |
| 11 | +These include [SpanExporter](https://github.com/open-telemetry/opentelemetry-rust/blob/eca1ce87084c39667061281e662d5edb9a002882/opentelemetry-sdk/src/trace/export.rs#L18), [LogExporter](https://github.com/open-telemetry/opentelemetry-rust/blob/eca1ce87084c39667061281e662d5edb9a002882/opentelemetry-sdk/src/logs/export.rs#L115), and [PushMetricExporter](https://github.com/open-telemetry/opentelemetry-rust/blob/eca1ce87084c39667061281e662d5edb9a002882/opentelemetry-sdk/src/metrics/exporter.rs#L11) which form part of the API surface of `opentelemetry-sdk`. |
| 12 | + |
| 13 | +There are various ways to handle errors on trait methods, including swallowing them and logging, panicing, returning a shared global error, or returning a method-specific error. We strive for consistency, and we want to be sure that we've put enough thought into what this looks like that we don't have to make breaking interface changes unecessarily in the future. |
| 14 | + |
| 15 | +## Design Guidance |
| 16 | + |
| 17 | +### 1. No panics from SDK APIs |
| 18 | +Failures during regular operation should not panic, instead returning errors to the caller where appropriate, _or_ logging an error if not appropriate. |
| 19 | +Some of the opentelemetry SDK interfaces are dictated by the specification in way such that they may not return errors. |
| 20 | + |
| 21 | +### 2. Consolidate error types within a trait where we can, let them diverge when we can't** |
| 22 | + |
| 23 | +We aim to consolidate error types where possible _without indicating a function may return more errors than it can actually return_. |
| 24 | + |
| 25 | +**Don't do this** - each function's signature indicates that it returns errors it will _never_ return, forcing the caller to write handlers for dead paths: |
| 26 | +```rust |
| 27 | +enum MegaError { |
| 28 | + TooBig, |
| 29 | + TooSmall, |
| 30 | + TooLong, |
| 31 | + TooShort |
| 32 | +} |
| 33 | + |
| 34 | +trait MyTrait { |
| 35 | + |
| 36 | + // Will only ever return TooBig,TooSmall errors |
| 37 | + fn action_one() -> Result<(), MegaError>; |
| 38 | + |
| 39 | + // These will only ever return TooLong,TooShort errors |
| 40 | + fn action_two() -> Result<(), MegaError>; |
| 41 | + fn action_three() -> Result<(), MegaError>; |
| 42 | +} |
| 43 | +``` |
| 44 | + |
| 45 | +**Instead, do this** - each function's signature indicates only the errors it can return, providing an accurate contract to the caller: |
| 46 | + |
| 47 | +```rust |
| 48 | +enum ErrorOne { |
| 49 | + TooBig, |
| 50 | + TooSmall, |
| 51 | +} |
| 52 | + |
| 53 | +enum ErrorTwo { |
| 54 | + TooLong, |
| 55 | + TooShort |
| 56 | +} |
| 57 | + |
| 58 | +trait MyTrait { |
| 59 | + fn action_one() -> Result<(), ErrorOne>; |
| 60 | + |
| 61 | + // Action two and three share the same error type. |
| 62 | + // We do not introduce a common error MyTraitError for all operations, as this would |
| 63 | + // force all methods on the trait to indicate they return errors they do not return, |
| 64 | + // complicating things for the caller. |
| 65 | + fn action_two() -> Result<(), ErrorTwo>; |
| 66 | + fn action_three() -> Result<(), ErrorTwo>; |
| 67 | +} |
| 68 | +``` |
| 69 | + |
| 70 | +## 3. Consolidate error types between signals where we can, let them diverge where we can't |
| 71 | + |
| 72 | +Consider the `Exporter`s mentioned earlier. Each of them has the same failure indicators - as dicated by the OpenTelemetry spec - and we will |
| 73 | +share the error types accordingly: |
| 74 | + |
| 75 | +**Don't do this** - each signal has its own error type, despite having exactly the same failure cases: |
| 76 | + |
| 77 | +```rust |
| 78 | +#[derive(Error, Debug)] |
| 79 | +pub enum OtelTraceError { |
| 80 | + #[error("Shutdown already invoked")] |
| 81 | + AlreadyShutdown, |
| 82 | + |
| 83 | + #[error("Operation failed: {0}")] |
| 84 | + InternalFailure(String), |
| 85 | + |
| 86 | + /** ... additional errors ... **/ |
| 87 | +} |
| 88 | + |
| 89 | +#[derive(Error, Debug)] |
| 90 | +pub enum OtelLogError { |
| 91 | + #[error("Shutdown already invoked")] |
| 92 | + AlreadyShutdown, |
| 93 | + |
| 94 | + #[error("Operation failed: {0}")] |
| 95 | + InternalFailure(String), |
| 96 | + |
| 97 | + /** ... additional errors ... **/ |
| 98 | +} |
| 99 | +``` |
| 100 | + |
| 101 | +**Instead, do this** - error types are consolidated between signals where this can be done appropriately: |
| 102 | + |
| 103 | +```rust |
| 104 | + |
| 105 | +/// opentelemetry-sdk::error |
| 106 | + |
| 107 | +#[derive(Error, Debug)] |
| 108 | +pub enum OTelSdkError { |
| 109 | + #[error("Shutdown already invoked")] |
| 110 | + AlreadyShutdown, |
| 111 | + |
| 112 | + #[error("Operation failed: {0}")] |
| 113 | + InternalFailure(String), |
| 114 | + |
| 115 | + /** ... additional errors ... **/ |
| 116 | +} |
| 117 | + |
| 118 | +pub type OTelSdkResult = Result<(), OTelSdkError>; |
| 119 | + |
| 120 | +/// signal-specific exporter traits all share the same |
| 121 | +/// result types for the exporter operations. |
| 122 | + |
| 123 | +// pub trait LogExporter { |
| 124 | +// pub trait SpanExporter { |
| 125 | +pub trait PushMetricExporter { |
| 126 | + fn export(&self, /* ... */) -> OtelSdkResult; |
| 127 | + fn force_flush(&self, /* ... */ ) -> OTelSdkResult; |
| 128 | + fn shutdown(&self, /* ... */ ) -> OTelSdkResult; |
| 129 | +``` |
| 130 | + |
| 131 | +If this were _not_ the case - if we needed to mark an extra error for instance for `LogExporter` that the caller could reasonably handle - |
| 132 | +we would let that error traits diverge at that point. |
| 133 | + |
| 134 | +### 4. Box custom errors where a savvy caller may be able to handle them, stringify them if not |
| 135 | + |
| 136 | +Note above that we do not box any `Error` into `InternalFailure`. Our rule here is that if the caller cannot reasonably be expected to handle a particular error variant, we will use a simplified interface that returns only a descriptive string. In the concrete example we are using with the exporters, we have a [strong signal in the opentelemetry-specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#export) that indicates that the error types _are not actionable_ by the caller. |
| 137 | + |
| 138 | +If the caller may potentially recover from an error, we will follow the generally-accepted best practice (e.g., see [canonical's guide](https://canonical.github.io/rust-best-practices/error-and-panic-discipline.html) and instead preserve the nested error: |
| 139 | + |
| 140 | +**Don't do this if the OtherError is potentially recoverable by a savvy caller**: |
| 141 | +```rust |
| 142 | + |
| 143 | +#[derive(Debug, Error)] |
| 144 | +pub enum MyError { |
| 145 | + #[error("Error one occurred")] |
| 146 | + ErrorOne, |
| 147 | + |
| 148 | + #[error("Operation failed: {0}")] |
| 149 | + OtherError(String), |
| 150 | +``` |
| 151 | + |
| 152 | +**Instead, do this**, allowing the caller to match on the nested error: |
| 153 | + |
| 154 | +```rust |
| 155 | +#[derive(Debug, Error)] |
| 156 | +pub enum MyError { |
| 157 | + #[error("Error one occurred")] |
| 158 | + ErrorOne, |
| 159 | + |
| 160 | + #[error("Operation failed: {source}")] |
| 161 | + OtherError { |
| 162 | + #[from] |
| 163 | + source: Box<dyn Error + Send + Sync>, |
| 164 | + }, |
| 165 | +} |
| 166 | +``` |
| 167 | + |
| 168 | +Note that at the time of writing, there is no instance we have identified within the project that has required this. |
| 169 | + |
| 170 | +### 5. Use thiserror by default |
| 171 | +We will use [thiserror](https://docs.rs/thiserror/latest/thiserror/) by default to implement Rust's [error trait](https://doc.rust-lang.org/core/error/trait.Error.html). |
| 172 | +This keeps our code clean, and as it does not appear in our interface, we can choose to replace any particular usage with a hand-rolled implementation should we need to. |
| 173 | + |
0 commit comments