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: make log topics consistent with nim-chronicles style #1334

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

LNSD
Copy link
Contributor

@LNSD LNSD commented Nov 3, 2022

It all started with me looking for a way to reduce the verbosity of the test cases in the CI. I hunch that test execution time is also impacted by the immense amount of logs the code is writing to the stdout.

Diving into nim-chronicles code, I noticed that the topics filtering mechanism works with "whole words" separated by spaces. Checking the nim-libp2p code base, together with some tests passing the -d:"chronicles_disabled_topics=libp2p,pubsubprotobuf" flag has confirmed it.

NOTE: I also noticed that in some cases, nim-chronicles is not respecting the logScope: topics = "<topics>" statement, logging without the topics tags.


Some of the rules in the coding guide might need to be updated to be compatible with nim-chronicles. These are the rules I have synthesized for better logging:

  • Log topics should contain "whole words" separated by spaces. In the case of a composed word, it should follow the snake_case style.
  • Log topics should be ordered from more general to less general (e.g., waku node message_store sqlite_store)
  • The modules' log topics within waku module should have waku as the first topic. This will allow any user of waku (as a library) to mute all logs coming by filtering the waku topic. The whisper module is the exception.
  • The apps' log topics should start with the name of the app (e.g., wakunode2 rest)
  • Waku protocols' log topics should be specified without the "waku" prefix (e.g., waku filter client)

@LNSD LNSD requested review from alrevuelta, rymnc and jm-clius November 3, 2022 10:12
@LNSD LNSD self-assigned this Nov 3, 2022
Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM

waku/v2/node/jsonrpc/debug_api.nim Outdated Show resolved Hide resolved
@LNSD LNSD force-pushed the chore-logging-topics branch 3 times, most recently from 1a4ce92 to 46dee2b Compare November 3, 2022 10:21
@LNSD LNSD force-pushed the chore-logging-topics branch from 46dee2b to 9c14d38 Compare November 3, 2022 10:24
@status-im-auto
Copy link
Collaborator

status-im-auto commented Nov 3, 2022

Jenkins Builds

Click to see older builds (4)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 19e6327 #1 2022-11-03 10:30:49 ~17 min macos 📦bin
✔️ 19e6327 #1 2022-11-03 10:33:07 ~19 min linux 📦bin
46dee2b #3 2022-11-03 10:37:17 ~15 min linux 📄log
✔️ 46dee2b #3 2022-11-03 10:40:43 ~18 min macos 📦bin
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 1a4ce92 #2 2022-11-03 10:39:12 ~19 min macos 📦bin
✔️ 1a4ce92 #2 2022-11-03 10:39:33 ~19 min linux 📦bin
✔️ 9c14d38 #4 2022-11-03 10:44:55 ~20 min linux 📦bin
✔️ 9c14d38 #4 2022-11-03 10:47:30 ~22 min macos 📦bin

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

+1 to having spaces instead of dots. Its used by nim-libp2p as you mention and also by nimbus

Not sure though if "4 levels" would be too much, i.e. waku node rest debug_api . Neither nim-libp2p nor nimbus have that many. Perhaps 3 is enough? waku+protocol+extra?

wdyt?

@jm-clius
Copy link
Contributor

jm-clius commented Nov 3, 2022

Great PR! +1 for the change to spaces.

Not sure though if "4 levels" would be too much, i.e. waku node rest debug_api . Neither nim-libp2p nor nimbus have that many. Perhaps 3 is enough? waku+protocol+extra?

wdyt?

Perhaps I'm missing some use cases, but I'd also prefer less (say 3) rather than more levels if feasible. Even in the current coding guide I felt uncomfortable with the number of levels I was suggesting because it becomes a bit artificial. Suggestion would then be what @alrevuelta suggested, but where "protocol" could also mean logical subcomponent (such as rest or node). I imagine it would depend on our log filtering use cases?

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Great. Feel free to merge! We can document as is in the coding guide. My question above implies whether we want to use waku node as a prefix for some logical components or just waku, e.g. waku json_rpc vs waku node json_rpc but this is a minor detail IMO.

@LNSD
Copy link
Contributor Author

LNSD commented Nov 3, 2022

Nim chronicles' log topics should be understood as "log tags/labels" that you can filter by using one of the following options:

  • chronicles_enabled_topics: Sets the log level per topic (e.g., -d:chronicles_enabled_topics:waku:TRACE,libp2p:ERROR)
  • chronicles_required_topics: Requests that only the listed topics are logged (e.g., -d:chronicles_required_topics:rest will only log the rest logs)
  • chronicles_disabled_topics: Filters out the messages that contain that topic (e.g., -d:chronicles_disabled_topics:filter,client will filter out the messages that contain those labels)

Said that. Setting a rule to limit the number of topics makes no sense to me. It is restricting our selves without any technical reason. When setting the log topics, we should be mindful as we are with using logging. We should not do it willy-nilly.

@LNSD
Copy link
Contributor Author

LNSD commented Nov 3, 2022

We can document as is in the coding guide.

Absolutely 👍🏼

My question above implies whether we want to use waku node as a prefix for some logical components or just waku, e.g. waku json_rpc vs waku node json_rpc but this is a minor detail IMO.

If I understood it correctly, you are asking the reason behind the node label.

I am just following the modules structure. I mapped the waku/v2/node/jsonrpc/debug_api.nim to topics = "waku node jsonrpc debug_api". But I don't have a strong opinion about adding node or not as a label.

And sincerely, currently, I am thinking of the possibility of moving jsonrpc and rest outside of node module since they should be independent of many of the things within that module. So, we will see how things evolve.

@jm-clius
Copy link
Contributor

jm-clius commented Nov 3, 2022

I agree. No need to limit here. I meant more as a general rule of thumb in order for outside developers following the coding guide not to overdesign log scope levels, but rather think of practical log filter use cases.

@LNSD
Copy link
Contributor Author

LNSD commented Nov 3, 2022

I have updated the coding guide to match the new style.

Merging the present PR.

@LNSD LNSD changed the title chore: make log topics conistent with nim-chronicles style chore: make log topics consistent with nim-chronicles style Nov 3, 2022
@LNSD LNSD merged commit 5c4f1ce into master Nov 3, 2022
@LNSD LNSD deleted the chore-logging-topics branch November 3, 2022 15:36
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
Development

Successfully merging this pull request may close these issues.

5 participants