Skip to content

feat: improve observability of BytesReader and DirStreamer #603

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

Merged
merged 14 commits into from
Aug 31, 2022

Conversation

ClSlaid
Copy link
Contributor

@ClSlaid ClSlaid commented Aug 30, 2022

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Why are so many files edited?

My code format was a mess so I ran a complete format...

Summary

By wrapping BytesReader and DirStreamer in a way like layers in v0.13.0, a leap of observability is done.

  1. logging with LoggingReader and LoggingStreamer, when Accessor::read, Accessor::write, Accessor::list and Accessor::write_multipart()
  2. tracing with TracingReader and TracingStreamer, when Accessor::read, Accessor::write, Accessor::list and Accessor::write_multipart()
  3. metrics with MetricReader, when Accessor::read, Accessor::write and Accessor::write_multipart()

Need discussion

  • The span of tracing
  • What to record in Metric

Related issue

@Xuanwo
Copy link
Member

Xuanwo commented Aug 30, 2022

My code format was a mess so I ran a complete format...

rustfmt confirmed that we get the same format (you will find the check didn't pass)

You can try this way:

  • Uncomment the rustfmt.toml, run cargo +nightly fmt
  • Comment the rustfmt.toml back, run cargo fmt

@ClSlaid
Copy link
Contributor Author

ClSlaid commented Aug 30, 2022

rustfmt confirmed that we get the same format (you will find the check didn't pass)

By "complete format" I mean uncommeting all lines in rustfmt.toml and running cargo fmt on nightly channel. Because I was too lazy to take a hundred imported modules out from curly brackets and sort them manually, and rustfmt will do nothing to imports by default.

Sadly, the default rustfmt style is not the same as databend. I should have checked before pushing. :(

@Xuanwo
Copy link
Member

Xuanwo commented Aug 30, 2022

Sadly, the default rustfmt style is not the same as databend.

Because OpenDAL should be run on stable rust, we can't use the same rustfmt.toml as databend.

I should have checked before pushing. :(

Don't worry. CI is always here until you make them happy.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 30, 2022

I should have checked before pushing. :(

Some more comments about statements like those

Don't worry about revokable errors: Everything is OK. It's just a simple mistake and is detected by CI. No one should be blamed here, especially in open-source projects like OpenDAL. I don't care about any mistakes that could be detected by CI, like styling and formatting.

But I do care:

  • The time and effort that you put into this PR.
  • The code quality you represent includes naming, comments, API design, and so on.

Let's focus on the essential part and let our CI worry about those slight mistakes.

@ClSlaid ClSlaid marked this pull request as ready for review August 31, 2022 01:27
1. typo fixes and remove unnecessary comment
2. changed HDFS's DirStream

Signed-off-by: ClSlaid <[email protected]>
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.

LGTM now!

Thanks for your patience~

@Xuanwo Xuanwo merged commit e7e49ad into apache:main Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: replace redundant log macros with logging layer Tracking Issues of OpenDAL Observability
2 participants