Skip to content

Commit

Permalink
check if monitor accepts endpoints before setting host/port
Browse files Browse the repository at this point in the history
Only select smart-agent monitors can be created via endpoints. This is
defined as a struct tag on the monitor configs "acceptsEndpoints". In
the case where we attempt to set Host/Port on a monitor that has it's
"acceptsEndpoints" value set to false, an error occurs and we do not
create the monitor.

This change would create the monitor but avoid setting the
Host/Port fields. This allows the receiver creator to
create any smart-agent monitor.
  • Loading branch information
Mark Stumpf committed Jan 19, 2022
1 parent 1f9a135 commit 02aea8c
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 6 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ This Splunk OpenTelemetry Collector release includes changes from the [opentelem
- [`docker_observer`](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/observer/dockerobserver) to detect and create container endpoints, to be used with the [`receiver_creator`](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/receivercreator) (#1044)
- [`ecs_task_observer`](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/observer/ecstaskobserver) to detect and create ECS task container endpoints, to be used with the [`receiver_creator`](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/receivercreator) (#1125)

### 🧰 Bug fixes 🧰

- [`smartagent` receiver](https://github.com/signalfx/splunk-otel-collector/tree/main/internal/receiver/smartagentreceiver) will now attempt to create _any_ monitor from a Receiver Creator instance, disregarding its provided `endpoint`. Previously would error out if a monitor did not accept endpoints ([#1107](https://github.com/signalfx/splunk-otel-collector/pull/1107))

## v0.41.0

This Splunk OpenTelemetry Collector release includes changes from the [opentelemetry-collector v0.41.0](https://github.com/open-telemetry/opentelemetry-collector/releases/tag/v0.41.0) and the [opentelemetry-collector-contrib v0.41.0](https://github.com/open-telemetry/opentelemetry-collector-contrib/releases/tag/v0.41.0) releases.
Expand Down
20 changes: 19 additions & 1 deletion internal/receiver/smartagentreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ type Config struct {
// Will expand to MonitorCustomConfig Host and Port values if unset.
Endpoint string `mapstructure:"endpoint"`
DimensionClients []string `mapstructure:"dimensionClients"`
acceptsEndpoints bool
}

func (cfg *Config) validate() error {
Expand Down Expand Up @@ -110,11 +111,18 @@ func (cfg *Config) Unmarshal(componentParser *config.Map) error {
return fmt.Errorf("failed setting Smart Agent Monitor config defaults: %w", err)
}

err = setHostAndPortViaEndpoint(cfg.Endpoint, monitorConfig)
cfg.acceptsEndpoints, err = monitorAcceptsEndpoints(monitorConfig)
if err != nil {
return err
}

if cfg.acceptsEndpoints {
err = setHostAndPortViaEndpoint(cfg.Endpoint, monitorConfig)
if err != nil {
return err
}
}

cfg.monitorConfig = monitorConfig.(saconfig.MonitorCustomConfig)
return nil
}
Expand Down Expand Up @@ -178,3 +186,13 @@ func setHostAndPortViaEndpoint(endpoint string, monitorConfig interface{}) error

return nil
}

// Monitors can only set the "Host" and "Port" fields if they accept endpoints,
// which is defined as a struct tag for each monitor config.
func monitorAcceptsEndpoints(monitorConfig interface{}) (bool, error) {
field, ok := reflect.TypeOf(monitorConfig).Elem().FieldByName("MonitorConfig")
if !ok {
return false, fmt.Errorf("could not reflect monitor config, top level MonitorConfig does not exist")
}
return field.Tag.Get("acceptsEndpoints") == strconv.FormatBool(true), nil
}
39 changes: 34 additions & 5 deletions internal/receiver/smartagentreceiver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/signalfx/signalfx-agent/pkg/monitors/filesystems"
"github.com/signalfx/signalfx-agent/pkg/monitors/haproxy"
"github.com/signalfx/signalfx-agent/pkg/monitors/kubernetes/volumes"
"github.com/signalfx/signalfx-agent/pkg/monitors/nagios"
"github.com/signalfx/signalfx-agent/pkg/monitors/prometheusexporter"
"github.com/signalfx/signalfx-agent/pkg/monitors/telegraf/common/parser"
"github.com/signalfx/signalfx-agent/pkg/monitors/telegraf/monitors/exec"
Expand Down Expand Up @@ -74,6 +75,7 @@ func TestLoadConfig(t *testing.T) {
SSLVerify: true,
Timeout: timeutil.Duration(5 * time.Second),
},
acceptsEndpoints: true,
}, haproxyCfg)
require.NoError(t, haproxyCfg.validate())

Expand All @@ -90,6 +92,7 @@ func TestLoadConfig(t *testing.T) {
Host: "localhost",
Port: 6379,
},
acceptsEndpoints: true,
}, redisCfg)
require.NoError(t, redisCfg.validate())

Expand All @@ -106,6 +109,7 @@ func TestLoadConfig(t *testing.T) {
Host: "localhost",
Port: 8088,
},
acceptsEndpoints: true,
}, hadoopCfg)
require.NoError(t, hadoopCfg.validate())

