talaia-labs / rust-teos

The Eye of Satoshi - Lightning Watchtower
https://talaia.watch
MIT License
128 stars 62 forks source link

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

Closed sr-gi closed 1 year ago

sr-gi commented 1 year ago

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

mariocynicys commented 1 year ago

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 commented 1 year ago

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

sr-gi commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

@mariocynicys