-
Notifications
You must be signed in to change notification settings - Fork 613
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
structured logs and logfile rollover features #2311
Conversation
c6d68f0
to
6d190e9
Compare
388100e
to
d25a549
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.
Looks good.
|
||
func init() { | ||
once.Do(initLogger) | ||
func logfmtFormatter(params string) seelog.FormatterFunc { |
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 we add a piece of example log file in the pr description? It will be more straightforward for us to know what the log look like now.
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.
sure!
reloadConfig() | ||
} | ||
} | ||
|
||
// GetLevel gets the log level | ||
func GetLevel() string { | ||
levelLock.RLock() | ||
defer levelLock.RUnlock() | ||
Config.lock.Lock() |
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.
Isn't read lock good enough here since we are just reading 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.
yes but I am using just a plain sync.Mutex....a RWMutex seemed like overkill given the low-throughput expectation here
|
||
SetLevel(os.Getenv(LOGLEVEL_ENV_VAR)) | ||
if RolloverType := os.Getenv(LOG_ROLLOVER_TYPE_ENV_VAR); RolloverType != "" { | ||
Config.RolloverType = RolloverType |
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.
If the rollover type and output format are not supported, what will happen? Will it fail some time? Or agent may just run with malformat log?
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.
seelog should fail the create the logger, which means it would fallback to the default seelog logger and log an error
@@ -14,87 +14,162 @@ | |||
package logger | |||
|
|||
import ( | |||
"fmt" |
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.
Any test we can add to double check the log is generated as we expected or maybe unit test to make sure the functions in the file work as expected?
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 can try to write some unit tests for the seelog formatter funcs
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
ac669f6
to
a10b413
Compare
When you rebase, can you have the list of logging changes available in the commit message itself? |
Summary
Addresses part of #2284
Example logfmt log:
Example json log:
Implementation details
We are using seelog's custom formatters to write our own logfmt and json formatters, and then adding configuration variables to switch between these.
Testing
This is being tested as part of a broader logging refactor project.
Description for the changelog
Enhancement - structured logs and logfile rollover features
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.