chore: remove unneeded logging before returning error #1544

phuhung273 · 2025-09-07T07:35:17Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Remove unneeded logging right before returning error.

How I decide which logging is useful:

Useful: logging with extra info

gateway-api-inference-extension/cmd/bbr/main.go

Lines 98 to 99 in 8b154ba

setupLog.Error(err, "Failed to create manager", "config", cfg)

return err
Not useful: logging which say "something failed"

Would be great if you can feedback if my decision is reasonable. Thanks team.

Which issue(s) this PR fixes:
Fixes #334

Does this PR introduce a user-facing change?:

NONE

netlify · 2025-09-07T07:35:23Z

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

Name	Link
🔨 Latest commit	`0b67e91`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/68c4ec93b4db0e00083119d5
😎 Deploy Preview	https://deploy-preview-1544--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 project configuration.

k8s-ci-robot · 2025-09-07T07:35:27Z

Welcome @phuhung273!

It looks like this is your first PR to kubernetes-sigs/gateway-api-inference-extension 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api-inference-extension has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-09-07T07:35:27Z

Hi @phuhung273. 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.

kfswain · 2025-09-10T21:11:47Z

I'm a little torn, I think the setupLog errors we may want to keep, but the others are probably okay to remove?

phuhung273 · 2025-09-11T00:06:52Z

Thanks @kfswain for your advice 👍 setupLog is restored. I'm new to the codebase so don't have much context.

nirrozenbaum · 2025-09-11T07:35:40Z

/ok-to-test

nirrozenbaum

@phuhung273 thanks for your PR.
I left comments in few places. I think we should address those (I left only in few examples, I assume you will find the other places).

basically the main point is to keep the original error msg and wrap the errors (wrap errors is the best practice for error handling).

nirrozenbaum · 2025-09-11T07:41:38Z

