-
Notifications
You must be signed in to change notification settings - Fork 360
Add HttpClientFactory setting to OpAmpClientSettings
#3589
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
Add HttpClientFactory setting to OpAmpClientSettings
#3589
Conversation
Introduce a new `HttpClientFactory` setting in the `OpAmpClientSettings` class to allow customization of `HttpClient` instances for OpAMP HTTP transport. Update `PlainHttpTransport` to utilize this factory, removing the `HttpClientHandler`. Modify `OpAmpClient` to pass settings to `PlainHttpTransport`. Add a test to verify the use of the configured `HttpClientFactory`, including custom headers. Update `OpAmpFakeHttpServer` to capture HTTP request headers and provide a method to retrieve them for testing. Update `CHANGELOG.md` and `PublicAPI.Unshipped.txt` to document the new setting.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3589 +/- ##
==========================================
+ Coverage 71.22% 71.24% +0.02%
==========================================
Files 442 442
Lines 17516 17515 -1
==========================================
+ Hits 12475 12479 +4
+ Misses 5041 5036 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
src/OpenTelemetry.OpAmp.Client/Internal/Transport/Http/PlainHttpTransport.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
…tpTransport.cs Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
|
In the issues description you have mentioned mutual TLS. Please consider this SDK PR open-telemetry/opentelemetry-dotnet#6343 in this context. |
|
@RassK taken longer break. He will be available next year. Is it fine to wait for him with this PR? |
|
@Kielek I have been chatting with @RassK on some blockers for us to start using the OpAmp client. This and one other (PR incoming today/tomorrow) are the main blockers I'm aware of immediately. Ideally we'd get these in sooner and potentially another release done so I can take that work further. Is that a possibility? Otherwise I'm blocked from working with this new feature. I'm hoping to them contribute in other areas as I identify other things we can improve. |
|
@martincostello should be also quite familiar with the opamp part. Helped me to shape it. I think if he is fine with changes, don't wait for me. @Kielek knows the issues of auto inst. side and it's contraints. So I think it's quite safe to go forward a bit faster. |
Kielek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTMRS
Fixes #3588
Changes
Introduces a new
HttpClientFactorysetting in theOpAmpClientSettingsclass to allow customization ofHttpClientinstances for OpAMP HTTP transport. UpdatesPlainHttpTransportto utilize this factory, removing theHttpClientHandlerand modifiesOpAmpClientto pass settings toPlainHttpTransport.This mostly copies the approach used for the
OtlpExporterOptionsin the main SDK.HttpClientFactory, including custom headers.OpAmpFakeHttpServerto capture HTTP request headers and provide a method to retrieve them for testing.Update
CHANGELOG.mdandPublicAPI.Unshipped.txtto document the new setting.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes