-
Notifications
You must be signed in to change notification settings - Fork 285
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
modify AzureMetadataRequestor to accept HttpClient in ctor. rewrite AzureInstanceMetadata tests to mock HttpClient instead of using in-proc server. #2358
Conversation
azureImsClient.DefaultRequestHeaders.Add("Metadata", "True"); | ||
azureImsClient.Timeout = this.AzureImsRequestTimeout; | ||
this.httpClient.MaxResponseContentBufferSize = AzureMetadataRequestor.AzureImsMaxResponseBufferSize; | ||
this.httpClient.DefaultRequestHeaders.Add("Metadata", "True"); |
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.
Would this end up adding multiple headers to the httpClient
?
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.
moved to constructor
azureImsClient.Timeout = this.AzureImsRequestTimeout; | ||
this.httpClient.MaxResponseContentBufferSize = AzureMetadataRequestor.AzureImsMaxResponseBufferSize; | ||
this.httpClient.DefaultRequestHeaders.Add("Metadata", "True"); | ||
this.httpClient.Timeout = this.AzureImsRequestTimeout; |
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.
Do we need to change Timeout each time?
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.
moved to constructor
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.
Please find my comments.
The change seems to touch things beyond test cases. Either we need to change the PR title/description or scope it down to test cases. |
/// </summary> | ||
internal TimeSpan AzureImsRequestTimeout = TimeSpan.FromSeconds(10); | ||
private TimeSpan AzureImsRequestTimeout = TimeSpan.FromSeconds(10); | ||
|
||
private readonly HttpClient httpClient; |
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.
Who owns the lifecycle management for this http client? Who is responsible for Dispose
?
What is the thread safety guarantee? Do we allow concurrent invocations?
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.
this is a miss on my part. I need to implement the correct Dispose pattern.
This code is only ever called once, when the SDK initializes TelemetryModules. There are no retries.
Lines 60 to 89 in a90eb8f
public void Initialize(TelemetryConfiguration unused) | |
{ | |
// Core SDK creates 1 instance of a module but calls Initialize multiple times | |
if (!this.IsInitialized) | |
{ | |
lock (this.lockObject) | |
{ | |
if (!this.IsInitialized) | |
{ | |
var hbeatManager = this.HeartbeatPropertyManager; | |
if (hbeatManager != null) | |
{ | |
// start off the heartbeat property collection process, but don't wait for it nor report | |
// any status from here, fire and forget. The thread running the collection will report | |
// to the core event log. | |
try | |
{ | |
var heartbeatProperties = new AzureComputeMetadataHeartbeatPropertyProvider(); | |
Task.Factory.StartNew( | |
async () => await heartbeatProperties.SetDefaultPayloadAsync(hbeatManager) | |
.ConfigureAwait(false)); | |
} | |
catch (Exception heartbeatAquisitionException) | |
{ | |
WindowsServerEventSource.Log.AzureInstanceMetadataFailureWithException(heartbeatAquisitionException.Message, heartbeatAquisitionException.InnerException?.Message); | |
} | |
} | |
this.IsInitialized = true; | |
} |
Lines 59 to 103 in a90eb8f
public async Task<bool> SetDefaultPayloadAsync(IHeartbeatPropertyManager provider) | |
{ | |
bool hasSetFields = false; | |
try | |
{ | |
if (!this.isAzureMetadataCheckCompleted) | |
{ | |
this.isAzureMetadataCheckCompleted = true; | |
var azureComputeMetadata = await this.azureInstanceMetadataRequestor.GetAzureComputeMetadataAsync() | |
.ConfigureAwait(false); | |
if (azureComputeMetadata != null) | |
{ | |
var enabledImdsFields = this.ExpectedAzureImsFields.Except(provider.ExcludedHeartbeatProperties); | |
foreach (string field in enabledImdsFields) | |
{ | |
string verifiedValue = azureComputeMetadata.VerifyExpectedValue(field); | |
bool addedProperty = provider.AddHeartbeatProperty( | |
propertyName: string.Concat(AzureComputeMetadataHeartbeatPropertyProvider.HeartbeatPropertyPrefix, field), | |
propertyValue: verifiedValue, | |
isHealthy: true); | |
if (!addedProperty) | |
{ | |
WindowsServerEventSource.Log.AzureInstanceMetadataWasntAddedToHeartbeatProperties(field, verifiedValue); | |
} | |
hasSetFields = hasSetFields || addedProperty; | |
} | |
} | |
else | |
{ | |
WindowsServerEventSource.Log.AzureInstanceMetadataNotAdded(); | |
} | |
} | |
} | |
catch (Exception setPayloadException) | |
{ | |
WindowsServerEventSource.Log.AzureInstanceMetadataFailureSettingDefaultPayload(setPayloadException.Message, setPayloadException.InnerException?.Message); | |
} | |
return hasSetFields; | |
} |
using System.Collections.Generic; | ||
using System.Threading.Tasks; | ||
using Microsoft.ApplicationInsights.WindowsServer.Implementation.DataContracts; | ||
|
||
internal interface IAzureMetadataRequestor | ||
internal interface IAzureMetadataRequestor : IDisposable |
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.
Do we need this?
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.
The call stack here is: TelemetryModule > Provider > Requestor > HttpClient.
We don't NEED this interface to be disposable.
Alternatively, the Provider can check if it's private requestor implements IDisposable.
This introduces a little more complexity and it's not as obvious that the Requestor needs to be disposed. (The Analyzers can't catch this scenario).
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.
If we don't need this, let's try not to add it.
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.
@@ -101,5 +103,25 @@ public async Task<bool> SetDefaultPayloadAsync(IHeartbeatPropertyManager provide | |||
|
|||
return hasSetFields; | |||
} | |||
|
|||
public void Dispose() |
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.
Who will call this method?
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.
The call stack here is: TelemetryModule > Provider > Requestor > HttpClient.
Inside the TelemetryModule, I moved the Provider into a using to ensure it gets disposed.
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.
Stepping back, if the whole purpose is to allow mock in the unit test, would reflection (setting some private member instead of having to change any production code) work?
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.
I'm not sure that a private member helps us here.
The original class doesn't have a private HttpClient. It creates the client just-in-time to make a request.
This is how it avoided the responsibility of Disposing.
Lines 88 to 110 in a90eb8f
private async Task<AzureInstanceComputeMetadata> MakeWebRequestAsync(string requestUrl) | |
{ | |
AzureInstanceComputeMetadata azureIms = null; | |
DataContractJsonSerializer deserializer = new DataContractJsonSerializer(typeof(AzureInstanceComputeMetadata)); | |
using (var azureImsClient = new HttpClient()) | |
{ | |
azureImsClient.MaxResponseContentBufferSize = AzureMetadataRequestor.AzureImsMaxResponseBufferSize; | |
azureImsClient.DefaultRequestHeaders.Add("Metadata", "True"); | |
azureImsClient.Timeout = this.AzureImsRequestTimeout; | |
Stream content = await azureImsClient.GetStreamAsync(new Uri(requestUrl)).ConfigureAwait(false); | |
azureIms = (AzureInstanceComputeMetadata)deserializer.ReadObject(content); | |
content.Dispose(); | |
if (azureIms == null) | |
{ | |
WindowsServerEventSource.Log.CannotObtainAzureInstanceMetadata(); | |
} | |
} | |
return azureIms; | |
} |
I can mock any HTTP response if I can provide a custom HttpMessageHandler into the HttpClient.
var mockHttpMessageHandler = new Mock<HttpMessageHandler>();
new HttpClient(mockHttpMessageHandler.Object)
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.
I came up with an alternate approach that works.
Tomorrow i'll clean it up and create a new PR.
superceded by #2360 |
When switching to FrameworkReferences, discovered a problem with some tests for AzureInstanceMetadada (see full write up in #2357).
Changes
AzureImsResponseTooLargeStopsCollection
. HttpClient correctly handles and parses the larger response.AzureImsResponseTimesOut
withAzureImsResponseHandlesException
. Could not force a timeout in a test context. HttpClient throws an exception when timeout. I changed this test to throw an exception and verify that this is handled correctly.Checklist
For significant contributions please make sure you have completed the following items:
The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.
Notes for authors:
Notes for reviewers:
/AzurePipelines run
will queue all builds/AzurePipelines run <pipeline-name>
will queue a specific build