refactor(observability): extract common logging options to shared pac… by yehuditkerido · Pull Request #2395 · kubernetes-sigs/gateway-api-inference-extension

yehuditkerido · 2026-02-23T09:39:36Z

What type of PR is this?

/kind cleanup
/kind refactor

What this PR does / why we need it:

Extracts duplicated logging configuration code from BBR and EPP into a shared package at pkg/common/observability/logging/, eliminating ~90 lines of code duplication.

Both components had identical implementations for managing logging options (verbosity, zap configuration, validation). The new shared logging.Options struct is embedded into BBR and EPP using Go's struct embedding, preserving backward compatibility while centralizing the logic.

Changes:

New: pkg/common/observability/logging/ with options.go, errors.go, and comprehensive tests (159 lines)
BBR: Removed ~45 lines of duplicated code, now embeds logging.Options
EPP: Removed ~43 lines of duplicated code, now embeds logging.Options

All existing functionality preserved (default verbosity, flag behavior, validation). All tests pass including BBR/EPP integration tests.

Which issue(s) this PR fixes:

Fixes #2378

Does this PR introduce a user-facing change?:
None

k8s-ci-robot · 2026-02-23T09:39:39Z

@yehudit1987: The label(s) kind/refactor cannot be applied, because the repository doesn't have them.

Details

In response to this:

What type of PR is this?

/kind cleanup
/kind refactor

What this PR does / why we need it:

Extracts duplicated logging configuration code from BBR and EPP into a shared package at pkg/common/observability/logging/, eliminating ~90 lines of code duplication.

Both components had identical implementations for managing logging options (verbosity, zap configuration, validation). The new shared logging.Options struct is embedded into BBR and EPP using Go's struct embedding, preserving backward compatibility while centralizing the logic.

Changes:

New: pkg/common/observability/logging/ with options.go, errors.go, and comprehensive tests (159 lines)

BBR: Removed ~45 lines of duplicated code, now embeds logging.Options

EPP: Removed ~43 lines of duplicated code, now embeds logging.Options

All existing functionality preserved (default verbosity, flag behavior, validation). All tests pass including BBR/EPP integration tests.

Which issue(s) this PR fixes:

Fixes #2378

Does this PR introduce a user-facing change?:
None

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 · 2026-02-23T09:39:42Z

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

Name	Link
🔨 Latest commit	`94092b0`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/69a800755491c20008766a3a
😎 Deploy Preview	https://deploy-preview-2395--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 · 2026-02-23T09:39:46Z

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

Details

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.

nirrozenbaum · 2026-02-23T10:40:25Z

/ok-to-test

nirrozenbaum · 2026-03-01T21:12:48Z

@elevran any chance you have capacity to review this one?
as a side note, it would probably be useful to use the same common logging options in llm-d scheduler sidecar.

nirrozenbaum · 2026-03-03T11:21:34Z

 		GRPCHealthPort:      DefaultGrpcHealthPort,
-		LogVerbosity:        logging.DEFAULT,
-		ZapOptions:          zap.Options{Development: true},
+		Options:             *logging.NewOptions(),


can we name this LoggingOptions?
Options is pretty confusing here.

Suggested change

Options: *logging.NewOptions(),

LoggingOptions: *logging.NewOptions(),

nirrozenbaum · 2026-03-03T11:22:16Z

-	EnablePprof         bool        // Enables pprof handlers.
-	SecureServing       bool        // Enables secure serving.
-	MetricsEndpointAuth bool        // Enables authentication and authorization of the metrics endpoint.
+	logging.Options          // Embedded logging configuration.


Suggested change

logging.Options // Embedded logging configuration.

LoggingOptions logging.Options // Embedded logging configuration.

@nirrozenbaum what's the benefit of not embedding? Won't this require a larger refactor of the options.go in EPP and BBR since would need to indirect via LoggingOptions.field vs just field?

I'm ok with embedding, no benefit of not embedding.
just wanted to highlight that when embedding, the way to use it is by opts.Options, and that's very unclear that it actually holds the logging options.

if we name the struct in logging package as logging.LoggingOptions that would solve the issue (can embed without it being confusing)

if we name the struct in logging package as logging.LoggingOptions

that would create struttering in the full type name and is often discouraged. logging.Options seems fine to me.

nirrozenbaum · 2026-03-03T11:25:00Z

+var (
+	// ErrInvalidLogVerbosity is returned when log verbosity is negative.
+	ErrInvalidLogVerbosity = errors.New("log verbosity must be >= 0")
+)


can we move this var to logging/options.go?
it's strongly related to logging options and doesn't seem to worth a separated file for it.

nirrozenbaum · 2026-03-03T11:25:28Z

