Docs: Document `log` limitations #921

Lorak-mmk · 2024-01-26T01:15:34Z

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 · 2024-02-02T00:50:28Z

@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 · 2024-02-02T06:38:55Z

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

piodul · 2024-02-02T06:45:24Z

docs/source/logging/logging.md

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


Is there an open GitHub issue about this in the tracing project? This sounds like a legitimate issue with the compatibility layer.

I didn't find any when I was searching some time ago, I should open one

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.

Ok, I'll research it more, and not include this PR in 0.12

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 · 2024-02-02T13:37:55Z

Still fails with the weird stdout error after rebasing on main

Lorak-mmk · 2024-02-02T13:49:14Z

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 · 2024-02-02T13:58:48Z

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.

wprzytula · 2024-03-05T10:30:22Z

docs/source/logging/logging.md

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:

https://www.baeldung.com/cs/directories-vs-folders -> folder is a "leaf directory", i.e. directory without directories in it;

https://superuser.com/questions/187900/what-is-the-difference-between-a-directory-and-a-folder -> no strong difference; directory more of a file system concept, folder more of a GUI thing.

wprzytula · 2024-03-05T10:33:28Z

docs/source/logging/logging.md

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


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.

wprzytula · 2024-03-05T10:39:36Z

docs/source/logging/logging.md

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


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

wprzytula · 2024-03-05T10:40:35Z

docs/source/logging/logging.md

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


A short explanation why we don't want this feature enabled might be appreciated by the audience.

wprzytula · 2024-03-05T10:44:55Z

examples/Cargo.toml

@@ -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"


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.

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 · 2024-03-18T16:00:49Z

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

Lorak-mmk · 2024-05-02T15:26:46Z

Succeeded by #992

Lorak-mmk requested a review from piodul January 26, 2024 01:15

Lorak-mmk self-assigned this Jan 26, 2024

Lorak-mmk force-pushed the document_logging branch 5 times, most recently from 6cc298f to e52e480 Compare February 2, 2024 00:43

piodul reviewed Feb 2, 2024

View reviewed changes

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

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

Docs: Document log limitations

3dba6c0

Lorak-mmk force-pushed the document_logging branch from ff195c0 to 3dba6c0 Compare February 2, 2024 13:56

wprzytula reviewed Mar 5, 2024

View reviewed changes

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

Lorak-mmk removed the waiting-on-author Waiting for a response from issue/PR author label Mar 25, 2024

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 closed this May 2, 2024

Lorak-mmk removed the waiting-on-author Waiting for a response from issue/PR author label May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docs: Document `log` limitations #921

Docs: Document `log` limitations #921

Lorak-mmk commented Jan 26, 2024

Lorak-mmk commented Feb 2, 2024

piodul commented Feb 2, 2024 •

edited

Loading

piodul Feb 2, 2024

Lorak-mmk Feb 2, 2024

piodul Feb 2, 2024

Lorak-mmk Feb 2, 2024

wprzytula Mar 5, 2024

Lorak-mmk commented Feb 2, 2024

Lorak-mmk commented Feb 2, 2024

piodul commented Feb 2, 2024

wprzytula Mar 5, 2024

wprzytula Mar 5, 2024

wprzytula Mar 5, 2024

wprzytula Mar 5, 2024

wprzytula Mar 5, 2024

wprzytula Mar 5, 2024

Lorak-mmk commented Mar 18, 2024

Lorak-mmk commented May 2, 2024

		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.

		```

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

Docs: Document log limitations #921

Docs: Document log limitations #921

Conversation

Lorak-mmk commented Jan 26, 2024

Pre-review checklist

Lorak-mmk commented Feb 2, 2024

piodul commented Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lorak-mmk commented Feb 2, 2024

Lorak-mmk commented Feb 2, 2024

piodul commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lorak-mmk commented Mar 18, 2024

Lorak-mmk commented May 2, 2024

Docs: Document `log` limitations #921

Docs: Document `log` limitations #921

piodul commented Feb 2, 2024 •

edited

Loading