-
Notifications
You must be signed in to change notification settings - Fork 33
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
[Telemetry] Apply shared logging module #420
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gustavobelfort great work! Thank you!
Do you think we could add more dimensions to the log lines, like I included in a couple of comments? This would allow to index/parse/query these fields easily, e.g. we could get logs related to one particular height
or step
. It just makes sense to make use of structured logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some REALLY high-quality work 🔥 💯 🔥
@Gustavobelfort Sorry for how long it took me to review this but this LFG
- I left a few comments & a few questions throughout the doc
- Since it took me so long to review this, you'll need to handle some merge conflicts with main.
- We'll need to update the documentation (general and how to use this). I'm totally okay with doing this in a followup so we can just merge this into main sooner.
As a starting point for the documentation (again, can be done in a followup), I've aggregated these notes while reviewing:
* When logging error with `err`: `logger.Global.Logger.Error().Err(err).Msg(msg)`
* When logging error without `err`: `logger.Global.Logger.Error().Msg(msg)`
* When logging a fatal error: `logger.Global.Fatal().Err(err).Msg(msg)`
* When do we have to do `logger.Create(runtimeMgr)`?
* `—decoration` flag
* If in a module, use `m.logger`
* Levels
* `.logger.Info()`
* `.logger.Debug()`
* Fields
* `.logger.Level().Fields()
* Parameters
* Configured parameters (applied everywhere)
* Fields parameter that (a map)
* Type specific parameters (`.Str`, `.Int`, etc…)
Co-authored-by: Daniel Olshansky <[email protected]>
@Gustavobelfort Saw that you pushed a lot of changes. Lmk if this is ready for another look |
Yes it is ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gustavobelfort Few things:
- I apologize for some repetitive comments you may find (I didn't go back to delete repetitive things)
- Do you plan to add documentation in this ticket or a follow-up?
- Please create the tickets for the follow-up items
- Minor comments & NITS left throughout
- The biggest issue with the PR is that LocalNet doesn't work:
Screen.Recording.2023-01-20.at.6.22.37.PM.mov
Co-authored-by: Daniel Olshansky <[email protected]>
@@ -194,6 +195,8 @@ func (m *consensusModule) Start() error { | |||
consensusTelemetry.CONSENSUS_BLOCKCHAIN_HEIGHT_COUNTER_DESCRIPTION, | |||
) | |||
|
|||
m.logger = logger.Global.CreateLoggerForModule(m.GetModuleName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the README, you specified that we should create this in Start
, but here we are creating it in Create
.
I realize it doesn't matter but just want to be consistent and set a good pattern to follow for other devs.
Can you pick up and update everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Olshansk from what I see you've left a comment in Start()
.
And I've checked other places CreateLoggerForModule
is called - they are also under Start()
.
@@ -1,6 +1,7 @@ | |||
package logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a README under logger/docs/README.md
?
I think a good outline (as a starting point) could be:
## 1. Configuration
* Mention ` —decoration` flag
## 2. Log Types
### 2.1 Levels
* When logging error with `err`: `logger.Global.Logger.Error().Err(err).Msg(msg)`
* When logging error without `err`: `logger.Global.Logger.Error().Msg(msg)`
* When logging a fatal error: `logger.Global.Fatal().Err(err).Msg(msg)`
### 2.2. Fields
* Fields
* `.logger.Level().Fields()
* Parameters
* Configured parameters (applied everywhere)
* Fields parameter that (a map)
* Type specific parameters (`.Str`, `.Int`, etc…)
## 3. Global Logging
## 4. Module Logging
* If in a module, use `m.logger`
### 4.1 Logger Initialisation
* When do we have to do `logger.Create(runtimeMgr)`?
* Show what we should do in start method
## 5. Accessing Logs
Go to localhost:XXX
### 5.1. Grafana
#### 5.2. Example Queries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thank you for starting that README!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, please beware I am not currently able to provide example queries.
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
logger/docs/README.md
Outdated
|
||
#### 5.2. Example Queries | ||
|
||
We will populate this section with useful queries as we go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With LocalNet and this branch not merged I can't build queries. We can populate this part later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okdas Reviewed your changes and pushed it over the finish.
Let's squash & merge it in and prepare the demo next week with some example queries!
Description
This:
log
calls with the logging moduleIssue
Fixes #288
Type of change
Please mark the relevant option(s):
List of changes
log
calls with the logging moduleTesting
make develop_test
README
Required Checklist
If Applicable Checklist
shared/docs/*
if I updatedshared/*
README(s)