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

Clean up and improve logs #2544

Merged
merged 35 commits into from
Sep 7, 2022
Merged

Clean up and improve logs #2544

merged 35 commits into from
Sep 7, 2022

Conversation

romac
Copy link
Member

@romac romac commented Aug 10, 2022 •
edited
Loading

See: #1538

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@ljoss17
Copy link
Contributor

ljoss17 commented Aug 10, 2022

Many CLI have a debug log which contains all the options given, but not all of them have this log.
Should we have this log for every CLI or remove it from every CLI ? I do not see a case where this log is very useful, so I tend to be of the opinion to remove it from the CLIs.
https://github.com/informalsystems/ibc-rs/blob/romac/cleanup-logs/relayer-cli/src/commands/query/client.rs#L122-L124

@romac
Copy link
Member Author

romac commented Aug 10, 2022

I do not see a case where this log is very useful, so I tend to be of the opinion to remove it from the CLIs.

Agreed!

seanchen1991
seanchen1991 reviewed Aug 11, 2022
View reviewed changes
Copy link
Contributor

@seanchen1991 seanchen1991 left a comment

Choose a reason for hiding this comment

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

Let's add some more docs, but other than that, looks good to me 🙂

relayer/src/chain/cosmos.rs Show resolved Hide resolved
relayer/src/supervisor.rs Outdated
@@ -700,8 +681,54 @@ fn process_batch<Chain: ChainHandle>(
Ok(())
}

fn send_telemetry<Src, Dst>(src: &Src, dst: &Dst, events: &[IbcEvent], path: &Packet)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document this function? Some questions that I think would be helpful for the docs to answer:

  1. Do the metrics get sent to both the source and destination? Only one of them? In which situations?
  2. A brief description of each of the metrics that are sent based on the IbcEvent variant in the match.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea for these metrics is the following:

  • These metrics count the number of SendPacket, WriteAcknowledgement and TimeoutPacket events observed by the supervisor.
  • In order to make it easier to see the duality SendPacket/WriteAcknowledgement or SendPacket/TimeoutPacket, the WriteAcknowledgement and TimeoutPacket will have the destination chain labelled as the source as these events are being "sent back" to the chain which submitted the corresponding SendPacket event.

So a SendPacket event from ibc-0 will increment the send_packet_events{chain="ibc-0",channel="channel-0",counterparty="ibc-1",port="transfer"} and the returning WriteAcknowledgment event will increment the acknowledgement_events{chain="ibc-0",channel="channel-0",counterparty="ibc-1",port="transfer”}.

But I realise that this might cause confusion. I can update it to label the chain and counterparty to be the submitting and receiving chains for the isolated events. Meaning the new send/acknowledgement/timeout events metrics label would have:

  • chain is always the chain that submits the event
  • counterparty is always the chain that will receive the packet generated from the event

Instead of being the submitting and receiving chain for the whole transfer. Current send/acknowledgement/timeout events metrics label is:

  • chain being the chain that sends the SendPacket event and receives the corresponding WriteAcknowledgment, TimeoutPacket events
  • counterparty being the chain that receives the SendPacket event and sends the WriteAcknowledgment, TimeoutPacket events

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

chain is always the chain that submits the event
counterparty is always the chain that will receive the packet generated from the event

I agree that labeling these in absolute terms, instead of relative terms, is probably a bit easier to parse 🙂

@romac romac added this to the v1.1 milestone Aug 22, 2022
@ljoss17 ljoss17 marked this pull request as ready for review August 24, 2022 12:53
@romac romac merged commit fd92eb6 into master Sep 7, 2022
@romac romac deleted the romac/cleanup-logs branch September 7, 2022 15:18
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@romac @ljoss17
Clean up and improve logs (informalsystems#2544)
c317e3c 
* Instrument more functions using `tracing`

* Use error level everywhere

* Bit more cleanup

* Instrument more functions under the `chain::cosmos` module

* Removed debug log which outputs the options passed to CLIs

* Cleanup imports

* Remove `Serialize` bound on `ChainHandle`

* Remove `Serialize` instances on `ChainHandle` imps

* Preserve current tracing span across the runtime boundary

* Remove ad-hoc Display and Debug impl for client and channel events

* Improve spawning logs

* Better span for some workers

* Extract telemetry code into its own function

* Few more improvements

* Tighter Display impl for account-related newtypes

* Add `Display` impl for `version::Specs`

* Fix clippy warning

* Cleanup `From` instace for `PathIdentifiers`

* Fixed bugs from merge. Clippy and cargo fmt

* Emit JSON to stdout

* More instrumentation in `relay_path`

* replaced {:#?} by {:?}

* Updated metrics for events and added comment for method recording metrics in the supervisor

* Replace all the Debug with Display fmt for 'info!', 'warn!' and 'error!' logs

* Fixed issues following merge with master. Added comment

* Updated std::time to core::time in modules pretty.rs file

* Removed line returns in info logs

* Add changelog entry

Co-authored-by: Luca Joss <luca@informal.systems>
Co-authored-by: ali <ali.benabdallah@epfl.ch>
Co-authored-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>
@adizere adizere mentioned this pull request Sep 14, 2022
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanchen1991 seanchen1991 seanchen1991 left review comments

@ljoss17 ljoss17 ljoss17 left review comments

@AlianBenabdallah AlianBenabdallah AlianBenabdallah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@romac @ljoss17 @seanchen1991 @AlianBenabdallah