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

Reduce RPC error log to debug when domain-level RPC service returns an error #4579

Closed
SWvheerden opened this issue Aug 31, 2022 · 4 comments · Fixed by #4611
Closed

Reduce RPC error log to debug when domain-level RPC service returns an error #4579

SWvheerden opened this issue Aug 31, 2022 · 4 comments · Fixed by #4611
Assignees

Comments

@SWvheerden
Copy link
Collaborator

 // comms/core/src/protocol/rpc/server/mod.rs:6662022-08-31 10:25:53.657760000 [comms::rpc] [Thread:6100299776] DEBUG (stream_id: 25, peer: 783939753066407bf8d1ba04af, protocol: t/bnwallet/1) RPC request completed in 378µs
 // comms/core/src/protocol/rpc/server/mod.rs:6202022-08-31 10:25:53.667513000 [comms::rpc] [Thread:6102446080] ERROR stream_id: 9, peer: 783939753066407bf8d1ba04af, protocol: t/bnwallet/1 Service returned an error: NotFound: Header not found at height 2663
 // comms/core/src/protocol/rpc/server/mod.rs:6202022-08-31 10:25:53.671530000 [comms::rpc] [Thread:6106738688] ERROR stream_id: 15, peer: 783939753066407bf8d1ba04af, protocol: t/bnwallet/1 Service returned an error: NotFound: Header not found at height 2664

These error messages should not be errors.
These are caused by a wallet requesting bad information

@stringhandler
Copy link
Collaborator

To add some context: When an RPC errors, it should not show an error. Perhaps warn is more reasonable?

@SWvheerden
Copy link
Collaborator Author

I think we should try and look more carefully at RPC errors.
If we receive an actual RPC error, then yes, we should log the error.
But in this case a WARN or perhaps a Debug is perfectly fine.

@CjS77
Copy link
Collaborator

CjS77 commented Sep 1, 2022

I've written about the log level philosophy before. Maybe we should include it in the docs / RFC somewhere.

INFO and above is what an average (non-dev) client / user will be reading. You should ask yourself whether a user would be interested in those messages. Basically, if they're recoverable errors, they just go to debug.

ERRORs are things that are going to stop the app, break the app in some way, or worse, e.g. of data corruption.
WARNings are errors the user should know about, but aren't show-stoppers.
INFO are short updates, like found new block, mined a block, tx received, etc.

Everything else is DEBUG (interesting for devs) or TRACE (really detailed info, and used sparingly)

@CjS77
Copy link
Collaborator

CjS77 commented Sep 1, 2022

So using this philosophy, I'd say they should be DEBUG most likely.

@jorgeantonio21 jorgeantonio21 self-assigned this Sep 2, 2022
@sdbondi sdbondi changed the title Suppress bad logs Reduce RPC error log to debug when domain-level RPC service returns an error Sep 5, 2022
stringhandler pushed a commit that referenced this issue Sep 5, 2022
…urns an error (fixes #4579) (#4611)

Description
---
The `comms` server code contains incorrect logs, as referred in #4579.

Motivation and Context
---
Fixes #4579.

How Has This Been Tested?
--- Log information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
4 participants