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

feat: replace log with tracing and use tracing::instrument where useful #316

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

simonsan
Copy link
Contributor

@simonsan simonsan commented Oct 5, 2024

Fixes #315

TODO:

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@simonsan simonsan added A-errors Area: error handling needs improvement C-enhancement Category: New feature or request A-dx Area: Related to Developer Experience labels Oct 5, 2024
Copy link

codecov bot commented Oct 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.4%. Comparing base (f68ffa3) to head (c9e8292).

Additional details and impacted files
Files with missing lines Coverage Δ
crates/backend/src/local.rs 0.0% <ø> (ø)
crates/backend/src/opendal.rs 90.6% <ø> (ø)
crates/backend/src/rclone.rs 9.7% <ø> (-0.3%) ⬇️
crates/backend/src/rest.rs 0.0% <ø> (ø)
crates/core/src/archiver.rs 64.7% <ø> (-1.5%) ⬇️
crates/core/src/archiver/parent.rs 65.2% <ø> (-1.5%) ⬇️
crates/core/src/archiver/tree_archiver.rs 73.2% <ø> (ø)
crates/core/src/backend.rs 57.5% <ø> (ø)
crates/core/src/backend/cache.rs 70.3% <ø> (-4.9%) ⬇️
crates/core/src/backend/ignore.rs 45.5% <ø> (-1.6%) ⬇️
... and 17 more

... and 13 files with indirect coverage changes

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@aawsome
Copy link
Member

aawsome commented Oct 6, 2024

While I agree that we might want to migrate from log to tracing, I actually would do so when we have decided for the architecture we are using to run rustic in some optimal sync/async/parallelized way. Then, it will be a first implementation step to go into this direction.

@simonsan
Copy link
Contributor Author

simonsan commented Oct 6, 2024

Hmm, I don't understand, why would that be related to an architectural decision? tracing is essential in async/parallelized contexts, for sure. But we can already profit a lot from it by just instrumenting some essential functions/methods so we get more context in our logging. Do you see any downside there? 🤔

@aawsome
Copy link
Member

aawsome commented Oct 6, 2024

Hmm, I don't understand, why would that be related to an architectural decision? tracing is essential in async/parallelized contexts, for sure. But we can already profit a lot from it by just instrumenting some essential functions/methods so we get more context in our logging. Do you see any downside there? 🤔

No downside. I just don't see any benefit we currently can get. That might change and tracing may help us finding bugs during a big refactor. But currently I just don't see the point in doing so. Better focus on topics like error handling..

@simonsan
Copy link
Contributor Author

simonsan commented Oct 6, 2024

(Sorry, I accidentally edited your comment instead of answering! 😅)

I collected all the advantages in here: #315

The benefits are structured logging, the tracing::instrument macro, and others. I think it's not a big effort, to be honest. It's just replacing log:: with tracing (log facade) and start to instrument certain methods and functions where we are often asking us what the input values to that functions were. For example, tree archiver. Everything where we already parallelize, for example to find race conditions/send errors:

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
- async
- threaded
- par_
- send on channels

Missing:
- create fn from closures that use above and instrument them for smaller scopes

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
@simonsan simonsan force-pushed the feat/tracing-logging branch from f7d1e0d to e95b61b Compare October 29, 2024 00:40
@simonsan
Copy link
Contributor Author

simonsan commented Oct 29, 2024

I started out to instrument the functions, that are related to:

  • async, e.g. WebDavFS and opendal backend
  • in general multithreading (TreeArchiver)
  • contain *_par_*/readahead* via rayon
  • send/recv data on channels

Follow up: I think what can be useful, is also to create functions from closures, where we spawn threads and instrument them, for smaller spans/scopes -- or we can just manually create the needed spans. (:

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dx Area: Related to Developer Experience A-errors Area: error handling needs improvement A-instrumentation Area: Related to tracing, instrumentation, metrics C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use tracing crate for logging
2 participants