Use contextual logging #337

tchap · 2025-02-14T11:45:16Z

All possible direct klog calls are removed.
Instead logr.Logger is loaded from the context
or passed around as an argument.

Related to #251 , this basically concludes the issue.

Testing

Regarding testing, this is pretty hard to test. I am not really able to run integration tests and failing to call ctrl.SetLogger and loading a logger from the context would result, I guess, in log entries being dropped. I don't think this can really happen as ctrl.SetLogger is being called for sure, but it does seem a bit fragile.

k8s-ci-robot · 2025-02-14T11:45:26Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

netlify · 2025-02-14T11:45:59Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`9712144`
🔍 Latest deploy log	https://app.netlify.com/sites/gateway-api-inference-extension/deploys/67af2ccf177a940008856f59
😎 Deploy Preview	https://deploy-preview-337--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2025-02-14T11:46:31Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`8e8f36f`
🔍 Latest deploy log	https://app.netlify.com/sites/gateway-api-inference-extension/deploys/67b0e388f8bcdb0009ef5b02
😎 Deploy Preview	https://deploy-preview-337--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

pkg/ext-proc/handlers/request.go

pkg/ext-proc/handlers/server.go

tchap · 2025-02-14T11:49:37Z

pkg/ext-proc/metrics/metrics_test.go