internal/runnable/grpc.go

 		if err != nil {
-			log.Error(err, "gRPC server failed to listen")
 			return err


it's ok to remove the log.Error here, but can we wrap err so that the original err test is kept?

Suggested change

log.Error(err, "gRPC server failed to listen")

return err

return fmt.Errorf("gRPC server failed to listen - %w", err)

Thanks for your instruction 👍 I couldnt commit your suggestion because GH doesn't support committing suggestion on deleted lines yet. Could you have a look on the manual changes.

nirrozenbaum · 2025-09-11T07:43:17Z

internal/runnable/grpc.go

 		if err := srv.Serve(lis); err != nil && err != grpc.ErrServerStopped {
-			log.Error(err, "gRPC server failed")
 			return err


ditto:

Suggested change

log.Error(err, "gRPC server failed")

return err

return fmt.Errof("gRPC server failed - %w", err)

Thanks, updated according to your suggestion

nirrozenbaum · 2025-09-11T07:44:27Z

pkg/bbr/server/runserver.go

 			if err != nil {
-				logger.Error(err, "Failed to create self signed certificate")
 				return err


ditto

Suggested change

logger.Error(err, "Failed to create self signed certificate")

return err

return fmt.Errorf("Failed to create self signed certificate - %w", err)

Thanks, updated according to your suggestion

nirrozenbaum · 2025-09-11T08:16:55Z

pkg/epp/controller/inferencepool_reconciler.go

 		if err != nil {
-			logger.Error(err, "Failed to convert XInferencePool to InferencePool")
 			return ctrl.Result{}, err


these seem like useful error messages when trying to debug:

Suggested change

logger.Error(err, "Failed to convert XInferencePool to InferencePool")

return ctrl.Result{}, err

return ctrl.Result{}, fmt.Errorf("Failed to convert XInferencePool to InferencePool - %w", err)

Thanks, updated according to your suggestion

nirrozenbaum · 2025-09-12T19:32:19Z

pkg/epp/handlers/server.go

 			}
 			if err := srv.Send(resp); err != nil {
-				logger.V(logutil.DEFAULT).Error(err, "Send failed")
 				return status.Errorf(codes.Unknown, "failed to send response back to Envoy: %v", err)


is this error logged in the pod?
(I think not, so we might need log line back in the server.go both under epp and bbr)

I really have no idea 😅 Maybe we can keep everything in server.go and remove only this part which the team is already complaining

gateway-api-inference-extension/pkg/bbr/handlers/server.go

Lines 64 to 66 in d03d3b6

// This error occurs very frequently, though it doesn't seem to have any impact.

// TODO Figure out if we can remove this noise.

loggerVerbose.Error(recvErr, "Cannot receive stream request")

nirrozenbaum

/lgtm
/approve

k8s-ci-robot · 2025-09-13T19:41:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nirrozenbaum, phuhung273

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~~ [nirrozenbaum]

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

…1544)

k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 7, 2025

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 7, 2025

k8s-ci-robot requested review from ahg-g and nirrozenbaum September 7, 2025 07:35

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 7, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 7, 2025

phuhung273 force-pushed the remove-logging-before-err branch from bc00e9f to c6c06b8 Compare September 11, 2025 00:04

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 11, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 11, 2025

nirrozenbaum reviewed Sep 11, 2025

View reviewed changes

phuhung273 force-pushed the remove-logging-before-err branch from c6c06b8 to 4dd2dcf Compare September 11, 2025 11:47

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 11, 2025

phuhung273 requested a review from nirrozenbaum September 12, 2025 11:22

nirrozenbaum reviewed Sep 12, 2025

View reviewed changes

chore: remove unneeded logging before returning err

0b67e91

phuhung273 force-pushed the remove-logging-before-err branch from 4dd2dcf to 0b67e91 Compare September 13, 2025 04:01

nirrozenbaum approved these changes Sep 13, 2025

View reviewed changes

k8s-ci-robot assigned nirrozenbaum Sep 13, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2025

k8s-ci-robot merged commit dcfa5fe into kubernetes-sigs:main Sep 13, 2025
9 checks passed

phuhung273 deleted the remove-logging-before-err branch September 14, 2025 01:24

BenjaminBraunDev pushed a commit to BenjaminBraunDev/gateway-api-inference-extension that referenced this pull request Oct 28, 2025

chore: remove unneeded logging before returning err (kubernetes-sigs#…

a43eba4

…1544)

	setupLog.Error(err, "Failed to create manager", "config", cfg)
	return err

	log.Error(err, "gRPC server failed to listen")
	return err
	return fmt.Errorf("gRPC server failed to listen - %w", err)

	log.Error(err, "gRPC server failed")
	return err
	return fmt.Errof("gRPC server failed - %w", err)

	logger.Error(err, "Failed to create self signed certificate")
	return err
	return fmt.Errorf("Failed to create self signed certificate - %w", err)

	logger.Error(err, "Failed to convert XInferencePool to InferencePool")
	return ctrl.Result{}, err
	return ctrl.Result{}, fmt.Errorf("Failed to convert XInferencePool to InferencePool - %w", err)

	// This error occurs very frequently, though it doesn't seem to have any impact.
	// TODO Figure out if we can remove this noise.
	loggerVerbose.Error(recvErr, "Cannot receive stream request")

chore: remove unneeded logging before returning error #1544

chore: remove unneeded logging before returning error #1544

Uh oh!

Conversation

phuhung273 commented Sep 7, 2025

Uh oh!

netlify bot commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

k8s-ci-robot commented Sep 7, 2025

Uh oh!

k8s-ci-robot commented Sep 7, 2025

Uh oh!

kfswain commented Sep 10, 2025

Uh oh!

phuhung273 commented Sep 11, 2025

Uh oh!

nirrozenbaum commented Sep 11, 2025

Uh oh!

nirrozenbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirrozenbaum left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify bot commented Sep 7, 2025 •

edited

Loading