-
Notifications
You must be signed in to change notification settings - Fork 302
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 context logging for all controllers #2436
Conversation
/assign @cezarygerard |
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.
Add some comments in pkg/healthshecks and pkg/backends. Most comments apply for other places too. PTAL!
37574fe
to
ea68ffe
Compare
ea68ffe
to
adf3010
Compare
adf3010
to
12fb4ff
Compare
/hold cancel |
@@ -81,16 +81,17 @@ func main() { | |||
os.Exit(0) | |||
} | |||
|
|||
klog.V(0).Infof("Starting GLBC image: %q, cluster name %q", version.Version, flags.F.ClusterName) | |||
klog.V(0).Infof("Latest commit hash: %q", version.GitCommit) | |||
rootLogger := klog.TODO() |
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.
maybe we should use klog.Background()
here? I tried finding a recommended way of creating a new root logger but I have no idea
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 don't know as well, but TODO()
technically just invokes Background()
@@ -109,18 +111,21 @@ type L4ILBParams struct { | |||
} | |||
|
|||
// NewL4Handler creates a new L4Handler for the given L4 service. | |||
func NewL4Handler(params *L4ILBParams) *L4 { | |||
func NewL4Handler(params *L4ILBParams, logger klog.Logger) *L4 { | |||
logger = logger.WithName("L4Handler") |
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.
maybe we should also do .WithValues("service", svcKey)
here so that all lines produced by this handler have service details?
we could do this as a follow up not to make things too complicated
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 believe if we do this right then we can get rid of the "serviceKey" key that is added to many log lines in the handlers
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 agree, but I'd rather postpone it for a follow-up. It feels like we already have a lot of changes, so I'd suggest to have straightforward change from klog...
to context logger, and then have another PR with all the improvements
|
||
if ctx.NetworkInformer != nil { | ||
l4c.networkLister = ctx.NetworkInformer.GetIndexer() | ||
} | ||
if ctx.GKENetworkParamsInformer != nil { | ||
l4c.gkeNetworkParamSetLister = ctx.GKENetworkParamsInformer.GetIndexer() | ||
} | ||
l4c.networkResolver = network.NewNetworksResolver(l4c.networkLister, l4c.gkeNetworkParamSetLister, ctx.Cloud, ctx.EnableMultinetworking, klog.TODO()) | ||
l4c.networkResolver = network.NewNetworksResolver(l4c.networkLister, l4c.gkeNetworkParamSetLister, ctx.Cloud, ctx.EnableMultinetworking, 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.
I think I messed up the Resolver. Probably it should not keep the logger reference but instead accept a param when resolving the service network so that the lines from there have all context. I'll have to redo it later
fa2022b
to
d254353
Compare
d254353
to
3d34563
Compare
Alex, add more description to this PR
|
Makes sense, thanks, added |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexkats, cezarygerard 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 |
This PR adds context logging, with these changes we'll have only one root logger, and all other loggers are children, which gives us the ability to track the context of the source of a log line. It also changes the way of logging, we have a pair of key/values instead of formatting string. It gives the ability to introduce a logger with some common key/value pairs and not repeat them every time in the following log lines. As an example, when we process some service and have 20 log lines, we can create a new logger with
serviceKey
and it will be logged in all of those 20 lines. This PR aims to do only the first iteration of introducing context loggers everywhere, and in order not to make it even bigger, all other improvements (especially with more complex contexts and loggers with specified key/value pair) will be in the follow-up PRs.