-
Notifications
You must be signed in to change notification settings - Fork 311
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
Add p2p sent messages logging #1970
Conversation
res, err := p2pChannel.Send(p2p.TopicSessionCreate, sessionRequest) | ||
} | ||
log.Info().Msgf("Sending P2P message to %q: %s", p2p.TopicSessionCreate, sessionRequest.String()) | ||
res, err := p2pChannel.Send(p2p.TopicSessionCreate, p2p.ProtoMessage(sessionRequest)) |
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.
Please do logging inside p2pChannel.Send()
, will be hard to remember to add this liners before each sending or receiving.
Also code will be less noisy.
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.
Send message accepts this struct
type Message struct {
Data []byte
}
It is not coupled to concrete message struct, you just send bytes. If I add it to Send I can't log formatted payload with fields. There are only 5 places there it is needed right now and we will not have much more of these messages.
Also, what if you want to add more context to logging? I suggest to keep it explicit as is.
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.
Well serialisation should happen in transport level also ;)
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.
p2p transport layer works with raw bytes, there is no need to for extra abstractions.
Some more logging for p2p messages.