-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 agent in logger setup #3060
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3060 +/- ##
==========================================
+ Coverage 54.49% 56.07% +1.57%
==========================================
Files 544 407 -137
Lines 38884 30241 -8643
==========================================
- Hits 21190 16957 -4233
+ Misses 16441 12430 -4011
+ Partials 1253 854 -399
|
9eae950
to
728a60b
Compare
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.
Left some thoughts.
@@ -20,8 +20,19 @@ import ( | |||
"github.com/cihub/seelog" | |||
) | |||
|
|||
// LoggerName is a string prepended to all logs to indicate which component is logging | |||
type LoggerName string |
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.
What's the advantage of having a type here vs. plain string constants? The documentation itself is a hint. Why not just add a string argument to SetupLogger
calling it prefix
? It would be much clearer what it does (imho).
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.
As it would be preprended to all logs I feel having a dedicated type here helps enforcing that you know what you're doing when calling the function. WDYT?
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.
I understand your motivation to underline the fact that this will show in every log. I'd be fine with that. Maybe we can change the documentation a bit and simply say:
// LoggerName specifies the name of an instantiated logger.
I feel a bit like saying "...is a string prepended to all logs..." is misleading because this declaration doesn't say anything about its usage. I'm thinking it'd be better suited to say that the name is prepended to all logs above SetupLogger
.
But this is just a nit, I'm fine to keep it like this.
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.
I still feel that this is misleading. This isn't the "name of an instantiated logger", it's just a "prefix that will be prepended to each logged line".
728a60b
to
8b66988
Compare
da0a427
to
4b568de
Compare
4b568de
to
0d4e807
Compare
@@ -20,8 +20,19 @@ import ( | |||
"github.com/cihub/seelog" | |||
) | |||
|
|||
// LoggerName is a string prepended to all logs to indicate which component is logging | |||
type LoggerName string |
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.
I still feel that this is misleading. This isn't the "name of an instantiated logger", it's just a "prefix that will be prepended to each logged line".
--- | ||
enhancements: | ||
- | | ||
Change the logging format to include the name of the agent 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.
Change the logging format to include the name of the agent logging | |
Change the logging format to include the name of the logging agent. |
Wasn't this included previously? We had [TRACE]
, [CORE]
, etc. What's the actual change? Or were some missing. Sorry if I missed something here.
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 was present in the agent container yes due to the s6 filtering, I should probably rephrase 🤔
What does this PR do?
Add logging agent in the logger setup.
Examples:
Json:
Motivation
To be merged once all agents are using the core
log
package as it will allow to remove thes6-format-filter
Fix #1599
Additional Notes
Anything else we should know when reviewing?