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

Remove .with_current_subscriber() calls #992

Merged
merged 3 commits into from May 14, 2024

Conversation

Lorak-mmk
Copy link
Collaborator

@Lorak-mmk Lorak-mmk commented May 2, 2024

Those calls were introduced in e557095,
with message:

Now, when the driver spawns a task to run a new future on it, that
future will use the same subscriber as the code that spawned the task in
the first place.

There is unfortunately no explanation about when it is necessary.
I don't see any problems after removing those - logs are still collected correctly using a tracing subscriber.
Those calls however have a harmful side-effect: they prevent usage of log loggers to listen to driver logs using log feature in tracing crate, which was reported in #860 .
After reporting the problem to tracing crate (tokio-rs/tracing#2913) one of maintainers said that using .with_current_subscriber() in a library is not necessary in general.

As I don't see any issue caused by removing these calls, but their existence cause an issue, they are removed in this PR.
The PR also introduces an example showing usage of the driver with env_logger and adjusts documentation about logging.

Fixes: #860

@piodul if you remember why those calls were added please explain, because I have no idea in which scenario they could be necessary.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@Lorak-mmk Lorak-mmk requested review from piodul and wprzytula May 2, 2024 15:25
@Lorak-mmk Lorak-mmk self-assigned this May 2, 2024
@Lorak-mmk Lorak-mmk mentioned this pull request May 2, 2024
8 tasks
Copy link

github-actions bot commented May 2, 2024

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 370b00a

@piodul
Copy link
Collaborator

piodul commented May 2, 2024

@piodul if you remember why those calls were added please explain, because I have no idea in which scenario they could be necessary.

I'm not entirely sure. I might have discussed the original problem long ago with Gor and/or @wprzytula. I think it might have been necessary for the cpp-rust-wrapper.

One use case that comes to my mind is:

  1. You spawn a task with a subscriber which is different than the global one,
  2. You create a session in that task,
  3. For me, it sounds reasonable that the tasks spawned internally by the session should inherit the tracing subscriber of the task that the session was created in. Without .with_current_subscriber(), that wouldn't be the case.

@piodul
Copy link
Collaborator

piodul commented May 2, 2024

Might be related to this: scylladb/cpp-rust-driver#110

@wprzytula
Copy link
Collaborator

Might be related to this: scylladb/cpp-rust-driver#110

Indeed, this cpp-rust-driver's logging might malfunction after those calls are removed. @Lorak-mmk please build the cpp-rust-driver against custom rust driver with those calls removed and check that logging still works.

@wprzytula wprzytula added the waiting-on-author Waiting for a response from issue/PR author label May 6, 2024
@Lorak-mmk
Copy link
Collaborator Author

If I understand correctly this is not in issue when global default subscriber is used, because it is a default for all threads. This is what cpp-rust-driver currently does.
To make sure, I've partially updated cpp-rust-driver to use current rust driver and run examples/basic (with added cass_log_set_level(CASS_LOG_TRACE) at the beggining of main).
Logs are properly printed both with main and this PR.

@wprzytula wprzytula removed the waiting-on-author Waiting for a response from issue/PR author label May 14, 2024
Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

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

Cargo.lock.msrv needs to be updated. Otherwise, looks acceptable to me, although I'm still reluctant to believe that cpp-rust-driver's logging really works without those calls; they really had some purpose and I feel that there is some trap that we forget about.

examples/logging_log.rs Outdated Show resolved Hide resolved
examples/logging_log.rs Outdated Show resolved Hide resolved
@Lorak-mmk
Copy link
Collaborator Author

Cargo.lock.msrv needs to be updated. Otherwise, looks acceptable to me, although I'm still reluctant to believe that cpp-rust-driver's logging really works without those calls; they really had some purpose and I feel that there is some trap that we forget about.

If there is something that I didn't notice I'd prefer to work around it in cpp-rust-driver (even if it's inconvenient or even requires a patched version of the driver) rather than break the experience for Rust Driver users.

@Lorak-mmk Lorak-mmk force-pushed the remove_with_current_subscriber branch from 20c8002 to dd457c7 Compare May 14, 2024 09:58
Those calls were introduced in e557095,
with message:
```
Now, when the driver spawns a task to run a new future on it, that
future will use the same subscriber as the code that spawned the task in
the first place.
```

There is unfortunately no explanation about when it is necessary.
I don't see any problems after removing those - logs are still collected
correctly using a tracing subscriber.
Those calls however have a harmful side-effect: they prevent usage of
`log` loggers to listen to driver logs using `log` feature in `tracing`
crate. After reporting the problem to `tracing` crate:
tokio-rs/tracing#2913
one of maintainers said that using `.with_current_subscriber()` in
a library is not necessary in general.

As I don't see any issue caused by removing these calls, but their
existence cause an issue, they are removed in this commit.
This example demonstrates usage of `env_logger` to listen to driver logs.
@Lorak-mmk Lorak-mmk force-pushed the remove_with_current_subscriber branch from dd457c7 to 2199bda Compare May 14, 2024 10:01
@Lorak-mmk Lorak-mmk requested a review from wprzytula May 14, 2024 10:08
@Lorak-mmk Lorak-mmk force-pushed the remove_with_current_subscriber branch from 2199bda to 370b00a Compare May 14, 2024 11:30
Comment on lines +59 to +64
You can use `cargo add tracing -F log` or edit `Cargo.toml`:
```toml
tracing = { version = "0.1.40" , features = ["log"] }
```
then you can setup `env_logger` os some other logger and it will output logs from the driver:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will enabling log feature in the end crate's Cargo.toml suffice, or is it required to have it enabled for scylla crate as well?
Another question. What if the end user crate uses tracing crate in version that is incompatible with the version that scylla crate uses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Yes, it is enough
  2. Not sure what you are asking. Are you asking if adding hypotetical tracing 0.2 to end user Cargo.toml and enabling log feature there will enable log feature in our tracing 0.1? No, it won't

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Quoting comment from the tracing issue I opened (tokio-rs/tracing#2913 (comment) ):

A library should not generally enable the log or log-always feature flags, and generally should never be responsible for setting the default subscriber. Instead, user code that depends on the library should enable tracing's log crate support if it wants to, and set the default subscriber if it wishes to use tracing. I don't think the calls to with_current_subscriber are necessary in a library in general.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. I'm asking about whether mismatched versions of tracing in scylla crate and the end crate can cause incompatility issues, resulting in misbehaving tracing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No idea, probably. It's a question to tracing crate maintainers, how are they going to handle new major version to avoid ecosystem split.

@wprzytula wprzytula merged commit 88bc8c1 into scylladb:main May 14, 2024
13 checks passed
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.

Logging in log is bugged, but tracing works
3 participants