Move Logs and Metrics error constructs to SDK

[lalitb]

Fixes #1042

Changes

Needs #2256 and #2260 to be merged first. Also the PR is on top of #2260, so better to review this once that PR is merged.

Summary of changes:

1. ExportError trait:

   • Moved from opentelemetry/src/common.rs to opentelemetry-sdk/src/export/mod.rs
   • Trace API now has its own ExportError trait in opentelemetry/src/trace/mod.rs. This will be eventually consolidated within SDK once we work on traces.

2. LogError enum and LogResult type alias :

   • Moved from opentelemetry/src/logs/mod.rs to opentelemetry-sdk/src/logs/error.rs

3. MetricError enum and MetricResult type alias:

   • Moved from opentelemetry/src/metrics/mod.rs to opentelemetry-sdk/src/metrics/error.rs

4. TraceError enum and TraceResult type alias:

   • No changes. Added TODO to move in future.

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[cijothomas]

Though this is moving from just one crate to another, I'd like to first confirm that these are indeed required. I understand that it is desirable to return specialized errors from user-facing APIs, but most of the errors here are only for internal logging only. Is there any additional value provided by this?

[cijothomas]

See #2241 (comment) for prior discussion.
If we don't plan to keep this, then its best to just remove it, instead of moving from api to sdk and then removing it again.
(Note: These are still public APIs in the sdk crate)

[cijothomas]

See https://github.com/open-telemetry/opentelemetry-rust/pull/2266/files#r1826889942
It maybe better off removing them altogether than moving from api to sdk crate.

[lalitb]

See https://github.com/open-telemetry/opentelemetry-rust/pull/2266/files#r1826889942 It maybe better off removing them altogether than moving from api to sdk crate.

I am not sure if we agreed on removing this based on that comment. This needs some more discussions. These error constructs were always used in the SDK, so it's logical to first move their definition to SDK. This would also enable us to proceed with the RC for the API and then make a decision regarding the SDK.

[cijothomas]

See https://github.com/open-telemetry/opentelemetry-rust/pull/2266/files#r1826889942 It maybe better off removing them altogether than moving from api to sdk crate.

I am not sure if we agreed on removing this based on that comment. This needs some more discussions. These error constructs were always used in the SDK, so it's logical to first move their definition to SDK. This would also enable us to proceed with the RC for the API and then make a decision regarding the SDK.

Please use this PR to drive a decision for the same.

[lalitb]

Please use this PR to drive a decision for the same.

Why do we need to do that here? The API cleanup is logical, and this PR make sense to go first without any breaking anything. We can have separate issue/PR to discuss the breaking changes in terms of removing the constructs altogether.

[cijothomas]

Please use this PR to drive a decision for the same.

Why do we need to do that here? The API cleanup is logical, and this PR make sense to go first without any breaking anything. We can have separate issue/PR to discuss the breaking changes in terms of removing the constructs altogether.

Because, as mentioned here, this PR is introducing new public API to the sdk crate, that we may need to remove in future again.

Totally agree with removing it from opentelemetry crate.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 36 lines in your changes missing coverage. Please review.

Project coverage is 79.4%. Comparing base (31bea19) to head (fa9c724).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-sdk/src/logs/error.rs	0.0%	12 Missing ⚠️
opentelemetry-otlp/src/exporter/http/mod.rs	0.0%	8 Missing ⚠️
opentelemetry-sdk/src/metrics/error.rs	0.0%	6 Missing ⚠️
opentelemetry-otlp/src/lib.rs	0.0%	3 Missing ⚠️
opentelemetry-zipkin/src/exporter/mod.rs	0.0%	3 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/mod.rs	0.0%	2 Missing ⚠️
opentelemetry-otlp/src/logs.rs	0.0%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2266     +/-   ##
=======================================
- Coverage   79.4%   79.4%   -0.1%     
=======================================
  Files        121     122      +1     
  Lines      20787   20795      +8     
=======================================
+ Hits       16512   16514      +2     
- Misses      4275    4281      +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lalitb]

The Result/Error are used within SDK in these public APIs:

• LogExporterBuilder::build() -> Result<LogExporter, opentelemetry_sdk::logs::LogError>
• LogExporter::export() -> LogResult<()>
• LoggerProvider::force_flush(&self) -> Vec<LogResult<()>>
• LoggerProvider::shutdown(&self) -> LogResult<()>

Similarly for Metrics, probably also while returning the error from View creation.
(could have missed some places, as this was quick grep)

The question is - if we want to remove the Result/Error, what are we instead looking for - returning bool as status, or just the error string? Returning structured error is in general better instead of letting user rely on doing string match to identify the error type. Result enum is already part of rust std lib, and idiomatic.

If the goal is to maintain a minimal public surface for the SDK, we can still achieve this using the result/error construct as needed, as shown below. However, once they are removed, reintroducing them later could be challenging without causing breaking changes.

/// Describe the result of operations in log SDK.
pub type LogResult<T> = Result<T, LogError>;

#[derive(Error, Debug)]
#[non_exhaustive]
/// Errors returned by the log SDK.
pub enum LogError {
    /// A generic error message for errors in the log SDK.
    #[error("{0}")]
    Message(String),
}

[cijothomas]

From Community Meeting:

1. This is mandatory to declare opentelemetry as RC.
2. The APIs being moved to SDK might need a re-review in the future, but that is okay.
3. Make changelog as this is user breaking change.
4. Must merge for 0.27, so not worth trying to split into shorter ones.
5. Traces is untouched, lets leave it for a future release.

[cijothomas]

Thanks for cleaning up the API crate!
Lets follow up in the future to review these to make everything is needed in sdk also as public.