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

feat(p2p): don't log empty order packets #1871

Merged
merged 1 commit into from
Sep 7, 2020
Merged

Conversation

sangaman
Copy link
Collaborator

@sangaman sangaman commented Sep 4, 2020

This skips the log statement that logs when a peer sends us zero orders for a given trading pair, as this is more or less a non-event and is currently logged fairly frequently. It also lowers the level of logging when we do get orders from verbose to debug as it is a common and not particularly important event.

This skips the log statement that logs when a peer sends us zero orders
for a given trading pair, as this is more or less a non-event and is
currently logged fairly frequently. It also lowers the level of logging
when we do get orders from `verbose` to `debug` as it is a common and
not particularly important event.
@sangaman sangaman added p2p Peer to peer networking logging Log messages and output labels Sep 4, 2020
@sangaman sangaman requested a review from a user September 4, 2020 16:00
@sangaman sangaman self-assigned this Sep 4, 2020
@raladev
Copy link
Contributor

raladev commented Sep 7, 2020

Not sure that we should remove receiving if sending is still logging.

Otherwise we have case when we see sending on peer side in the logs, but dont see receiving on our side.

It would be better to remove sending for such case or just do nothing. WDYT @sangaman @erkarl ?

@raladev raladev self-requested a review September 7, 2020 09:08
Copy link
Contributor

@raladev raladev left a comment

Choose a reason for hiding this comment

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

Above

@kilrau
Copy link
Contributor

kilrau commented Sep 7, 2020

Agree, that we should ommit logs on the sending side in the case of "no orders" too.

It would be better to remove sending for such case or just do nothing.

Not sure I understand. The sender still needs to send the empty order packet, otherwise the receiver doesn't know the sender doesn't have any orders for that pair.

@raladev
Copy link
Contributor

raladev commented Sep 7, 2020

Not sure I understand. The sender still needs to send the empty order packet, otherwise the receiver doesn't know the sender doesn't have any orders for that pair.

So, it looks pretty strange - "Lets not log 0 order packets because there is too much such strings in logs, but we still will log non-empty order packets"

U said that sending empty order packet is required action, then it definitely SHOULD be logged, there is no difference between empty order packet and non-empty, both are required. And if we will log sending, we need to log receiving too. (Sending it is an action that should be logged and logging should not be based on action parameters (2 or 0 orders).)

Then maybe lets just move this logging to trace level and will not omit it?

@sangaman
Copy link
Collaborator Author

sangaman commented Sep 7, 2020

I agree with not logging empty order packets when they're being sent, I wasn't aware that was being done but I'll go check and make sure again.

Not sure I understand. The sender still needs to send the empty order packet, otherwise the receiver doesn't know the sender doesn't have any orders for that pair.

So, it looks pretty strange - "Lets not log 0 order packets because there is too much such strings in logs, but we still will log non-empty order packets"

U said that sending empty order packet is required action, then it definitely SHOULD be logged, there is no difference between empty order packet and non-empty, both are required. And if we will log sending, we need to log receiving too. (Sending it is an action that should be logged and logging should not be based on action parameters (2 or 0 orders).)

Then maybe lets just move this logging to trace level and will not omit it?

So we do indeed need the empty orders packet on a p2p level, as kilian said. We already log every single packet on trace level (here https://github.com/ExchangeUnion/xud/blob/master/lib/p2p/Peer.ts#L797), so when we really want to track every single action we'll be able to see that. I was just thinking that we don't need additional logging for empty order packets, particularly at the verbose level, which is what I was trying to change here.

@raladev raladev self-requested a review September 7, 2020 13:44
@raladev raladev merged commit 1b6d6d6 into master Sep 7, 2020
rsercano pushed a commit that referenced this pull request Sep 11, 2020
This skips the log statement that logs when a peer sends us zero orders
for a given trading pair, as this is more or less a non-event and is
currently logged fairly frequently. It also lowers the level of logging
when we do get orders from `verbose` to `debug` as it is a common and
not particularly important event.
@sangaman sangaman deleted the no-log-empty-orders branch October 14, 2020 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging Log messages and output p2p Peer to peer networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants