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

Unnecessary call to config.Validate during component's initialization #33498

Closed
rogercoll opened this issue Jun 11, 2024 · 5 comments
Closed

Comments

@rogercoll
Copy link
Contributor

Component(s)

exporter/dataset, exporter/elasticsearch, exporter/opensearch, receiver/dockerstats, receiver/podman

Describe the issue you're reporting

All collector's components must implement the component.ValidateConfig interface. During collector's startup, it goes through all components and calls the Validate method. Its execution is finished if any component returns an error. Nonetheless, some components call the Validate method during its initialization, which is redundant taking into account the collector life cycle. (e.g. Podman receiver)

I think that the Validate interface should be called by the configuration validator consumer (e.g. collector) instead of the configuration producers (components).

Context for Podman's receiver: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32981/files#r1621849257

I linked to this issue the components that seems to be internally calling the configuration Validate method. I would not say calling Validate is forbidden, just that might be redundant during collector's execution. However, feel free to unlink your component from this issue if the Validate method call is necessary for your specific use case or if it performs additional checks that are not covered during the startup validation phase.

Copy link
Contributor

Pinging code owners:

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

mx-psi pushed a commit that referenced this issue Jun 24, 2024
**Description:** This PR is upgrading dataset-go to version v0.19.0 -
https://github.com/scalyr/dataset-go/releases/tag/v0.19.0

This PR is also fixing:
* #33498 - config validation is not called during conversion, therefore
I had to remove tests from logs and trace exporter. These tests are now
moved to the factory.
* #32533 - fixing failing integration tests, that were never updated and
therefore the configuration contained invalid values

**Link to tracking Issue:** #33498, #32533, #33675

**Testing:** Unit tests and integration tests.

**Documentation:** <Describe the documentation added.>

fixes #32533

---------

Signed-off-by: Alex Boten <[email protected]>
Co-authored-by: Curtis Robert <[email protected]>
Co-authored-by: Dmitrii Anoshin <[email protected]>
Co-authored-by: Ishan Shanware <[email protected]>
Co-authored-by: Evan Bradley <[email protected]>
Co-authored-by: Tyler Helmuth <[email protected]>
Co-authored-by: Alex Boten <[email protected]>
Co-authored-by: Paulo Janotti <[email protected]>
Co-authored-by: Joshua MacDonald <[email protected]>
Co-authored-by: Rong Hu <[email protected]>
andrzej-stencel added a commit that referenced this issue Jul 11, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

Configuration validation is done during collector's startup, making it
redundant when being called inside component's logic. This PR removes
the Validate call done during Podman's receiver start function.

**Link to tracking Issue:** <Issue number if applicable>

#33498

Context:
https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32981/files#r1621849257

**Testing:** <Describe what testing was performed and which tests were
added.>
Tests moved to the `config_test.go` file following the testdata
strategy.

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Andrzej Stencel <[email protected]>
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

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

@github-actions github-actions bot added the Stale label Aug 12, 2024
@mx-psi mx-psi removed Stale needs triage New item requiring triage labels Aug 12, 2024
@mx-psi mx-psi added the priority:p2 Medium label Aug 12, 2024
@mx-psi
Copy link
Member

mx-psi commented Aug 12, 2024

@rogercoll Do you know what components from your original list still do the unnecessary call?

@rogercoll
Copy link
Contributor Author

I think all but podman receiver. I will try to work on the docker one asap.

mx-psi pushed a commit that referenced this issue Aug 28, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Configuration validation is done during collector's startup, making it
redundant when being called inside component's logic. This PR removes
the Validate call done during Dockers's receiver start function.

**Link to tracking Issue:** <Issue number if applicable>

#33498

**Testing:** <Describe what testing was performed and which tests were
added.>
No testing needed to be modified as the `Validate` functionality is
already covered.

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Sean Marciniak <[email protected]>
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this issue Sep 12, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Configuration validation is done during collector's startup, making it
redundant when being called inside component's logic. This PR removes
the Validate call done during Dockers's receiver start function.

**Link to tracking Issue:** <Issue number if applicable>

open-telemetry#33498

**Testing:** <Describe what testing was performed and which tests were
added.>
No testing needed to be modified as the `Validate` functionality is
already covered.

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Sean Marciniak <[email protected]>
mx-psi pushed a commit that referenced this issue Sep 16, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Configuration validation is done during collector's startup, making it
redundant when being called inside component's logic. This PR removes
the Validate call done during exporter's constructor.

**Link to tracking Issue:** <Issue number if applicable>
#33498

**Testing:** <Describe what testing was performed and which tests were
added.> Same coverage after removed lines: `coverage: 77.8% of
statements`

`TestFactory_CreateLogsExporter_Fail` and
`TestFactory_CreateTracesExporter_Fail` functions are covered by the
already available `config_test.go` cases.

**Documentation:** <Describe the documentation added.>
@rogercoll
Copy link
Contributor Author

rogercoll commented Sep 17, 2024

  • exporter/elasticsearch
  • exporter/dataset
  • receiver/dockerstats
  • receiver/podmanstats
  • exporter/opensearch

jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this issue Oct 4, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Configuration validation is done during collector's startup, making it
redundant when being called inside component's logic. This PR removes
the Validate call done during exporter's constructor.

**Link to tracking Issue:** <Issue number if applicable>
open-telemetry#33498

**Testing:** <Describe what testing was performed and which tests were
added.> Same coverage after removed lines: `coverage: 77.8% of
statements`

`TestFactory_CreateLogsExporter_Fail` and
`TestFactory_CreateTracesExporter_Fail` functions are covered by the
already available `config_test.go` cases.

**Documentation:** <Describe the documentation added.>
MovieStoreGuy pushed a commit that referenced this issue Oct 18, 2024
…35233)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Configuration validation is done during collector's startup, making it
redundant when being called inside component's logic. This PR removes
the Validate call done during exporter's constructor.

**Link to tracking Issue:** <Issue number if applicable>
#33498
(Last component, will close the issue)

**Testing:** <Describe what testing was performed and which tests were
added.> Added default config use case (validate error)

**Documentation:** <Describe the documentation added.>
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this issue Dec 17, 2024
…pen-telemetry#35233)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Configuration validation is done during collector's startup, making it
redundant when being called inside component's logic. This PR removes
the Validate call done during exporter's constructor.

**Link to tracking Issue:** <Issue number if applicable>
open-telemetry#33498
(Last component, will close the issue)

**Testing:** <Describe what testing was performed and which tests were
added.> Added default config use case (validate error)

**Documentation:** <Describe the documentation added.>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants