Closed freak12techno closed 1 year ago
julienrbrt
commented
1 year ago This is already the case. Which Cosmovisor version are you using?
You have to run:
$ cosmovisor version --output jsonWhen using the run command, it will always show logs.
freak12techno
commented
1 year ago @julienrbrt when running your command, it indeed works correctly (though I am running in another issue I've described here https://github.com/cosmos/cosmos-sdk/issues/15360#issuecomment-1464632111, but this is not the subject of this issue). But this only solves part of the problem, while it allows silencing logs querying for version, it still displays them for every other command, for example:
$ cosmovisor run q staking params --output json
11:52PM INF running app args=["q","staking","params","--output","json"] module=cosmovisor path=/home/validator/.bitsongd/cosmovisor/upgrades/v014/bin/bitsongd
{"unbonding_time":"1814400s","max_validators":100,"max_entries":7,"historical_entries":10000,"bond_denom":"ubtsg"}and this also results in the response not being a valid JSON (while it is a valid JSON if running this command without Cosmovisor).
I assume checking for every command in cosmovisor run <something> if it has --output json is too expensive and complicated, therefore I guess the env variable solution might be nice. What do you think?
This is already the case. Which Cosmovisor version are you using?
1.3.0, also tested with 1.4.0
julienrbrt
commented
1 year ago I think your proposal makes sense. Disabling cosmovisor logger altogether could be an option.
freak12techno
commented
1 year ago I may help implementing it, if needed, just need some guidance on how to do it in a better way. @julienrbrt jfyi
julienrbrt
commented
1 year ago I may help implementing it, if needed, just need some guidance on how to do it in a better way. @julienrbrt jfyi
Sure, if you want to 👍 You need to change the logger created with the NoOp logger when that environment variable is present.
freak12techno
commented
1 year ago @julienrbrt okay I am mostly done, I have one issue though. In tools/cosmovisor/process.go#NewLauncher, it casts logger into zerolog.Logger, which would fail if it's not zerolog.Logger (which isn't true if it's noopLogger), and zerolog.Logger and noopLogger are not compatible as they have different methods and signatures. I see the following solutions here:
log/* and update Logger signatures to be the same as the ones in zerolog.Logger (so all of the methods should return Logger, and there should be also methods like Msg, Int, Str etc.), then remove casting to zerolog.Logger and use Logger interface directly therezerolog.Logger) on every place we use it.What do you think would be better approach, or do you maybe know some better ways to deal with it?
julienrbrt
commented
1 year ago No, you do not need to modify anything in log/*.
You can indeed choose to type cast everything when Impl() is used or use zerolog no-op logger with logger := log.NewCustomLogger(zerolog.Nop()).
We should not need to have zerolog.Logger type anywhere I think actually, if not yet done, we can change it do log.Logger`.
freak12techno
commented
1 year ago @julienrbrt okay submitted this https://github.com/cosmos/cosmos-sdk/pull/15362, can you review please? also pretty sure we also need to update at least docs here https://docs.cosmos.network/main/tooling/cosmovisor#command-line-arguments-and-environment-variables to add this, not sure where should I put it
julienrbrt
commented
1 year ago @julienrbrt okay submitted this https://github.com/cosmos/cosmos-sdk/pull/15362, can you review please?
also pretty sure we also need to update at least docs here https://docs.cosmos.network/main/tooling/cosmovisor#command-line-arguments-and-environment-variables to add this, not sure where should I put it
I'll have a look, thank you! You can update the README and it will update on the website afterwards.
freak12techno
commented
1 year ago You can update the README and it will update on the website afterwards.
Done, thanks for letting me know.
One thing I am not sure are tests, they fail even without my PR, so it's not something I broke within my changes I assume, so I didn't bother fixing them:
--- FAIL: TestInitTestSuite (0.02s)
--- FAIL: TestInitTestSuite/TestInitializeCosmovisorNegativeValidation (0.00s)
init_test.go:60: Clearing environment variables.
--- FAIL: TestInitTestSuite/TestInitializeCosmovisorNegativeValidation/no_args (0.00s)
testing.go:1319: race detected during execution of test
init_test.go:74: Restoring environment variables.
init_test.go:92: done unsetting DAEMON_HOME
init_test.go:92: done unsetting DAEMON_NAME
testing.go:1319: race detected during execution of test
testing.go:1319: race detected during execution of test
cosmovisor version: (devel)
FAIL
FAIL cosmossdk.io/tools/cosmovisor/cmd/cosmovisor 0.765s
ok cosmossdk.io/tools/cosmovisor/errors (cached)
FAIL
make: *** [test] Error 1
Summary
I have some tools that are running some cosmovisor subcommands and are expecting a valid JSON in a response, and when I use the binary itself without cosmovisor, it returns a valid json, but with cosmovisor, it adds its own logs, making the result an invalid JSON. Example: querying for version (note the
running appline):Would be nice to have the way to omit such logs, see below for suggestions.
I am all in for implementing it by myself as I want to get my hands on cosmos-sdk as a contributor, just saying ;) I'd need some guidance for it though.
Problem Definition
Adding the way to silence cosmovisor logs would make it easier for other tools that rely on a plain output to be a valid JSON to use it. I do not see any disadvantages on being able to set it up.
Proposal
I suggest having an environmental variable (like
DAEMON_DISABLE_LOGS) that will silence logs if set to true.