modify AzureMetadataRequestor to accept HttpClient in ctor. rewrite AzureInstanceMetadata tests to mock HttpClient instead of using in-proc server. by TimothyMothra · Pull Request #2358 · microsoft/ApplicationInsights-dotnet

TimothyMothra · 2021-08-10T19:26:05Z

When switching to FrameworkReferences, discovered a problem with some tests for AzureInstanceMetadada (see full write up in #2357).

Changes

Replace local inproc server with HttpClient and mock HttpMessageHandler.
Remove test AzureImsResponseTooLargeStopsCollection. HttpClient correctly handles and parses the larger response.
Replace test AzureImsResponseTimesOut with AzureImsResponseHandlesException. 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

I ran Unit Tests locally.
CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public surface reviewed

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

FxCop and other analyzers will fail the build. To see these errors yourself, compile localy using the Release configuration.

Notes for reviewers:

We support comment build triggers
- /AzurePipelines run will queue all builds
- /AzurePipelines run <pipeline-name> will queue a specific build

reyang · 2021-08-10T19:46:44Z

WEB/Src/WindowsServer/WindowsServer/Implementation/AzureMetadataRequestor.cs

-                azureImsClient.DefaultRequestHeaders.Add("Metadata", "True");
-                azureImsClient.Timeout = this.AzureImsRequestTimeout;
+            this.httpClient.MaxResponseContentBufferSize = AzureMetadataRequestor.AzureImsMaxResponseBufferSize;
+            this.httpClient.DefaultRequestHeaders.Add("Metadata", "True");


Would this end up adding multiple headers to the httpClient?

moved to constructor

reyang · 2021-08-10T19:47:06Z

WEB/Src/WindowsServer/WindowsServer/Implementation/AzureMetadataRequestor.cs

-                azureImsClient.Timeout = this.AzureImsRequestTimeout;
+            this.httpClient.MaxResponseContentBufferSize = AzureMetadataRequestor.AzureImsMaxResponseBufferSize;
+            this.httpClient.DefaultRequestHeaders.Add("Metadata", "True");
+            this.httpClient.Timeout = this.AzureImsRequestTimeout;


Do we need to change Timeout each time?

moved to constructor

reyang

Please find my comments.

reyang · 2021-08-10T19:48:18Z

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.

reyang · 2021-08-10T20:12:57Z

WEB/Src/WindowsServer/WindowsServer/Implementation/AzureMetadataRequestor.cs

-        internal TimeSpan AzureImsRequestTimeout = TimeSpan.FromSeconds(10);
+        private TimeSpan AzureImsRequestTimeout = TimeSpan.FromSeconds(10);

        private readonly HttpClient httpClient;


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?

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.

ApplicationInsights-dotnet/WEB/Src/WindowsServer/WindowsServer/AzureInstanceMetadataTelemetryModule.cs

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;

}

ApplicationInsights-dotnet/WEB/Src/WindowsServer/WindowsServer/Implementation/AzureComputeMetadataHeartbeatPropertyProvider.cs

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;

}

reyang · 2021-08-10T21:36:36Z

WEB/Src/WindowsServer/WindowsServer/Implementation/IAzureMetadataRequestor.cs

    using Microsoft.ApplicationInsights.WindowsServer.Implementation.DataContracts;

-    internal interface IAzureMetadataRequestor
+    internal interface IAzureMetadataRequestor : IDisposable


Do we need this?

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).

If we don't need this, let's try not to add it.

FYI this might be a good approach https://github.com/open-telemetry/opentelemetry-dotnet/blob/95893457080a772eb75e7885e640d2c055b2a0e1/src/OpenTelemetry/Trace/TracerProviderSdk.cs#L304

reyang · 2021-08-10T21:37:04Z

.../WindowsServer/WindowsServer/Implementation/AzureComputeMetadataHeartbeatPropertyProvider.cs

            return hasSetFields;
        }
+
+        public void Dispose()


Who will call this method?

The call stack here is: TelemetryModule > Provider > Requestor > HttpClient.

Inside the TelemetryModule, I moved the Provider into a using to ensure it gets disposed.

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?

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.

ApplicationInsights-dotnet/WEB/Src/WindowsServer/WindowsServer/Implementation/AzureMetadataRequestor.cs

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)

I came up with an alternate approach that works.
Tomorrow i'll clean it up and create a new PR.

TimothyMothra · 2021-08-11T17:27:19Z

superceded by #2360

rewrite tests

0fae348

TimothyMothra requested review from cijothomas, rajkumar-rangaraj and reyang August 10, 2021 19:27

TimothyMothra linked an issue Aug 10, 2021 that may be closed by this pull request

AzureInstanceMetadata: replace tests #2357

Closed

reyang reviewed Aug 10, 2021

View reviewed changes

reyang requested changes Aug 10, 2021

View reviewed changes

TimothyMothra changed the title ~~rewrite AzureInstanceMetadata tests~~ modify AzureMetadataRequestor to accept HttpClient in ctor. rewrite AzureInstanceMetadata tests to mock HttpClient instead of using in-proc server. Aug 10, 2021

TimothyMothra added 2 commits August 10, 2021 13:05

code review comments

25e91b5

fxcop

a2bff81

reyang reviewed Aug 10, 2021

View reviewed changes

dispose pattern

d72598e

reyang reviewed Aug 10, 2021

View reviewed changes

TimothyMothra added 4 commits August 10, 2021 16:22

remove IDisposable from IAzureMetadatarequestor

768e1b0

merge develop

d722454

remove ignore

321c0ad

cleanup

f51ec52

TimothyMothra removed a link to an issue Aug 11, 2021

AzureInstanceMetadata: replace tests #2357

Closed

TimothyMothra mentioned this pull request Aug 11, 2021

modify AzureMetadataRequestor. rewrite tests to mock HttpClient instead of using in-proc server (2) #2360

Merged

4 tasks

TimothyMothra closed this Aug 11, 2021

TimothyMothra deleted the tilee/fix_azureinstancemetadata_tests branch August 11, 2021 20:11

	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;
	}

	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;
	}

	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;
	}

Conversation

TimothyMothra commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Checklist

Notes for authors:

Notes for reviewers:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

reyang commented Aug 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyMothra commented Aug 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TimothyMothra commented Aug 10, 2021 •

edited

Loading