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

Adds debug logs to RPC requests and improves the ones for the http interface #177

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

sr-gi
Copy link
Member

@sr-gi sr-gi commented Jan 23, 2023

Also updates the severity from the http logs from info to debug

@sr-gi sr-gi added feature request New feature or request easy to review easypeasy labels Jan 23, 2023
@sr-gi sr-gi requested a review from mariocynicys January 23, 2023 14:30
@mariocynicys
Copy link
Collaborator

I'm debating whether to move the logs from http.rs to internal.rs so everything is in one place (and future interfaces also log without having to replicate the same code). We may lose track of what interface this comes from though. Thoughts?

ACK moving logs to internal.rs, but haven't found a way to tell the module path of the caller (http.rs, or other module) in the log without passing it in args :(.

@sr-gi sr-gi added the Seeking Code Review review me pls label Jan 23, 2023
@sr-gi
Copy link
Member Author

sr-gi commented Jan 23, 2023

I'm debating whether to move the logs from http.rs to internal.rs so everything is in one place (and future interfaces also log without having to replicate the same code). We may lose track of what interface this comes from though. Thoughts?

ACK moving logs to internal.rs, but haven't found a way to tell the module path of the caller (http.rs, or other module) in the log without passing it in args :(.

I don't really think there is any using the current logging library

Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

Think we better set the logs from the private api to level info because they don't occur pretty often (does this translate to importance?), but not very sure should.

LGTM otherwise.

@sr-gi sr-gi added this to the 0.2 milestone Jan 23, 2023
@sr-gi
Copy link
Member Author

sr-gi commented Jan 23, 2023

Think we better set the logs from the private api to level info because they don't occur pretty often (does this translate to importance?), but not very sure should.

LGTM otherwise.

I think it should be debug. Paraphrasing bitcoind:

  -debug=<category>
       Output debug and trace logging (default: -nodebug, supplying <category>
       is optional). If <category> is not supplied or if <category> = 1,
       output all debug and trace logging. <category> can be: addrman,
       bench, blockstorage, cmpctblock, coindb, estimatefee, http, i2p,
       ipc, leveldb, libevent, mempool, mempoolrej, net, proxy, prune,
       qt, rand, reindex, **rpc**, selectcoins, tor, util, validation,
       walletdb, zmq. This option can be specified multiple times to
       output multiple categories.

@sr-gi
Copy link
Member Author

sr-gi commented Jan 23, 2023

I'm debating whether to move the logs from http.rs to internal.rs so everything is in one place (and future interfaces also log without having to replicate the same code). We may lose track of what interface this comes from though. Thoughts?

Nvm about this. We should not do it given we're keeping track of the IP that sends the request. Given this is a HTTP-> gRPC bridge, if we log requests from the internal API they will always come from us, losing that info.

I don't think there's a point either in forwarding that info to the internal API, so we may want to keep the logs where they are.

@mariocynicys
Copy link
Collaborator

I think it should be debug. Paraphrasing bitcoind:

I guess they have an RPC category in the debug because a bitcoin core daemon would usually get many RPC calls from various applications depending on it.

The thing is, private api RPC calls are not that frequent, that's why im thinking of them as info. Tried searching SoF for best practice but still unable to categorize this case :/.

@sr-gi
Copy link
Member Author

sr-gi commented Jan 23, 2023

Ok, as discussed in discord, we're reducing the severity of the logs to reduce the size of the log file

@mariocynicys

Also updates the severity from the http logs from info to debug
@sr-gi sr-gi merged commit a0e2978 into talaia-labs:master Jan 23, 2023
@sr-gi sr-gi mentioned this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy to review easypeasy feature request New feature or request Seeking Code Review review me pls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants