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

chore: add initial logging #42

Merged
merged 9 commits into from
Aug 14, 2024
Merged

chore: add initial logging #42

merged 9 commits into from
Aug 14, 2024

Conversation

diegomrsantos
Copy link
Collaborator

This PR adds the logs used to find the cause of the deadlock fixed in ca9a029.

Related to #41

@diegomrsantos diegomrsantos changed the base branch from main to update-nimcrypto July 2, 2024 16:37
@diegomrsantos diegomrsantos changed the title chore: add inital logging chore: add initial logging Jul 2, 2024
@arnetheduck
Copy link

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
Copy link

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
Copy link
Collaborator Author

diegomrsantos commented Jul 2, 2024

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
Copy link
Collaborator Author

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

@diegomrsantos
Copy link
Collaborator Author

diegomrsantos commented Jul 2, 2024

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
Copy link
Collaborator Author

diegomrsantos commented Jul 3, 2024

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

@arnetheduck
Copy link

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
Copy link
Collaborator Author

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.

Base automatically changed from update-nimcrypto to main July 5, 2024 12:24
This reverts commit 9f265c8.
@diegomrsantos diegomrsantos merged commit f0c6b05 into main Aug 14, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants