-
Notifications
You must be signed in to change notification settings - Fork 523
docs: Add ADR dir and error handling ADR #2664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
a6d6fad
docs: Add ADR dir and error handling ADR
scottgerring 7faa9a6
chore: Add more info about ADRs
scottgerring 95090ea
Some more detail
scottgerring f2a9bf8
fix links
scottgerring f152a0a
Update ADR format to be more prescriptive
scottgerring 9c46625
changed startup wording
scottgerring 8135270
Merge branch 'main' into main
cijothomas fc86870
Merge branch 'main' into main
cijothomas 749454d
Apply suggestions from code review
scottgerring 73df360
address review feedback
scottgerring 6ec00ae
Merge branch 'main' into main
scottgerring File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
# Error handling patterns in public API interfaces | ||
## Date | ||
27 Feb 2025 | ||
|
||
## Summary | ||
|
||
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. | ||
|
||
We will focus on the exporter traits in this example, but the outcome should be applied to _all_ public traits and their fallible operations. | ||
|
||
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`. | ||
|
||
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. | ||
|
||
## Design Guidance | ||
|
||
### 1. No panics from SDK APIs | ||
Failures during regular operation should not panic, instead returning errors to the caller where appropriate, _or_ logging an error if not appropriate. | ||
Some of the opentelemetry SDK interfaces are dictated by the specification in way such that they may not return errors. | ||
|
||
### 2. Consolidate error types within a trait where we can, let them diverge when we can't** | ||
|
||
We aim to consolidate error types where possible _without indicating a function may return more errors than it can actually return_. | ||
|
||
**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: | ||
```rust | ||
enum MegaError { | ||
TooBig, | ||
TooSmall, | ||
TooLong, | ||
TooShort | ||
} | ||
|
||
trait MyTrait { | ||
|
||
// Will only ever return TooBig,TooSmall errors | ||
fn action_one() -> Result<(), MegaError>; | ||
|
||
// These will only ever return TooLong,TooShort errors | ||
fn action_two() -> Result<(), MegaError>; | ||
fn action_three() -> Result<(), MegaError>; | ||
} | ||
``` | ||
|
||
**Instead, do this** - each function's signature indicates only the errors it can return, providing an accurate contract to the caller: | ||
|
||
```rust | ||
enum ErrorOne { | ||
TooBig, | ||
TooSmall, | ||
} | ||
|
||
enum ErrorTwo { | ||
TooLong, | ||
TooShort | ||
} | ||
|
||
trait MyTrait { | ||
fn action_one() -> Result<(), ErrorOne>; | ||
|
||
// Action two and three share the same error type. | ||
// We do not introduce a common error MyTraitError for all operations, as this would | ||
// force all methods on the trait to indicate they return errors they do not return, | ||
// complicating things for the caller. | ||
fn action_two() -> Result<(), ErrorTwo>; | ||
fn action_three() -> Result<(), ErrorTwo>; | ||
} | ||
``` | ||
|
||
## 3. Consolidate error types between signals where we can, let them diverge where we can't | ||
|
||
Consider the `Exporter`s mentioned earlier. Each of them has the same failure indicators - as dicated by the OpenTelemetry spec - and we will | ||
share the error types accordingly: | ||
|
||
**Don't do this** - each signal has its own error type, despite having exactly the same failure cases: | ||
|
||
```rust | ||
#[derive(Error, Debug)] | ||
pub enum OtelTraceError { | ||
#[error("Shutdown already invoked")] | ||
AlreadyShutdown, | ||
|
||
#[error("Operation failed: {0}")] | ||
InternalFailure(String), | ||
|
||
/** ... additional errors ... **/ | ||
} | ||
|
||
#[derive(Error, Debug)] | ||
pub enum OtelLogError { | ||
#[error("Shutdown already invoked")] | ||
AlreadyShutdown, | ||
|
||
#[error("Operation failed: {0}")] | ||
InternalFailure(String), | ||
|
||
/** ... additional errors ... **/ | ||
} | ||
``` | ||
|
||
**Instead, do this** - error types are consolidated between signals where this can be done appropriately: | ||
|
||
```rust | ||
scottgerring marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// opentelemetry-sdk::error | ||
|
||
#[derive(Error, Debug)] | ||
pub enum OTelSdkError { | ||
#[error("Shutdown already invoked")] | ||
AlreadyShutdown, | ||
|
||
#[error("Operation failed: {0}")] | ||
InternalFailure(String), | ||
|
||
/** ... additional errors ... **/ | ||
} | ||
|
||
pub type OTelSdkResult = Result<(), OTelSdkError>; | ||
|
||
/// signal-specific exporter traits all share the same | ||
/// result types for the exporter operations. | ||
|
||
// pub trait LogExporter { | ||
// pub trait SpanExporter { | ||
pub trait PushMetricExporter { | ||
fn export(&self, /* ... */) -> OtelSdkResult; | ||
fn force_flush(&self, /* ... */ ) -> OTelSdkResult; | ||
fn shutdown(&self, /* ... */ ) -> OTelSdkResult; | ||
``` | ||
|
||
If this were _not_ the case - if we needed to mark an extra error for instance for `LogExporter` that the caller could reasonably handle - | ||
we would let that error traits diverge at that point. | ||
scottgerring marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
### 4. Box custom errors where a savvy caller may be able to handle them, stringify them if not | ||
scottgerring marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. | ||
|
||
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: | ||
scottgerring marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
**Don't do this if the OtherError is potentially recoverable by a savvy caller**: | ||
```rust | ||
|
||
#[derive(Debug, Error)] | ||
pub enum MyError { | ||
#[error("Error one occurred")] | ||
ErrorOne, | ||
|
||
#[error("Operation failed: {0}")] | ||
OtherError(String), | ||
``` | ||
|
||
**Instead, do this**, allowing the caller to match on the nested error: | ||
|
||
```rust | ||
#[derive(Debug, Error)] | ||
pub enum MyError { | ||
#[error("Error one occurred")] | ||
ErrorOne, | ||
|
||
#[error("Operation failed: {source}")] | ||
OtherError { | ||
#[from] | ||
source: Box<dyn Error + Send + Sync>, | ||
}, | ||
} | ||
``` | ||
|
||
Note that at the time of writing, there is no instance we have identified within the project that has required this. | ||
|
||
### 5. Use thiserror by default | ||
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). | ||
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. | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# Architectural Decision Records | ||
|
||
This directory contains architectural decision records made for the opentelemetry-rust project. These allow us to consolidate discussion, options, and outcomes, around key architectural decisions. | ||
|
||
* [001 - Error Handling](001_error_handling.md) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.