[confighttp, configgrpc] Replace host parameter by WithXExtensions option

[jade-guiton-dd]

Description

Modifies the ToServer and ToClient(Conn) methods in confighttp and configgrpc to no longer take a host component.Host parameter, replacing it with a With(Client|Server)Extensions option, to be created with the result of host.GetExtensions.

Link to tracking issue

Fixes #13640

Testing

Existing tests make extensive use of the option

Documentation

Added line in ToServer/ToClient(Conn) documentation describing the intended use of WithXExtensions.

[codecov]

Codecov Report

❌ Patch coverage is 92.45283% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.22%. Comparing base (507664f) to head (d200fc3).
⚠️ Report is 41 commits behind head on main.

Files with missing lines	Patch %	Lines
config/configgrpc/configgrpc.go	93.54%	2 Missing ⚠️
config/confighttp/client.go	90.00%	1 Missing ⚠️
receiver/otlpreceiver/otlp.go	50.00%	0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (92.45%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14162      +/-   ##
==========================================
- Coverage   92.27%   92.22%   -0.05%     
==========================================
  Files         657      658       +1     
  Lines       41093    41213     +120     
==========================================
+ Hits        37917    38010      +93     
- Misses       2174     2193      +19     
- Partials     1002     1010       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jade-guiton-dd]

The unsatisfactory coverage seems to be because CodeCov doesn't understand the sealed interface pattern.

[mx-psi]

I am a bit worried that component developers may forget to pass this option.

One thing that would help in that regard is if we could have an error message with a coda like:

No extensions were passed when building the server

(resp. client)

or soemthing else that makes it clear that this is a build-time/logic error, not something an end user did wrong

[axw]

What do you think about combining WithClientExtensions and WithServerExtensions, so it's just WithExtensions returning a type that implements both interfaces? The names feel a bit awkward, since there's no such thing as client and server extensions

[jade-guiton-dd]

I tried doing that initially, but that would mean either exposing the underlying option type, or inventing a GenericOption interface that combines ToClientConnOption and ToServerOption.

[axw]

The latter is what I had in mind - I was thinking ToCommonOption FWIW

[jade-guiton-dd]

Alright, I can try doing that.

[jade-guiton-dd]

I am a bit worried that component developers may forget to pass this option.

One thing that would help in that regard is if we could have an error message with a coda like:

No extensions were passed when building the server

(resp. client)

or soemthing else that makes it clear that this is a build-time/logic error, not something an end user did wrong

The issue is: how would we deal with tests in that case?

For the record, there is already a special error message if a nil extensions map is passed but a middleware was requested (as opposed to an empty extensions map).

[evan-bradley]

The issue is: how would we deal with tests in that case?

For the record, there is already a special error message if a nil extensions map is passed but a middleware was requested (as opposed to an empty extensions map).

I think nil and not passing in the option are equivalent. In my mind it's similar to using a nop host that returns nil from GetExtensions.

As for which one to prefer, I could go either way assuming there are no major usability issues for either one. Ideally we take a nil or non-presence of the option as the component author explicitly disabling middlewares for their component and print out a message indicating such. I like that making it a required parameter makes the caller actually think about whether they need it, but it's also nice that WithMiddlewares kinda acts as a named parameter when you call it (which I suppose is purely a personal preference).

[axw]

I think nil and not passing in the option are equivalent. In my mind it's similar to using a nop host that returns nil from GetExtensions.

My 2c: you have to be a bit more intentional about passing in nil vs. not passing in an option. i.e. you might easily miss the need to pass in an option since they're variadic, whereas you can't miss the need to pass in another parameter.

Ideally we take a nil or non-presence of the option as the component author explicitly disabling middlewares for their component and print out a message indicating such.

Is it even something that should be supported, components disabling support for middleware? If it is, and a user configures middleware, I think it should fail spectacularly rather than log and continue (if that's what you meant), so users don't get surprised that their config is ineffective.

[evan-bradley]

Is it even something that should be supported, components disabling support for middleware?

I don't think we should promote disabling middleware support, but I still think we need to handle a nil extensions map as an edge case. I mostly just called it out as what we would communicate to the user before exiting. The alternative would be to say it's likely a code issue and the user should file a bug report. I can see cases where e.g. a protocol is implemented on top of HTTP and standard middleware extensions won't work well, therefore a component author disables middleware support, but we may never hit that case.

If it is, and a user configures middleware, I think it should fail spectacularly rather than log and continue (if that's what you meant), so users don't get surprised that their config is ineffective.

Sorry, I could have been more clear. My intent was that passing nil/not passing an option should print out a message clarifying the situation, as opposed to just saying an extension couldn't be found. Basically what Pablo said here. I agree we should fail fast.

[jade-guiton-dd]

Based on the previous discussion, I've created an alternate PR (was simpler than updating this one) where the map is passed as a positional parameter instead of an option: #14190

Will look into whether I need to update error messages to make it clearer that extensions: nil is probably due to a component taking the shortcut of not passing in host.GetExtensions() and mark as ready for review