suggestions from code review

Author: jackgopack4

extension/datadogfleetautomationextension/config.go

@@ -4,7 +4,6 @@
 package datadogfleetautomationextension // import "github.com/open-telemetry/opentelemetry-collector-contrib/extension/datadogfleetautomationextension"
 
 import (
-	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -17,24 +16,22 @@ import (
 
 var (
 	// ErrUnsetAPIKey is returned when the API key is not set.
-	ErrUnsetAPIKey = datadogconfig.ErrUnsetAPIKey
+	errUnsetAPIKey = datadogconfig.ErrUnsetAPIKey
 	// ErrEmptyEndpoint is returned when endpoint is empty
-	ErrEmptyEndpoint = datadogconfig.ErrEmptyEndpoint
+	errEmptyEndpoint = datadogconfig.ErrEmptyEndpoint
 	// ErrAPIKeyFormat is returned if API key contains invalid characters
-	ErrAPIKeyFormat = datadogconfig.ErrAPIKeyFormat
+	errAPIKeyFormat = datadogconfig.ErrAPIKeyFormat
 	// NonHexRegex is a regex of characters that are always invalid in a Datadog API Key
-	NonHexRegex = datadogconfig.NonHexRegex
+	nonHexRegex = datadogconfig.NonHexRegex
 )
 
 var _ component.Config = (*Config)(nil)
 
 const (
-	// DefaultSite is the default site for the Datadog API.
-	DefaultSite = datadogconfig.DefaultSite
-	// NonHexChars is a regex of characters that are always invalid in a Datadog API key.
-	NonHexChars = datadogconfig.NonHexChars
-	// DefaultReporterPeriod is the default amount of time between sending fleet automation payloads to Datadog.
-	DefaultReporterPeriod = 20 * time.Minute
+	// defaultSite is the default site for the Datadog API.
+	defaultSite = datadogconfig.DefaultSite
+	// defaultReporterPeriod is the default amount of time between sending fleet automation payloads to Datadog.
+	defaultReporterPeriod = 20 * time.Minute
 )
 
 // Config contains the information necessary for enabling the Datadog Fleet
@@ -44,8 +41,6 @@ type Config struct {
 	API                     APIConfig `mapstructure:"api"`
 	// If Hostname is empty extension will use available system APIs and cloud provider endpoints.
 	Hostname string `mapstructure:"hostname"`
-	// ReporterPeriod sets the amount of time between sending fleet automation payloads to Datadog.
-	ReporterPeriod time.Duration `mapstructure:"reporter_period"`
 }
 
 // APIConfig contains the information necessary for configuring the Datadog API.
@@ -54,17 +49,14 @@ type APIConfig = datadogconfig.APIConfig
 // Validate ensures that the configuration is valid.
 func (c *Config) Validate() error {
 	if c.API.Site == "" {
-		return ErrEmptyEndpoint
+		return errEmptyEndpoint
 	}
 	if c.API.Key == "" {
-		return ErrUnsetAPIKey
+		return errUnsetAPIKey
 	}
-	invalidAPIKeyChars := NonHexRegex.FindAllString(string(c.API.Key), -1)
+	invalidAPIKeyChars := nonHexRegex.FindAllString(string(c.API.Key), -1)
 	if len(invalidAPIKeyChars) > 0 {
-		return fmt.Errorf("%w: invalid characters: %s", ErrAPIKeyFormat, strings.Join(invalidAPIKeyChars, ", "))
-	}
-	if c.ReporterPeriod < 5*time.Minute {
-		return errors.New("reporter_period must be 5 minutes or higher")
+		return fmt.Errorf("%w: invalid characters: %s", errAPIKeyFormat, strings.Join(invalidAPIKeyChars, ", "))
 	}
 	return nil
 }

extension/datadogfleetautomationextension/config_test.go

@@ -6,7 +6,6 @@ package datadogfleetautomationextension // import "github.com/open-telemetry/ope
 import (
 	"fmt"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -21,10 +20,9 @@ func TestConfig_Validate(t *testing.T) {
 			name: "Valid configuration",
 			config: Config{
 				API: APIConfig{
-					Site: DefaultSite,
+					Site: defaultSite,
 					Key:  "1234567890abcdef1234567890abcdef",
 				},
-				ReporterPeriod: DefaultReporterPeriod,
 			},
 			wantErr: nil,
 		},
@@ -35,42 +33,28 @@ func TestConfig_Validate(t *testing.T) {
 					Site: "",
 					Key:  "1234567890abcdef1234567890abcdef",
 				},
-				ReporterPeriod: DefaultReporterPeriod,
 			},
-			wantErr: ErrEmptyEndpoint,
+			wantErr: errEmptyEndpoint,
 		},
 		{
 			name: "Unset API key",
 			config: Config{
 				API: APIConfig{
-					Site: DefaultSite,
+					Site: defaultSite,
 					Key:  "",
 				},
-				ReporterPeriod: DefaultReporterPeriod,
 			},
-			wantErr: ErrUnsetAPIKey,
+			wantErr: errUnsetAPIKey,
 		},
 		{
 			name: "Invalid API key characters",
 			config: Config{
 				API: APIConfig{
-					Site: DefaultSite,
+					Site: defaultSite,
 					Key:  "1234567890abcdef1234567890abcdeg",
 				},
-				ReporterPeriod: DefaultReporterPeriod,
 			},
-			wantErr: fmt.Errorf("%w: invalid characters: %s", ErrAPIKeyFormat, "g"),
-		},
-		{
-			name: "too small reporter period",
-			config: Config{
-				API: APIConfig{
-					Site: DefaultSite,
-					Key:  "1234567890abcdef1234567890abcdef",
-				},
-				ReporterPeriod: 2 * time.Minute,
-			},
-			wantErr: fmt.Errorf("reporter_period must be 5 minutes or higher"),
+			wantErr: fmt.Errorf("%w: invalid characters: %s", errAPIKeyFormat, "g"),
 		},
 	}
 