-				targetModelName: "t20",
-				receivedTime:    timeBaseline,
-				completeTime:    timeBaseline.Add(time.Millisecond * 120),
+	}{


Formatting improved as a side effect, ignore.

tchap · 2025-02-14T11:50:54Z

pkg/ext-proc/backend/provider.go

-			if err := p.refreshMetricsOnce(); err != nil {
-				klog.V(logutil.TRACE).ErrorS(err, "Failed to refresh metrics")
+			if err := p.refreshMetricsOnce(logger); err != nil {
+				logger.V(logutil.DEFAULT).Error(err, "Failed to refresh metrics")


I actually changed this to DEFAULT as I feel logging errors on TRACE is weird, but unsure.

This can just get very spammy if the errors are transient, but I suppose it's a fair point that you want to be aware of the error regardless.

ahg-g

I have a few comments that don't block this PR and we can address as followup since it is huge. But overall I am onboard with this. will let @kfswain make the final pass and lgtm when ready.

pkg/ext-proc/backend/datastore.go

pkg/ext-proc/handlers/request.go

pkg/ext-proc/handlers/server.go

pkg/ext-proc/scheduling/filter.go

kfswain · 2025-02-14T17:48:06Z

This overall lgtm, I do agree with the context as the param so long as the logger lookup is not expensive. However I'm familiarizing myself with https://github.com/go-logr/logr/tree/v1.4.2 before I LGTM. Will follow up shortly

kfswain · 2025-02-14T18:16:00Z

Read through the blog post linked in: https://github.com/go-logr/logr/tree/v1.4.2?tab=readme-ov-file#inspiration As well as where logr chooses to disagree. And I'm aligned with those opinions for certain.

/lgtm
/hold

I think the context comments can be handled in a following PR, just holding to give time for an Ack. Thanks for all the help!

tchap · 2025-02-15T13:14:33Z

Aligned the log levels and passing a ctx around instead of a logger, so all review issues should be fixed now.

Seems I also still need ok-to-test...

tchap · 2025-02-15T13:49:56Z

/unhold

kfswain · 2025-02-15T14:12:52Z

/ok-to-test

All possible direct klog calls are removed. Instead logr.Logger is loaded from the context or passed around as an argument.

pkg/ext-proc/handlers/request.go

ahg-g

Sorry, a couple more nits

pkg/ext-proc/backend/pod_reconciler.go

Co-authored-by: Abdullah Gharaibeh <[email protected]>

ahg-g · 2025-02-15T19:02:16Z

/lgtm
/approve

This is awesome, great work!

k8s-ci-robot · 2025-02-15T19:02:27Z

[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:

~~OWNERS~~ [ahg-g]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from Jeffwan and robscott February 14, 2025 11:45

k8s-ci-robot added cncf-cla: yes needs-ok-to-test labels Feb 14, 2025

k8s-ci-robot added the size/XL label Feb 14, 2025

tchap force-pushed the contextual-logging branch from 9712144 to 4c6d700 Compare February 14, 2025 11:45

tchap commented Feb 14, 2025

View reviewed changes

pkg/ext-proc/handlers/request.go Outdated Show resolved Hide resolved

tchap commented Feb 14, 2025

View reviewed changes

pkg/ext-proc/handlers/server.go Outdated Show resolved Hide resolved

tchap commented Feb 14, 2025

View reviewed changes

ahg-g reviewed Feb 14, 2025

View reviewed changes

pkg/ext-proc/backend/datastore.go Outdated Show resolved Hide resolved

pkg/ext-proc/handlers/request.go Outdated Show resolved Hide resolved

pkg/ext-proc/handlers/server.go Outdated Show resolved Hide resolved

pkg/ext-proc/scheduling/filter.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold label Feb 14, 2025

k8s-ci-robot assigned kfswain Feb 14, 2025

k8s-ci-robot added lgtm and removed lgtm labels Feb 14, 2025

tchap force-pushed the contextual-logging branch from df7a7ae to dc4c922 Compare February 15, 2025 13:17

k8s-ci-robot removed the do-not-merge/hold label Feb 15, 2025

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 15, 2025

tchap force-pushed the contextual-logging branch from dc4c922 to 2320fe5 Compare February 15, 2025 15:21

Use contextual logging

Loading
Loading status checks…

04b2967

All possible direct klog calls are removed. Instead logr.Logger is loaded from the context or passed around as an argument.

tchap force-pushed the contextual-logging branch from 2320fe5 to 04b2967 Compare February 15, 2025 15:23

ahg-g reviewed Feb 15, 2025

View reviewed changes

pkg/ext-proc/handlers/request.go Outdated Show resolved Hide resolved

pkg/ext-proc/handlers/request.go Outdated Show resolved Hide resolved

tchap added 2 commits February 15, 2025 19:03

Fix log levels

Loading
Loading status checks…

9ff11b6

server: Handle context canceled

Loading
Loading status checks…

2943613

ahg-g reviewed Feb 15, 2025

View reviewed changes

pkg/ext-proc/backend/pod_reconciler.go Outdated Show resolved Hide resolved

pkg/ext-proc/backend/pod_reconciler.go Outdated Show resolved Hide resolved

tchap and others added 2 commits February 15, 2025 19:55

pod_reconciler: Use TRACE log level

Loading
Loading status checks…

d5d6c7e

Co-authored-by: Abdullah Gharaibeh <[email protected]>

pod_reconciler: Don't log pod not found err

Loading
Loading status checks…

8e8f36f

k8s-ci-robot assigned ahg-g Feb 15, 2025

k8s-ci-robot added the lgtm label Feb 15, 2025

k8s-ci-robot added the approved label Feb 15, 2025

k8s-ci-robot merged commit 6b42ab8 into kubernetes-sigs:main Feb 15, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use contextual logging #337

Use contextual logging #337

tchap commented Feb 14, 2025 •

edited

Loading

k8s-ci-robot commented Feb 14, 2025

netlify bot commented Feb 14, 2025 •

edited

Loading

netlify bot commented Feb 14, 2025 •

edited

Loading

tchap Feb 14, 2025

tchap Feb 14, 2025

kfswain Feb 14, 2025

ahg-g left a comment

kfswain commented Feb 14, 2025

kfswain commented Feb 14, 2025

tchap commented Feb 15, 2025

tchap commented Feb 15, 2025

kfswain commented Feb 15, 2025

ahg-g left a comment

ahg-g commented Feb 15, 2025

k8s-ci-robot commented Feb 15, 2025

Use contextual logging #337

Use contextual logging #337

Conversation

tchap commented Feb 14, 2025 • edited Loading

Testing

k8s-ci-robot commented Feb 14, 2025

netlify bot commented Feb 14, 2025 • edited Loading

✅ Deploy Preview for gateway-api-inference-extension ready!

netlify bot commented Feb 14, 2025 • edited Loading

✅ Deploy Preview for gateway-api-inference-extension ready!

tchap Feb 14, 2025

Choose a reason for hiding this comment

tchap Feb 14, 2025

Choose a reason for hiding this comment

kfswain Feb 14, 2025

Choose a reason for hiding this comment

ahg-g left a comment

Choose a reason for hiding this comment

kfswain commented Feb 14, 2025

kfswain commented Feb 14, 2025

tchap commented Feb 15, 2025

tchap commented Feb 15, 2025

kfswain commented Feb 15, 2025

ahg-g left a comment

Choose a reason for hiding this comment

ahg-g commented Feb 15, 2025

k8s-ci-robot commented Feb 15, 2025

tchap commented Feb 14, 2025 •

edited

Loading

netlify bot commented Feb 14, 2025 •

edited

Loading

netlify bot commented Feb 14, 2025 •

edited

Loading