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

85: fix logs #190

Merged
merged 5 commits into from
Oct 29, 2024
Merged

85: fix logs #190

merged 5 commits into from
Oct 29, 2024

Conversation

zupzup
Copy link
Collaborator

@zupzup zupzup commented Oct 25, 2024

Draft until #188 is merged

This fixes #85.

All instances of println were replaced with variants of log::info!/warn!/error! and I updated the docs, so the app is ran with RUST_LOG set to the desired log level.

I removed the logs from the web handlers, since their being called is logged anyway using rocket, if RUST_LOG is set to info.

I didn't add any additional logging for now, since dht is well instrumented and the web handlers are as well via rocket. Once we fully implement Result-based error handling, we'll add logs for error-cases anyway.

For critical parts, we can also add trace, or debug logs, as soon as we run into issues.

@zupzup zupzup requested review from tompro, upoi and mtbitcr October 25, 2024 14:01
@zupzup zupzup self-assigned this Oct 25, 2024
@zupzup zupzup linked an issue Oct 25, 2024 that may be closed by this pull request
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 10.73171% with 183 lines in your changes missing coverage. Please review.

Project coverage is 4.09%. Comparing base (072752c) to head (08fa92a).

Files with missing lines Patch % Lines
src/dht/client.rs 0.00% 51 Missing ⚠️
src/external/bitcoin.rs 0.00% 27 Missing ⚠️
src/dht/event_loop.rs 0.00% 26 Missing ⚠️
src/blockchain/block.rs 0.00% 21 Missing ⚠️
src/web/handlers/bill.rs 0.00% 15 Missing ⚠️
src/bill/mod.rs 0.00% 14 Missing ⚠️
src/dht/mod.rs 0.00% 13 Missing ⚠️
src/blockchain/chain.rs 0.00% 12 Missing ⚠️
src/util/rsa.rs 0.00% 2 Missing ⚠️
src/external/mint.rs 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff            @@
##           master    #190      +/-   ##
=========================================
- Coverage    4.19%   4.09%   -0.10%     
=========================================
  Files          29      29              
  Lines        7175    6832     -343     
=========================================
- Hits          301     280      -21     
+ Misses       6874    6552     -322     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mtbitcr mtbitcr left a comment

Choose a reason for hiding this comment

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

LGFM

src/dht/client.rs Outdated Show resolved Hide resolved
@zupzup zupzup marked this pull request as ready for review October 29, 2024 08:42
@zupzup zupzup force-pushed the 85-fix-all-logs-make-it-better branch from 08fa92a to 3cbf985 Compare October 29, 2024 08:42
@zupzup zupzup merged commit b4c47ba into master Oct 29, 2024
2 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.

Fix all logs (make it better)
2 participants