-
Notifications
You must be signed in to change notification settings - Fork 66
feat: enhance Sync logs and metrics #3370
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
Conversation
You can find the image built from this PR at
Built from 8b2c1f8 |
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, thank you!
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.
Thanks
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! Thanks for it! 💯
Just added some nitpick comments that I hope you find useful
return err("connection write error: " & writeRes.error.msg) | ||
await connection.close() | ||
return err( | ||
"remote " & $connection.peerId & " connection write error: " & writeRes.error.msg |
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.
Just to differentiate it a little from the other:
"remote " & $connection.peerId & " connection write error: " & writeRes.error.msg | |
"remote " & $connection.peerId & " connection write error initiate: " & writeRes.error.msg |
@@ -217,7 +229,7 @@ proc storeSynchronization*( | |||
let connOpt = await self.peerManager.dialPeer(peer, WakuReconciliationCodec) | |||
|
|||
let conn: Connection = connOpt.valueOr: | |||
return err("cannot establish sync connection") | |||
return err("fail to dial remote " & $peer.peerId) |
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.
Maybe?
return err("fail to dial remote " & $peer.peerId) | |
return err("fail to dial remote " & $peer.peerId & " store sync conn error: " & $error) |
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.
It's an Option
there's no error to log sadly.
@@ -69,7 +70,7 @@ proc openConnection( | |||
let connOpt = await self.peerManager.dialPeer(peerId, WakuTransferCodec) | |||
|
|||
let conn: Connection = connOpt.valueOr: | |||
return err("Cannot establish transfer connection") | |||
return err("fail to dial remote " & $peerId) |
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.
Maybe :)?
return err("fail to dial remote " & $peerId) | |
return err("fail to dial remote " & $peerId & " open conn error: " & $error) |
Description
I added the remote peer id to most logs so that it's easier to know which connection failed.
Also, added metrics for differences found.