search

ChainSafe / forest

🌲 Rust Filecoin Node Implementation
https://forest.chainsafe.io
Apache License 2.0
638 stars 160 forks source link

Remove crate name from logs #4649

Open ansermino opened 3 months ago

ansermino commented 3 months ago

Issue summary

Forest's logs are seemingly bloated with forest_filecoin.


2024-08-19T02:35:36.945364Z  INFO forest_filecoin::chain::store::chain_store: ...
2024-08-19T02:35:36.945419Z  INFO forest_filecoin::chain_sync::tipset_syncer: ...
2024-08-19T02:36:18.359253Z  INFO forest_filecoin::blocks::tipset: ...

This should be omitted to improve readability.

LesnyRumcajs commented 3 months ago

@ansermino I wouldn't say this should be removed. It's an important part of the path. While in a happy path, forest_filecoin is indeed just a noise, in case of errors or different logging settings, more modules might pop up with logs originating from libraries upon which Forest depends.

❯ RUST_LOG=debug forest --chain calibnet --encrypt-keystore=false | head
2024-08-19T08:31:30.331649Z  INFO forest_filecoin::daemon::main: Using default calibnet config
2024-08-19T08:31:30.335780Z  INFO forest_filecoin::daemon: Starting Forest daemon, version 0.19.2+git.76266421b1e
2024-08-19T08:31:30.335797Z DEBUG forest_filecoin::daemon: Increased file descriptor limit from 1024 to 8192
2024-08-19T08:31:30.335859Z DEBUG forest_filecoin::libp2p::keypair: Recovered libp2p keypair from /home/rumcajs/.local/share/forest/libp2p/keypair
2024-08-19T08:31:30.335866Z  WARN forest_filecoin::daemon: Forest has encryption disabled
2024-08-19T08:31:30.335922Z  INFO forest_filecoin::daemon: Admin token: eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJBbGxvdyI6WyJyZWFkIiwid3JpdGUiLCJzaWduIiwiYWRtaW4iXSwiZXhwIjo0ODc3NjU2MjkwfQ.8noM7_yNYD0VMhLgRvbAt1aXSi0CuSFtlb0AuXQG_6Y
2024-08-19T08:31:30.335955Z  INFO forest_filecoin::db::migration::db_migration: No database migration required
2024-08-19T08:31:30.336493Z DEBUG parity-db: Opened value table t00-00 with 15 entries, entry_size=32, removed=0
2024-08-19T08:31:30.336507Z DEBUG parity-db: Opened value table t00-01 with 3 entries, entry_size=33, removed=0
2024-08-19T08:31:30.336520Z DEBUG parity-db: Opened value table t00-02 with 2 entries, entry_size=34, removed=0

That said, it might make sense to shorten it to just forest, if anything, just to save on log retention costs. What do you think?

ansermino commented 3 months ago

@LesnyRumcajs Whats the utility of these paths in the logs? What purpose (and who) do they serve?

Shortening to forest definitely makes sense. However, I'm still convinced it can be omitted entirely. If dependencies have full paths it should stil be trivial to infer the full path for forest modules

LesnyRumcajs commented 3 months ago

It helps with parsing post-factum the logs. Say, you'd like to get logs from the p2p layer of Forest (but not the nitty-gritty details from rust-libp2p) - you could do ... | grep forest | grep p2p. If we omit this, it would be ambiguous.

2024-08-19T14:14:09.354040Z DEBUG Swarm::poll: libp2p_gossipsub::behaviour: Completed message handling for message c75cb23c229e145b869ab5e485f3d7d0e02c48bfc7432ed862c43eddbbbd5982
2024-08-19T14:14:09.720972Z DEBUG forest_filecoin::libp2p::peer_manager: logging global success

You could do grep -v p2p_, but in general, I'd keep the module. It's also informative for developers to immediately see if a particular log message is coming from Forest or one of its dependencies.

ansermino commented 3 months ago

In your example it seems there would still be a clear distinction between the modules, it just requires the right module name(s) to be queried (eg. libp2p_gossipsub).

This also touches on a philosophical question—who are the logs for? Forest devs or users? It's presumably a bit of both, but it does seem sensible to optimize for the majority (users) in most cases. We might consider hiding the full path behind a config option, or adding more stack traces to error logs.

Replacing filecoin-forest with forest seems like a tangible improvement. Lets just do that for now, and revisit this as needed.

LesnyRumcajs commented 3 months ago

On the philosophical question - logs of INFO and above levels are for devs AND users, anything below - strictly for devs. In any case, I'd make drastic changes if there's a strict requirement from node providers (it may also include even further shortening the logs for data retention savings, e.g., DEBUG - D, INFO - I...).

I'm for shortening to forest, given that filecoin- doesn't bring any supplementary information.