Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[exporter/datadog] Host metrics export is never fully disabled #36522

Closed
jade-guiton-dd opened this issue Nov 25, 2024 · 2 comments · Fixed by #36669
Closed

[exporter/datadog] Host metrics export is never fully disabled #36522

jade-guiton-dd opened this issue Nov 25, 2024 · 2 comments · Fixed by #36669
Labels
bug Something isn't working exporter/datadog Datadog components

Comments

@jade-guiton-dd
Copy link
Contributor

Component(s)

exporter/datadog

Describe the issue you're reporting

Description

When setting exporters.datadog.host_metadata.enabled: false, the inframetadata.Reporter.ConsumeResource and ConsumeHostMetadata methods are no longer called, which means no host data is collected. However, an inframetadata.Reporter is still created, and the Reporter.Run function, which periodically exports the collected data, is still called.

Presumably, this has no user impact, but it caused a PR to require some unintuitive workarounds.

I believe a solution would be to skip the call to factory.Reporter in create(Metrics|Logs|Traces)Exporter when enabled is false, and leave (metrics|logs|traces)Exporter.metadataReporter as nil. Some additional nil checks may be necessary. Moreover, removing aforementioned workaround would be good.

@jade-guiton-dd jade-guiton-dd added the needs triage New item requiring triage label Nov 25, 2024
@github-actions github-actions bot added the exporter/datadog Datadog components label Nov 25, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@jade-guiton-dd
Copy link
Contributor Author

/label -needs-triage bug

@github-actions github-actions bot added bug Something isn't working and removed needs triage New item requiring triage labels Nov 25, 2024
mx-psi pushed a commit that referenced this issue Dec 10, 2024
…_metadata.enabled` (#36669)

#### Description

This PR avoids creating an `inframetadata.Reporter` and starting the
metadata-sending goroutine when `host_metadata.enabled` is `false`, and
removes some code that was used to work around the previous behavior.
See tracking issue for context.

When `enabled` is `false`, we leave the `exporter.metadataReporter`
field as `nil`. The parts of the code accessing this field were already
gated behind `enabled: true`, or `only_metadata: true` (which, in a
valid config, implies `enabled: false`), so it should not cause issues.
However, tests that modify the config after the exporter is created, or
which manually create invalid `Config` structs may encounter segfaults.
I assumed we do not mind segfaults in cases of incorrect internal use,
and fixed the one test that was failing because of this.

#### Link to tracking issue
Fixes #36522
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this issue Dec 17, 2024
…_metadata.enabled` (open-telemetry#36669)

#### Description

This PR avoids creating an `inframetadata.Reporter` and starting the
metadata-sending goroutine when `host_metadata.enabled` is `false`, and
removes some code that was used to work around the previous behavior.
See tracking issue for context.

When `enabled` is `false`, we leave the `exporter.metadataReporter`
field as `nil`. The parts of the code accessing this field were already
gated behind `enabled: true`, or `only_metadata: true` (which, in a
valid config, implies `enabled: false`), so it should not cause issues.
However, tests that modify the config after the exporter is created, or
which manually create invalid `Config` structs may encounter segfaults.
I assumed we do not mind segfaults in cases of incorrect internal use,
and fixed the one test that was failing because of this.

#### Link to tracking issue
Fixes open-telemetry#36522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exporter/datadog Datadog components
Projects
None yet
1 participant