Add logging policy #5299
Merged
Add logging policy #5299
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dapplion
added
the
meta-discussion
Performance Report✔️ no performance regression detected Full benchmark results
|
wemeetagain
reviewed
Mar 23, 2023
CONTRIBUTING.md
Outdated
| - `verbose`: _TBD_ | ||
| - `debug`: Detailed diagnostic information that can help developers or users troubleshoot specific issues. Examples include individual request logs for every REST API, networking interactions, or internal components status changes. | ||
|
|
||
| ### Guideliness |
There was a problem hiding this comment.
I feel like we need a guideline about when/where to include a stack trace
There was a problem hiding this comment.
That's a good point. How could we summarize it? I would say that when the error message provides all possible location information the stack trace can be omitted. We do not support suppressing stack traces out of the box, and the uncontrolled nature of Javascript exceptions could be an issue if we start omitting stack traces. very relevant to this topic https://www.youtube.com/watch?v=6cap2HiBFVQ
There was a problem hiding this comment.
Are we ok with not adding a bit more details about stack traces? I think the ApiError is a pretty good example of when not to include the stack trace as changed in #5285
- it is thrown on a higher level in the stack
- it is explicitly thrown based on simple assertions
- the error is expected to happen due to invalid client requests
- the error message is usually good which makes it easy to find out where it was thrown
Not including the stack trace of course can come with a cost of not being able to easily debug an issue but this should be rarely the case if we are careful to only do it for some errors but let's not forget the cost of including the stack trace in terms of UX. Not being able to easily read the logs because of too much stack trace is also not ideal.
I would argue that debug logs can include stack traces more often/always but for levels higher than or equal info need to be more careful.
There was a problem hiding this comment.
I just approved the PR so this didn't sit for too long. Happy to merge more verbiage about when stack traces are acceptable. Totally agree with your comment.
|
sounds pretty good to me 👍 I don't (yet) have that strong opinions on how the logging should be done within Lodestar myself but I have written (3 years ago) a log level usage/policy guide myself before which could be used as additional inspiration. Just to add some context first, the defined logging policy was for a SaaS solution, meaning the logs were mostly meant to be consumed by developers/administrators and were used to create a log-based alerting system on top. It also has to be noted that logs were not the only tool used to diagnose issues, there were also metrics and additionally traces (jaeger) which were directly linked to logs. This is how the logging policy was defined:
|
nflaig
reviewed
Mar 24, 2023
Co-authored-by: Cayman <[email protected]>
maschad
reviewed
Mar 28, 2023
maschad
reviewed
Mar 28, 2023
|
TBH from #5320 it's not clear to me what log would be debug or trace. Do you have any specific test if a specific log should go in one or the other? I would even argue to log everything that's not info to debug and call it a day, super simple. In our deployments we only track debug. In CI we only persist debug. And in Javascript logging is not free, if we start adding crazy frequent trace statements it's gonna impact performance |
Given that we may want to consider whether it's ideal to have trace logs then. The primary intention of debug logs imo should be to assist developers rather than consumers, and so perhaps we should omit trace logs for now. |
IMO using logs to do some sort of tracing is not the best option (maybe during development). Something like opentelemetry is much better to trace the application because you can nicely visualize it in jaeger and not have to read through 1000 lines of logs. Also I would assume that |
nflaig
previously requested changes
Mar 30, 2023
wemeetagain
reviewed
Mar 31, 2023
wemeetagain
approved these changes
Apr 5, 2023
nflaig
added a commit
that referenced
this pull request
Apr 17, 2023
Replaced by "Logging policy" section added in #5299
wemeetagain
pushed a commit
that referenced
this pull request
Apr 17, 2023
Replaced by "Logging policy" section added in #5299
|
🎉 This PR is included in v1.8.0 🎉 |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Lodestar's logging should be more consistent across all contributors. It also not clear what should be logged at error vs warn, or at verbose vs debug.
This PR adds my own strong opinions and is marked as draft to receive feedback from other contributors.