[exporter/datadog] Add configurable reporter_period for host metadata #36451
Conversation
- Introduced a `reporter_period` field in `HostMetadataConfig` to allow customization of the metadata reporting interval. - Default value set to 30 minutes for backward compatibility. - Updated `pkg/datadog/config/host.go` to support the new `reporter_period` field. - Validated `reporter_period` in `pkg/datadog/config/config.go` to ensure positive durations. - Passed `reporter_period` to the metadata pusher in `exporter/datadogexporter/internal/hostmetadata/config.go`. - Modified `exporter/datadogexporter/hostmetadata.go` and `factory.go` to integrate `reporter_period` into the reporter logic. - Removed hardcoded constant for metadata reporting interval in favor of the configurable value. Fixes open-telemetry#36450
- Added a changelog entry
github-actions
bot
requested review from
ankitpatel96,
dineshg13,
liustanley and
mackjmr
…igurable-reporter-period-for-host-metadata
- Fixed formatting
…atadogexporter-add-configurable-reporter-period-for-host-metadata
…igurable-reporter-period-for-host-metadata
| @@ -79,8 +79,6 @@ func enableZorkianMetricExport() error { | |||
| return featuregate.GlobalRegistry().Set(metricExportNativeClientFeatureGate.ID(), false) | |||
| } | |||
|
|
|||
| const metadataReporterPeriod = 30 * time.Minute | |||
There was a problem hiding this comment.
I asked the same question about this last year.. #24290 (comment)
@mx-psi is this a requirement from backend etc.?
There was a problem hiding this comment.
Yeah, in my testing, I set the period to 2 minutes and observed no issues with the Datadog backend. By default, the period is set to 30 minutes, but users can adjust it to a shorter interval if they need more frequent updates.
…igurable-reporter-period-for-host-metadata
| @@ -230,6 +230,9 @@ func TestLogsExporter(t *testing.T) { | |||
| Endpoint: server.URL, | |||
| }, | |||
| }, | |||
| HostMetadata: HostMetadataConfig{ | |||
| ReporterPeriod: 30 * time.Minute, | |||
| }, | |||
There was a problem hiding this comment.
There are 8 places where you add a HostMetadataConfig without Enabled: true. My understanding is that when creating the config struct manually, the default config is not taken into account, and this will set Enabled: false, making the whole section have no effect. Could you confirm whether sending host metadata is actually required in those tests? If no, you can probably forgo setting the HostMetadata field altogether.
There was a problem hiding this comment.
You are correct that when creating the config struct manually, the default config isn't built, and Enabled defaults to false. However, it's not the entire section that has no effect.
In the test, there's a call to exp, err := f.CreateLogs(ctx, params, cfg), which creates the logs exporter via func (f *factory) createLogsExporter. This will call func (f *factory) Reporter, which instantiates the reporter with inframetadata.NewReporter(params.Logger, pusher, pcfg.ReporterPeriod).
At this point, if pcfg.ReporterPeriod isn't initialized from the HostMetadataConfig, it defaults to 0. This causes a panic in the internal reporter of inframetadata because we can't initialize its time ticker time.NewTicker(period) with a period equal to 0.
So, even though Enabled is false, we still need to initialize ReporterPeriod to prevent a runtime panic. Removing the HostMetadata field altogether would lead to an uninitialized ReporterPeriod, causing the issue.
Now, you might wonder what the purpose of Enabled is. In fact, it's used later during the consumption of logs, as seen here: datadogexporter/logs_exporter.go#L104.
I hope this clarifies why we included the HostMetadataConfig without explicitly setting Enabled: true.
There was a problem hiding this comment.
I see, thank you for the detailed explanation. In other words, it seems to me that Enabled: false disables the calls to Reporter.ConsumeX, so no host data is recorded from incoming signals, but it doesn't disable the creation of a Reporter in the first place, or the call to Reporter.Run, which periodically pushes that empty data to the backend, so a non-zero ReporterPeriod is still needed for the corresponding Ticker.
There was a problem hiding this comment.
Let's go with your current solution for this PR. We can implement a more elegant solution later (disabling the creation of the Reporter when enabled: false and removing the workaround).
Description
This pull request introduces a new configurable parameter
reporter_periodfor the Datadog exporter’s host metadata configuration. This enhancement allows users to specify the frequency at which host metadata is sent to Datadog. The changes span multiple files and include updates to configuration, factory methods, and test cases.Enhancements to Datadog exporter:
.chloggen/add-configurable-reporter-period-for-host-metadata.yaml: Added a changelog entry for the newreporter_periodparameter.exporter/datadogexporter/examples/collector.yaml: Added documentation for the newreporter_periodparameter in the example configuration file.exporter/datadogexporter/factory.go: Updated the factory to use thereporter_periodfrom the configuration instead of a hardcoded value.Configuration updates:
exporter/datadogexporter/internal/hostmetadata/config.go: AddedReporterPeriodto thePusherConfigstruct.pkg/datadog/config/config.go: Added validation for thereporter_periodto ensure it is a positive duration and set a default value. [1] [2]Test case updates:
exporter/datadogexporter/factory_test.go,exporter/datadogexporter/logs_exporter_test.go,exporter/datadogexporter/metrics_exporter_test.go,exporter/datadogexporter/traces_exporter_test.go: Updated test cases to include the newreporter_periodparameter. [1] [2] [3] [4]pkg/datadog/config/config_test.go: Added test cases to validate thereporter_periodconfiguration. [1] [2]Link to tracking issue
Fixes #36450
Testing
Documentation