From e5e9e879cba62ad3b7079f5825cf9021a443835d Mon Sep 17 00:00:00 2001 From: Roger Coll Date: Mon, 16 Sep 2024 23:16:52 +0200 Subject: [PATCH] chore: remove redundant config Validate call (#35199) **Description:** Configuration validation is done during collector's startup, making it redundant when being called inside component's logic. This PR removes the Validate call done during exporter's constructor. **Link to tracking Issue:** https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/33498 **Testing:** Same coverage after removed lines: `coverage: 77.8% of statements` `TestFactory_CreateLogsExporter_Fail` and `TestFactory_CreateTracesExporter_Fail` functions are covered by the already available `config_test.go` cases. **Documentation:** --- exporter/elasticsearchexporter/config_test.go | 7 +++--- exporter/elasticsearchexporter/exporter.go | 8 ++----- exporter/elasticsearchexporter/factory.go | 22 ++++++------------- .../elasticsearchexporter/factory_test.go | 18 --------------- 4 files changed, 12 insertions(+), 43 deletions(-) diff --git a/exporter/elasticsearchexporter/config_test.go b/exporter/elasticsearchexporter/config_test.go index 8bc9de0b5f3f..a27a28ccfe6e 100644 --- a/exporter/elasticsearchexporter/config_test.go +++ b/exporter/elasticsearchexporter/config_test.go @@ -395,8 +395,7 @@ func TestConfig_Validate(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { - err := tt.config.Validate() - assert.EqualError(t, err, tt.err) + assert.EqualError(t, component.ValidateConfig(tt.config), tt.err) }) } } @@ -405,13 +404,13 @@ func TestConfig_Validate_Environment(t *testing.T) { t.Run("valid", func(t *testing.T) { t.Setenv("ELASTICSEARCH_URL", "http://test:9200") config := withDefaultConfig() - err := config.Validate() + err := component.ValidateConfig(config) require.NoError(t, err) }) t.Run("invalid", func(t *testing.T) { t.Setenv("ELASTICSEARCH_URL", "http://valid:9200, *:!") config := withDefaultConfig() - err := config.Validate() + err := component.ValidateConfig(config) assert.EqualError(t, err, `invalid endpoint "*:!": parse "*:!": first path segment in URL cannot contain colon`) }) } diff --git a/exporter/elasticsearchexporter/exporter.go b/exporter/elasticsearchexporter/exporter.go index 2bf4c0250fa4..6f70a9ac6b84 100644 --- a/exporter/elasticsearchexporter/exporter.go +++ b/exporter/elasticsearchexporter/exporter.go @@ -42,11 +42,7 @@ func newExporter( set exporter.Settings, index string, dynamicIndex bool, -) (*elasticsearchExporter, error) { - if err := cfg.Validate(); err != nil { - return nil, err - } - +) *elasticsearchExporter { model := &encodeModel{ dedot: cfg.Mapping.Dedot, mode: cfg.MappingMode(), @@ -72,7 +68,7 @@ func newExporter( model: model, logstashFormat: cfg.LogstashFormat, otel: otel, - }, nil + } } func (e *elasticsearchExporter) Start(ctx context.Context, host component.Host) error { diff --git a/exporter/elasticsearchexporter/factory.go b/exporter/elasticsearchexporter/factory.go index da97ba7d9f05..3f48ca1e2ec7 100644 --- a/exporter/elasticsearchexporter/factory.go +++ b/exporter/elasticsearchexporter/factory.go @@ -7,7 +7,6 @@ package elasticsearchexporter // import "github.com/open-telemetry/opentelemetry import ( "context" - "fmt" "net/http" "time" @@ -113,10 +112,7 @@ func createLogsExporter( } logConfigDeprecationWarnings(cf, set.Logger) - exporter, err := newExporter(cf, set, index, cf.LogsDynamicIndex.Enabled) - if err != nil { - return nil, fmt.Errorf("cannot configure Elasticsearch exporter: %w", err) - } + exporter := newExporter(cf, set, index, cf.LogsDynamicIndex.Enabled) return exporterhelper.NewLogsExporter( ctx, @@ -135,10 +131,8 @@ func createMetricsExporter( cf := cfg.(*Config) logConfigDeprecationWarnings(cf, set.Logger) - exporter, err := newExporter(cf, set, cf.MetricsIndex, cf.MetricsDynamicIndex.Enabled) - if err != nil { - return nil, fmt.Errorf("cannot configure Elasticsearch exporter: %w", err) - } + exporter := newExporter(cf, set, cf.MetricsIndex, cf.MetricsDynamicIndex.Enabled) + return exporterhelper.NewMetricsExporter( ctx, set, @@ -150,15 +144,13 @@ func createMetricsExporter( func createTracesExporter(ctx context.Context, set exporter.Settings, - cfg component.Config) (exporter.Traces, error) { - + cfg component.Config, +) (exporter.Traces, error) { cf := cfg.(*Config) logConfigDeprecationWarnings(cf, set.Logger) - exporter, err := newExporter(cf, set, cf.TracesIndex, cf.TracesDynamicIndex.Enabled) - if err != nil { - return nil, fmt.Errorf("cannot configure Elasticsearch exporter: %w", err) - } + exporter := newExporter(cf, set, cf.TracesIndex, cf.TracesDynamicIndex.Enabled) + return exporterhelper.NewTracesExporter( ctx, set, diff --git a/exporter/elasticsearchexporter/factory_test.go b/exporter/elasticsearchexporter/factory_test.go index a6e2c356d981..5acc985e008e 100644 --- a/exporter/elasticsearchexporter/factory_test.go +++ b/exporter/elasticsearchexporter/factory_test.go @@ -35,15 +35,6 @@ func TestFactory_CreateLogsExporter(t *testing.T) { require.NoError(t, exporter.Shutdown(context.Background())) } -func TestFactory_CreateLogsExporter_Fail(t *testing.T) { - factory := NewFactory() - cfg := factory.CreateDefaultConfig() - params := exportertest.NewNopSettings() - _, err := factory.CreateLogsExporter(context.Background(), params, cfg) - require.Error(t, err, "expected an error when creating a logs exporter") - assert.EqualError(t, err, "cannot configure Elasticsearch exporter: exactly one of [endpoint, endpoints, cloudid] must be specified") -} - func TestFactory_CreateMetricsExporter(t *testing.T) { factory := NewFactory() cfg := withDefaultConfig(func(cfg *Config) { @@ -70,15 +61,6 @@ func TestFactory_CreateTracesExporter(t *testing.T) { require.NoError(t, exporter.Shutdown(context.Background())) } -func TestFactory_CreateTracesExporter_Fail(t *testing.T) { - factory := NewFactory() - cfg := factory.CreateDefaultConfig() - params := exportertest.NewNopSettings() - _, err := factory.CreateTracesExporter(context.Background(), params, cfg) - require.Error(t, err, "expected an error when creating a traces exporter") - assert.EqualError(t, err, "cannot configure Elasticsearch exporter: exactly one of [endpoint, endpoints, cloudid] must be specified") -} - func TestFactory_CreateLogsAndTracesExporterWithDeprecatedIndexOption(t *testing.T) { factory := NewFactory() cfg := withDefaultConfig(func(cfg *Config) {