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

Remove crate name from logs #4649

Open
ansermino opened this issue Aug 19, 2024 · 5 comments · May be fixed by #5123
Open

Remove crate name from logs #4649

ansermino opened this issue Aug 19, 2024 · 5 comments · May be fixed by #5123
Assignees

Comments

@ansermino
Copy link
Member

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.

@ansermino ansermino added the Priority: 4 - Low Limited impact and can be implemented at any time label Aug 19, 2024
@LesnyRumcajs
Copy link
Member

LesnyRumcajs commented Aug 19, 2024

@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
Copy link
Member Author

ansermino commented Aug 19, 2024

@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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

LesnyRumcajs commented Aug 20, 2024

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.

@ansermino ansermino moved this from New to Ready in Forest Backlog 🌲 Aug 23, 2024
@LesnyRumcajs LesnyRumcajs removed the Priority: 4 - Low Limited impact and can be implemented at any time label Sep 26, 2024
@hanabi1224 hanabi1224 linked a pull request Jan 10, 2025 that will close this issue
4 tasks
@hanabi1224 hanabi1224 self-assigned this Jan 10, 2025
@hanabi1224 hanabi1224 moved this from Ready to In review in Forest Backlog 🌲 Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In review
Development

Successfully merging a pull request may close this issue.

3 participants