-
Notifications
You must be signed in to change notification settings - Fork 326
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
Reverse PendingTx chain id args and remove its Display impl #1843
Conversation
relayer/src/link/pending.rs
Outdated
"[{}] putting error response in error event queue: {:?} ", | ||
self, | ||
&relay_path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better, but I'd rather rework all these [{}]
constructs into span-wise KVs whenever the opportunity arises, as was partially done in #1491.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rework all these [{}] constructs into span-wise KVs
I'm not entirely clear what this means. What will the resulting log lines look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely clear what this means. What will the resulting log lines look like?
That will depend on the tracing backend. One of the benefits of using KVs is that they are converted into similar structural fields in logging backends supporting this (e.g. journald). In the terminal output subscriber that is used by Hermes now, a tracing event in a span created with span!(tracing::Level::ERROR, "some_op", chain="foo", connection="bar")
will look something like this (with some bold and italic font styling added on an ANSI terminal):
some_op{chain=foo connection=bar}: here comes the event message
So, the information will still be flattened in this simple backend, but it will be easy to decipher and search for (if a bit more verbose).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's perhaps add a span to this method, as @mzabaluev suggests:
let span = trace_span!(
"inserting new pending txs",
chain = %relay_path.chain_id()
counterparty_chain = %relay_path.counterparty_chain_id,
port = %relay_path.port_id,
channel = %relay_path.channel_id,
);
let _guard = span.enter();
relayer/src/link/pending.rs
Outdated
format!( | ||
"{}:{}/{} -> {}", | ||
self.counterparty_chain_id, | ||
self.port_id, | ||
self.channel_id, | ||
self.chain_id() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these could rather be in KV pairs to potentially aid log filtering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess they could but they're also useful when combing through the logs "by hand". But perhaps we can have both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But perhaps we can have both?
I'm not sure what's the best way to go about achieving this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it like this for now and see if we can improve things in another PR. Nevermind, let's do this: https://github.com/informalsystems/ibc-rs/pull/1843/files/31d0316899456fe9de31fe4991fb10b98b8b5707#r803620700
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good inasmuch as it resolves the representation problem in logs.
Incorporating the changes suggested by @mzabaluev and @romac produces pending transaction logs structured like this:
compared to the previous style:
|
@adizere What do you think? |
I think the new style is better! Will try to give a more thorough review
but briefly I think we’re good to go!
…On Fri, 11 Feb 2022 at 15:35, Romain Ruetschi ***@***.***> wrote:
@adizere <https://github.com/adizere> What do you think?
—
Reply to this email directly, view it on GitHub
<#1843 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANM6SX237RVKM4N2M3CI53U2UNERANCNFSM5NSMULVQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, neat work here! Glad we're slowly getting rid of fmt::Display
s.
I went ahead and merged, though I overlooked theres was no changelog entry. Cc @seanchen1991 to remember the changelogs in future PRs. |
…systems#1843) * Reverse PendingTx chain id args and remove its Display impl * Add `trace_spans` and remove `relay_path` method.
Closes: #1488
Description
Reverses the order of the
chain_id
andcounterparty_chain_id
arguments when displaying aPendingTx
.PendingTx
'sDisplay
impl was also removed as per the recent log reformatting work.I'm not sure I went about this in the best way though. Is there a better way to handle constructing the relay path? Also, why is that for
PendingTx
's, the order of the chain IDs need to be reversed in the first place? Is this actually correct?PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.