Skip to content
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

Refactor handling of non-fatal errors, including warnings. #337

Merged
merged 12 commits into from
Aug 23, 2024

Conversation

lquerel
Copy link
Contributor

@lquerel lquerel commented Aug 21, 2024

This PR introduces a new result type named WResult, which distinguishes between non-fatal errors (NFEs) and fatal errors. NFEs do not prevent subsequent operations from completing successfully. For example, if a semconv file is invalid, a non-fatal error is generated, but processing of the other files continues.

This design will reduce the risk of promoting non-fatal errors to fatal errors, as occurred in the previous design.

With this PR:

  • warnings detected in the baseline registry are now ignored,
  • non-fatal errors will not interrupt any command before it completes.

Closes #334

@lquerel lquerel self-assigned this Aug 21, 2024
@lquerel lquerel added the bug Something isn't working label Aug 21, 2024
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ironically, this looks a lot like my initial proposal :)

This looks like a good approach.

test_data/compatibility_check.rego Outdated Show resolved Hide resolved
let semconv_spec = SemConvSpecWithProvenance::from_file(path_buf.as_path())?;
registry.add_semconv_spec(semconv_spec);
pub fn try_from_path_pattern(registry_id: &str, path_pattern: &str) -> WResult<Self, Error> {
fn create_registry_or_fatal(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern simplifies the management of fatal errors using a simple ? operator. This internal method populates a list of non-fatal errors and returns a Result. This Result is then transformed into a WResult depending on the presence or absence of non-fatal errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets around limitations in the current expeirmental custom type ? support for sure. I'd still like to go that direction with WResult when it stabilizes, but for now this seems to be our best bet.

I may take some time to write up a guide on using WResult that includes this, but sadly I've been highly limited on time recently (possibly when entities OTEP has progressed past prototyping, I can come back and revisit this)

@lquerel lquerel changed the title [WIP] Refactor handling of non-fatal errors, including warnings. Refactor handling of non-fatal errors, including warnings. Aug 23, 2024
@lquerel lquerel marked this pull request as ready for review August 23, 2024 00:26
@lquerel lquerel requested a review from a team August 23, 2024 00:26
crates/weaver_common/src/result.rs Outdated Show resolved Hide resolved
Err(e) => {
error.push(e);
None
// Process all the results of the previous parallel processing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIt: We should turn this into a helper method eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest implementing this helper once we encounter more than one instance of this pattern.

let semconv_spec = SemConvSpecWithProvenance::from_file(path_buf.as_path())?;
registry.add_semconv_spec(semconv_spec);
pub fn try_from_path_pattern(registry_id: &str, path_pattern: &str) -> WResult<Self, Error> {
fn create_registry_or_fatal(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets around limitations in the current expeirmental custom type ? support for sure. I'd still like to go that direction with WResult when it stabilizes, but for now this seems to be our best bet.

I may take some time to write up a guide on using WResult that includes this, but sadly I've been highly limited on time recently (possibly when entities OTEP has progressed past prototyping, I can come back and revisit this)

@lquerel lquerel merged commit 2559820 into open-telemetry:main Aug 23, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

--future is also applied to resolving --baseline_registry
2 participants