You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "evenyag (via GitHub)" <gi...@apache.org> on 2024/07/23 06:54:33 UTC
[I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
evenyag opened a new issue, #4918:
URL: https://github.com/apache/opendal/issues/4918
### Feature Description
The logging layer provides a flag `backtrace_output` to control how the layer formats the error, mainly for displaying the backtrace.
https://github.com/apache/opendal/blob/5e735cc53cd8638f47bd10b67d5d6e258ff22b67/core/src/layers/logging.rs#L188-L200
By default, the layer formats the error using the `Display` trait. Even if we enable the `backtrace_output` flag, the layer may still use `Display` if the error kind isn't unexpected.
Sometimes, printing the `Error` in debug format is helpful because the display format may lack some information for debugging. The layer could add a flag to control how to format the error. e.g.
- Adds a new flag `debug_format_error`
- Adds a callback to `format_error()` and use that callback to format the error (This might conflict with `backtrace_output`)
### Problem and Solution
The main issue is that reqwest no longer prints the source error in its `Display` implementation. It only prints the source in `Debug`. See: https://github.com/seanmonstar/reqwest/pull/2199
So we can't get the source error while opendal's HTTP client returns an error opendal's error also uses `Display` to format the source error.
Here is an error log we captured:
```
service=s3 operation=read path=data/bdf28185-5eaa-4a52-a71a-795686b1d039.parquet -> Unexpected (persistent) at => Unexpected (temporary) at read, context: { url: https://s3.amazonaws.com/my-bucket/data/bdf28185-5eaa-4a52-a71a-795686b1d039.parquet, called: http_util::Client::send, service: s3, path: data/bdf28185-5eaa-4a52-a71a-795686b1d039.parquet, range: 1030-2412 } => send http request, source: error sending request for url (https://s3.us-west-2.amazonaws.com/my-bucket/data/bdf28185-5eaa-4a52-a71a-795686b1d039.parquet)
```
The root cause of the error while sending the request is missing. It's quite painful to further diagnose the problem.
Alternative Solution:
- wraps the reqwest error into a new type that displays the source error
- It is more straightforward
- The pitfall is that we must remember to wrap errors by that type
- attaches the source to the context
### Additional Context
_No response_
### Are you willing to contribute to the development of this feature?
- [X] Yes, I am willing to contribute to the development of this feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo closed issue #4918: new feature: Provide more ways to format the error in `LoggingLayer`
URL: https://github.com/apache/opendal/issues/4918
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #4918:
URL: https://github.com/apache/opendal/issues/4918#issuecomment-2247941060
> Can we define a `struct Record` as the argument?
Add a new struct means we need to expose another new entry along with many `getter/setter` API at the layer level which I want to avoid.
> What's more, should we pass the [`enum Operation`](https://docs.rs/opendal/latest/opendal/raw/enum.Operation.html) instead of a `&'static str` for `operation`?
We have `ReadOperation` and `WriteOperation` which can't be represent by `Operation` directly. However, maybe it's meaningful for us to merge `ReadOperation` into `Operation` directly. We can discuss this in another issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #4918:
URL: https://github.com/apache/opendal/issues/4918#issuecomment-2247174889
> Should it be a part of the `LoggingLayer`?
Yes, my current idea is make it a part of `LoggingLayer`.
> Is it an ErrorInterceptor or an EventInterceptor?
I wish to have an `EventInterceptor` like:
```rust
trait LoggingInterceptor {
fn log(&self, scheme: Scheme, operation: &'static str, path: &str, message: &str, err: Option<&Error>);
}
```
The default behaivor will be like:
```rust
struct DefaultLoggingInterceptor;
impl LoggingInterceptor for DefaultLoggingInterceptor {
fn log(&self, scheme: Scheme, operation: &'static str, path: &str, message: &str, err: Option<&Error>) {
// We can decide level based on `operation` and `err`.
let lvl = xxx;
if err.is_none {
log!(
target: LOGGING_TARGET,
lvl,
"service={scheme} operation={operation} path={path} -> {message}",
)
} else {
// print with error
}
}
}
```
Some other change may also needed to make it work. We might can remove `with_error_level`, `with_failure_level` and `backtrace_output`.
---
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "evenyag (via GitHub)" <gi...@apache.org>.
evenyag commented on issue #4918:
URL: https://github.com/apache/opendal/issues/4918#issuecomment-2244630275
Makes sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "evenyag (via GitHub)" <gi...@apache.org>.
evenyag commented on issue #4918:
URL: https://github.com/apache/opendal/issues/4918#issuecomment-2249564803
> Add a new struct means we need to expose another new API along with many getter/setter API at the layer level which I want to avoid.
Got it. I'll implement a prototype first. If I encounter any problems, I'll come back here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #4918:
URL: https://github.com/apache/opendal/issues/4918#issuecomment-2244459045
Maybe we can introduce something like `RetryInterceptor`?
https://github.com/apache/opendal/blob/5e735cc53cd8638f47bd10b67d5d6e258ff22b67/core/src/layers/retry.rs#L236-L265
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "evenyag (via GitHub)" <gi...@apache.org>.
evenyag commented on issue #4918:
URL: https://github.com/apache/opendal/issues/4918#issuecomment-2247104609
@Xuanwo Oh, sorry. I found people can do this via the `RetryInterceptor` so I closed the issue. But I forgot that we might not always set a `RetryLayer`.......
> do you have interest to implement this?
Sure.
I'm not sure what the "interceptor" should be.
- Should it be a part of the `LoggingLayer`?
- Is it an `ErrorInterceptor` or an `EventInterceptor`?
- The logging layer logs in many places so we may need to define many events. Maybe we can only provide the `ErrorInterceptor`
- If we log the error in the interceptor, how to avoid the logging layer log it again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "evenyag (via GitHub)" <gi...@apache.org>.
evenyag commented on issue #4918:
URL: https://github.com/apache/opendal/issues/4918#issuecomment-2269163488
> I'll implement a prototype first.
#4961 It should be ready for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "evenyag (via GitHub)" <gi...@apache.org>.
evenyag commented on issue #4918:
URL: https://github.com/apache/opendal/issues/4918#issuecomment-2247926032
Can we define a `struct Record` as the argument? Similar to https://docs.rs/log/latest/log/struct.Record.html
```rust
pub struct Record<'a> {
scheme: Scheme,
operation: &'static str,
path: &'a str,
message: &'a str,
err: Option<&'a Error>,
}
impl<'a> Record<'a> {
fn scheme(&self) -> Scheme {}
// ...
}
```
What's more, should we pass the [`enum Operation`](https://docs.rs/opendal/latest/opendal/raw/enum.Operation.html) instead of a `&'static str`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #4918:
URL: https://github.com/apache/opendal/issues/4918#issuecomment-2247053847
Let's make it open to track the progress of this feature. @evenyag do you have interest to implement this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Provide more ways to format the error in `LoggingLayer` [opendal]
Posted by "evenyag (via GitHub)" <gi...@apache.org>.
evenyag closed issue #4918: new feature: Provide more ways to format the error in `LoggingLayer`
URL: https://github.com/apache/opendal/issues/4918
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org