-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
create DefaultFieldsFormatter and inject component into all prow messages #6850
Conversation
This also eliminates most of the |
/assign @cjwagner @Kargakis @stevekuznetsov |
*/ | ||
|
||
// Package logrusutil implements some helpers for using logrus | ||
package logrusutil |
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.
the interesting part is in this package, the rest is just migrating everything to it.
4fc0aa8
to
4f49730
Compare
Side note: I've deployed this for the bazel cache experiment (not prow) because it also fixes a bug in the DefaultFormatter... 😬 |
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
/hold
Feel free to ignore my nit. This change is great!
prow/cmd/sinker/main.go
Outdated
@@ -79,7 +84,7 @@ func main() { | |||
kubeClients[alias] = kubeClient(client) | |||
} | |||
c := controller{ | |||
logger: logger, | |||
logger: logrus.WithFields(logrus.Fields{}), |
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.
logrus.NewEntry(logrus.StandardLogger())
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 👍
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, cjwagner The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4f49730
to
0811332
Compare
// DefaultFields into each Format() call, existing fields are preserved | ||
// if they have the same key | ||
type DefaultFieldsFormatter struct { | ||
WrappedFormatter logrus.Formatter |
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.
We could make this default to the JSON formatter if it is nil.
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.
OK how about we have NewDefaultFieldsFormatter(wrapped logrus.Formatter, defaultFields logrus.Fields)
and default to json if wrapped is nil?
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, switched to this everywhere
0811332
to
1fe65b1
Compare
/hold cancel |
Cool! |
I had this idea when working on the bazel remote cache server: Wrap the logrus formatter with one that injects default fields so we can use the
logrus.Fields{"component": "tide"}
pattern across all log messages within a binary.I've created a small package implementing this and migrated all of the prow components to use it and standardized the logging a bit.
/area prow