Use structured logging #330
Conversation
|
Hi @tchap. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
08605e5 to
63d12ea
Compare
be2fca6 to
dfab5ce
Compare
| @@ -16,7 +17,7 @@ func (f *FakePodMetricsClient) FetchMetrics(ctx context.Context, pod Pod, existi | |||
| if err, ok := f.Err[pod]; ok { | |||
| return nil, err | |||
| } | |||
| klog.V(1).Infof("pod: %+v\n existing: %+v \n new: %+v \n", pod, existing, f.Res[pod]) | |||
There was a problem hiding this comment.
This seems like a VERBOSE message to me, but not sure.
| @@ -101,7 +102,7 @@ func (s *Server) HandleRequestBody(reqCtx *RequestContext, req *extProcPb.Proces | |||
| } | |||
| // Print headers for debugging | |||
| for _, header := range headers { | |||
| klog.V(logutil.VERBOSE).Infof("[request_body] Header Key: %s, Header Value: %s\n", header.Header.Key, header.Header.RawValue) | |||
| klog.V(logutil.DEBUG).InfoS("Request body header", "key", header.Header.Key, "value", header.Header.RawValue) | |||
There was a problem hiding this comment.
Seems like DEBUG level to me.
|
/ok-to-test |
dfab5ce to
1074d01
Compare
| @@ -16,10 +17,10 @@ type healthServer struct { | |||
|
|
|||
| func (s *healthServer) Check(ctx context.Context, in *healthPb.HealthCheckRequest) (*healthPb.HealthCheckResponse, error) { | |||
| if !s.datastore.HasSynced() { | |||
| klog.Infof("gRPC health check not serving: %s", in.String()) | |||
There was a problem hiding this comment.
This seems like at least VERBOSE logging to me.
9a94693 to
9de3439
Compare
All logging calls are rewritten to use structured logging.
9de3439 to
c79e378
Compare
| @@ -26,19 +26,21 @@ func (c *InferenceModelReconciler) Reconcile(ctx context.Context, req ctrl.Reque | |||
| if req.Namespace != c.PoolNamespacedName.Namespace { | |||
| return ctrl.Result{}, nil | |||
| } | |||
| klog.V(1).Infof("Reconciling InferenceModel %v", req.NamespacedName) | |||
|
|
|||
| klogV := klog.V(logutil.DEFAULT) | |||
There was a problem hiding this comment.
I like this pattern, but what happens when I want to use a non-default log level, mixing the logging objects might be odd. WDYT?
There was a problem hiding this comment.
Well, you can always just do klog.V with another level when needed. I think that there is a function somewhere here that actually uses klogV for VERBOSE and klog.V(logutil.DEFAULT) is done explicitly. There is no rule really, just saving some typing.
Actually doing klogDEBUG := klog.V(logutil.DEBUG) would be more readable as klogV does not always point to the same verbose level, but I don't have a strong feeling about this.
There was a problem hiding this comment.
I think that I will reconsider this when implementing contextual logging, don't feel like postponing this.
|
overall lgtm! Just one real comment, and the decision to change some of the logging levels is totally fine to me. Thanks! This looks great |
|
/lgtm Feel free to unhold at your discretion |
|
/unhold |
|
Can someone approve this for me pls? The last thing missing 🙂 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, tchap 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 |
* Use structured logging All logging calls are rewritten to use structured logging. * test/integration: Use logutil.Fatal
All logging calls are rewritten to use structured logging.
I did
go run k8s.io/klog/hack/tools/logcheck -check-structured ./...and fixed all issues detected.Related to #251