✨ Improve startup logs #1687
✨ Improve startup logs #1687
Conversation
Signed-off-by: Stefan Büringer [email protected]
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
| @@ -132,14 +132,14 @@ func (blder *WebhookBuilder) registerValidatingWebhook() { | |||
| func (blder *WebhookBuilder) registerConversionWebhook() error { | |||
| ok, err := conversion.IsConvertible(blder.mgr.GetScheme(), blder.apiType) | |||
| if err != nil { | |||
| log.Error(err, "conversion check failed", "object", blder.apiType) | |||
| log.Error(err, "conversion check failed", "GVK", blder.gvk) | |||
There was a problem hiding this comment.
apiType is usually just an empty API type (e.g. https://github.com/kubernetes-sigs/cluster-api/blob/f4380fc82fa2eef5119967d9bfea9d428e103e76/api/v1beta1/cluster_webhook.go#L34-L38)
So it's better to log gvk
We're also doing it already for the other webhooks, e.g. l.125
| @@ -175,7 +175,7 @@ func (c *Controller) Start(ctx context.Context) error { | |||
| // caches to sync so that they have a chance to register their intendeded | |||
| // caches. | |||
| for _, watch := range c.startWatches { | |||
| c.Log.Info("Starting EventSource", "source", watch.src) | |||
| c.Log.Info("Starting EventSource", "source", fmt.Sprintf("%s", watch.src)) | |||
There was a problem hiding this comment.
Otherwise the String() func is not called and instead the watch.src is logged, which is again mostly an empty api struct (link). Which looks roughly like that:
I1007 16:29:17.183304 1 controller.go:178] controller/machinehealthcheck "msg"="Starting EventSource" "reconciler group"="cluster.x-k8s.io" "reconciler kind"="MachineHealthCheck" "source"={"Type":{"metadata":{"creationTimestamp":null},"spec":{"clusterName":"","selector":{},"unhealthyConditions":null},"status":{"expectedMachines":0,"currentHealthy":0,"remediationsAllowed":0}}}
There was a problem hiding this comment.
Interesting, I would have expected the logger to call .String() if it is implemented. Maybe get rid of the fmt.Sprintf anduse just watch.src.String()?
There was a problem hiding this comment.
That was my first thought. But as String() is not part of the source.Source interface it's not possible without a cast (or adding it to the interface).
| @@ -142,10 +142,10 @@ func (ks *Kind) Start(ctx context.Context, handler handler.EventHandler, queue w | |||
| } | |||
|
|
|||
| func (ks *Kind) String() string { | |||
| if ks.Type != nil && ks.Type.GetObjectKind() != nil { | |||
| return fmt.Sprintf("kind source: %v", ks.Type.GetObjectKind().GroupVersionKind().String()) | |||
| if ks.Type != nil { | |||
There was a problem hiding this comment.
Assuming that source.Kind is usually created with an empty struct like here there is in ObjectKind but it's empty. So this would look something like this kind source: .
So if we don't want to look up the gvk via the struct, the best we can do is simply log the Go type.
|
Lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, sbueringer 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 |
k8s-ci-robot
added
the
approved
|
/cherrypick release-0.10 |
|
@sbueringer: new pull request created: #1692 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Stefan Büringer [email protected]
Just a few improvements to the startup log:
Full startup log example in CAPI with these changes: