Skip to content

Commit

Permalink
[chore] Remove manual calls to Validate
Browse files Browse the repository at this point in the history
  • Loading branch information
evan-bradley committed Jan 31, 2025
1 parent 349b84b commit cb1fb4f
Show file tree
Hide file tree
Showing 7 changed files with 21 additions and 72 deletions.
4 changes: 2 additions & 2 deletions otelcol/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func (col *Collector) setupConfigurationComponents(ctx context.Context) error {
return fmt.Errorf("failed to get config: %w", err)
}

if err = cfg.Validate(); err != nil {
if err = component.ValidateConfig(cfg); err != nil {
return fmt.Errorf("invalid configuration: %w", err)
}

Expand Down Expand Up @@ -261,7 +261,7 @@ func (col *Collector) DryRun(ctx context.Context) error {
return fmt.Errorf("failed to get config: %w", err)
}

return cfg.Validate()
return component.ValidateConfig(cfg)
}

func newFallbackLogger(options []zap.Option) (*zap.Logger, error) {
Expand Down
38 changes: 1 addition & 37 deletions otelcol/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,39 +54,14 @@ func (cfg *Config) Validate() error {
return errMissingReceivers
}

// Validate the receiver configuration.
for recvID, recvCfg := range cfg.Receivers {
if err := component.ValidateConfig(recvCfg); err != nil {
return fmt.Errorf("receivers::%s: %w", recvID, err)
}
}

// Currently, there is no default exporter enabled.
// The configuration must specify at least one exporter to be valid.
if len(cfg.Exporters) == 0 {
return errMissingExporters
}

// Validate the exporter configuration.
for expID, expCfg := range cfg.Exporters {
if err := component.ValidateConfig(expCfg); err != nil {
return fmt.Errorf("exporters::%s: %w", expID, err)
}
}

// Validate the processor configuration.
for procID, procCfg := range cfg.Processors {
if err := component.ValidateConfig(procCfg); err != nil {
return fmt.Errorf("processors::%s: %w", procID, err)
}
}

// Validate the connector configuration.
for connID, connCfg := range cfg.Connectors {
if err := component.ValidateConfig(connCfg); err != nil {
return fmt.Errorf("connectors::%s: %w", connID, err)
}

for connID := range cfg.Connectors {
if _, ok := cfg.Exporters[connID]; ok {
return fmt.Errorf("connectors::%s: ambiguous ID: Found both %q exporter and %q connector. "+
"Change one of the components' IDs to eliminate ambiguity (e.g. rename %q connector to %q)",
Expand All @@ -99,17 +74,6 @@ func (cfg *Config) Validate() error {
}
}

// Validate the extension configuration.
for extID, extCfg := range cfg.Extensions {
if err := component.ValidateConfig(extCfg); err != nil {
return fmt.Errorf("extensions::%s: %w", extID, err)
}
}

if err := cfg.Service.Validate(); err != nil {
return err
}

// Check that all enabled extensions in the service are configured.
for _, ref := range cfg.Service.Extensions {
// Check that the name referenced in the Service extensions exists in the top-level extensions.
Expand Down
3 changes: 2 additions & 1 deletion otelcol/otelcoltest/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package otelcoltest // import "go.opentelemetry.io/collector/otelcol/otelcoltest
import (
"context"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/confmap"
"go.opentelemetry.io/collector/confmap/provider/envprovider"
"go.opentelemetry.io/collector/confmap/provider/fileprovider"
Expand Down Expand Up @@ -40,5 +41,5 @@ func LoadConfigAndValidate(fileName string, factories otelcol.Factories) (*otelc
if err != nil {
return nil, err
}
return cfg, cfg.Validate()
return cfg, component.ValidateConfig(cfg)
}
14 changes: 0 additions & 14 deletions service/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
package service // import "go.opentelemetry.io/collector/service"

import (
"fmt"

"go.opentelemetry.io/collector/service/extensions"
"go.opentelemetry.io/collector/service/pipelines"
"go.opentelemetry.io/collector/service/telemetry"
Expand All @@ -22,15 +20,3 @@ type Config struct {
// Pipelines are the set of data pipelines configured for the service.
Pipelines pipelines.Config `mapstructure:"pipelines"`
}

func (cfg *Config) Validate() error {
if err := cfg.Pipelines.Validate(); err != nil {
return fmt.Errorf("service::pipelines config validation failed: %w", err)
}

if err := cfg.Telemetry.Validate(); err != nil {
return fmt.Errorf("service::telemetry config validation failed: %w", err)
}

return nil
}
10 changes: 5 additions & 5 deletions service/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestConfigValidate(t *testing.T) {
pipe.Processors = append(pipe.Processors, pipe.Processors...)
return cfg
},
expected: fmt.Errorf(`service::pipelines config validation failed: %w`, fmt.Errorf(`pipeline "traces": %w`, errors.New(`references processor "nop" multiple times`))),
expected: fmt.Errorf(`pipeline "traces": %w`, errors.New(`references processor "nop" multiple times`)),
},
{
name: "invalid-service-pipeline-type",
Expand All @@ -61,7 +61,7 @@ func TestConfigValidate(t *testing.T) {
}
return cfg
},
expected: fmt.Errorf(`service::pipelines config validation failed: %w`, errors.New(`pipeline "wrongtype": unknown signal "wrongtype"`)),
expected: errors.New(`pipeline "wrongtype": unknown signal "wrongtype"`),
},
{
name: "invalid-telemetry-metric-config",
Expand All @@ -71,16 +71,16 @@ func TestConfigValidate(t *testing.T) {
cfg.Telemetry.Metrics.Readers = nil
return cfg
},
expected: errors.New("service::telemetry config validation failed: collector telemetry metrics reader should exist when metric level is not none"),
expected: errors.New("collector telemetry metrics reader should exist when metric level is not none"),
},
}

for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
cfg := tt.cfgFn()
err := cfg.Validate()
err := component.ValidateConfig(cfg)
if tt.expected != nil {
assert.EqualError(t, err, tt.expected.Error())
assert.ErrorContains(t, err, tt.expected.Error())
} else {
assert.NoError(t, err)
}
Expand Down
7 changes: 1 addition & 6 deletions service/pipelines/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func (cfg Config) Validate() error {

// Check that all pipelines have at least one receiver and one exporter, and they reference
// only configured components.
for pipelineID, p := range cfg {
for pipelineID := range cfg {
switch pipelineID.Signal() {
case pipeline.SignalTraces, pipeline.SignalMetrics, pipeline.SignalLogs:
// Continue
Expand All @@ -53,11 +53,6 @@ func (cfg Config) Validate() error {
default:
return fmt.Errorf("pipeline %q: unknown signal %q", pipelineID.String(), pipelineID.Signal())
}

// Validate pipeline has at least one receiver.
if err := p.Validate(); err != nil {
return fmt.Errorf("pipeline %q: %w", pipelineID.String(), err)
}
}

return nil
Expand Down
17 changes: 10 additions & 7 deletions service/pipelines/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package pipelines

import (
"errors"
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -36,7 +35,7 @@ func TestConfigValidate(t *testing.T) {
pipe.Processors = append(pipe.Processors, pipe.Processors...)
return cfg
},
expected: fmt.Errorf(`pipeline "traces": %w`, errors.New(`references processor "nop" multiple times`)),
expected: errors.New(`references processor "nop" multiple times`),
},
{
name: "missing-pipeline-receivers",
Expand All @@ -45,7 +44,7 @@ func TestConfigValidate(t *testing.T) {
cfg[pipeline.NewID(pipeline.SignalTraces)].Receivers = nil
return cfg
},
expected: fmt.Errorf(`pipeline "traces": %w`, errMissingServicePipelineReceivers),
expected: errMissingServicePipelineReceivers,
},
{
name: "missing-pipeline-exporters",
Expand All @@ -54,7 +53,7 @@ func TestConfigValidate(t *testing.T) {
cfg[pipeline.NewID(pipeline.SignalTraces)].Exporters = nil
return cfg
},
expected: fmt.Errorf(`pipeline "traces": %w`, errMissingServicePipelineExporters),
expected: errMissingServicePipelineExporters,
},
{
name: "missing-pipelines",
Expand All @@ -74,7 +73,7 @@ func TestConfigValidate(t *testing.T) {
}
return cfg
},
expected: errors.New(`pipeline "wrongtype": unknown signal "wrongtype"`),
expected: errors.New(`unknown signal "wrongtype"`),
},
{
name: "disabled-featuregate-profiles",
Expand All @@ -87,7 +86,7 @@ func TestConfigValidate(t *testing.T) {
}
return cfg
},
expected: errors.New(`pipeline "profiles": profiling signal support is at alpha level, gated under the "service.profilesSupport" feature gate`),
expected: errors.New(`profiling signal support is at alpha level, gated under the "service.profilesSupport" feature gate`),
},
{
name: "enabled-featuregate-profiles",
Expand All @@ -109,7 +108,11 @@ func TestConfigValidate(t *testing.T) {
for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
cfg := tt.cfgFn(t)
assert.Equal(t, tt.expected, cfg.Validate())
if tt.expected != nil {
assert.ErrorContains(t, component.ValidateConfig(cfg), tt.expected.Error())
} else {
assert.NoError(t, component.ValidateConfig(cfg))
}

// Clean up the profiles support gate, which may have been enabled in `cfgFn`.
require.NoError(t, featuregate.GlobalRegistry().Set(serviceProfileSupportGateID, false))
Expand Down

0 comments on commit cb1fb4f

Please sign in to comment.