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

change HTTPClientSettings.ToClient to use component.Host as input #5584

Merged
merged 5 commits into from
Jun 27, 2022

Conversation

fatsheep9146
Copy link
Contributor

Signed-off-by: Ziqi Zhao [email protected]

Description: <Describe what has changed.
Fix #5061

change httpconfig.HTTPClientSettings.ToClient to use component.Host as input parameters

Link to tracking Issue:
#5061

@fatsheep9146 fatsheep9146 requested review from a team and dmitryax June 23, 2022 10:59
@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #5584 (6c2a0e8) into main (d4f1569) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5584   +/-   ##
=======================================
  Coverage   91.29%   91.29%           
=======================================
  Files         191      191           
  Lines       11291    11294    +3     
=======================================
+ Hits        10308    10311    +3     
  Misses        782      782           
  Partials      201      201           
Impacted Files Coverage Δ
exporter/otlphttpexporter/otlp.go 81.73% <ø> (ø)
config/confighttp/confighttp.go 100.00% <100.00%> (ø)
config/configtls/configtls.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4f1569...6c2a0e8. Read the comment docs.

@fatsheep9146
Copy link
Contributor Author

@mx-psi @dmitryax Does this pr need changelog ?

@@ -99,7 +98,7 @@ func NewDefaultHTTPClientSettings() HTTPClientSettings {
}

// ToClient creates an HTTP client.
func (hcs *HTTPClientSettings) ToClient(ext map[config.ComponentID]component.Extension, settings component.TelemetrySettings) (*http.Client, error) {
func (hcs *HTTPClientSettings) ToClient(host component.Host, settings component.TelemetrySettings) (*http.Client, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to do this in several steps, since this is a breaking change. See example 3 on the contributing guidelines.

Based on these guidelines on this PR we should add a new method ToClientWithHost where we pass a host instead of an extension map and deprecate the ToClient method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I will change like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to do this in several steps, since this is a breaking change. See example 3 on the contributing guidelines.

Based on these guidelines on this PR we should add a new method ToClientWithHost where we pass a host instead of an extension map and deprecate the ToClient method.

I added a new function ToClientWithHost, but I have no idea about how to change ToClient to call ToClientWithHost since I can not find a way to create a component.Host interface from map[config.ComponentID]component.Extension , do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea about how to change ToClient to call ToClientWithHost since I can not find a way to create a component.Host interface from map[config.ComponentID]component.Extension , do you have any suggestions?

I don't think you can implement ToClient by calling ToClientWithHost. The point of that rule is to avoid duplicating the implementation, so one thing you can do (assuming no linter would complain) is to do it the other way around:

func (hcs *HTTPClientSettings) ToClientWithHost(host component.Host, settings component.TelemetrySettings) (*http.Client, error) {
    return hcs.ToClient(host.GetExtensions(), settings)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -183,6 +183,91 @@ func (hcs *HTTPClientSettings) ToClient(ext map[config.ComponentID]component.Ext
}, nil
}

// ToClientWithHost creates an HTTP client.
func (hcs *HTTPClientSettings) ToClientWithHost(host component.Host, settings component.TelemetrySettings) (*http.Client, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to deprecate ToClient in favor of this function (see paragraph about "deprecation notice" here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

fatsheep9146 and others added 2 commits June 27, 2022 18:46
Co-authored-by: Pablo Baeyens <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
@bogdandrutu bogdandrutu merged commit bd7e054 into open-telemetry:main Jun 27, 2022
@fatsheep9146 fatsheep9146 deleted the httpconfig-toclient branch July 9, 2022 15:47
@fatsheep9146 fatsheep9146 restored the httpconfig-toclient branch July 9, 2022 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPClientSettings.ToClient is inconsistent with other To* config methods
4 participants