-
Notifications
You must be signed in to change notification settings - Fork 110
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
Docs: Document log
limitations
#921
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,4 +45,71 @@ To start this example execute: | |
RUST_LOG=info cargo run | ||
``` | ||
|
||
The full [example](https://github.com/scylladb/scylla-rust-driver/tree/main/examples/logging.rs) is available in the `examples` folder | ||
The full [example](https://github.com/scylladb/scylla-rust-driver/tree/main/examples/logging.rs) is available in the `examples` folder | ||
|
||
## 'log' compatibility | ||
|
||
It may be surprising for some that viewing drivers logs using `log` ecosystem doesn't work out of the box despite `tracing` having a compatiblity layer | ||
behind `log` / `log-always` feature flags. | ||
|
||
The problem is that this compatibility using `log` feature (which is recommended for libraries) seems to not work well with `.with_current_subscriber()` / Tokio tasks. | ||
For example, for the following program: | ||
```rust | ||
# extern crate env_logger; | ||
# extern crate log; | ||
# extern crate tokio; | ||
# extern crate tracing; | ||
// main.rs | ||
|
||
use tracing::instrument::WithSubscriber; | ||
|
||
#[tokio::main] | ||
async fn main() { | ||
env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); | ||
|
||
log::info!("info log"); | ||
tracing::info!("info tracing"); | ||
|
||
tokio::spawn( | ||
async move { | ||
log::info!("spawned info log"); | ||
tracing::info!("spawned info tracing") | ||
} | ||
.with_current_subscriber(), | ||
) | ||
.await | ||
.unwrap(); | ||
|
||
log::info!("another info log"); | ||
tracing::info!("another info tracing"); | ||
} | ||
|
||
``` | ||
|
||
```toml | ||
# Cargo.toml | ||
|
||
[package] | ||
name = "reproducer" | ||
version = "1.0.0" | ||
edition = "2021" | ||
|
||
[dependencies] | ||
env_logger = "0.10" | ||
log = "0.4" | ||
tracing = { version = "0.1.36", features = [ "log" ] } | ||
tokio = { version = "1.27", features = [ "full" ] } | ||
``` | ||
|
||
the output is: | ||
```bash | ||
[2024-01-26T00:51:06Z INFO reproducer] info log | ||
[2024-01-26T00:51:06Z INFO reproducer] info tracing | ||
[2024-01-26T00:51:06Z INFO reproducer] spawned info log | ||
[2024-01-26T00:51:06Z INFO reproducer] another info log | ||
``` | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A short explanation why we don't want this feature enabled might be appreciated by the audience. |
||
The other feature, `log-always`, works with the driver - but is not something we want to enable in this library. | ||
We recommend using tracing ecosystem, but if for some reason you need to stick with `log`, you can try | ||
enabling `log-always` feature in `tracing` by adding it to your direct dependencies (`tracing = { version = "0.1", features = [ "log-always" ] }`). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you confirmed that this indeed works @Lorak-mmk?
Let's not introduce any needless uncertainty (even though in software engineering it might come out as a suitable attitude...). |
||
This should enable driver log collection via `log` loggers. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ uuid = "1.0" | |
tower = "0.4" | ||
stats_alloc = "0.1" | ||
clap = { version = "3.2.4", features = ["derive"] } | ||
env_logger = "0.10" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the future clarity, let's add a comment saying that this dependency is currently used only by the Book (as one might wonder why it's put there, having checked that Or, even better, I suggest visually extracting a section in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a higher perspective, this possible confusion arises because the Book is actually a secondary project that is configured by the same |
||
|
||
[[example]] | ||
name = "auth" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an open GitHub issue about this in the
tracing
project? This sounds like a legitimate issue with the compatibility layer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find any when I was searching some time ago, I should open one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to know more about this before we merge this documentation page. It feels wrong to document workarounds for bugs in other projects, especially if they can just be fixed by the other project.
Maybe it is a documented limitation? If so, then perhaps we can just point to that place instead of having to maintain our own description of the workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll research it more, and not include this PR in 0.12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, in case that we don't manage to investigate this further before the next release, we should probably merge this to aid confused users, even if that's
log
's fault that could be fixed soon.