-
Notifications
You must be signed in to change notification settings - Fork 311
Populate HttpClientFactory, utilize in BaseAzureService
#1239
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
Populate HttpClientFactory, utilize in BaseAzureService
#1239
Conversation
…ifferent question. time to test that now
…Factory injected. We now use that to create a new clienttransport for CreateArmClient within BaseAzureService. Now it's time to actually integration test this before we do any major cleanup
…AzureService. time to get this up, handle other feedback from steven, and get it merged
IHttpClientFactory, utilize in BaseAzureServiceClientFactory, utilize in BaseAzureService
ClientFactory, utilize in BaseAzureServiceHttpClientFactory, utilize in BaseAzureService
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.
Pull request overview
This PR addresses the issue where GetArmClient() in BaseAzureService was ignoring recording configuration that HttpClientService properly handles. The solution introduces IHttpClientFactory configuration and injection throughout the codebase to ensure consistent HTTP client setup across Azure SDK calls.
Key changes:
- Introduces
HttpClientFactoryConfiguratorto configureIHttpClientFactorywith recording support, proxy settings, and user-agent headers - Updates
TenantServiceto acceptIHttpClientFactoryand provide configured clients viaGetClient() - Modifies
BaseAzureService.CreateArmClientAsync()to use the configured HTTP client transport
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| core/Azure.Mcp.Core/src/Services/Http/HttpClientFactoryConfigurator.cs | New configurator class that sets up IHttpClientFactory with recording handler, proxy support, and user-agent configuration |
| core/Azure.Mcp.Core/src/Services/Http/RecordingRedirectHandler.cs | Changed visibility from internal sealed to public and fixed trailing whitespace in XML comment |
| core/Azure.Mcp.Core/src/Services/Azure/Tenant/TenantService.cs | Added IHttpClientFactory dependency and GetClient() method to provide configured HTTP clients |
| core/Azure.Mcp.Core/src/Services/Azure/Tenant/ITenantService.cs | Added GetClient() method to the interface |
| core/Azure.Mcp.Core/src/Services/Azure/BaseAzureService.cs | Updated CreateArmClientAsync() to use HttpClientTransport with configured client from TenantService |
| servers/Azure.Mcp.Server/src/Program.cs | Registered configured HTTP client factory in DI container |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Tests/Helpers/TestHttpClientFactoryProvider.cs | New test helper to create configured IHttpClientFactory for live tests |
| tools/Azure.Mcp.Tools.Monitor/tests/Azure.Mcp.Tools.Monitor.LiveTests/MonitorCommandTests.cs | Updated to use TestHttpClientFactoryProvider for creating TenantService instances |
| tools/Azure.Mcp.Tools.Marketplace/tests/Azure.Mcp.Tools.Marketplace.LiveTests/ProductListCommandTests.cs | Updated to use TestHttpClientFactoryProvider and added disposal pattern for proper resource cleanup |
| tools/Azure.Mcp.Tools.Marketplace/tests/Azure.Mcp.Tools.Marketplace.LiveTests/ProductGetCommandTests.cs | Updated to use TestHttpClientFactoryProvider and added disposal pattern for proper resource cleanup |
| tools/Azure.Mcp.Tools.AppConfig/tests/Azure.Mcp.Tools.AppConfig.LiveTests/AppConfigCommandTests.cs | Updated to use TestHttpClientFactoryProvider and added disposal pattern for proper resource cleanup |
core/Azure.Mcp.Core/src/Services/Azure/Tenant/ITenantService.cs
Outdated
Show resolved
Hide resolved
core/Azure.Mcp.Core/src/Services/Http/RecordingRedirectHandler.cs
Outdated
Show resolved
Hide resolved
core/Azure.Mcp.Core/src/Services/Http/HttpClientFactoryConfigurator.cs
Outdated
Show resolved
Hide resolved
tools/Azure.Mcp.Tools.Monitor/tests/Azure.Mcp.Tools.Monitor.LiveTests/MonitorCommandTests.cs
Show resolved
Hide resolved
core/Azure.Mcp.Core/src/Services/Http/HttpClientFactoryConfigurator.cs
Outdated
Show resolved
Hide resolved
core/Azure.Mcp.Core/tests/Azure.Mcp.Tests/Helpers/TestHttpClientFactoryProvider.cs
Show resolved
Hide resolved
vukelich
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.
Some quick early comments to unblock. Will look more later, too.
core/Azure.Mcp.Core/src/Services/Http/HttpClientFactoryConfigurator.cs
Outdated
Show resolved
Hide resolved
core/Azure.Mcp.Core/src/Services/Http/HttpClientFactoryConfigurator.cs
Outdated
Show resolved
Hide resolved
vukelich
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.
Another dump of comments as I continue reading.
core/Azure.Mcp.Core/src/Services/Azure/Tenant/ITenantService.cs
Outdated
Show resolved
Hide resolved
core/Azure.Mcp.Core/src/Services/Http/HttpClientFactoryConfigurator.cs
Outdated
Show resolved
Hide resolved
core/Azure.Mcp.Core/src/Services/Http/HttpClientFactoryConfigurator.cs
Outdated
Show resolved
Hide resolved
core/Azure.Mcp.Core/src/Services/Http/HttpClientFactoryConfigurator.cs
Outdated
Show resolved
Hide resolved
vukelich
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.
Suggestions but all things that could be follow ups.
…t#1239) * Configure IHttpClientFactory and inject to TenantService in preparation for record/playback for any class inheriting from BaseAzureService.
Following up original closed PR #1218 .
Hey @vukelich please take a look when you have a couple minutes? Not entirely done yet but not much more to work through.
This PR is addressing the fact that the
GetArmClient()implementation inBaseAzureServiceignores the recording configuration thatHttpClientServicedoesn't. We do this by:client configuratorthat is basically copy/pasta of HttpClientServiceazure mcp serverutilizing said configurator.IHttpClientFactoryintoTenantServiceBaseAzureServiceI must be missing something with the default client from AddHttpClient(). I should be able to override the default without actually naming it right? Seems super weird that I can't.Yep I was ignorant.IHttpClientFactoryin Azure.Fabric and Azure.Template servers so that the changes toTenantServicedon't melt the world. We only don't see failures because of the fact that we're not running those servers.Todo:
I am going to InjectVery next PR. I want to unblock record/playback.IHttpClientFactoryintoHttpClientServiceand utilize the _clientFactory.CreateClient() over the existing CreateClientInternal(). From there should be easy to entirely replace IHttpClientService in favor of the clientFactory created client.