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

Hermes: replace default JSON with human-readable output #796

Merged
merged 19 commits into from
Apr 8, 2021
Merged

Conversation

adizere
Copy link
Member

@adizere adizere commented Apr 6, 2021

Closes: #805

Many thanks @romac for code snippets & helping with navigating the code around abscissa!

Description

Primary change in this PR:

  • Introduced the -j/--json flag to enable output in JSON format:
     Running `target/debug/hermes -j query channel end ibc-0 transfer channel-0`
{"timestamp":"Apr 07 21:48:04.410","level":"INFO","fields":{"message":"Using default configuration from: '.hermes/config.toml'"},"target":"ibc_relayer_cli::commands"}
{"timestamp":"Apr 07 21:48:04.415","level":"INFO","fields":{"message":"Options QueryChannelEndCmd { chain_id: ChainId { id: \"ibc-0\", version: 0 }, port_id: PortId(\"transfer\"), channel_id: ChannelId(\"channel-0\"), height: None }"},"target":"ibc_relayer_cli::commands::query::channel"}
{"status":"success","result":{"connection_hops":["connection-0"],"ordering":"Unordered","remote":{"channel_id":"channel-0","port_id":"transfer"},"state":"Open","version":"ics20-1"}}

versus pretty-printing (the default):

    Running `target/debug/hermes query channel end ibc-0 transfer channel-0`
Apr 07 21:48:31.510  INFO ibc_relayer_cli::commands: Using default configuration from: '.hermes/config.toml'
Apr 07 21:48:31.514  INFO ibc_relayer_cli::commands::query::channel: Options QueryChannelEndCmd { chain_id: ChainId { id: "ibc-0", version: 0 }, port_id: PortId("transfer"), channel_id: ChannelId("channel-0"), height: None }
Success
{
  "connection_hops": [
    "connection-0"
  ],
  "ordering": "Unordered",
  "remote": {
    "channel_id": "channel-0",
    "port_id": "transfer"
  },
  "state": "Open",
  "version": "ics20-1"
}

Notable code changes in this PR:

  • override the default abscissa::EntryPoint with out custom EntryPoint in relayer-cli/src/entry.rs per suggestion;
    • this allowed us to introduce the global application flag -j or --json to enable JSON only output (default is pretty printing)
    • also allowed us to remove the global -v verbose parameter
  • introduced the json() method in conclude.rs to peek into the application state and infer if JSON output should be on or off
  • we can dispense with relayer-cli/build.rs

For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@adizere adizere requested review from ancazamfir and romac as code owners April 6, 2021 10:31
@adizere adizere mentioned this pull request Apr 6, 2021
11 tasks
@adizere adizere marked this pull request as draft April 6, 2021 19:09
@romac
Copy link
Member

romac commented Apr 7, 2021

imho whether or not we output JSON should be a global command-line flag (available on every commands) rather than a config option, otherwise if one wants to use the CLI programmatically (either in a script or in the e2e test suite) one has to ensure that the config file contains the proper value for log_json.

I wonder if we could do this by defining our own [Entrypoint struct](https://docs.rs/abscissa_core/0.5.2/src/abscissa_core/command/entrypoint.rs.html#11-33, adding the option there, and telling the framework about it?

What do you think?

@@ -13,7 +13,6 @@ authors = [

description = """
Implementation of `hermes`, an IBC Relayer developed in Rust.
This crate is a CLI wrapper over the `ibc-relayer` library.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@codecov-io
Copy link

Codecov Report

Merging #796 (2e2dc0b) into master (b1b37f5) will increase coverage by 31.3%.
The diff coverage is n/a.

❗ Current head 2e2dc0b differs from pull request most recent head 573a5a9. Consider uploading reports for the commit 573a5a9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           master    #796      +/-   ##
=========================================
+ Coverage    13.6%   45.0%   +31.3%     
=========================================
  Files          69     165      +96     
  Lines        3752   11309    +7557     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    5090    +4577     
- Misses       2618    6219    +3601     
+ Partials      621       0     -621     
Impacted Files Coverage Δ
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 22.2% <ø> (ø)
..._transfer/relay_application_logic/send_transfer.rs 85.7% <ø> (ø)
modules/src/events.rs 0.0% <ø> (ø)
modules/src/handler.rs 100.0% <ø> (ø)
modules/src/ics02_client/client_consensus.rs 55.8% <ø> (ø)
modules/src/ics02_client/client_def.rs 32.3% <ø> (ø)
modules/src/ics02_client/client_state.rs 65.1% <ø> (ø)
modules/src/ics02_client/client_type.rs 79.1% <ø> (+31.5%) ⬆️
modules/src/ics02_client/context.rs 90.4% <ø> (ø)
... and 266 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfbb766...573a5a9. Read the comment docs.

@adizere adizere marked this pull request as ready for review April 7, 2021 19:46
@romac
Copy link
Member

romac commented Apr 7, 2021

Looks great, thanks @adizere!

One last thing I wonder now that I am looking at an example: in the human-readable case (ie. when --json is not supplied), how about using the standard dbg!/Debug output instead of still outputting JSON? Since is the output meant to be human-readable, I find the Debug output to be more readable than JSON.

Just a thought, based on a personal preference. What do you think?

Copy link
Contributor

@cezarad cezarad left a comment

Choose a reason for hiding this comment

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

The result of command and raw txs is different: E.g., create channel will print "Channel handshake finished for Channel{ ... } " in a different style than the "Success" message.

@adizere
Copy link
Member Author

adizere commented Apr 8, 2021

Status of this PR as of commit a35a225:

  • the last commit enforces filtering by modules, so that only log from crates ibc_relayer and ibc_relayer_cli are now printed
  • with this, we can use debug! or trace! to print lower-level tracing information (without being overwhelmed by tendermint or hyper debug lines)
  • this filter is enabled both for JSON output and pretty-printing

The feature of having debug and trace logging appeared to be useful in debugging recent work (#747).

@romac romac merged commit 3e9d97f into master Apr 8, 2021
@romac romac deleted the adi/v0.2_ux branch April 8, 2021 19:44
@romac romac mentioned this pull request Apr 9, 2021
15 tasks
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…ems#796)

* Added log_json flag to config

* Add missing JSON flag for CI workflow

* One more toml config file

* Introduce custom-made EntryPoint

* Tracing with custom writer to ensure stderr output. h/t @romac

* JSON-aware output in conclude.rs

* Enabled json output in e2e suite

* Switch to pretty formatter when JSON is off.

* Replaced the clumsy impl of json() with more direct approach h/t @romac.

* More explicit json flag in e2e

* changelog

* Cleanup

* More cleanup

* Use human-readable output for the final output result when JSON output is disabled

* Enabled tracing filter by crate. Added custom pretty tracing component.

* Replace /tx raw create-client/ with /create client/

* Revert "Replace /tx raw create-client/ with /create client/"

This reverts commit 4dd8982.
The commit was mistakenly pushed on the wrong branch.

Co-authored-by: Romain Ruetschi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hermes default should be pretty-printing instead of JSON
4 participants