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

Replace env_logger with tracing #2381

Merged
merged 6 commits into from
Aug 15, 2023

Conversation

ffuugoo
Copy link
Contributor

@ffuugoo ffuugoo commented Aug 3, 2023

This PR replaces env_logger with tracing/tracing-subscriber/tracing-log as our log backend (and also removes half of the custom features added in #2187).

(Currently based on top of #2187. Would be rebased onto dev once #2187 is merged.)

TODO:

  • Fix DEVELOPMENT.md documentation after removing custom tracing-related features from qdrant crate
  • Make tracing required dependency of lib/* crates and remove custom tracing feature everywhere?
    • There's practically no downside to leaving tracing optional in lib/* crates, so I crossed it out.

Further Improvements:

All Submissions:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
  3. Have you checked your code using cargo clippy --all --all-features command?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

TODO:
- Implement slog drain that would *properly* convert slog records into tracing events
  (`slog-stdlog` is fine for now, though)
@ffuugoo ffuugoo requested a review from generall August 3, 2023 16:32
ffuugoo added 4 commits August 3, 2023 18:52
…`/`tracing-log`

Forget to `unwrap` the result 🙈
…`/`tracing-log`

`tracing_subscriber::fmt::init` initializes `tracing_log::LogTracer`
automatically with default `tracing-subscriber` features enabled 💁‍♀️
Copy link
Member

@timvisee timvisee left a comment

Choose a reason for hiding this comment

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

Nice work!

In development mode it is a lot more noisy now. Can we disable logging on some modules by default to reduce it a bit? Or do you think keeping these is more practical?

Before:

image

After:

image

src/tracing.rs Show resolved Hide resolved
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Aug 9, 2023

In development mode it is a lot more noisy now. Can we disable logging on some modules by default to reduce it a bit? Or do you think keeping these is more practical?

Some crates are using tracing instead of log, so we are suddenly seeing these logs now. I agree that it makes sense to disable some of the more verbose logs. I personally use debug,hyper=info,h2=info,tower=info,rustls=info.

@timvisee
Copy link
Member

I personally use debug,hyper=info,h2=info,tower=info,rustls=info.

Yeah, that seems more reasonable.

Maybe we can set that as default here. What do you think?

@generall generall merged commit 78da281 into tracing-improvements Aug 15, 2023
generall pushed a commit that referenced this pull request Aug 15, 2023
* Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

TODO:
- Implement slog drain that would *properly* convert slog records into tracing events
  (`slog-stdlog` is fine for now, though)

* fixup! Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

Fix `consensus` test

* fixup! fixup! Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

Forget to `unwrap` the result 🙈

* fixup! fixup! Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

`tracing_subscriber::fmt::init` initializes `tracing_log::LogTracer`
automatically with default `tracing-subscriber` features enabled 💁‍♀️

* Update `DEVELOPMENT.md` documentation regarding `tracing`

* Update default log level for development to make it less noisy

---------

Co-authored-by: timvisee <tim@visee.me>
@ffuugoo ffuugoo deleted the replace-env_logger-with-tracing branch August 15, 2023 12:35
generall pushed a commit that referenced this pull request Aug 15, 2023
* Add optional `tracing` crate dependency to all `lib/*` sub-crates

* Add optional alternative "tracing-logger"...

...that works a bit better with logs produced by the `tracing` crate

* Make `tracing-logger` reuse `log_level` config parameter (and `QDRANT__LOG_LEVEL` env-var)...

...instead of default `RUST_LOG` env-var only

* Replace `env_logger` with `tracing` (#2381)

* Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

TODO:
- Implement slog drain that would *properly* convert slog records into tracing events
  (`slog-stdlog` is fine for now, though)

* fixup! Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

Fix `consensus` test

* fixup! fixup! Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

Forget to `unwrap` the result 🙈

* fixup! fixup! Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

`tracing_subscriber::fmt::init` initializes `tracing_log::LogTracer`
automatically with default `tracing-subscriber` features enabled 💁‍♀️

* Update `DEVELOPMENT.md` documentation regarding `tracing`

* Update default log level for development to make it less noisy

---------

Co-authored-by: timvisee <tim@visee.me>

* Implement "intelligent" default log-level filtering

---------

Co-authored-by: timvisee <tim@visee.me>
generall pushed a commit that referenced this pull request Sep 4, 2023
* Add optional `tracing` crate dependency to all `lib/*` sub-crates

* Add optional alternative "tracing-logger"...

...that works a bit better with logs produced by the `tracing` crate

* Make `tracing-logger` reuse `log_level` config parameter (and `QDRANT__LOG_LEVEL` env-var)...

...instead of default `RUST_LOG` env-var only

* Replace `env_logger` with `tracing` (#2381)

* Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

TODO:
- Implement slog drain that would *properly* convert slog records into tracing events
  (`slog-stdlog` is fine for now, though)

* fixup! Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

Fix `consensus` test

* fixup! fixup! Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

Forget to `unwrap` the result 🙈

* fixup! fixup! Replace `env_logger` with `tracing`/`tracing-subscriber`/`tracing-log`

`tracing_subscriber::fmt::init` initializes `tracing_log::LogTracer`
automatically with default `tracing-subscriber` features enabled 💁‍♀️

* Update `DEVELOPMENT.md` documentation regarding `tracing`

* Update default log level for development to make it less noisy

---------

Co-authored-by: timvisee <tim@visee.me>

* Implement "intelligent" default log-level filtering

---------

Co-authored-by: timvisee <tim@visee.me>
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.

3 participants