extension/datadogfleetautomationextension/factory.go

@@ -30,9 +30,8 @@ func createDefaultConfig() component.Config {
 	return &Config{
 		ClientConfig: confighttp.NewDefaultClientConfig(),
 		API: APIConfig{
-			Site: DefaultSite,
+			Site: defaultSite,
 		},
-		ReporterPeriod: DefaultReporterPeriod,
 	}
 }
 

extension/datadogfleetautomationextension/factory_test.go

@@ -20,9 +20,8 @@ func TestFactory_CreateDefaultConfig(t *testing.T) {
 	expectedConfig := &Config{
 		ClientConfig: confighttp.NewDefaultClientConfig(),
 		API: APIConfig{
-			Site: DefaultSite,
+			Site: defaultSite,
 		},
-		ReporterPeriod: DefaultReporterPeriod,
 	}
 	cfg := createDefaultConfig()
 	assert.Equal(t, expectedConfig, cfg)
@@ -50,9 +49,8 @@ func TestFactory_NewFactory(t *testing.T) {
 	expectedConfig := &Config{
 		ClientConfig: confighttp.NewDefaultClientConfig(),
 		API: APIConfig{
-			Site: DefaultSite,
+			Site: defaultSite,
 		},
-		ReporterPeriod: DefaultReporterPeriod,
 	}
 	assert.Equal(t, expectedConfig, defaultConfig)
 

extension/datadogfleetautomationextension/fleetautomationextension.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"net/http"
 	"strings"
-	"sync"
 	"time"
 
 	coreconfig "github.com/DataDog/datadog-agent/comp/core/config"
@@ -37,12 +36,12 @@ import (
 )
 
 type (
-	// APIKeyValidator is a function that validates the API key, provided to newExtension for mocking
-	APIKeyValidator func(context.Context, string, *zap.Logger, *datadog.APIClient) error
-	// SourceProviderGetter is a function that returns a source.Provider, provided to newExtension for mocking
-	SourceProviderGetter func(component.TelemetrySettings, string, time.Duration) (source.Provider, error)
-	// ForwarderGetter is a function that returns a defaultforwarder.Forwarder, provided to newExtension for mocking
-	ForwarderGetter func(coreconfig.Component, corelog.Component) defaultforwarder.Forwarder
+	// apiKeyValidator is a function that validates the API key, provided to newExtension for mocking
+	apiKeyValidator func(context.Context, string, *zap.Logger, *datadog.APIClient) error
+	// sourceProviderGetter is a function that returns a source.Provider, provided to newExtension for mocking
+	sourceProviderGetter func(component.TelemetrySettings, string, time.Duration) (source.Provider, error)
+	// forwarderGetter is a function that returns a defaultforwarder.Forwarder, provided to newExtension for mocking
+	forwarderGetter func(coreconfig.Component, corelog.Component) defaultforwarder.Forwarder
 )
 
 // defaultForwarderInterface is wrapper for methods in datadog-agent DefaultForwarder struct
@@ -69,14 +68,12 @@ type fleetAutomationExtension struct {
 	collectorConfigStringMap map[string]any
 	ticker                   *time.Ticker
 	done                     chan bool
-	mu                       sync.RWMutex
 	uuid                     uuid.UUID
 
-	buildInfo            payload.CustomBuildInfo
-	moduleInfo           service.ModuleInfos
-	ModuleInfoJSON       *payload.ModuleInfoJSON
-	activeComponentsJSON *payload.ActiveComponentsJSON
-	version              string
+	buildInfo      payload.CustomBuildInfo
+	moduleInfo     service.ModuleInfos
+	ModuleInfoJSON *payload.ModuleInfoJSON
+	version        string
 
 	forwarder  defaultForwarderInterface
 	compressor *compression.Compressor
@@ -111,14 +108,15 @@ var (
 // This method is called during the startup process by the Collector's Service right after
 // calling Start.
 func (e *fleetAutomationExtension) NotifyConfig(ctx context.Context, conf *confmap.Conf) error {
-	e.mu.Lock()
-	defer e.mu.Unlock()
 
 	e.collectorConfig = conf
 	e.telemetry.Logger.Info("Received new collector configuration")
 	e.collectorConfigStringMap = e.collectorConfig.ToStringMap()
 
 	e.updateHostname(ctx)
+	if e.hostnameSource == "unset" {
+		return fmt.Errorf("collector hostname is unset, please set hostname manually in config")
+	}
 
 	// create agent metadata payload. most fields are not relevant to OSS collector.
 	e.agentMetadataPayload = payload.PrepareAgentMetadataPayload(
@@ -165,7 +163,6 @@ func (e *fleetAutomationExtension) NotifyConfig(ctx context.Context, conf *confm
 		e.otelCollectorPayload,
 	)
 	if err != nil {
-		e.telemetry.Logger.Error("Failed to prepare and send fleet automation payloads", zap.Error(err))
 		return err
 	}
 
@@ -174,12 +171,9 @@ func (e *fleetAutomationExtension) NotifyConfig(ctx context.Context, conf *confm
 
 // Start starts the extension via the component interface.
 func (e *fleetAutomationExtension) Start(_ context.Context, host component.Host) error {
-	if e.forwarder != nil {
-		err := e.forwarder.Start()
-		if err != nil {
-			e.telemetry.Logger.Error("Failed to start forwarder", zap.Error(err))
-			return err
-		}
+	err := e.forwarder.Start()
+	if err != nil {
+		return err
 	}
 
 	// Store the host for component status tracking
@@ -194,7 +188,6 @@ func (e *fleetAutomationExtension) Start(_ context.Context, host component.Host)
 	// Create and start HTTP server
 	e.httpServer = httpserver.NewServer(e.telemetry.Logger, e.serializer, e.forwarder)
 	if e.httpServer == nil {
-		e.telemetry.Logger.Error("Failed to create HTTP server")
 		return fmt.Errorf("failed to create HTTP server")
 	}
 	e.httpServer.Start(func(w http.ResponseWriter, r *http.Request) {
@@ -265,8 +258,6 @@ func (e *fleetAutomationExtension) processComponentStatusEvents() {
 
 // updateComponentStatus updates the componentStatus map with the latest status
 func (e *fleetAutomationExtension) updateComponentStatus(esp *eventSourcePair) {
-	e.mu.Lock()
-	defer e.mu.Unlock()
 
 	if e.componentStatus == nil {
 		e.componentStatus = make(map[string]any)
@@ -351,9 +342,9 @@ func newExtension(
 	ctx context.Context,
 	config *Config,
 	settings extension.Settings,
-	apiKeyValidator APIKeyValidator,
-	sourceProviderGetter SourceProviderGetter,
-	forwarderGetter ForwarderGetter,
+	apiKeyValidator apiKeyValidator,
+	sourceProviderGetter sourceProviderGetter,
+	forwarderGetter forwarderGetter,
 ) (*fleetAutomationExtension, error) {
 	// API Key validation
 	// TODO: consider moving common logic to pkg/datadog or internal/datadog
@@ -412,7 +403,7 @@ func newExtension(
 		serializer:       serializer,
 		buildInfo:        buildInfo,
 		version:          version,
-		ticker:           time.NewTicker(config.ReporterPeriod),
+		ticker:           time.NewTicker(defaultReporterPeriod),
 		done:             make(chan bool),
 		hostnameProvider: sp,
 		hostnameSource:   hostnameSource,