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

Docs: Document log limitations #921

Closed
wants to merge 1 commit into from

Conversation

Lorak-mmk
Copy link
Collaborator

This PR describes reasons why driver doesn't work with log out of the box and how the user can fix this.

Fixes: #860

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 a review from piodul January 26, 2024 01:15
@Lorak-mmk Lorak-mmk self-assigned this Jan 26, 2024
@Lorak-mmk Lorak-mmk force-pushed the document_logging branch 5 times, most recently from 6cc298f to e52e480 Compare February 2, 2024 00:43
@Lorak-mmk
Copy link
Collaborator Author

@piodul I have no idea what is going on with the book job

  • I added env_logger to dev dependencies so why is it complaining that it can't find it?
  • What does the first error even mean, why is it trying to parse the output of this code snippet?

@piodul
Copy link
Collaborator

piodul commented Feb 2, 2024

  • I added env_logger to dev dependencies so why is it complaining that it can't find it?

The CI job takes the dependencies from the examples/Cargo.toml file, not scylla/Cargo.toml (mdbook test -L target/debug/deps docs).

  • What does the first error even mean, why is it trying to parse the output of this code snippet?

I assume it's #926

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.
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

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.

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.

@Lorak-mmk Lorak-mmk force-pushed the document_logging branch 3 times, most recently from ecd5992 to a8bf067 Compare February 2, 2024 13:24
@Lorak-mmk
Copy link
Collaborator Author

Still fails with the weird stdout error after rebasing on main

@Lorak-mmk Lorak-mmk force-pushed the document_logging branch 2 times, most recently from 963ad6d to ff195c0 Compare February 2, 2024 13:48
@Lorak-mmk
Copy link
Collaborator Author

Fixed, it didn't parse my stdout. It interprets code blocks without language specified as Rust and tries to run them.
Obviously block with some logs didn't parse, hence the error. I'm so stupid.

@piodul
Copy link
Collaborator

piodul commented Feb 2, 2024

Fixed, it didn't parse my stdout. It interprets code blocks without language specified as Rust and tries to run them. Obviously block with some logs didn't parse, hence the error. I'm so stupid.

Happens to the best of us.

The full [example](https://github.com/scylladb/scylla-rust-driver/tree/main/examples/logging.rs) is available in the `examples` folder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quite a side topic, but I couldn't resist investigating it.

This usage of folder instead of the usual Unixy directory made me wonder about the very difference between these two terms, and these are some opinions:

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.
Copy link
Collaborator

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.

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.

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" ] }`).
This should enable driver log collection via `log` loggers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you confirmed that this indeed works @Lorak-mmk?
If so, I suggest phrasing this as:

This will enable ...

Let's not introduce any needless uncertainty (even though in software engineering it might come out as a suitable attitude...).

```

The other feature, `log-always`, works with the driver - but is not something we want to enable in this library.
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

@@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 cargo check --all-targets succeeds even with that dependency removed).

Or, even better, I suggest visually extracting a section in examples/Cargo.toml that lists Book-only deps.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 Cargo.toml as the primary one (examples set of crates).

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

I opened an issue in tracing: tokio-rs/tracing#2913

@Lorak-mmk Lorak-mmk removed the waiting-on-author Waiting for a response from issue/PR author label Mar 25, 2024
@wprzytula wprzytula added waiting-on-author Waiting for a response from issue/PR author area/documentation Improvements or additions to documentation labels Mar 27, 2024
@Lorak-mmk
Copy link
Collaborator Author

Succeeded by #992

@Lorak-mmk Lorak-mmk closed this May 2, 2024
@Lorak-mmk Lorak-mmk removed the waiting-on-author Waiting for a response from issue/PR author label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging in log is bugged, but tracing works
3 participants