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.

Sign up for GitHub

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

Jump to bottom

Use Config File to Drive Components Log Configuration #1851

Merged
merged 46 commits into from
Sep 7, 2022
Merged

Use Config File to Drive Components Log Configuration #1851

merged 46 commits into from
Sep 7, 2022

Conversation

tyshko5
Copy link
Contributor

@tyshko5 tyshko5 commented Aug 23, 2022

Summary of changes
Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes #1520

Other information and links

@tyshko5 tyshko5 marked this pull request as ready for review August 24, 2022 07:50
lemmih
lemmih requested changes Aug 24, 2022
View reviewed changes
Copy link
Contributor

@lemmih lemmih left a comment

Choose a reason for hiding this comment

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

I think there's some confusion here. The --log-file flag is for directing log messages to a file rather than stdout. It shouldn't read a log configuration from that file.

The LogConfig should be part of the regular configuration file. And those log options shouldn't be an alternative to the default options, they should just be applied after the default options have been set.

@tyshko5
Copy link
Contributor Author

tyshko5 commented Aug 24, 2022

I think there's some confusion here. The --log-file flag is for directing log messages to a file rather than stdout. It shouldn't read a log configuration from that file.

The LogConfig should be part of the regular configuration file. And those log options shouldn't be an alternative to the default options, they should just be applied after the default options have been set.

Second part is fixed, but the logger we are using, pretty_env_logger, doesn't have a feature to save logs to a file. Should we change the logger?

@tyshko5 tyshko5 requested a review from lemmih August 24, 2022 11:09
lemmih
lemmih requested changes Aug 24, 2022
View reviewed changes
Copy link
Contributor

@lemmih lemmih left a comment

Choose a reason for hiding this comment

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

