Datadogfleetautomation extension POC #5272

jackgopack4 · 2025-03-03T14:42:29Z

Description

initial POC for Datadog Fleet Automation Extension

Link to tracking issue

Testing

Full unit testing (todo: will open a card to write a few integration tests)

Documentation

Have only written a barebones README.md to pass checks; figured that would make sense to do after we make changes or add/remove/adjust functionality.

Additional notes

Does not yet send the new, third payload type. I am working on a separate branch to add that payload and am planning to open a second PR after this one is accepted to our fork

datadog-datadog-prod-us1 · 2025-03-03T14:43:05Z

extension/datadogfleetautomationextension/fleetautomationextension.go

+		GetModuleInfos() service.ModuleInfos
+	}
+
+	if host, ok := host.(exportModules); ok {


🔵 Code Quality Violation

Avoid modifying function parameters (...read more)

Assigning new values to function parameters exhibits several bad coding practices and should be avoided for several reasons:

Redefining parameters: The code redefines a parameter within the function body by assigning a new value. This is considered a bad practice because it can lead to confusion and make the code harder to understand. Modifying a function parameter in this manner breaks the expected behavior and can cause unexpected side effects. It is generally best to treat function parameters as read-only values and avoid reassigning them.

Shadowing variables: The code further exacerbates the issue by using the short variable declaration := to define a new variable within the function body. This shadows the original parameter, making it inaccessible within the function. Shadowing variables can cause confusion and make the code harder to reason about. It is better to use distinct variable names to maintain clarity and avoid any unintended side effects.

To write more maintainable and understandable code, it is advisable to adhere to the following practices:

Avoid redefining function parameters.

Use descriptive and unambiguous variable names.

Avoid shadowing variables.

Maintain consistency in variable references.

By following these best practices, the code becomes more readable and easier to manage and avoids introducing unnecessary complexity and confusion.

TODO: update to GetModulesInfo() in service/hostcapabilities

extension/datadogfleetautomationextension/http.go

extension/datadogfleetautomationextension/fleetautomationextension.go

extension/datadogfleetautomationextension/transform.go

extension/datadogfleetautomationextension/fleetautomationextension.go

extension/datadogfleetautomationextension/http_test.go

extension/datadogfleetautomationextension/fleetautomationextension_test.go

extension/datadogfleetautomationextension/fleetautomationextension.go

jackgopack4 · 2025-03-03T17:29:09Z

Addressing the golint violations

dineshg13 · 2025-03-04T20:15:31Z

extension/datadogfleetautomationextension/fleetautomationextension.go

+	extensionConfig          *Config
+	extensionID              component.ID
+	telemetry                component.TelemetrySettings
+	collectorConfig          *confmap.Conf
+	collectorConfigStringMap map[string]any
+	ticker                   *time.Ticker
+	done                     chan bool
+	mu                       sync.RWMutex
+	uuid                     uuid.UUID
+
+	buildInfo            component.BuildInfo
+	moduleInfo           service.ModuleInfos
+	moduleInfoJSON       *moduleInfoJSON
+	activeComponentsJSON *activeComponentsJSON
+	version              string
+
+	forwarder  defaultForwarderInterface
+	compressor *compression.Compressor
+	serializer serializer.MetricSerializer
+
+	agentMetadataPayload AgentMetadata
+	otelMetadataPayload  OtelMetadata
+
+	httpServer           *http.Server
+	healthCheckV2Enabled bool
+	healthCheckV2ID      *component.ID // currently first healthcheckv2 extension found; could expand to multiple health checks if needed
+	healthCheckV2Config  map[string]any
+	componentStatus      map[string]any // retrieved from healthcheckv2 extension, if enabled/configured
+
+	hostnameProvider source.Provider


This looks like it is doing many things. Can we split this multiple internal packages. Like http server can be its own package

dineshg13 · 2025-03-04T20:22:57Z

extension/datadogfleetautomationextension/payload.go

+package datadogfleetautomationextension // import "github.com/open-telemetry/opentelemetry-collector-contrib/extension/datadogfleetautomationextension"
+
+import (
+	"encoding/json"
+	"fmt"
+
+	"github.com/DataDog/datadog-agent/pkg/serializer/marshaler"
+	"go.opentelemetry.io/collector/component"
+)
+
+type OtelCollector struct {
+	HostKey           string              `json:"host_key"`


move it to internal/payload package.

dineshg13 · 2025-03-04T20:26:48Z

extension/datadogfleetautomationextension/agent_components.go

+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package datadogfleetautomationextension // import "github.com/open-telemetry/opentelemetry-collector-contrib/extension/datadogfleetautomationextension"


may be name it datadogfaextension ?

dineshg13 · 2025-03-04T20:32:07Z

extension/datadogfleetautomationextension/transform.go

+type defaultComponentChecker struct {
+	extension *fleetAutomationExtension
+}
+
+func (d *defaultComponentChecker) isComponentConfigured(typ string, componentsKind string) (bool, *component.ID) {
+	return d.extension.isComponentConfigured(typ, componentsKind)
+}
+
+func (d *defaultComponentChecker) isModuleAvailable(componentType string, componentKind string) bool {
+	return d.extension.isModuleAvailable(componentType, componentKind)
+}
+
+func (d *defaultComponentChecker) isHealthCheckV2Enabled() (bool, error) {
+	return d.extension.isHealthCheckV2Enabled()
+}


can we decouple component checker and extension. may be make component checker its own module

dineshg13 · 2025-03-04T20:33:43Z

extension/datadogfleetautomationextension/transform.go

+		if componentsKind == extensionsKind {
+			if componentStatus, ok := componentsConfig[componentsKind].(map[string]any); ok {
+				if receiversStatus, ok := componentStatus["components"].(map[string]any); ok {
+					componentNameInPipeline := componentKind + ":" + id
+					if componentStatus, ok := receiversStatus[componentNameInPipeline].(map[string]any); ok {
+						result = componentStatus
+					}
+				}
+			}


We should try to avoid arrow pattern. see https://blog.codinghorror.com/flattening-arrow-code/
try changing to

if componentsKind != extensionsKind { continue }

dineshg13 · 2025-03-04T20:33:54Z

extension/datadogfleetautomationextension/transform.go

+				if pipelineMap, ok := value.(map[string]any); ok {
+					if components, ok := pipelineMap["components"].(map[string]any); ok {
+						for component, status := range components {
+							componentParts := strings.Split(component, ":")
+							if len(componentParts) != 2 {
+								continue
+							}
+							kind := componentParts[0]
+							fullID := componentParts[1]
+							if kind == componentKind && id == fullID {
+								if componentStatus, ok := status.(map[string]any); ok {
+									result[key] = map[string]any{
+										component: componentStatus,
+									}
+								}
+							}
+						}
+					}
+				}


same as above

dineshg13 · 2025-03-04T20:34:54Z

extension/datadogfleetautomationextension/fleetautomationextension.go

+	e.mu.Lock()
+	defer e.mu.Unlock()
+


why do we need mutex here ?

I didn't think too hard about this one; just knew that other extensions that used ConfigWatcher also used a mutex, and I also am not too clear on how multiple instances of the same extension would work. maybe not necessary for this.

mx-psi · 2025-03-05T10:30:40Z

extension/datadogfleetautomationextension/config.go

+	// ErrUnsetAPIKey is returned when the API key is not set.
+	ErrUnsetAPIKey = datadogconfig.ErrUnsetAPIKey
+	// ErrEmptyEndpoint is returned when endpoint is empty
+	ErrEmptyEndpoint = datadogconfig.ErrEmptyEndpoint
+	// ErrAPIKeyFormat is returned if API key contains invalid characters
+	ErrAPIKeyFormat = datadogconfig.ErrAPIKeyFormat


We don't need to export all of these, right?

yeah that's true I can probably move most things to un-exported variables/functions

mx-psi · 2025-03-05T10:30:47Z

extension/datadogfleetautomationextension/config.go

+	// NonHexRegex is a regex of characters that are always invalid in a Datadog API Key
+	NonHexRegex = datadogconfig.NonHexRegex


mx-psi · 2025-03-05T10:30:55Z

extension/datadogfleetautomationextension/config.go

+	// 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


mx-psi · 2025-03-05T10:31:28Z

extension/datadogfleetautomationextension/config.go

+	// ReporterPeriod sets the amount of time between sending fleet automation payloads to Datadog.
+	ReporterPeriod time.Duration `mapstructure:"reporter_period"`


Why do we want to make this configurable?

we allow it to be configured in hostmetadata reporter so I figured it made sense to keep the configuration the same. I don't actually care if we let users configure it or not and would just as easily remove it

I would just remove it for this first version

mx-psi · 2025-03-05T10:32:37Z

extension/datadogfleetautomationextension/fleetautomationextension.go

+	// 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


These can all be private

mx-psi · 2025-03-05T10:45:26Z

extension/datadogfleetautomationextension/fleetautomationextension.go

+	collectorConfig          *confmap.Conf
+	collectorConfigStringMap map[string]any
+	ticker                   *time.Ticker
+	done                     chan bool


What is the done channel used for?

it is used to close the ticker which sends the metadata every "reporterperiod" amount of time, but it looks like you found it and we can just do a cancellable context instead

mx-psi · 2025-03-05T10:49:18Z

extension/datadogfleetautomationextension/fleetautomationextension.go

+	}
+}
+
+func (e *fleetAutomationExtension) checkHealthCheckV2() {


I don't get why we do all this. We could just call the same API that the healthcheck extension uses and I think this would be much simpler

I see what you mean. I don't know that it makes sense to duplicate healthCheckV2 functionality in our own extension, but it would make us less tightly coupled to healthcheckv2 and allow us to iterate on our own cadence.
I'll work on a refactor that does it that way instead and see if that is simpler

I don't think we need to duplicate its functionality, just the bits that get the info from the host. We don't need to necessarily use the format that the extension is using

mx-psi · 2025-03-05T10:49:50Z

extension/datadogfleetautomationextension/http.go

+				if err != nil {
+					e.telemetry.Logger.Error("Failed to prepare and send fleet automation payloads", zap.Error(err))
+				}
+			case <-e.done:


Instead of this I would pass a context and wait for the context to be cancelled

mx-psi · 2025-03-05T10:51:04Z

extension/datadogfleetautomationextension/http.go

+	e.mu.RLock()
+	defer e.mu.RUnlock()
+	if e.hostnameSource == "unset" {
+		e.telemetry.Logger.Info("Skipping fleet automation payloads since the hostname is empty")


This case should just not happen, we should have bailed out if the extension cannot work properly with the user provided configuration

sure, I thought maybe it made sense to log it, and allow the user to update the config (collector technically allows config changes while running, right?)

Hm, well, there is very limited functionality related to this, we can always revert back to not adding an error in the future (but it's harder to go from "no error" -> error)

mx-psi · 2025-03-05T10:52:37Z

extension/datadogfleetautomationextension/transform.go

I think this should be in its own internal package

jackgopack4 requested review from dineshg13 and mx-psi March 3, 2025 14:42

datadog-datadog-prod-us1 bot reviewed Mar 3, 2025

View reviewed changes

extension/datadogfleetautomationextension/fleetautomationextension.go Outdated Show resolved Hide resolved

datadog-datadog-prod-us1 bot reviewed Mar 3, 2025

View reviewed changes

extension/datadogfleetautomationextension/fleetautomationextension.go Outdated Show resolved Hide resolved

jackgopack4 force-pushed the prod branch from 98ff3c0 to d225c14 Compare March 3, 2025 20:28

jackgopack4 force-pushed the datadogfleetautomation-extension branch 5 times, most recently from cb116ca to 34ae621 Compare March 4, 2025 19:50

jackgopack4 requested review from ankitpatel96, liustanley, songy23, mackjmr, jade-guiton-dd, IbraheemA and billmeyer as code owners March 4, 2025 19:50

jackgopack4 force-pushed the prod branch from d225c14 to dceb5cc Compare March 4, 2025 19:53

jackgopack4 force-pushed the datadogfleetautomation-extension branch from 34ae621 to 48930cd Compare March 4, 2025 19:54

jackgopack4 force-pushed the prod branch from dceb5cc to 6b0c316 Compare March 4, 2025 19:54

dineshg13 requested changes Mar 4, 2025

View reviewed changes

jackgopack4 force-pushed the prod branch from 6b0c316 to a0ac256 Compare March 4, 2025 20:49

jackgopack4 force-pushed the datadogfleetautomation-extension branch from 48930cd to 9effea7 Compare March 4, 2025 20:50

mx-psi requested changes Mar 5, 2025

View reviewed changes

jackgopack4 force-pushed the datadogfleetautomation-extension branch 2 times, most recently from 7bb0557 to 3176d4f Compare March 6, 2025 14:09

jackgopack4 force-pushed the prod branch from 0e07532 to 93d4d76 Compare March 6, 2025 16:38

squash and rebase

e7ed96a

jackgopack4 force-pushed the datadogfleetautomation-extension branch from 3176d4f to e7ed96a Compare March 6, 2025 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datadogfleetautomation extension POC #5272

Datadogfleetautomation extension POC #5272

jackgopack4 commented Mar 3, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot Mar 3, 2025

jackgopack4 Mar 3, 2025

jackgopack4 commented Mar 3, 2025

dineshg13 Mar 4, 2025

dineshg13 Mar 4, 2025

dineshg13 Mar 4, 2025

dineshg13 Mar 4, 2025

dineshg13 Mar 4, 2025

dineshg13 Mar 4, 2025

dineshg13 Mar 4, 2025

jackgopack4 Mar 5, 2025

mx-psi Mar 5, 2025

jackgopack4 Mar 5, 2025

mx-psi Mar 5, 2025

mx-psi Mar 5, 2025

mx-psi Mar 5, 2025

jackgopack4 Mar 5, 2025

mx-psi Mar 5, 2025

mx-psi Mar 5, 2025

mx-psi Mar 5, 2025

jackgopack4 Mar 5, 2025

mx-psi Mar 5, 2025

jackgopack4 Mar 5, 2025

mx-psi Mar 5, 2025

mx-psi Mar 5, 2025

mx-psi Mar 5, 2025

jackgopack4 Mar 5, 2025

mx-psi Mar 5, 2025

mx-psi Mar 5, 2025

		// NonHexRegex is a regex of characters that are always invalid in a Datadog API Key
		NonHexRegex = datadogconfig.NonHexRegex

		// ReporterPeriod sets the amount of time between sending fleet automation payloads to Datadog.
		ReporterPeriod time.Duration `mapstructure:"reporter_period"`

Datadogfleetautomation extension POC #5272

Are you sure you want to change the base?

Datadogfleetautomation extension POC #5272

Conversation

jackgopack4 commented Mar 3, 2025 • edited Loading

Description

Link to tracking issue

Testing

Documentation

Additional notes

datadog-datadog-prod-us1 bot Mar 3, 2025

Choose a reason for hiding this comment

🔵 Code Quality Violation

Choose a reason for hiding this comment

jackgopack4 commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackgopack4 commented Mar 3, 2025 •

edited

Loading