search

vacp2p / nim-quic

QUIC for Nim
40 stars 3 forks source link

chore: add initial logging #42

Closed diegomrsantos closed 2 months ago

diegomrsantos commented 4 months ago

This PR adds the logs used to find the cause of the deadlock fixed in https://github.com/status-im/nim-quic/commit/ca9a029b63a2c00e3b936928846af9233abd13d6.

Related to https://github.com/status-im/nim-quic/pull/41

arnetheduck commented 4 months ago

a low-level library like quic should not do logging - it is too far from the application domain and there are many cases where logging in quic would be invasive to the application using it

arnetheduck commented 4 months ago

if error information is needed, it should not be logged - it should be handed to the callers of the api so they can make logging decisions

diegomrsantos commented 4 months ago

This logging isn't meant for applications using it, but rather to debug issues in the code. It was used to understand why a test wasn't finishing and to discover the deadlock. It is mostly defined as trace and should always be disabled unless otherwise specified. Do you think it makes sense?

diegomrsantos commented 4 months ago

It seems other implementations do use logging https://github.com/mozilla/neqo?tab=readme-ov-file#debugging-neqo.

diegomrsantos commented 4 months ago

If we run nim c -r -d:chronicles_log_level=TRACE -d:chronicles_enabled_topics=quic:ERROR tests/quic/testConnection, no log is generated. See https://github.com/status-im/nim-chronicles?tab=readme-ov-file#chronicles_enabled_topics

diegomrsantos commented 4 months ago

it can also be disabled with nim c -r -d:chronicles_log_level=TRACE -d:chronicles_disabled_topics=quic tests/quic/testConnection

arnetheduck commented 4 months ago

trace is fine, debug+ is not - ideally all logging in the library would be disabled by default at which point it could do whatever logging it wants, but I don't think that's possible with chronicles - if you can find a way to arrange it in such a way that it's fully opt-in (at compile-time), that's fine too.

diegomrsantos commented 4 months ago

You can use the chronicles_enabled_topics option to specify the list of topics for which the logging statements should produce output. All other logging statements will be erased at compile-time from the final code. When the list includes multiple topics, any of them is considered a match.

I believe it means that if we run nim c -r -d:chronicles_enabled_topics=libp2p:TRACE tests/pubsub/testgossipsub only nim-libp2p trace logs or above should be shown. Everything else will be erased at compile time. Applications could selectively enable what topics they want with chronicles_enabled_topics instead of using chronicles_log_level.