Expand All @@ -125,6 +129,7 @@ func TestLoadConfig(t *testing.T) {
Port: 5309,
MetricPath: "/metrics",
},
acceptsEndpoints: true,
}, etcdCfg)
require.NoError(t, etcdCfg.validate())

Expand All @@ -140,6 +145,7 @@ func TestLoadConfig(t *testing.T) {
},
DNSLookup: &tr,
},
acceptsEndpoints: true,
}, ntpqCfg)
require.NoError(t, ntpqCfg.validate())
}
Expand Down Expand Up @@ -213,6 +219,7 @@ func TestLoadInvalidConfigs(t *testing.T) {
DatapointsToExclude: []saconfig.MetricFilter{},
},
},
acceptsEndpoints: true,
}, negativeIntervalCfg)
err = negativeIntervalCfg.validate()
require.Error(t, err)
Expand All @@ -231,6 +238,7 @@ func TestLoadInvalidConfigs(t *testing.T) {
TelemetryHost: "0.0.0.0",
TelemetryPort: 8125,
},
acceptsEndpoints: true,
}, missingRequiredCfg)
err = missingRequiredCfg.validate()
require.Error(t, err)
Expand Down Expand Up @@ -270,6 +278,7 @@ func TestLoadConfigWithEndpoints(t *testing.T) {
SSLVerify: true,
Timeout: timeutil.Duration(5 * time.Second),
},
acceptsEndpoints: true,
}, haproxyCfg)
require.NoError(t, haproxyCfg.validate())

Expand All @@ -286,6 +295,7 @@ func TestLoadConfigWithEndpoints(t *testing.T) {
Host: "redishost",
Port: 6379,
},
acceptsEndpoints: true,
}, redisCfg)
require.NoError(t, redisCfg.validate())

Expand All @@ -303,6 +313,7 @@ func TestLoadConfigWithEndpoints(t *testing.T) {
Host: "localhost",
Port: 8088,
},
acceptsEndpoints: true,
}, hadoopCfg)
require.NoError(t, hadoopCfg.validate())

Expand All @@ -323,6 +334,7 @@ func TestLoadConfigWithEndpoints(t *testing.T) {
Port: 5555,
MetricPath: "/metrics",
},
acceptsEndpoints: true,
}, etcdCfg)
require.NoError(t, etcdCfg.validate())
}
Expand All @@ -342,7 +354,9 @@ func TestLoadInvalidConfigWithInvalidEndpoint(t *testing.T) {
require.Nil(t, cfg)
}

func TestLoadInvalidConfigWithUnsupportedEndpoint(t *testing.T) {
// Even though this smart-agent monitor does not accept endpoints,
// we can create it without setting Host/Port fields.
func TestLoadConfigWithUnsupportedEndpoint(t *testing.T) {
factories, err := componenttest.NopFactories()
assert.Nil(t, err)

Expand All @@ -351,10 +365,25 @@ func TestLoadInvalidConfigWithUnsupportedEndpoint(t *testing.T) {
cfg, err := servicetest.LoadConfig(
path.Join(".", "testdata", "unsupported_endpoint.yaml"), factories,
)
require.Error(t, err)
require.EqualError(t, err,
`error reading receivers configuration for "smartagent/nagios": unable to set monitor Host field using Endpoint-derived value of localhost: no field Host of type string detected`)
require.Nil(t, cfg)
require.NoError(t, err)
require.NotNil(t, cfg)

nagiosCfg := cfg.Receivers[config.NewComponentIDWithName(typeStr, "nagios")].(*Config)
require.Equal(t, &Config{
Endpoint: "localhost:12345",
ReceiverSettings: config.NewReceiverSettings(config.NewComponentIDWithName(typeStr, "nagios")),
monitorConfig: &nagios.Config{
MonitorConfig: saconfig.MonitorConfig{
Type: "nagios",
DatapointsToExclude: []saconfig.MetricFilter{},
},
Command: "some_command",
Service: "some_service",
Timeout: 9,
},
acceptsEndpoints: false,
}, nagiosCfg)
require.NoError(t, nagiosCfg.validate())
}

func TestLoadInvalidConfigWithNonArrayDimensionClients(t *testing.T) {
Expand Down
3 changes: 3 additions & 0 deletions internal/receiver/smartagentreceiver/receiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ func (r *Receiver) Start(_ context.Context, host component.Host) error {
monitorType: r.config.monitorConfig.MonitorConfigCore().Type,
}, r.logger)

if !r.config.acceptsEndpoints {
r.logger.Info("This Smart Agent monitor does not use Host/Port config fields. If either are set, they will be ignored.", zap.String("monitor_type", monitorType))
}
r.monitor, err = r.createMonitor(monitorType, host)
if err != nil {
return fmt.Errorf("failed creating monitor %q: %w", monitorType, err)
Expand Down

0 comments on commit 02aea8c

Please sign in to comment.