From 8a59a9add37171c70cded91020453bdada23989b Mon Sep 17 00:00:00 2001 From: odubajDT Date: Thu, 14 Nov 2024 11:13:35 +0100 Subject: [PATCH] pr review Signed-off-by: odubajDT --- receiver/zipkinreceiver/config.go | 11 ++- receiver/zipkinreceiver/config_test.go | 77 +++++++++++++++----- receiver/zipkinreceiver/factory.go | 3 + receiver/zipkinreceiver/factory_test.go | 36 +++++++++ receiver/zipkinreceiver/testdata/config.yaml | 5 ++ 5 files changed, 112 insertions(+), 20 deletions(-) diff --git a/receiver/zipkinreceiver/config.go b/receiver/zipkinreceiver/config.go index f0d869c344c7..0682f89f665e 100644 --- a/receiver/zipkinreceiver/config.go +++ b/receiver/zipkinreceiver/config.go @@ -18,6 +18,8 @@ var disallowHTTPDefaultProtocol = featuregate.GlobalRegistry().MustRegister( featuregate.WithRegisterFromVersion("v0.114.0"), ) +const deprecationConfigMsg = "the inline setting of http server parameters has been deprecated, please use .protocols.http parameter instead." + // Config defines configuration for Zipkin receiver. type Config struct { // Configures the receiver server protocol. @@ -41,9 +43,12 @@ var _ component.Config = (*Config)(nil) // Validate checks the receiver configuration is valid func (cfg *Config) Validate() error { - if cfg.isServerConfigDefined() { + if isServerConfigDefined(cfg.ServerConfig) { if disallowHTTPDefaultProtocol.IsEnabled() { - return fmt.Errorf("the inline setting of http server parameters has been deprecated, please use protocols.http parameter instead") + return fmt.Errorf(deprecationConfigMsg) + } + if isServerConfigDefined(cfg.Protocols.HTTP) { + return fmt.Errorf("cannot use .protocols.http together with default server config setup") } cfg.Protocols.HTTP = cfg.ServerConfig cfg.ServerConfig = confighttp.ServerConfig{} @@ -53,7 +58,7 @@ func (cfg *Config) Validate() error { } // IsServerConfigDefined checks if the ServerConfig is defined by the user -func (cfg *Config) isServerConfigDefined() bool { +func isServerConfigDefined(cfg confighttp.ServerConfig) bool { return cfg.Endpoint != "" || cfg.TLSSetting != nil || cfg.CORS != nil || diff --git a/receiver/zipkinreceiver/config_test.go b/receiver/zipkinreceiver/config_test.go index 718fd6735744..24c3413c8853 100644 --- a/receiver/zipkinreceiver/config_test.go +++ b/receiver/zipkinreceiver/config_test.go @@ -4,6 +4,7 @@ package zipkinreceiver import ( + "fmt" "path/filepath" "testing" @@ -17,7 +18,7 @@ import ( "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/zipkinreceiver/internal/metadata" ) -func TestLoadConfig(t *testing.T) { +func TestValidateConfig(t *testing.T) { t.Parallel() cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) @@ -27,12 +28,8 @@ func TestLoadConfig(t *testing.T) { id component.ID disallowInline bool expected component.Config - wantErr bool + wantErr error }{ - { - id: component.NewID(metadata.Type), - expected: createDefaultConfig(), - }, { id: component.NewIDWithName(metadata.Type, "customname"), expected: &Config{ @@ -47,7 +44,7 @@ func TestLoadConfig(t *testing.T) { { id: component.NewIDWithName(metadata.Type, "customname"), disallowInline: true, - wantErr: true, + wantErr: fmt.Errorf(deprecationConfigMsg), }, { id: component.NewIDWithName(metadata.Type, "protocols"), @@ -75,26 +72,26 @@ func TestLoadConfig(t *testing.T) { { id: component.NewIDWithName(metadata.Type, "parse_strings"), expected: &Config{ - Protocols: ProtocolTypes{ - HTTP: confighttp.ServerConfig{ - Endpoint: defaultBindEndpoint, - }, - }, ParseStringTags: true, }, }, { id: component.NewIDWithName(metadata.Type, "parse_strings"), expected: &Config{ - Protocols: ProtocolTypes{ - HTTP: confighttp.ServerConfig{ - Endpoint: defaultBindEndpoint, - }, - }, ParseStringTags: true, }, disallowInline: true, }, + { + id: component.NewIDWithName(metadata.Type, "deprecated"), + disallowInline: true, + wantErr: fmt.Errorf(deprecationConfigMsg), + }, + { + id: component.NewIDWithName(metadata.Type, "deprecated"), + disallowInline: false, + wantErr: fmt.Errorf("cannot use .protocols.http together with default server config setup"), + }, } for _, tt := range tests { @@ -105,6 +102,52 @@ func TestLoadConfig(t *testing.T) { require.NoError(t, featuregate.GlobalRegistry().Set(disallowHTTPDefaultProtocol.ID(), false)) }) } + cfg := &Config{} + + sub, err := cm.Sub(tt.id.String()) + require.NoError(t, err) + require.NoError(t, sub.Unmarshal(cfg)) + + if tt.wantErr != nil { + assert.Equal(t, tt.wantErr, component.ValidateConfig(cfg)) + } else { + assert.NoError(t, component.ValidateConfig(cfg)) + assert.Equal(t, tt.expected, cfg) + } + }) + } +} + +func TestLoadConfig(t *testing.T) { + t.Parallel() + + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) + require.NoError(t, err) + + tests := []struct { + id component.ID + expected component.Config + wantErr bool + }{ + { + id: component.NewID(metadata.Type), + expected: createDefaultConfig(), + }, + { + id: component.NewIDWithName(metadata.Type, "parse_strings"), + expected: &Config{ + Protocols: ProtocolTypes{ + HTTP: confighttp.ServerConfig{ + Endpoint: defaultBindEndpoint, + }, + }, + ParseStringTags: true, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.id.String(), func(t *testing.T) { factory := NewFactory() cfg := factory.CreateDefaultConfig() diff --git a/receiver/zipkinreceiver/factory.go b/receiver/zipkinreceiver/factory.go index d8bcf0a062b3..47cb7a600955 100644 --- a/receiver/zipkinreceiver/factory.go +++ b/receiver/zipkinreceiver/factory.go @@ -51,5 +51,8 @@ func createTracesReceiver( nextConsumer consumer.Traces, ) (receiver.Traces, error) { rCfg := cfg.(*Config) + if isServerConfigDefined(rCfg.ServerConfig) { + set.Logger.Warn(deprecationConfigMsg) + } return newReceiver(rCfg, nextConsumer, set) } diff --git a/receiver/zipkinreceiver/factory_test.go b/receiver/zipkinreceiver/factory_test.go index c2051fa198a3..b1e70db4d34a 100644 --- a/receiver/zipkinreceiver/factory_test.go +++ b/receiver/zipkinreceiver/factory_test.go @@ -4,13 +4,18 @@ package zipkinreceiver import ( + "bytes" "context" "testing" + "github.com/open-telemetry/opentelemetry-collector-contrib/internal/common/testutil" "github.com/stretchr/testify/assert" "go.opentelemetry.io/collector/component/componenttest" + "go.opentelemetry.io/collector/config/confighttp" "go.opentelemetry.io/collector/consumer/consumertest" "go.opentelemetry.io/collector/receiver/receivertest" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" ) func TestCreateDefaultConfig(t *testing.T) { @@ -38,3 +43,34 @@ func TestCreateReceiver(t *testing.T) { assert.NoError(t, err, "receiver creation failed") assert.NotNil(t, tReceiver, "receiver creation failed") } + +func TestCreateReceiverDeprecatedWarning(t *testing.T) { + cfg := &Config{ + ServerConfig: confighttp.ServerConfig{ + Endpoint: testutil.EndpointForPort(defaultHTTPPort), + }, + ParseStringTags: false, + } + + var buffer bytes.Buffer + writer := zapcore.AddSync(&buffer) + + encoder := zapcore.NewConsoleEncoder(zap.NewDevelopmentEncoderConfig()) + core := zapcore.NewCore(encoder, writer, zapcore.DebugLevel) + logger := zap.New(core) + + set := receivertest.NewNopSettings() + set.Logger = logger + + _, _ = createTracesReceiver( + context.Background(), + set, + cfg, + consumertest.NewNop(), + ) + + logOutput := buffer.String() + if !bytes.Contains([]byte(logOutput), []byte(deprecationConfigMsg)) { + t.Errorf("Expected log message not found. Got: %s", logOutput) + } +} diff --git a/receiver/zipkinreceiver/testdata/config.yaml b/receiver/zipkinreceiver/testdata/config.yaml index 8c58da0e0e28..6028b8d5eea2 100644 --- a/receiver/zipkinreceiver/testdata/config.yaml +++ b/receiver/zipkinreceiver/testdata/config.yaml @@ -7,3 +7,8 @@ zipkin/protocols: endpoint: "localhost:8765" zipkin/parse_strings: parse_string_tags: true +zipkin/deprecated: + endpoint: "localhost:8766" + protocols: + http: + endpoint: "localhost:8765"