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

feat(core)!: implement an interceptor for the logging layer #4961

Merged
merged 16 commits into from
Aug 9, 2024

Conversation

evenyag
Copy link
Contributor

@evenyag evenyag commented Aug 5, 2024

Which issue does this PR close?

Closes #4918.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Breaking changes: This PR removes with_error_level(), with_failure_level() and with_backtrace_output() from the DefaultLoggingLayer.

It adds a new trait LoggingInterceptor to log messages.

pub trait LoggingInterceptor: Debug + Send + Sync + 'static {
    fn log(
        &self,
        scheme: Scheme,
        operation: Operation,
        path: &str,
        context: &str,
        message: &str,
        err: Option<&Error>,
    );
}

The LoggingLayer has a new method to construct a new layer from an interceptor.

fn new<I: LoggingInterceptor>(notify: I) -> LoggingLayer<I> {}

@evenyag evenyag changed the title feat: implement an interceptor for the logging layer feat(core): implement an interceptor for the logging layer Aug 5, 2024
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR!

core/src/layers/logging.rs Show resolved Hide resolved
core/src/layers/logging.rs Show resolved Hide resolved
core/src/layers/logging.rs Outdated Show resolved Hide resolved
core/src/layers/logging.rs Outdated Show resolved Hide resolved
core/src/layers/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

After this refactor, the LoggingLayer is no longer limited to just log. Users can now employ tracing or any other logging libraries they prefer. We could potentially offer enhanced support for these use cases by providing default tracing/log support.

It's not part of this PR, we can discuss them later.

@Xuanwo Xuanwo changed the title feat(core): implement an interceptor for the logging layer feat(core)!: implement an interceptor for the logging layer Aug 5, 2024
core/src/layers/logging.rs Outdated Show resolved Hide resolved
core/src/layers/logging.rs Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Aug 8, 2024

Hi, I plan to include this PR in our upcoming 0.49.0 release. However, if you are currently busy, we can also postpone it to the next release. Which do you prefer?

@evenyag
Copy link
Contributor Author

evenyag commented Aug 8, 2024

Okay, I'll address the comments in these two days.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 8, 2024

Okay, I'll address the comments in these two days.

Thanks a lot!

@evenyag evenyag marked this pull request as ready for review August 8, 2024 12:37
@evenyag evenyag requested a review from Xuanwo August 8, 2024 14:16
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Others look great to me, thanks!

core/src/raw/tests/utils.rs Outdated Show resolved Hide resolved
core/src/raw/tests/utils.rs Outdated Show resolved Hide resolved
@evenyag evenyag requested a review from Xuanwo August 9, 2024 07:29
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot, mostly LGTM! We only need to get CI happy.

  • the haskell test doesn't make sense, we can remove the testLogger part

https://github.com/apache/opendal/blob/690e49467228602cccabe78ba8403fd9123146d4/bindings/haskell/test/BasicTest.hs#L35C29-L35C39

  • The unit test of core failed. Please note we can't use crate in doc test, we should use opendal::Error.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Xuanwo Xuanwo merged commit c5cc990 into apache:main Aug 9, 2024
222 of 223 checks passed
@evenyag evenyag deleted the feat/logging-interceptor branch August 9, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new feature: Provide more ways to format the error in LoggingLayer
2 participants