Skip to content

Commit 4bff0c4

Browse files
yehuditkeridoRyanRosario
authored andcommitted
refactor(observability): extract common logging options to shared package (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 <yehudit1987@gmail.com>
1 parent abac41c commit 4bff0c4

5 files changed

Lines changed: 289 additions & 67 deletions

File tree

pkg/bbr/server/options.go

Lines changed: 15 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,9 @@ limitations under the License.
1717
package server
1818

1919
import (
20-
"flag"
2120
"fmt"
2221

2322
"github.com/spf13/pflag"
24-
uberzap "go.uber.org/zap"
25-
"go.uber.org/zap/zapcore"
26-
"sigs.k8s.io/controller-runtime/pkg/log/zap"
2723

2824
"sigs.k8s.io/gateway-api-inference-extension/pkg/bbr/config"
2925
"sigs.k8s.io/gateway-api-inference-extension/pkg/common/observability/logging"
@@ -32,7 +28,6 @@ import (
3228
const (
3329
DefaultGrpcPort = 9004
3430
DefaultGrpcHealthPort = 9005
35-
ZapLogLevelFlagName = "zap-log-level"
3631
)
3732

3833
// Options contains the command-line configuration for the BBR server.
@@ -45,29 +40,27 @@ type Options struct {
4540
//
4641
// Diagnostics.
4742
//
48-
LogVerbosity int // Number for the log level verbosity.
49-
ZapOptions zap.Options // Zap logging options.
50-
MetricsPort int // The metrics port exposed by BBR.
51-
GRPCHealthPort int // The port for gRPC liveness and readiness probes.
52-
EnablePprof bool // Enables pprof handlers.
53-
SecureServing bool // Enables secure serving.
54-
MetricsEndpointAuth bool // Enables authentication and authorization of the metrics endpoint.
43+
logging.LoggingOptions // Logging configuration.
44+
MetricsPort int // The metrics port exposed by BBR.
45+
GRPCHealthPort int // The port for gRPC liveness and readiness probes.
46+
EnablePprof bool // Enables pprof handlers.
47+
SecureServing bool // Enables secure serving.
48+
MetricsEndpointAuth bool // Enables authentication and authorization of the metrics endpoint.
5549
//
5650
// Plugins.
5751
//
5852
PluginSpecs config.BBRPluginSpecs // Repeatable --plugin <type>:<name>[:<json>] flag values.
5953

6054
// internal
61-
fs *pflag.FlagSet // FlagSet used in AddFlags() and consulted in Complete()
55+
fs *pflag.FlagSet // FlagSet used in AddFlags()
6256
}
6357

6458
// NewOptions returns a new Options struct initialized with default values.
6559
func NewOptions() *Options {
6660
return &Options{
6761
GRPCPort: DefaultGrpcPort,
6862
GRPCHealthPort: DefaultGrpcHealthPort,
69-
LogVerbosity: logging.DEFAULT,
70-
ZapOptions: zap.Options{Development: true},
63+
LoggingOptions: *logging.NewOptions(),
7164
MetricsPort: 9090,
7265
EnablePprof: true,
7366
SecureServing: true,
@@ -80,6 +73,7 @@ func (opts *Options) AddFlags(fs *pflag.FlagSet) {
8073
if fs == nil {
8174
fs = pflag.CommandLine
8275
}
76+
8377
opts.fs = fs
8478

8579
fs.IntVar(&opts.GRPCPort, "grpc-port", opts.GRPCPort,
@@ -94,30 +88,18 @@ func (opts *Options) AddFlags(fs *pflag.FlagSet) {
9488
"Enables streaming support for Envoy full-duplex streaming mode.")
9589
fs.BoolVar(&opts.SecureServing, "secure-serving", opts.SecureServing,
9690
"Enables secure serving.")
97-
fs.IntVarP(&opts.LogVerbosity, "v", "v", opts.LogVerbosity,
98-
"Number for the log level verbosity.")
9991
fs.BoolVar(&opts.EnablePprof, "enable-pprof", opts.EnablePprof,
10092
"Enables pprof handlers. Defaults to true. Set to false to disable pprof handlers.")
10193

10294
fs.Var(&opts.PluginSpecs, "plugin", `Repeatable. --plugin <type>:<name>[:<json>]`)
10395

104-
// Bind zap flags (zap expects a standard Go FlagSet; pflag.FlagSet is not compatible).
105-
gofs := flag.NewFlagSet("zap", flag.ExitOnError)
106-
opts.ZapOptions.BindFlags(gofs)
107-
fs.AddGoFlagSet(gofs)
96+
opts.LoggingOptions.AddFlags(fs) // Add logging flags.
10897
}
10998

11099
// Complete performs post-processing of parsed command-line arguments.
111100
func (opts *Options) Complete() error {
112-
// Derive the zap log level from the -v flag when --zap-log-level is not set explicitly.
113-
zapLogLevelFlag := opts.fs.Lookup(ZapLogLevelFlagName)
114-
if zapLogLevelFlag != nil && !zapLogLevelFlag.Changed {
115-
// See https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/log/zap#Options.Level
116-
lvl := -1 * (opts.LogVerbosity)
117-
opts.ZapOptions.Level = uberzap.NewAtomicLevelAt(zapcore.Level(int8(lvl)))
118-
zapLogLevelFlag.Changed = true
119-
}
120-
return nil
101+
// Complete logging options.
102+
return opts.LoggingOptions.Complete()
121103
}
122104

123105
// Validate checks the Options for invalid or conflicting values.
@@ -147,9 +129,9 @@ func (opts *Options) Validate() error {
147129
opts.GRPCPort, opts.GRPCHealthPort, opts.MetricsPort)
148130
}
149131

150-
// Validate log verbosity is non-negative.
151-
if opts.LogVerbosity < 0 {
152-
return fmt.Errorf("invalid value %d for flag %q: must be >= 0", opts.LogVerbosity, "v")
132+
// Validate logging options.
133+
if err := opts.LoggingOptions.Validate(); err != nil {
134+
return err
153135
}
154136

155137
return nil

pkg/bbr/server/options_test.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ import (
2020
"testing"
2121

2222
"github.com/spf13/pflag"
23+
24+
"sigs.k8s.io/gateway-api-inference-extension/pkg/common/observability/logging"
2325
)
2426

2527
func TestNewOptionsDefaults(t *testing.T) {
@@ -161,9 +163,9 @@ func TestValidate(t *testing.T) {
161163
},
162164
// Log verbosity validation.
163165
{
164-
name: "negative log verbosity",
166+
name: "negative log verbosity corrected to default",
165167
mutate: func(o *Options) { o.LogVerbosity = -1 },
166-
expectError: true,
168+
expectError: false,
167169
},
168170
{
169171
name: "zero log verbosity is valid",
@@ -213,7 +215,7 @@ func TestCompleteDerivesZapLogLevel(t *testing.T) {
213215

214216
// After Complete(), the zap-log-level flag should be marked as changed
215217
// and the zap level should be set to -5.
216-
zapFlag := fs.Lookup(ZapLogLevelFlagName)
218+
zapFlag := fs.Lookup(logging.ZapLogLevelFlagName)
217219
if zapFlag == nil {
218220
t.Fatal("Expected zap-log-level flag to exist after AddFlags")
219221
}
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package logging
18+
19+
import (
20+
"flag"
21+
22+
"github.com/spf13/pflag"
23+
uberzap "go.uber.org/zap"
24+
"go.uber.org/zap/zapcore"
25+
"sigs.k8s.io/controller-runtime/pkg/log/zap"
26+
)
27+
28+
const (
29+
ZapLogLevelFlagName = "zap-log-level"
30+
)
31+
32+
// LoggingOptions contains logging configuration for command-line flags.
33+
type LoggingOptions struct {
34+
LogVerbosity int // Number for the log level verbosity.
35+
ZapOptions zap.Options // Zap logging options.
36+
37+
// internal
38+
loggingFS *pflag.FlagSet // FlagSet used in AddFlags() and consulted in Complete()
39+
}
40+
41+
// NewOptions returns a new LoggingOptions struct initialized with default values.
42+
func NewOptions() *LoggingOptions {
43+
return &LoggingOptions{
44+
LogVerbosity: DEFAULT,
45+
ZapOptions: zap.Options{Development: true},
46+
}
47+
}
48+
49+
// AddFlags binds the LoggingOptions fields to command-line flags on the given FlagSet.
50+
func (opts *LoggingOptions) AddFlags(fs *pflag.FlagSet) {
51+
if fs == nil {
52+
fs = pflag.CommandLine
53+
}
54+
opts.loggingFS = fs
55+
56+
fs.IntVarP(&opts.LogVerbosity, "v", "v", opts.LogVerbosity,
57+
"Number for the log level verbosity.")
58+
59+
// Bind zap flags (zap expects a standard Go FlagSet; pflag.FlagSet is not compatible).
60+
gofs := flag.NewFlagSet("zap", flag.ExitOnError)
61+
opts.ZapOptions.BindFlags(gofs)
62+
fs.AddGoFlagSet(gofs)
63+
}
64+
65+
// Complete performs post-processing of parsed command-line arguments.
66+
// Derives the zap log level from the -v flag when --zap-log-level is not set explicitly.
67+
func (opts *LoggingOptions) Complete() error {
68+
zapLogLevelFlag := opts.loggingFS.Lookup(ZapLogLevelFlagName)
69+
if zapLogLevelFlag != nil && !zapLogLevelFlag.Changed {
70+
// See https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/log/zap#Options.Level
71+
lvl := -1 * (opts.LogVerbosity)
72+
opts.ZapOptions.Level = uberzap.NewAtomicLevelAt(zapcore.Level(int8(lvl)))
73+
zapLogLevelFlag.Changed = true
74+
}
75+
return nil
76+
}
77+
78+
// Validate checks the LoggingOptions for invalid values.
79+
func (opts *LoggingOptions) Validate() error {
80+
// Log verbosity must be non-negative; set to default if invalid.
81+
if opts.LogVerbosity < 0 {
82+
opts.LogVerbosity = DEFAULT
83+
}
84+
return nil
85+
}
Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package logging
18+
19+
import (
20+
"flag"
21+
"testing"
22+
23+
"github.com/spf13/pflag"
24+
uberzap "go.uber.org/zap"
25+
"go.uber.org/zap/zapcore"
26+
)
27+
28+
func TestNewOptions(t *testing.T) {
29+
opts := NewOptions()
30+
if opts.LogVerbosity != DEFAULT {
31+
t.Errorf("Expected LogVerbosity to be %d, got %d", DEFAULT, opts.LogVerbosity)
32+
}
33+
if !opts.ZapOptions.Development {
34+
t.Error("Expected ZapOptions.Development to be true")
35+
}
36+
}
37+
38+
func TestAddFlags(t *testing.T) {
39+
opts := NewOptions()
40+
fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
41+
opts.AddFlags(fs)
42+
43+
// Check that the -v flag was added
44+
if fs.Lookup("v") == nil {
45+
t.Error("Expected -v flag to be added")
46+
}
47+
48+
// Check that zap flags were added
49+
if fs.Lookup(ZapLogLevelFlagName) == nil {
50+
t.Errorf("Expected %s flag to be added", ZapLogLevelFlagName)
51+
}
52+
}
53+
54+
func TestComplete(t *testing.T) {
55+
tests := []struct {
56+
name string
57+
args []string
58+
expectedVerbosity int
59+
expectedZapLevel zapcore.Level
60+
zapShouldDerive bool
61+
}{
62+
{
63+
name: "derive zap level from v flag",
64+
args: []string{"-v=3"},
65+
expectedVerbosity: 3,
66+
expectedZapLevel: zapcore.Level(-3),
67+
zapShouldDerive: true,
68+
},
69+
{
70+
name: "explicit zap level takes precedence",
71+
args: []string{"-v=5", "--zap-log-level=info"},
72+
expectedVerbosity: 5,
73+
expectedZapLevel: zapcore.InfoLevel,
74+
zapShouldDerive: false,
75+
},
76+
}
77+
78+
for _, tt := range tests {
79+
t.Run(tt.name, func(t *testing.T) {
80+
opts := NewOptions()
81+
fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
82+
opts.AddFlags(fs)
83+
84+
err := fs.Parse(tt.args)
85+
if err != nil {
86+
t.Fatalf("Failed to parse flags: %v", err)
87+
}
88+
89+
err = opts.Complete()
90+
if err != nil {
91+
t.Fatalf("Complete() failed: %v", err)
92+
}
93+
94+
if opts.LogVerbosity != tt.expectedVerbosity {
95+
t.Errorf("Expected LogVerbosity to be %d, got %d", tt.expectedVerbosity, opts.LogVerbosity)
96+
}
97+
98+
atomicLevel, ok := opts.ZapOptions.Level.(uberzap.AtomicLevel)
99+
if !ok {
100+
t.Fatalf("Expected ZapOptions.Level to be zap.AtomicLevel, got %T", opts.ZapOptions.Level)
101+
}
102+
actualLevel := atomicLevel.Level()
103+
if actualLevel != tt.expectedZapLevel {
104+
t.Errorf("Expected zap level to be %v, got %v", tt.expectedZapLevel, actualLevel)
105+
}
106+
107+
zapLogLevelFlag := fs.Lookup(ZapLogLevelFlagName)
108+
if zapLogLevelFlag == nil {
109+
t.Fatal("zap-log-level flag not found")
110+
}
111+
if !zapLogLevelFlag.Changed {
112+
t.Error("Expected zap-log-level flag to be marked as changed after Complete()")
113+
}
114+
})
115+
}
116+
}
117+
118+
func TestValidate(t *testing.T) {
119+
tests := []struct {
120+
name string
121+
verbosity int
122+
expectError bool
123+
}{
124+
{"valid verbosity 0", 0, false},
125+
{"valid verbosity 2", 2, false},
126+
{"valid verbosity 5", 5, false},
127+
{"negative verbosity corrected to default", -1, false},
128+
}
129+
130+
for _, tt := range tests {
131+
t.Run(tt.name, func(t *testing.T) {
132+
opts := NewOptions()
133+
opts.LogVerbosity = tt.verbosity
134+
135+
err := opts.Validate()
136+
if tt.expectError && err == nil {
137+
t.Error("Expected error but got nil")
138+
}
139+
if !tt.expectError && err != nil {
140+
t.Errorf("Expected no error but got: %v", err)
141+
}
142+
})
143+
}
144+
}
145+
146+
func TestValidate_SetsDefaultForNegativeVerbosity(t *testing.T) {
147+
opts := NewOptions()
148+
opts.LogVerbosity = -1
149+
150+
err := opts.Validate()
151+
if err != nil {
152+
t.Errorf("Expected no error, got %v", err)
153+
}
154+
if opts.LogVerbosity != DEFAULT {
155+
t.Errorf("Expected LogVerbosity to be set to DEFAULT (%d), got %d", DEFAULT, opts.LogVerbosity)
156+
}
157+
}
158+
159+
func init() {
160+
// Clear any global flags from other tests
161+
flag.CommandLine = flag.NewFlagSet("", flag.ContinueOnError)
162+
}

0 commit comments

Comments
 (0)