Skip to content

Commit

Permalink
[otlpexporter] Validate the configuration explicitly (#9523)
Browse files Browse the repository at this point in the history
**Description:**
Use `Config.Validate` to validate the presence of the gRPC endpoint
instead of making an assertion at creation time.
  • Loading branch information
atoulme authored Feb 9, 2024
1 parent a58768f commit de6287d
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 36 deletions.
9 changes: 9 additions & 0 deletions exporter/otlpexporter/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package otlpexporter // import "go.opentelemetry.io/collector/exporter/otlpexporter"

import (
"errors"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configgrpc"
"go.opentelemetry.io/collector/config/configretry"
Expand All @@ -19,4 +21,11 @@ type Config struct {
configgrpc.ClientConfig `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct.
}

func (c *Config) Validate() error {
if c.SanitizedEndpoint() == "" {
return errors.New(`requires a non-empty "endpoint"`)
}
return nil
}

var _ component.Config = (*Config)(nil)
36 changes: 36 additions & 0 deletions exporter/otlpexporter/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,39 @@ func TestUnmarshalConfig(t *testing.T) {
},
}, cfg)
}

func TestUnmarshalInvalidConfig(t *testing.T) {
cm, err := confmaptest.LoadConf(filepath.Join("testdata", "invalid_configs.yaml"))
require.NoError(t, err)
factory := NewFactory()
for _, test := range []struct {
name string
errorMsg string
}{
{
name: "no_endpoint",
errorMsg: `requires a non-empty "endpoint"`,
},
{
name: "http_endpoint",
errorMsg: `requires a non-empty "endpoint"`,
},
{
name: "invalid_timeout",
errorMsg: `'timeout' must be non-negative`,
},
{
name: "invalid_retry",
errorMsg: `'randomization_factor' must be within [0, 1]`,
},
} {
t.Run(test.name, func(t *testing.T) {
cfg := factory.CreateDefaultConfig()
sub, err := cm.Sub(test.name)
require.NoError(t, err)
assert.NoError(t, component.UnmarshalConfig(sub, cfg))
assert.ErrorContains(t, component.ValidateConfig(cfg), test.errorMsg)
})
}

}
15 changes: 3 additions & 12 deletions exporter/otlpexporter/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,7 @@ func createTracesExporter(
set exporter.CreateSettings,
cfg component.Config,
) (exporter.Traces, error) {
oce, err := newExporter(cfg, set)
if err != nil {
return nil, err
}
oce := newExporter(cfg, set)
oCfg := cfg.(*Config)
return exporterhelper.NewTracesExporter(ctx, set, cfg,
oce.pushTraces,
Expand All @@ -68,10 +65,7 @@ func createMetricsExporter(
set exporter.CreateSettings,
cfg component.Config,
) (exporter.Metrics, error) {
oce, err := newExporter(cfg, set)
if err != nil {
return nil, err
}
oce := newExporter(cfg, set)
oCfg := cfg.(*Config)
return exporterhelper.NewMetricsExporter(ctx, set, cfg,
oce.pushMetrics,
Expand All @@ -89,10 +83,7 @@ func createLogsExporter(
set exporter.CreateSettings,
cfg component.Config,
) (exporter.Logs, error) {
oce, err := newExporter(cfg, set)
if err != nil {
return nil, err
}
oce := newExporter(cfg, set)
oCfg := cfg.(*Config)
return exporterhelper.NewLogsExporter(ctx, set, cfg,
oce.pushLogs,
Expand Down
20 changes: 3 additions & 17 deletions exporter/otlpexporter/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,10 @@ func TestCreateMetricsExporter(t *testing.T) {
func TestCreateTracesExporter(t *testing.T) {
endpoint := testutil.GetAvailableLocalAddress(t)
tests := []struct {
name string
config *Config
mustFailOnCreate bool
mustFailOnStart bool
name string
config *Config
mustFailOnStart bool
}{
{
name: "NoEndpoint",
config: &Config{
ClientConfig: configgrpc.ClientConfig{
Endpoint: "",
},
},
mustFailOnCreate: true,
},
{
name: "UseSecure",
config: &Config{
Expand Down Expand Up @@ -178,10 +168,6 @@ func TestCreateTracesExporter(t *testing.T) {
factory := NewFactory()
set := exportertest.NewNopCreateSettings()
consumer, err := factory.CreateTracesExporter(context.Background(), set, tt.config)
if tt.mustFailOnCreate {
assert.NotNil(t, err)
return
}
assert.NoError(t, err)
assert.NotNil(t, consumer)
err = consumer.Start(context.Background(), componenttest.NewNopHost())
Expand Down
9 changes: 2 additions & 7 deletions exporter/otlpexporter/otlp.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package otlpexporter // import "go.opentelemetry.io/collector/exporter/otlpexpor

import (
"context"
"errors"
"fmt"
"runtime"
"time"
Expand Down Expand Up @@ -47,17 +46,13 @@ type baseExporter struct {
userAgent string
}

func newExporter(cfg component.Config, set exporter.CreateSettings) (*baseExporter, error) {
func newExporter(cfg component.Config, set exporter.CreateSettings) *baseExporter {
oCfg := cfg.(*Config)

if oCfg.Endpoint == "" {
return nil, errors.New("OTLP exporter config requires an Endpoint")
}

userAgent := fmt.Sprintf("%s/%s (%s/%s)",
set.BuildInfo.Description, set.BuildInfo.Version, runtime.GOOS, runtime.GOARCH)

return &baseExporter{config: oCfg, settings: set.TelemetrySettings, userAgent: userAgent}, nil
return &baseExporter{config: oCfg, settings: set.TelemetrySettings, userAgent: userAgent}
}

// start actually creates the gRPC connection. The client construction is deferred till this point as this
Expand Down
55 changes: 55 additions & 0 deletions exporter/otlpexporter/testdata/invalid_configs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
no_endpoint:
timeout: 10s
sending_queue:
enabled: true
num_consumers: 2
queue_size: 10
retry_on_failure:
enabled: true
initial_interval: 10s
randomization_factor: 0.7
multiplier: 1.3
max_interval: 60s
max_elapsed_time: 10m
http_endpoint:
endpoint: http://
timeout: 10s
sending_queue:
enabled: true
num_consumers: 2
queue_size: 10
retry_on_failure:
enabled: true
initial_interval: 10s
randomization_factor: 0.7
multiplier: 1.3
max_interval: 60s
max_elapsed_time: 10m
invalid_timeout:
endpoint: example.com:443
timeout: -5s
sending_queue:
enabled: true
num_consumers: 2
queue_size: 10
retry_on_failure:
enabled: true
initial_interval: 10s
randomization_factor: 0.7
multiplier: 1.3
max_interval: 60s
max_elapsed_time: 10m
invalid_retry:
endpoint: example.com:443
timeout: 30s
sending_queue:
enabled: true
num_consumers: 2
queue_size: 10
retry_on_failure:
enabled: true
initial_interval: 10s
randomization_factor: -5
multiplier: 1.3
max_interval: 60s
max_elapsed_time: 10m

0 comments on commit de6287d

Please sign in to comment.