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.

Sign up for GitHub

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

Jump to bottom

Remove deprecated error reporting patterns from vdk plugins and vdk-core #2680

Closed
DeltaMichael opened this issue Sep 21, 2023 · 1 comment
Closed

Remove deprecated error reporting patterns from vdk plugins and vdk-core #2680

DeltaMichael opened this issue Sep 21, 2023 · 1 comment
Assignees
@duyguHsnHsn
Labels
enhancement New feature or request initiative: VDK Run Logs story Task for an Epic
Milestone
VDK Run Logs: Cle...

Comments

@DeltaMichael
Copy link
Contributor

DeltaMichael commented Sep 21, 2023 •
edited
Loading

Overview

After #2666, log_and_throw, log_and_rethrow and ErrorMessage were just kept for compatibility with vdk plugins that still use them. The ResolvableBy enums also have incorrect names under the current paradigm

DoD

  1. Replace log_and_throw and log_and_rethrow calls with report_and_throw and report_and_rethrow. Include separate logging where necessary
  2. Remove the use of ErrorMessage and just pass the message lines
  3. Remove the use of ErrorMessage to convert notifications to html and replace it with a private method
  4. Rename ResolvableBy to ErrorType and ResolvableByActual to ResolvableBy
  5. Split the error reporting, resolvable context and error classes into separate files (if this causes too much of a ripple in the code, open a separate ticket for it)
@DeltaMichael DeltaMichael added enhancement New feature or request story Task for an Epic initiative: VDK Run Logs labels Sep 21, 2023
@DeltaMichael DeltaMichael mentioned this issue Sep 21, 2023
Merged
DeltaMichael added a commit that referenced this issue Sep 21, 2023
@DeltaMichael
vdk-core: separate error logging, error reporting and error classific…
cdb51dc 
…ation (#2666)

## Why

As part of the run logs initiative, we should add classification
attributes to exceptions that are handled in vdk. These attributes are
used for easier error classification and will help us enrich the vdk
exception class family, so we can pass more info to our users.

This is not possible without separating the error logging and error
reporting mechanisms, so some refactoring was required.

## What

The main changes are in `errors.py`. All other changes (tests, other
modules) are a result of changes in `errors.py`. The changes for
specific plugins are out of scope for this PR due to compatibility
issues.

### Add attributes to exceptions

Base vdk exceptions now have two new attributes - `to_be_fixed_by` and
`type` (names subject to change). These help the error classification
functions determine who to blame when an exception is raised. They're
also displayed when pretty-printing the vdk exceptions. UserCodeError,
VdkConfigurationError and PlatformService error are preserved as
convenience classes that hardcode these attributes, based on who we
decide is responsible for these exceptions. They can be extended into
more specific exceptions in subsequent PRs, related to
#2572

### Introduce a reporting API

```
report(error_type, exception: BaseException)
report_and_throw(exception: BaseVdkException)
report_and_rethrow(error_type, exception: BaseException)
```

The report functions adds any base exception (not necessarily a vdk one)
to the resolvable context. It determines who to blame for the error and
sets the `to_be_fixed_by` and `type` attributes of the exception based
on the error classification.

report_and_throw is used for convenience. It adds the vdk exception that
was passed to the resolvable context and throws it.

report_and_rethrow just calls report and throws the exception that was
passed.

Finally, we have the `log_exception(log, exception: BaseException,
*lines)` function. It builds a message from the lines we pass and logs
it at the warn level. It then logs the exception that was passed.

The result is that we've effectively decoupled logging from the error
reporting and error classification functions. We can choose to report an
exception and not log it, or log and exception and not report it,
depending on how we want to handle it. This provides a certain amount of
freedom if we want to eliminate log statements we consider unnecessary.

### Change the way vdk exceptions are represented

Override the __str__ and __repr__ methods in BaseVdkException. The
constructor takes a variable number of lines that are used to create the
error message and exception representation. We print a nice box that
tells us what kind of exception occurred and who should fix it, along
with the details.

The constructor can still take an ErrorMessage or dict instance for
compatibility reasons. That logic should be removed when we remove
ErrorMessage from the plugins.

### What was removed

Passing `ErrorMessage` to vdk exceptions is not necessary anymore, so
the pattern has been removed wherever it's used for instantiation in
vdk-core.

Formatting the error message also had separate functions, which are no
longer needed, so they've been removed.

The decorator for error messages for some special cases has been removed
and the logic added to the only function that actually used it.

DomainError, `log_and_throw` and `log_and_rethrow` are kept for
compatibility reasons, but they should be phased out in all plugins that
call them as separate PRs.

## Resulting user experience

**Before:** https://pastebin.com/raw/YsgzpUj4

Configuration error is printed with verbose stack trace

**After:** https://pastebin.com/raw/MUsLcjB2

Configuration error is printed with shorter stack trace

**Before:** https://pastebin.com/raw/qbfRfAUn

Exception coming from user code is wrapped in UserCodeError

**After:** https://pastebin.com/raw/1HGfDqye

Exception coming from user code is not wrapped, but attributes are set
correctly, e.g. blamee is displayed as user error in the subsequent
result.

**Alternative**: https://pastebin.com/raw/AsjDeyHZ

Alternatively, we can wrap non-vdk exceptions on re-throw, but this
means we should re-visit some of our logging statements, because the
logs become too verbose. Since logging exceptions is now separate from
reporting them, we can dive deeper into the issue.

### Folow-up

#2677
#2678
#2679
#2680
#2681
#2682
#2683
#2684

## How was this tested

Ran locally with jobs that cause errors, ran the vdk-core tests locally
as well. For plugin tests, we rely on CI.

## What kind of change is this

Potentially breaking change

Signed-off-by: Dilyan Marinov <mdilyan@vmware.com>
This was referenced Oct 4, 2023
Merged
Merged
duyguHsnHsn added a commit that referenced this issue Oct 5, 2023
@duyguHsnHsn @pre-commit-ci
vdk-plugins: add new error handling methods (#2750)
6f4b825 
As part of [Remove deprecated error reporting patterns from vdk plugins
and
vdk-core#2680](#2680)
I have Removed the log_and_throw and log_and_rethrow patterns from all
the plugins except vdk-impala(which will be done in another PR).
Also renamed ResolvableBy to ErrorType and ResolvableByActual to
ResolvableBy in most places (it is not removed in some places because it
requires vdk-core modifications, those changes will be introduced in
later PRs
)

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
duyguHsnHsn added a commit that referenced this issue Oct 5, 2023
@duyguHsnHsn
vdk-impala: introduce new error handling (#2759)
2f8cc6f 
As part of #2680
I have Removed the log_and_throw and log_and_rethrow patterns from
vdk-impala.
Also renamed ResolvableBy to ErrorType and ResolvableByActual to
ResolvableBy in most places (it is not removed in some places because it
requires vdk-core modifications, those changes will be introduced in
later PRs)
@duyguHsnHsn
Copy link
Collaborator

We will not be solving 4. here. It is described as new issue here: #2768

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@duyguHsnHsn duyguHsnHsn

Labels
enhancement New feature or request initiative: VDK Run Logs story Task for an Epic
Projects
None yet
Milestone
VDK Run Logs: Clean Error Handling
Development

No branches or pull requests
3 participants
@duyguHsnHsn @sabadzhiev @DeltaMichael