Getting closer. Still need the --log-file flag (or a comment saying why it isn't possible).

forest/src/logger/mod.rs Outdated Show resolved Hide resolved
forest/src/main.rs Outdated Show resolved Hide resolved
forest/src/main.rs Show resolved Hide resolved
@tyshko5
Copy link
Contributor Author

tyshko5 commented Aug 24, 2022

Getting closer. Still need the --log-file flag (or a comment saying why it isn't possible).

the logger we are using, pretty_env_logger, doesn't have a feature to save logs to a file. Should we change the logger?

@tyshko5 tyshko5 requested a review from lemmih August 24, 2022 11:57
@lemmih
Copy link
Contributor

lemmih commented Aug 24, 2022

Getting closer. Still need the --log-file flag (or a comment saying why it isn't possible).

the logger we are using, pretty_env_logger, doesn't have a feature to save logs to a file. Should we change the logger?

pretty_env_logger is layered on top of env_logger. You can get the Builder for env_logger and then change the target.

@tyshko5
Copy link
Contributor Author

tyshko5 commented Aug 24, 2022

Getting closer. Still need the --log-file flag (or a comment saying why it isn't possible).

the logger we are using, pretty_env_logger, doesn't have a feature to save logs to a file. Should we change the logger?

pretty_env_logger is layered on top of env_logger. You can get the Builder for env_logger and then change the target.

The only targets that can be used by env_logger are stdout and stderr

@lemmih
Copy link
Contributor

lemmih commented Aug 24, 2022

Getting closer. Still need the --log-file flag (or a comment saying why it isn't possible).

the logger we are using, pretty_env_logger, doesn't have a feature to save logs to a file. Should we change the logger?

pretty_env_logger is layered on top of env_logger. You can get the Builder for env_logger and then change the target.

The only targets that can be used by env_logger are stdout and stderr

Ah, the Pipe option was added at a later time. Add an entry for --log-file to the configuration and comment it out. Then write a comment describing why it cannot yet be implemented (and link to seanmonstar/pretty-env-logger#52).

lemmih
lemmih reviewed Aug 24, 2022
View reviewed changes
forest/src/cli/config.rs Outdated Show resolved Hide resolved
lemmih
lemmih reviewed Aug 24, 2022
View reviewed changes
forest/src/cli/mod.rs Outdated Show resolved Hide resolved
lemmih
lemmih reviewed Aug 24, 2022
View reviewed changes
forest/src/cli/mod.rs Outdated Show resolved Hide resolved
lemmih
lemmih reviewed Aug 24, 2022
View reviewed changes
forest/src/cli/mod.rs Outdated Show resolved Hide resolved
@tyshko5 tyshko5 requested a review from lemmih August 24, 2022 14:38
tyshko5 and others added 18 commits September 7, 2022 10:32
@tyshko5
Use constant data instead of lazy_static in forest_networks (#1876)
1e3e87d
@elmattic @lemmih @tyshko5
Daemonize Forest node (#1856)
3ce45da 
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
@tyshko5
fix after merge
06bc6f6
@connormullett @tyshko5
Add --halt-after-import flag to CLI (#1835)
5c616fe 
* add flag to cliopts

* and options to config

* move exit logig to sync_from_snapshot

* rename to match lotus cli

* changes for testing; move halt to after import chain

* verify that rocksdb is dropped

* requested changes and remove testing logs

* move comment

* default rocks log level debug -> warn

* fix workflow errors
@connormullett @tyshko5
Enable spellcheck for ipld/ (#1822)
582d488
@jdjaustin @tyshko5
spellcheck changes for vm/interpreter (#1844)
144cf7f
@jdjaustin @tyshko5
Implement cargo spellcheck for vm/actor_interface (#1843)
fb7ee95
@elmattic @lemmih @tyshko5
Cleanly shutdown all tasks and make sure RocksDB is closed properly (#…
52bccb8 
…1816)

Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
@lemmih @tyshko5
Run cargo spellcheck over every internal crate (#1853)
17b8fe7
@tyshko5 @LesnyRumcajs
Improve the naming of exported snapshots. (#1771)
4373808 
Co-authored-by: Hubert <hubert@chainsafe.io>
@aakoshh @tyshko5
Change Proposer to return a vector of tasks. (#1865)
47f0298
@connormullett @lemmih @tyshko5
Start RPC server before loading snapshot (#1831)
b16917d 
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
@tyshko5
Validate the ChainConfig struct at construction time (#1845)
6501d51 
* most changes

* requested change

* fix fmt

* new ways: delete claus, version_schedule, sort

* fix lint

* requested changes

* compile time check

* from implementation

* requested changes

* requested changes

* requested changes

* requested changes

* requested changes

* delete Claus from deleg cns config
@tyshko5
Explicitly import the lazy_static macro. (#1872)
54187d9
@tyshko5
Use constant data instead of lazy_static in forest_networks (#1876)
1b7dcfe
@elmattic @lemmih @tyshko5
Daemonize Forest node (#1856)
2632809 
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
@tyshko5
Merge branch 'main' into tyshko5/issue-1520
ce2d57b
@tyshko5
Merge branch 'main' into tyshko5/issue-1520
f2def15
@lemmih lemmih disabled auto-merge September 7, 2022 08:55
lemmih
lemmih requested changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@lemmih lemmih left a comment

Choose a reason for hiding this comment

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

Something definitely went wrong while resolving conflicts.

@lemmih
Copy link
Contributor

lemmih commented Sep 7, 2022

@tyshko5 The dictionary is still wrong. Look at the changes to see why.

@tyshko5 tyshko5 requested a review from lemmih September 7, 2022 09:34
lemmih
lemmih requested changes Sep 7, 2022
View reviewed changes
.config/forest.dic Outdated Show resolved Hide resolved
@tyshko5 tyshko5 requested a review from lemmih September 7, 2022 09:58
@lemmih lemmih merged commit 0198492 into main Sep 7, 2022
@lemmih lemmih deleted the tyshko5/issue-1520 branch September 7, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LesnyRumcajs LesnyRumcajs LesnyRumcajs left review comments

@lemmih lemmih lemmih approved these changes

@connormullett connormullett Awaiting requested review from connormullett

@jdjaustin jdjaustin Awaiting requested review from jdjaustin

@elmattic elmattic Awaiting requested review from elmattic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use Config File to Drive Components Log Configuration
8 participants
@tyshko5 @lemmih @LesnyRumcajs @elmattic @connormullett @jdjaustin @jimluo @aakoshh