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

Add logging #576

Closed
2 of 6 tasks
danwt opened this issue Dec 9, 2022 · 1 comment · Fixed by #628
Closed
2 of 6 tasks

Add logging #576

danwt opened this issue Dec 9, 2022 · 1 comment · Fixed by #628
Assignees
Labels
type: feature-request New feature or request improvement

Comments

@danwt
Copy link
Contributor

danwt commented Dec 9, 2022

Problem

We need logging to understand the running system.

Closing criteria

Add

  • provider module logging
  • consumer module logging
  • provider module logging - game of chains (high prio)
  • consumer module logging - game of chains (high prio)

TODOs

  • Labels have been added for issue
  • Issue has been added to the ICS project

# Other

Ill add logging for goc-december first, and then we can worry about getting it on main.

Note, the logger only has debug, info and error, and doesn't have warn.

https://github.com/tendermint/tendermint/blob/v0.34.23/libs/log/logger.go#L10-L16

Related issue

Shawns MVP logging branch

I will take inspiration from the tips here

@danwt danwt changed the title Add structured logging to the codebase Add structured logging Dec 9, 2022
@danwt danwt self-assigned this Dec 9, 2022
@danwt danwt added the type: feature-request New feature or request improvement label Dec 9, 2022
@danwt danwt changed the title Add structured logging Add logging Dec 9, 2022
@danwt danwt removed their assignment Dec 13, 2022
@danwt
Copy link
Contributor Author

danwt commented Jan 3, 2023

Here's some additional advice from @greg-szabo and @mircea-c

Hi Dan! I think network node logging is usually similar. Hermes is the outstanding "other project" that improved it's logging twofold as we went through some feedback.
A few general takeaways for logging:
Have JSON output as an alternative
If you can't have JSON output, define the log line strictly, so it can be cut up and mangled with string-mangling functions
One line per entry. Multiline log entries (like panics dumped into the log) create havoc and they are hard to manage outside of the casual developer execution.
Try to make atomic entries: the entry fully says when, what happened, to whom in a standardized, detailed format. Time is well-defined and either in UTC or in the local time of the server. (I prefer the latter.) If a packet was involved, the packet number is mentioned. The log severity indicates how bad it is what happened. If there are only a limited amount of error types, there is an error type mentioned so the operator doesn't have to rely on the plain text error message.
If the entry is part of a set of entries (for example you dump multiple thread's logs into one logfile), there is an identifier for log entry sets so it's easy to filter for entry groups. (thread ID, request ID, etc)
Causality is hard to represent in logs. Developers tend to look at a page of logs and find relations in the order of the entries. In operations, the logs entries might be vastly more and the processing of them might be line-by-line. If there is an order to things, the order is indicated in the entry. (Step 1, step 2, step 3 or similar of a logic, or like PreVote, PreCommit, etc.)

As an add-on to this, would be nice if the types that are output are consistent.
For example, the node software logs a height field that would be expected to be a float. But the ibc module logs it as 1-xxxx or 2-xxxx I am guessing based on which chain it's logged from. This breaks automatic type detection in log analysis software.

@mpoke mpoke closed this as completed in #628 Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature-request New feature or request improvement
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants