Move Logs and Metrics error constructs to SDK #2266

lalitb · 2024-11-01T17:51:52Z

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:

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.
LogError enum and LogResult type alias :
- Moved from opentelemetry/src/logs/mod.rs to opentelemetry-sdk/src/logs/error.rs
MetricError enum and MetricResult type alias:
- Moved from opentelemetry/src/metrics/mod.rs to opentelemetry-sdk/src/metrics/error.rs
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)

…elemetry-rust into remove-global-error-handler

cijothomas · 2024-11-03T02:28:02Z

opentelemetry-sdk/src/error.rs

+#[cfg(feature = "trace")]
+use opentelemetry::trace::TraceError;
+
+/// Wrapper for error from both tracing and metrics part of open telemetry.


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?

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 · 2024-11-03T03:34:08Z

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 · 2024-11-04T18:59:38Z

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 · 2024-11-04T19:36:48Z

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 · 2024-11-04T19:42:27Z

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 · 2024-11-05T06:55:13Z

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 · 2024-11-05T07:34:01Z

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:

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

opentelemetry-sdk/src/export/mod.rs

opentelemetry/src/trace/mod.rs

opentelemetry/CHANGELOG.md

opentelemetry/src/global/mod.rs

opentelemetry/CHANGELOG.md

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.

lalitb and others added 6 commits October 28, 2024 11:56

remove global error handler

f35e0ec

Merge branch 'main' into remove-global-error-handler

699124c

bring back error_handler.rs, and remove the error_handler from there

dc1994b

Merge branch 'remove-global-error-handler' of github.com:lalitb/opent…

00d1c1c

…elemetry-rust into remove-global-error-handler

Merge branch 'main' into remove-global-error-handler

a4381ce

initial commit

9cf5898

lalitb requested a review from a team as a code owner November 1, 2024 17:51

Merge branch 'main' into move-errors-to-sdk

a047fad

cijothomas reviewed Nov 3, 2024

View reviewed changes

cijothomas requested changes Nov 3, 2024

View reviewed changes

Merge branch 'main' into move-errors-to-sdk

56ba6a3

Merge branch 'main' into move-errors-to-sdk

7a4c75b

lalitb added 5 commits November 4, 2024 11:48

Merge branch 'main' into move-errors-to-sdk

e86bf1b

merge conflict

16ffb3e

fix build

fa9237b

build error

f2b0156

build fix - continue

139ed9b

cijothomas requested changes Nov 5, 2024

View reviewed changes

changeloG

11e6acb

utpilla reviewed Nov 5, 2024

View reviewed changes

opentelemetry-sdk/src/export/mod.rs Outdated Show resolved Hide resolved

utpilla reviewed Nov 5, 2024

View reviewed changes

opentelemetry/src/trace/mod.rs Outdated Show resolved Hide resolved

utpilla reviewed Nov 5, 2024

View reviewed changes

opentelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

utpilla reviewed Nov 5, 2024

View reviewed changes

opentelemetry/src/global/mod.rs Show resolved Hide resolved

Update mod.rs

e7e879d

lalitb added 2 commits November 5, 2024 16:58

Update CHANGELOG.md

8e2b1ae

Update mod.rs

e64b544

utpilla approved these changes Nov 6, 2024

View reviewed changes

Merge branch 'main' into move-errors-to-sdk

e9c4d51

cijothomas reviewed Nov 6, 2024

View reviewed changes

opentelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

cijothomas approved these changes Nov 6, 2024

View reviewed changes

lalitb and others added 3 commits November 6, 2024 13:10

Merge branch 'main' into move-errors-to-sdk

a5a8409

Merge branch 'main' into move-errors-to-sdk

9918947

error from merge conflict

345ba47

lalitb added the integration tests Run integration tests label Nov 6, 2024

update otel changelog

fa9c724

cijothomas merged commit ebf1bb6 into open-telemetry:main Nov 6, 2024
23 of 25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Logs and Metrics error constructs to SDK #2266

Move Logs and Metrics error constructs to SDK #2266

lalitb commented Nov 1, 2024 •

edited

Loading

cijothomas Nov 3, 2024

cijothomas Nov 3, 2024

cijothomas left a comment

lalitb commented Nov 3, 2024 •

edited

Loading

cijothomas commented Nov 4, 2024

lalitb commented Nov 4, 2024

cijothomas commented Nov 4, 2024

codecov bot commented Nov 5, 2024 •

edited

Loading

lalitb commented Nov 5, 2024 •

edited

Loading

cijothomas left a comment

cijothomas left a comment

Move Logs and Metrics error constructs to SDK #2266

Move Logs and Metrics error constructs to SDK #2266

Conversation

lalitb commented Nov 1, 2024 • edited Loading

Changes

Merge requirement checklist

cijothomas Nov 3, 2024

Choose a reason for hiding this comment

cijothomas Nov 3, 2024

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

lalitb commented Nov 3, 2024 • edited Loading

cijothomas commented Nov 4, 2024

lalitb commented Nov 4, 2024

cijothomas commented Nov 4, 2024

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

lalitb commented Nov 5, 2024 • edited Loading

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

lalitb commented Nov 1, 2024 •

edited

Loading

lalitb commented Nov 3, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

lalitb commented Nov 5, 2024 •

edited

Loading