+)
+
+// Options contains logging configuration for command-line flags.
+type Options struct {


here it can remain Options

nirrozenbaum · 2026-03-03T11:27:19Z

-	EnableCertReload    bool        // Enables certificate reloading of the certificates specified in --cert-path.
-	SecureServing       bool        // Enables secure serving.
-	MetricsEndpointAuth bool        // Enables authentication and authorization of the metrics endpoint.
+	logging.Options            // Embedded logging configuration.


elevran · 2026-03-03T12:50:31Z

-	EnablePprof         bool        // Enables pprof handlers.
-	SecureServing       bool        // Enables secure serving.
-	MetricsEndpointAuth bool        // Enables authentication and authorization of the metrics endpoint.
+	logging.Options          // Embedded logging configuration.


@nirrozenbaum what's the benefit of not embedding? Won't this require a larger refactor of the options.go in EPP and BBR since would need to indirect via LoggingOptions.field vs just field?

elevran · 2026-03-03T12:52:53Z

 	PluginSpecs config.BBRPluginSpecs // Repeatable --plugin <type>:<name>[:<json>] flag values.
-
-	// internal
-	fs *pflag.FlagSet // FlagSet used in AddFlags() and consulted in Complete()


IIRC, the FlagSet is saved once and then used in other functions. Perhaps to facilitate testing which can use multiple sets to options and not use the "global" commandline set.
I did not implement the BBR options so I might be confusing with the EPP options.
Regardless, it should continue to work as long as we use embedding for logging.Options.

The current implementation works correctly, but for pattern consistency with EPP I added an fs field to BBR's Options struct as well.

elevran · 2026-03-03T13:00:15Z

+func (opts *Options) Validate() error {
+	// Log verbosity must be non-negative.
+	if opts.LogVerbosity < 0 {
+		return ErrInvalidLogVerbosity


or set to some default instead?

elevran · 2026-03-03T13:03:10Z

+	}
+}
+
+func TestComplete_ExplicitZapLevel(t *testing.T) {


nits:

consider moving this into the previous function so it can share the setup. Seems like could benefit from table-driven tests as in TestValidate() below.

is use of _ in test name a common practice? Curiosity, not change request (unless it is non-standard...)

Good observation on the naming. Coming from Python, the underscore convention didn't flag for me. I've changed it to the CamelCase standard.

elevran · 2026-03-03T13:05:24Z


 	// internal
-	fs *pflag.FlagSet // FlagSet used in AddFlags() and consulted in Complete()
+	fs *pflag.FlagSet // FlagSet used in AddFlags() and consulted in Validate()


Q: is there shadowing of fs here since logging.Options is embedded and has the same field name/type?

Yes, there was shadowing. I renamed the field in LoggingOptions from fs to loggingFS to avoid confusion.

elevran · 2026-03-03T13:09:09Z

+
+	// Add logging flags.
+	opts.Options.AddFlags(fs)
+


Be consistent in use of comments and empty whitespace.

Suggested change

// Add logging flags.

opts.Options.AddFlags(fs)

opts.Options.AddFlags(fs) // add logging flags.

question/observation: part of the reuse implies that EPP and BBR must use the same logging options and deprecate atthe same time? I don't see a case where this is not happening, but something to be aware of.
Can we independently deprecate in EPP or in BBR?

You're right - independent deprecation would need extra handling. But I think the trade-off is worth it. We're avoiding significant duplication (~90 lines) for functionality that should be consistent across components. If we ever need component-specific logging changes, we can handle them after calling AddFlags().

…kage EPP and BBR duplicated logging configuration code. Extract to pkg/common/observability/logging for reusability and consistent behavior across components. Signed-off-by: Yehudit Kerido <[email protected]>

elevran · 2026-03-04T12:04:35Z

/lgtm
leaving approval and additional feedback to @nirrozenbaum discretion.

nirrozenbaum

/lgtm
/approve

Thanks!

k8s-ci-robot · 2026-03-06T10:10:22Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

…kage (kubernetes-sigs#2395) EPP and BBR duplicated logging configuration code. Extract to pkg/common/observability/logging for reusability and consistent behavior across components. Signed-off-by: Yehudit Kerido <[email protected]>

…kage (kubernetes-sigs/gateway-api-inference-extension#2395) EPP and BBR duplicated logging configuration code. Extract to pkg/common/observability/logging for reusability and consistent behavior across components. Signed-off-by: Yehudit Kerido <[email protected]>

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 23, 2026

k8s-ci-robot requested review from danehans and robscott February 23, 2026 09:39

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 Feb 23, 2026

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 23, 2026

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 Feb 23, 2026

yehuditkerido force-pushed the refactor/extract-logging-options branch 2 times, most recently from a3074ea to ba66186 Compare February 23, 2026 12:07

nirrozenbaum reviewed Mar 3, 2026

View reviewed changes

Comment thread pkg/common/observability/logging/options.go Outdated

nirrozenbaum reviewed Mar 3, 2026

View reviewed changes

elevran reviewed Mar 3, 2026

View reviewed changes

refactor(observability): extract common logging options to shared pac…

94092b0

…kage EPP and BBR duplicated logging configuration code. Extract to pkg/common/observability/logging for reusability and consistent behavior across components. Signed-off-by: Yehudit Kerido <[email protected]>

yehuditkerido force-pushed the refactor/extract-logging-options branch from ba66186 to 94092b0 Compare March 4, 2026 09:50

k8s-ci-robot assigned elevran Mar 4, 2026

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2026

nirrozenbaum approved these changes Mar 6, 2026

View reviewed changes

k8s-ci-robot assigned nirrozenbaum Mar 6, 2026

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2026

k8s-ci-robot merged commit a538240 into kubernetes-sigs:main Mar 6, 2026
11 checks passed

yehuditkerido deleted the refactor/extract-logging-options branch March 8, 2026 06:18

	Options: *logging.NewOptions(),
	LoggingOptions: *logging.NewOptions(),

	logging.Options // Embedded logging configuration.
	LoggingOptions logging.Options // Embedded logging configuration.

Conversation

yehuditkerido commented Feb 23, 2026

Uh oh!

k8s-ci-robot commented Feb 23, 2026

Uh oh!

netlify Bot commented Feb 23, 2026 • 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 Feb 23, 2026

Uh oh!

nirrozenbaum commented Feb 23, 2026

Uh oh!

nirrozenbaum commented Mar 1, 2026

Uh oh!

nirrozenbaum Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

elevran commented Mar 4, 2026

Uh oh!

nirrozenbaum left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify Bot commented Feb 23, 2026 •

edited

Loading

nirrozenbaum Mar 3, 2026 •

edited

Loading

nirrozenbaum Mar 3, 2026 •

edited

Loading