Skip to content

Commit

Permalink
chore: remove redundant config Validate call (open-telemetry#35199)
Browse files Browse the repository at this point in the history
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
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:** <Issue number if applicable>
open-telemetry#33498

**Testing:** <Describe what testing was performed and which tests were
added.> 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:** <Describe the documentation added.>
  • Loading branch information
rogercoll authored and jriguera committed Oct 4, 2024
1 parent a698a4a commit 1e8fc35
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 43 deletions.
7 changes: 3 additions & 4 deletions exporter/elasticsearchexporter/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
Expand All @@ -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`)
})
}
Expand Down
8 changes: 2 additions & 6 deletions exporter/elasticsearchexporter/exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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 {
Expand Down
22 changes: 7 additions & 15 deletions exporter/elasticsearchexporter/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package elasticsearchexporter // import "github.com/open-telemetry/opentelemetry

import (
"context"
"fmt"
"net/http"
"time"

Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
18 changes: 0 additions & 18 deletions exporter/elasticsearchexporter/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down

0 comments on commit 1e8fc35

Please sign in to comment.