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

Default to default logging sync=info #448

Merged
merged 10 commits into from Sep 1, 2017
Merged

Default to default logging sync=info #448

merged 10 commits into from Sep 1, 2017

Conversation

5chdn
Copy link
Contributor

@5chdn 5chdn commented Aug 29, 2017

Incldues:

  • Default to default logging sync=info.
  • Introduce --quiet flag to suppress that.
  • Improve help messages wording a little bit.
  • Updated README.md to reflect the changes.

Rationale:

  • It certainly makes sense to default to sync=info logging at some point. (Or to write a beautiful logger.)
  • Downside is that any use of RUST_LOG only works in combination with the -q switch to suppress the default logging.

@5chdn 5chdn added A0-pleasereview Pull request needs code review. M3-docs Documentation. M4-core Core client code / Rust. labels Aug 29, 2017
Copy link
Collaborator

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

Perfect technically, however I'm not sure if it is a good idea to disable RUST_LOG by default, in favor of defaulting sync=info. Afaik, RUST_LOG=trace works in parity along with informer (I can be wrong, but I'm pretty sure I used RUST_LOG to enable secretstore traces). So maybe we could do the same someday. I'll approve, but will leave it for merge to someone else.

@5chdn
Copy link
Contributor Author

5chdn commented Aug 30, 2017

Just tested, sync=info overrides RUST_LOG=trace and the latter only works with -q.

@svyatonik
Copy link
Collaborator

I've started parity with cl:

RUST_LOG=secretstore=trace,secretstore_net=trace ./parity --config kovan_ss1.toml

and with config:

[parity]
chain = "kovan"
base_path = "db.kovan_ss1"

[ui]
disable = true

[rpc]
disable = true

[ipc]
disable = true

[dapps]
disable = true

[network]
port = 30303

[secretstore]
disable = false
self_secret = "6c26a76e9b31048d170873a791401c7e799a11f0cefc0171cc31a49800967509"
nodes = ["cac6c205eb06c8308d65156ff6c862c62b000b8ead121a4455a8ddeff7248128d895692136f240d5d1614dc7cc4147b1bd584bd617e30560bb872064d09ea325@127.0.0.1:8085", "6b8d9b9ecde2f0d8c7b2685dd10d181000fa0b26462674dece3096488d6b8d6e3c0e4e1262e3f0eb3b997783b3d6471c281905c5fafeb7908f3aeb7326274db6@127.0.0.1:8087"]
interface = "local"
port = 8083
http_interface = "local"
http_port = 8082
path = "db.kovan_ss1/secretstore"

[ipfs]
enable = false

[snapshots]
disable_periodic = true

So there's no -q in cl. But I still can see secretstore' TRACE-level messages in output:

2017-08-30 09:35:46  IO Worker #3 INFO import  Syncing snapshot 12/31        #0    4/25 peers   8 KiB chain 7 KiB db 0 bytes queue 10 KiB sync  RPC:  0 conn,  0 req/s,   0 µs
2017-08-30 09:35:51  IO Worker #3 INFO import  Syncing snapshot 19/31        #0    4/25 peers   8 KiB chain 7 KiB db 0 bytes queue 10 KiB sync  RPC:  0 conn,  0 req/s,   0 µs
2017-08-30 09:35:56   TRACE secretstore_net  d013…1923: executing maintain procedures
2017-08-30 09:35:56   WARN secretstore_net  d013…1923: network error Connection refused (os error 111) when establishing outbound connection with 127.0.0.1:8085
2017-08-30 09:35:56   WARN secretstore_net  d013…1923: network error Connection refused (os error 111) when establishing outbound connection with 127.0.0.1:8087
2017-08-30 09:35:56  IO Worker #1 INFO import  Syncing snapshot 26/31        #0    4/25 peers   8 KiB chain 7 KiB db 0 bytes queue 10 KiB sync  RPC:  0 conn,  0 req/s,   0 µs

I maybe missing something, but looks like it works even without '-q' flag

@5chdn
Copy link
Contributor Author

5chdn commented Aug 30, 2017

I'm talking about pbtc -q, sorry :)

but introducing an -l flag like parity has, would be nice, too.

@svyatonik
Copy link
Collaborator

@5chdn Hope you don't mind that I've added this change to your PR: now RUST_LOG works even without '-q' flag. So now pbtc behaves the same way as parity does

@5chdn
Copy link
Contributor Author

5chdn commented Sep 1, 2017

Just noticed. Compiling.

@5chdn 5chdn added A3-inprogress Pull request is in progress. No review needed at this stage. and removed A0-pleasereview Pull request needs code review. labels Sep 1, 2017
@5chdn
Copy link
Contributor Author

5chdn commented Sep 1, 2017

Switching to WIP. Will test it when I'm back at some place with real internet.

@5chdn 5chdn added A0-pleasereview Pull request needs code review. and removed A3-inprogress Pull request is in progress. No review needed at this stage. labels Sep 1, 2017
@5chdn
Copy link
Contributor Author

5chdn commented Sep 1, 2017

@5chdn Hope you don't mind that I've added this change to your PR: now RUST_LOG works even without '-q' flag. So now pbtc behaves the same way as parity does

Yes, works for me. I updated the README, this is good to go now. :shipit:

@svyatonik
Copy link
Collaborator

Cool, thanks :)

@svyatonik svyatonik merged commit 5a20710 into master Sep 1, 2017
@svyatonik svyatonik deleted the a5-improve-help branch September 1, 2017 13:23
@@ -46,7 +47,12 @@ impl LogFormatter for DateAndColorLogFormatter {

pub fn init<T>(filters: &str, formatter: T) where T: LogFormatter {
let mut builder = LogBuilder::new();
builder.parse(filters);
let filters = match env::var("RUST_LOG") {
Ok(env_filters) => format!("{},{}", env_filters, filters),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to understand this, where does env_filters come from in this context?

Copy link
Collaborator

Choose a reason for hiding this comment

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

from environment variable RUST_LOG . and in format!() I just merge these two together, so that it would be either (RUST_LOG and sync=info) or (RUST_LOG), depending on -q existance => RUST_LOG always works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Super elegant. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. M3-docs Documentation. M4-core Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants