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

Consider logging the packets involved in the clear packets CLI flow #2623

Closed
6 tasks
seanchen1991 opened this issue Sep 6, 2022 · 3 comments · Fixed by #2653
Closed
6 tasks

Consider logging the packets involved in the clear packets CLI flow #2623

seanchen1991 opened this issue Sep 6, 2022 · 3 comments · Fixed by #2653
Labels
I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic O: usability Objective: cause to improve the user experience (UX) and ease using the product
Milestone

Comments

@seanchen1991
Copy link
Contributor

Problem Definition

Per @greg-szabo's suggestion in #2612, should we consider logging the packets that are involved in the clear packets CLI flow. I don't have context into why these were removed in the first place, so some additional context as to the motivation of why these were removed would be appreciated 🙂

I'm sure there's a balance here between the verbosity of the logs and the usefulness of this particular output.

Acceptance Criteria

  • Discuss whether it is appropriate or not to add back the logs indicating the packets involved in the clear packets CLI flow.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@seanchen1991 seanchen1991 added the A: question Admin: further information is requested label Sep 6, 2022
@romac
Copy link
Member

romac commented Sep 7, 2022

We should definitely add this back, not sure when/why it was ever removed?

@seanchen1991
Copy link
Contributor Author

Per @ancazamfir's follow-up to Greg in Slack, were these perhaps moved into the trace-level logs?

@adizere
Copy link
Member

adizere commented Sep 14, 2022

I re-ran clear packets and noticed that indeed we do not log the individual packet sequence numbers, we only say

2022-09-14T14:43:11.296590Z  INFO ThreadId(01) PacketRecvCmd{src_chain=ibc-1 src_port=transfer src_channel=channel-0 dst_chain=ibc-0}: unreceived packets found: 11
2022-09-14T14:43:11.385795Z  INFO ThreadId(01) PacketRecvCmd{src_chain=ibc-1 src_port=transfer src_channel=channel-0 dst_chain=ibc-0}: pulled packet data for 11 events; events_total=11 events_left=0
2022-09-14T14:43:12.139841Z  INFO ThreadId(01) PacketRecvCmd{src_chain=ibc-1 src_port=transfer src_channel=channel-0 dst_chain=ibc-0}:relay{odata=packet-recv ->Destination @1-103; len=11}: assembled batch of 12 message(s)

We eventually do log the individual sequence numbers, but in a very messy way, by printing out the event data after we submit a transaction, eg

...
	WriteAcknowledgement(WriteAcknowledgement { packet: seq:1, path:channel-0/transfer->channel-0/transfer, toh:no timeout, tos:Timestamp(2022-09-14T14:44:18.682503Z)), ack: [ 123, 34, 114, 101, 115, 117, 108, 116, 34, 58, 34, 65, 81, 61, 61, 34, 125 ] }) at height 0-105
	WriteAcknowledgement(WriteAcknowledgement { packet: seq:2, path:channel-0/transfer->channel-0/transfer, toh:no timeout, tos:Timestamp(2022-09-14T14:44:18.682503Z)), ack: [ 123, 34, 114, 101, 115, 117, 108, 116, 34, 58, 34, 65, 81, 61, 61, 34, 125 ] }) at height 0-105
...

I think I might have removed (likely in #2167) the log for individual packet s.n. because we had (and still do) logs that are too verbose. I believe we should:

  • add back the individual sequence numbers as part of the log lines saying "unreceived packets found" and "pulled packet data for X events"
  • trim out the other logs that shows too much packet data (eg the ACK with binary data above) tracked in Make the Hermes CLI output less verbose #2179

@adizere adizere added I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic O: usability Objective: cause to improve the user experience (UX) and ease using the product and removed A: question Admin: further information is requested labels Sep 14, 2022
@adizere adizere added this to the v1.1 milestone Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: CLI Internal: related to the relayer's CLI I: logic Internal: related to the relaying logic O: usability Objective: cause to improve the user experience (UX) and ease using the product
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants