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.

Sign up for GitHub

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

Jump to bottom

chore: use default confighttp struct for tests #36681

Merged
merged 3 commits into from
Dec 6, 2024
Merged

chore: use default confighttp struct for tests #36681

merged 3 commits into from
Dec 6, 2024

Conversation

rogercoll
Copy link
Contributor

Description

These components are constructing confighttp.ClientConfig field by field and adding the corresponding defaults. Instead of using default values to construct a new struct, the new struct should already contain the defaults.

This becomes an issue when adding new fields with defaults in the ClientConfig structure, see: open-telemetry/opentelemetry-collector#10877

Error in the core collector CI:

=== FAIL: . TestLoadConfig/elasticsearch (re-run 1) (0.00s)
    config_test.go:200: Config mismatch (-expected +actual):
          &elasticsearchreceiver.Config{
          	ControllerConfig: {CollectionInterval: s"2m0s", InitialDelay: s"1s"},
          	ClientConfig: confighttp.ClientConfig{
          		... // 15 identical fields
          		HTTP2PingTimeout: s"0s",
          		Cookies:          nil,
        - 		MaxRedirects:     0,
        + 		MaxRedirects:     10,
          	},
          	MetricsBuilderConfig: {Metrics: {ElasticsearchBreakerMemoryEstimated: {Enabled: true}, ElasticsearchBreakerMemoryLimit: {Enabled: true}, ElasticsearchBreakerTripped: {Enabled: true}, ElasticsearchClusterDataNodes: {Enabled: true}, ...}, ResourceAttributes: {ElasticsearchClusterName: {Enabled: true}, ElasticsearchIndexName: {Enabled: true}, ElasticsearchNodeName: {Enabled: true}, ElasticsearchNodeVersion: {Enabled: true}}},
          	Nodes:                {"_local"},
          	... // 4 identical fields
          }

Link to tracking issue

Fixes

Testing

Documentation

@rogercoll rogercoll requested review from djaglowski and a team as code owners December 4, 2024 18:12
@github-actions github-actions bot requested review from mcube8 and sokoide December 4, 2024 18:12
@rogercoll
Copy link
Contributor Author

Skip changelog?

@rogercoll rogercoll mentioned this pull request Dec 5, 2024
Open
@mx-psi mx-psi added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 5, 2024
@djaglowski djaglowski merged commit 585bfc5 into open-telemetry:main Dec 6, 2024
167 of 168 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 6, 2024
ZenoCC-Peng pushed a commit to ZenoCC-Peng/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2024
@rogercoll @ZenoCC-Peng
chore: use default confighttp struct for tests (open-telemetry#36681)
a8c8bfe 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

These components are constructing `confighttp.ClientConfig` field by
field and adding the corresponding defaults. Instead of using default
values to construct a new struct, the new struct should already contain
the defaults.

This becomes an issue when adding new fields with defaults in the
`ClientConfig` structure, see:
open-telemetry/opentelemetry-collector#10877

Error in the core collector CI:

```
=== FAIL: . TestLoadConfig/elasticsearch (re-run 1) (0.00s)
    config_test.go:200: Config mismatch (-expected +actual):
          &elasticsearchreceiver.Config{
          	ControllerConfig: {CollectionInterval: s"2m0s", InitialDelay: s"1s"},
          	ClientConfig: confighttp.ClientConfig{
          		... // 15 identical fields
          		HTTP2PingTimeout: s"0s",
          		Cookies:          nil,
        - 		MaxRedirects:     0,
        + 		MaxRedirects:     10,
          	},
          	MetricsBuilderConfig: {Metrics: {ElasticsearchBreakerMemoryEstimated: {Enabled: true}, ElasticsearchBreakerMemoryLimit: {Enabled: true}, ElasticsearchBreakerTripped: {Enabled: true}, ElasticsearchClusterDataNodes: {Enabled: true}, ...}, ResourceAttributes: {ElasticsearchClusterName: {Enabled: true}, ElasticsearchIndexName: {Enabled: true}, ElasticsearchNodeName: {Enabled: true}, ElasticsearchNodeVersion: {Enabled: true}}},
          	Nodes:                {"_local"},
          	... // 4 identical fields
          }
```
          

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->
@rogercoll rogercoll deleted the extensible_confighttp_init branch December 7, 2024 13:04
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
@rogercoll @sbylica-splunk
chore: use default confighttp struct for tests (open-telemetry#36681)
cd2d240 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

These components are constructing `confighttp.ClientConfig` field by
field and adding the corresponding defaults. Instead of using default
values to construct a new struct, the new struct should already contain
the defaults.

This becomes an issue when adding new fields with defaults in the
`ClientConfig` structure, see:
open-telemetry/opentelemetry-collector#10877

Error in the core collector CI:

```
=== FAIL: . TestLoadConfig/elasticsearch (re-run 1) (0.00s)
    config_test.go:200: Config mismatch (-expected +actual):
          &elasticsearchreceiver.Config{
          	ControllerConfig: {CollectionInterval: s"2m0s", InitialDelay: s"1s"},
          	ClientConfig: confighttp.ClientConfig{
          		... // 15 identical fields
          		HTTP2PingTimeout: s"0s",
          		Cookies:          nil,
        - 		MaxRedirects:     0,
        + 		MaxRedirects:     10,
          	},
          	MetricsBuilderConfig: {Metrics: {ElasticsearchBreakerMemoryEstimated: {Enabled: true}, ElasticsearchBreakerMemoryLimit: {Enabled: true}, ElasticsearchBreakerTripped: {Enabled: true}, ElasticsearchClusterDataNodes: {Enabled: true}, ...}, ResourceAttributes: {ElasticsearchClusterName: {Enabled: true}, ElasticsearchIndexName: {Enabled: true}, ElasticsearchNodeName: {Enabled: true}, ElasticsearchNodeVersion: {Enabled: true}}},
          	Nodes:                {"_local"},
          	... // 4 identical fields
          }
```
          

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atoulme atoulme atoulme approved these changes

@djaglowski djaglowski Awaiting requested review from djaglowski djaglowski is a code owner

@mcube8 mcube8 Awaiting requested review from mcube8

@sokoide sokoide Awaiting requested review from sokoide

Assignees

@fatsheep9146 fatsheep9146

Labels
exporter/alertmanager receiver/elasticsearch Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
v0.116.0
Development

Successfully merging this pull request may close these issues.
5 participants
@rogercoll @atoulme @djaglowski @mx-psi @fatsheep9146