Skip to content
This repository has been archived by the owner on Jun 10, 2020. It is now read-only.

Performance fixes, wave 1. #864

Merged
merged 14 commits into from
Apr 18, 2019
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
- [Enables Microsoft.Extensions.Logging.ApplicationInsights.ApplicationInsightsLoggerProvider by default. If ApplicationInsightsLoggerProvider was enabled previously using ILoggerFactory extension method, please remove it to prevent duplicate logs.](https://github.com/Microsoft/ApplicationInsights-aspnetcore/issues/854)
- [Remove reference to Microsoft.Extensions.DiagnosticAdapter and use DiagnosticSource subscription APIs directly](https://github.com/Microsoft/ApplicationInsights-aspnetcore/pull/852)
- [Fix: NullReferenceException in ApplicationInsightsLogger.Log when exception contains a Data entry with a null value](https://github.com/Microsoft/ApplicationInsights-aspnetcore/issues/848)
- [Performance fixes for GetUri, SetKeyHeaderValue, ConcurrentDictionary use and Telemetry Initializers](https://github.com/Microsoft/ApplicationInsights-aspnetcore/pull/864)

## Version 2.7.0-beta2
- Added NetStandard2.0 target.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners
using System.Linq;
using System.Text.RegularExpressions;
using Microsoft.ApplicationInsights.Common;
using Microsoft.Extensions.Primitives;

/// <summary>
/// Generic functions that can be used to get and set Http headers.
Expand All @@ -20,7 +21,7 @@ internal static class HeadersUtilities
public static string GetHeaderKeyValue(IEnumerable<string> headerValues, string keyName)
{
if (headerValues != null)
{
{
foreach (string keyNameValue in headerValues)
{
string[] keyNameValueParts = keyNameValue.Trim().Split('=');
Expand All @@ -38,22 +39,33 @@ public static string GetHeaderKeyValue(IEnumerable<string> headerValues, string
/// <summary>
/// Given the provided list of header value strings, return a comma-separated list of key
/// name/value pairs with the provided keyName and keyValue. If the initial header value
/// strings contains the key name, then the original key value should be replaced with the
/// string contains the key name, then the original key value should be replaced with the
/// provided key value. If the initial header value strings don't contain the key name,
/// then the key name/value pair should be added to the comma-separated list and returned.
/// </summary>
/// <param name="headerValues">The existing header values that the key/value pair should be added to.</param>
/// <param name="keyName">The name of the key to add.</param>
/// <param name="keyValue">The value of the key to add.</param>
/// <returns>The result of setting the provided key name/value pair into the provided headerValues.</returns>
public static IEnumerable<string> SetHeaderKeyValue(IEnumerable<string> headerValues, string keyName, string keyValue)
public static StringValues SetHeaderKeyValue(string[] currentHeaders, string key, string value)
{
string[] newHeaderKeyValue = new[] { string.Format(CultureInfo.InvariantCulture, "{0}={1}", keyName.Trim(), keyValue.Trim()) };
return headerValues == null || !headerValues.Any()
? newHeaderKeyValue
: headerValues
.Where(headerValue => !HeaderMatchesKey(headerValue, keyName))
.Concat(newHeaderKeyValue);
if (currentHeaders != null)
{
for (int index = 0; index < currentHeaders.Length; index++)
{
if (HeaderMatchesKey(currentHeaders[index], key))
{
currentHeaders[index] = string.Concat(key, "=", value);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Tried making it a variable, IL adds an extra allocation for that, so kept in three places on purpose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds suspicious. Are you counting locals (stack slots) from IL? Those aren't the kind of allocations (GC heap) to be worried about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. ldloc.1 and similar. Memory hit here is fine, i'd like to avoid extra instructions if possible :)
Not to the point of one allocation of course, but no allocation simply sounded better than "an allocation" upon review of IL here. Not a problem, can move to a variable if you feel it's absolutely necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IL instruction count isn't really that important. Sure, fewer is generally better (you can fit more on a cache line) but, of course, it's the generated machine code that really matters. An IL "slot" is required for local variables (and args) and they'll usually map to a register or a BP-relative memory location. We might call the latter "stack allocation", but it's nowhere near the same as a GC allocation. In this case, I'd expect that the total IL instruction count for this method when hoisting the expression to a local variable (common subexpression elimination) will be less overall -- because of the saved duplicate code. I doubt the C# compiler will do the hoisting itself -- and if it did, it'd use a stack slot.

This is all a long explanation for why you shouldn't worry about IL slots for perf.

For this case, I don't mind how you write it. I just wanted to explain why "IL adds an extra allocation" raised my Spock-like eyebrow and, hopefully, educate the team :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Great to know!

return currentHeaders;
}
}

return StringValues.Concat(currentHeaders, string.Concat(key, "=", value));
}
else
{
return string.Concat(key, "=", value);
}
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,15 @@ public void OnHttpRequestInStart(HttpContext httpContext)
// with W3C support on .NET https://github.com/dotnet/corefx/issues/30331

newActivity = new Activity(ActivityCreatedByHostingDiagnosticListener);
newActivity.SetParentId(StringUtilities.GenerateTraceId());
if (this.enableW3CHeaders)
{
newActivity.GenerateW3CContext();
newActivity.SetParentId(newActivity.GetTraceId());
}
else
{
newActivity.SetParentId(W3CUtilities.GenerateTraceId());
}
// end of workaround
}

Expand Down Expand Up @@ -257,7 +265,9 @@ public void OnBeginRequest(HttpContext httpContext, long timestamp)
InjectionGuardConstants.RequestHeaderMaxLength);
}
}
else if(!activity.IsW3CActivity())

// no headers
else if (originalParentId == null)
{
// As a first step in supporting W3C protocol in ApplicationInsights,
// we want to generate Activity Ids in the W3C compatible format.
Expand All @@ -266,8 +276,16 @@ public void OnBeginRequest(HttpContext httpContext, long timestamp)
// So if there is no current Activity (i.e. there were no Request-Id header in the incoming request), we'll override ParentId on
// the current Activity by the properly formatted one. This workaround should go away
// with W3C support on .NET https://github.com/dotnet/corefx/issues/30331

activity.SetParentId(StringUtilities.GenerateTraceId());

if (this.enableW3CHeaders)
{
activity.GenerateW3CContext();
activity.SetParentId(activity.GetTraceId());
}
else
{
activity.SetParentId(W3CUtilities.GenerateTraceId());
}

// end of workaround
}
Expand Down Expand Up @@ -337,6 +355,10 @@ private RequestTelemetry InitializeRequestTelemetry(HttpContext httpContext, Act
requestTelemetry.Context.Operation.Id = activity.RootId;
requestTelemetry.Id = activity.Id;
}
else
{
activity.UpdateTelemetry(requestTelemetry, false);
}

foreach (var prop in activity.Baggage)
{
Expand All @@ -346,7 +368,7 @@ private RequestTelemetry InitializeRequestTelemetry(HttpContext httpContext, Act
}
}

this.client.Initialize(requestTelemetry);
this.client.InitializeInstrumentationKey(requestTelemetry);

requestTelemetry.Source = GetAppIdFromRequestHeader(httpContext.Request.Headers, requestTelemetry.Context.InstrumentationKey);

Expand Down Expand Up @@ -490,10 +512,6 @@ private void SetW3CContext(IHeaderDictionary requestHeaders, Activity activity,
InjectionGuardConstants.TraceParentHeaderMaxLength);
activity.SetTraceparent(parentTraceParent);
}
else
{
activity.GenerateW3CContext();
}

string[] traceStateValues = HttpHeadersUtilities.SafeGetCommaSeparatedHeaderValues(requestHeaders, W3C.W3CConstants.TraceStateHeader,
InjectionGuardConstants.TraceStateHeaderMaxLength, InjectionGuardConstants.TraceStateMaxPairs);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,36 +65,19 @@ internal static bool ContainsRequestContextKeyValue(IHeaderDictionary headers, s
return !string.IsNullOrEmpty(GetHeaderKeyValue(headers, RequestResponseHeaders.RequestContextHeader, keyName));
}

internal static void SetRequestContextKeyValue(HttpHeaders headers, string keyName, string keyValue)
{
SetHeaderKeyValue(headers, RequestResponseHeaders.RequestContextHeader, keyName, keyValue);
}

internal static void SetRequestContextKeyValue(IHeaderDictionary headers, string keyName, string keyValue)
{
SetHeaderKeyValue(headers, RequestResponseHeaders.RequestContextHeader, keyName, keyValue);
}

internal static void SetHeaderKeyValue(HttpHeaders headers, string headerName, string keyName, string keyValue)
{
if (headers == null)
{
throw new ArgumentNullException(nameof(headers));
}

IEnumerable<string> headerValues = GetHeaderValues(headers, headerName);
headers.Remove(headerName);
headers.Add(headerName, HeadersUtilities.SetHeaderKeyValue(headerValues, keyName, keyValue));
}

internal static void SetHeaderKeyValue(IHeaderDictionary headers, string headerName, string keyName, string keyValue)
{
if (headers == null)
{
throw new ArgumentNullException(nameof(headers));
}

headers[headerName] = new StringValues(HeadersUtilities.SetHeaderKeyValue(headers[headerName].AsEnumerable(), keyName, keyValue).ToArray());
headers[headerName] = HeadersUtilities.SetHeaderKeyValue(headers[headerName], keyName, keyValue);
}

internal static string[] SafeGetCommaSeparatedHeaderValues(IHeaderDictionary headers, string headerName, int maxLength, int maxItems)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,26 +28,12 @@ public static Uri GetUri(this HttpRequest request)
{
throw new ArgumentException("Http request Scheme is not specified");
}

string hostName = request.Host.HasValue ? request.Host.ToString() : UnknownHostName;

var builder = new StringBuilder();

builder.Append(request.Scheme)
.Append("://")
.Append(hostName);

if (true == request.Path.HasValue)
{
builder.Append(request.Path.Value);
}

if (true == request.QueryString.HasValue)
{
builder.Append(request.QueryString);
}

return new Uri(builder.ToString());
return new Uri(string.Concat(request.Scheme,
"://",
request.Host.HasValue ? request.Host.Value : UnknownHostName,
request.Path.HasValue ? request.Path.Value : "",
request.QueryString.HasValue ? request.QueryString.Value : ""));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ public AspNetCoreEnvironmentTelemetryInitializer(IHostingEnvironment environment

/// <inheritdoc />
public void Initialize(ITelemetry telemetry)
{
if (environment != null && !telemetry.Context.GlobalProperties.ContainsKey(AspNetCoreEnvironmentPropertyName))
{
if (environment != null && !telemetry.Context.Properties.ContainsKey(AspNetCoreEnvironmentPropertyName))
{
telemetry.Context.GlobalProperties.Add(AspNetCoreEnvironmentPropertyName, environment.EnvironmentName);
telemetry.Context.Properties.Add(AspNetCoreEnvironmentPropertyName, environment.EnvironmentName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can avoid touching properties at all in default scenarios, then that'll also save the perf hit from instantiating the Properties Conc.Dictionary.

Can you try if changes similar to this(https://github.com/Microsoft/ApplicationInsights-dotnet/pull/929/files) can be applied he as well? We'll need to modify all telemetry types to have this special internal field.

https://github.com/Microsoft/ApplicationInsights-dotnet/pull/929/files

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, don't we stamp each request with Pre-Aggregation's "_MS.AggregatedByMetricExtractors" or similar? Which dictionary does it use - we should just use that one since it's initialized for each request anyway.

}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
namespace Microsoft.ApplicationInsights.AspNetCore.Tests
{
using System.Collections.Generic;
using System.Linq;
using DiagnosticListeners;
using Xunit;

Expand Down Expand Up @@ -30,36 +29,35 @@ public void ShouldReturnNullWhenKeyNotExists()
[Fact]
public void ShouldReturnHeadersWhenNoHeaderValues()
{
IEnumerable<string> newHeaders = HeadersUtilities.SetHeaderKeyValue(null, "Key", "Value");
string[] newHeaders = HeadersUtilities.SetHeaderKeyValue(null, "Key", "Value");

Assert.NotNull(newHeaders);
Assert.Equal(1, newHeaders.Count());
Assert.Equal("Key=Value", newHeaders.First());
Assert.Single(newHeaders);
Assert.Equal("Key=Value", newHeaders[0]);
}

[Fact]
public void ShouldAppendHeaders()
{
IEnumerable<string> existing = new List<string>() { "ExistKey=ExistValue" };
IEnumerable<string> result = HeadersUtilities.SetHeaderKeyValue(existing, "NewKey", "NewValue");
string[] existing = new string[] { "ExistKey=ExistValue" };
string[] result = HeadersUtilities.SetHeaderKeyValue(existing, "NewKey", "NewValue");

Assert.NotNull(result);
Assert.Equal(2, result.Count());
Assert.NotNull(result.FirstOrDefault(headerValue => headerValue.Equals("ExistKey=ExistValue")));
Assert.NotNull(result.FirstOrDefault(headerValue => headerValue.Equals("NewKey=NewValue")));
Assert.Equal(2, result.Length);
Assert.Equal("ExistKey=ExistValue", result[0]);
Assert.Equal("NewKey=NewValue", result[1]);
}

[Fact]
public void ShouldUpdateExistingHeader()
{
IEnumerable<string> existing = new List<string>() { "ExistKey=ExistValue", "NoiseKey=NoiseValue" };
IEnumerable<string> result = HeadersUtilities.SetHeaderKeyValue(existing, "ExistKey", "NewValue");
string[] existing = new string[] { "ExistKey=ExistValue", "NoiseKey=NoiseValue" };
string[] result = HeadersUtilities.SetHeaderKeyValue(existing, "ExistKey", "NewValue");

Assert.NotNull(result);
Assert.Equal(2, result.Count());
Assert.Null(result.FirstOrDefault(headerValue => headerValue.Equals("ExistKey=ExistValue")));
Assert.NotNull(result.FirstOrDefault(headerValue => headerValue.Equals("ExistKey=NewValue")));
Assert.NotNull(result.FirstOrDefault(headerValue => headerValue.Equals("NoiseKey=NoiseValue")));
Assert.Equal(2, result.Length);
Assert.Equal("ExistKey=NewValue", result[0]);
Assert.Equal("NoiseKey=NoiseValue", result[1]);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -836,10 +836,6 @@ public void OnBeginRequestWithW3CSupportAndNoHeadersIsTrackedCorrectly(bool isAs

var activityInitializedByW3CHeader = Activity.Current;

if (!isAspNetCore2)
{
Assert.Null(activityInitializedByW3CHeader.ParentId);
}
Assert.NotNull(activityInitializedByW3CHeader.GetTraceId());
Assert.Equal(32, activityInitializedByW3CHeader.GetTraceId().Length);
Assert.Equal(16, activityInitializedByW3CHeader.GetSpanId().Length);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ public void InitializeDoesNotOverrideExistingProperty()
{
var initializer = new AspNetCoreEnvironmentTelemetryInitializer(new HostingEnvironment() { EnvironmentName = "Production"});
var telemetry = new RequestTelemetry();
telemetry.Context.GlobalProperties.Add("AspNetCoreEnvironment", "Development");
telemetry.Context.Properties.Add("AspNetCoreEnvironment", "Development");
initializer.Initialize(telemetry);

Assert.Equal("Development", telemetry.Context.GlobalProperties["AspNetCoreEnvironment"]);
Assert.Equal("Development", telemetry.Context.Properties["AspNetCoreEnvironment"]);
}

[Fact]
Expand All @@ -32,7 +32,7 @@ public void InitializeSetsCurrentEnvironmentNameToProperty()
var telemetry = new RequestTelemetry();
initializer.Initialize(telemetry);

Assert.Equal("Production", telemetry.Context.GlobalProperties["AspNetCoreEnvironment"]);
Assert.Equal("Production", telemetry.Context.Properties["AspNetCoreEnvironment"]);
}
}
}