-
Notifications
You must be signed in to change notification settings - Fork 799
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 an source
to all log statements
#103
Conversation
3166019
to
8bd109f
Compare
Source or Origin are good. 👍 |
So |
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.
There are a couple of small things worth considering. Otherwise, the consolidation of logrus
use into the runtime
package is very nice.
cmd/controller/main.go
Outdated
var ( | ||
log = runtime.NewLoggerWithOrigin("main") | ||
) | ||
|
||
func init() { | ||
logrus.SetFormatter(&logrus.JSONFormatter{}) |
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.
Now that the logging concerns are encapsulated within the runtime
package, I think it would be nice to move this line into runtime
as well. This means all uses of logrus
are gathered in a single place.
cmd/controller/main.go
Outdated
@@ -46,6 +46,10 @@ const ( | |||
keyFileFlag = "key-file" | |||
) | |||
|
|||
var ( | |||
log = runtime.NewLoggerWithOrigin("main") |
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.
Granted the log
package isn't being used here, but to avoid an ambiguity logger
might be better.
cmd/sdk-server/main.go
Outdated
var ( | ||
log = runtime.NewLoggerWithOrigin("main") | ||
) | ||
|
||
func init() { | ||
logrus.SetFormatter(&logrus.JSONFormatter{}) |
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.
See above about moving this line into the runtime
package as well as renaming log
to logger
.
@Kuqd - I think I like @enocom I like all the above as well, I'll also make those changes. 👍 |
This includes a `logger` as a data members for all objects, which sets a default "source" field on every log statement so that it's easy to track where log messages are coming from. This is particularly useful as the controller for Agones gets more complicated.
8bd109f
to
d99b932
Compare
origin
to all log statementssource
to all log statements
Changes made! PTAL! |
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.
LGTM
This includes a
logger
as a data members for all objects, which sets a default "source" field on every log statement so that it's easy to track where log messages are coming from.This is particularly useful as the overall controller for Agones gets more complicated, and we have many interacting controllers in a single binary.
Log statements now look like this:
So now you can see the context of the log statement through the "source" field.
Would love to hear your thoughts on if this is a good idea, if there is a better word than "origin", or anything else - @dzlier-gcp @enocom @Kuqd