Skip to content
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.

Sign up for GitHub

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

Jump to bottom

remove httpcontext from rolename initializer #1340

Merged
merged 7 commits into from
Dec 6, 2019
Merged

remove httpcontext from rolename initializer #1340

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f3f36b
remove httpcontext from rolename initializer
TimothyMothra Dec 4, 2019
af459d6
cleanup
TimothyMothra Dec 4, 2019
85e6d12
cleanup
TimothyMothra Dec 4, 2019
93e5227
add bunch of tests
TimothyMothra Dec 5, 2019
ca6f93d
Merge branch 'develop' into tilee/httpcontext_rolename
TimothyMothra Dec 5, 2019
23bdb30
changelog
TimothyMothra Dec 5, 2019
6ff03aa
change RoleNameContainer to streamline use
TimothyMothra Dec 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...cationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ public void OnHttpRequestInStart(HttpContext httpContext)
bool traceParentPresent = false;
var headers = httpContext.Request.Headers;

// Update the static RoleName while we have access to the httpContext.
RoleNameContainer.Set(headers);

// 3 possibilities when TelemetryConfiguration.EnableW3CCorrelation = true
// 1. No incoming headers. originalParentId will be null. Simply use the Activity as such.
// 2. Incoming Request-ID Headers. originalParentId will be request-id, but Activity ignores this for ID calculations.
Expand Down Expand Up @@ -318,6 +321,10 @@ public void OnBeginRequest(HttpContext httpContext, long timestamp)
// 1.XX does not create Activity and SDK is responsible for creating Activity.
var activity = new Activity(ActivityCreatedByHostingDiagnosticListener);
IHeaderDictionary requestHeaders = httpContext.Request.Headers;

// Update the static RoleName while we have access to the httpContext.
RoleNameContainer.Set(requestHeaders);

string originalParentId = null;
string legacyRootId = null;

Expand Down
10 changes: 5 additions & 5 deletions ...licationInsights.AspNetCore/Extensibility/Implementation/Tracing/AspNetCoreEventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public void NotActiveListenerNoTracking(string evntName, string activityId, stri
[Event(
14,
Keywords = Keywords.Diagnostics,
Message = "An error has occured which may prevent application insights from functioning. Error message: '{0}' ",
Message = "An error has occurred which may prevent application insights from functioning. Error message: '{0}' ",
Level = EventLevel.Error)]
public void LogError(string errorMessage, string appDomainName = "Incorrect")
{
Expand All @@ -133,7 +133,7 @@ public void LogError(string errorMessage, string appDomainName = "Incorrect")
[Event(
15,
Keywords = Keywords.Diagnostics,
Message = "An error has occured while initializing RequestTrackingModule. Requests will not be auto collected. Error message: '{0}' ",
Message = "An error has occurred while initializing RequestTrackingModule. Requests will not be auto collected. Error message: '{0}' ",
Level = EventLevel.Error)]
public void RequestTrackingModuleInitializationFailed(string errorMessage, string appDomainName = "Incorrect")
{
Expand All @@ -146,7 +146,7 @@ public void RequestTrackingModuleInitializationFailed(string errorMessage, strin
[Event(
16,
Keywords = Keywords.Diagnostics,
Message = "An error has occured in DiagnosticSource listener. Callback: '{0}'. Error message: '{1}' ",
Message = "An error has occurred in DiagnosticSource listener. Callback: '{0}'. Error message: '{1}' ",
Level = EventLevel.Warning)]
public void DiagnosticListenerWarning(string callback, string errorMessage, string appDomainName = "Incorrect")
{
Expand All @@ -159,7 +159,7 @@ public void DiagnosticListenerWarning(string callback, string errorMessage, stri
[Event(
17,
Keywords = Keywords.Diagnostics,
Message = "An error has occured while setting up TelemetryConfiguration. Error message: '{0}' ",
Message = "An error has occurred while setting up TelemetryConfiguration. Error message: '{0}' ",
Level = EventLevel.Error)]
public void TelemetryConfigurationSetupFailure(string errorMessage, string appDomainName = "Incorrect")
{
Expand Down Expand Up @@ -244,7 +244,7 @@ public void LogInformational(string message, string appDomainName = "Incorrect")
/// </summary>
/// <param name="exception">Exception message.</param>
/// <param name="appDomainName">An ignored placeholder to make EventSource happy.</param>
[Event(24, Message = "An error has occured in AzureAppServiceRoleNameFromHostNameHeaderInitializer. Exception: '{0}'", Level = EventLevel.Warning, Keywords = Keywords.Diagnostics)]
[Event(24, Message = "An error has occurred in AzureAppServiceRoleNameFromHostNameHeaderInitializer. Exception: '{0}'", Level = EventLevel.Warning, Keywords = Keywords.Diagnostics)]
public void LogAzureAppServiceRoleNameFromHostNameHeaderInitializerWarning(string exception, string appDomainName = "Incorrect")
{
this.WriteEvent(24, exception, this.applicationNameProvider.Name);
Expand Down
66 changes: 66 additions & 0 deletions NETCORE/src/Microsoft.ApplicationInsights.AspNetCore/Implementation/RoleNameContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
namespace Microsoft.ApplicationInsights.AspNetCore.Implementation
{
using System;
using System.Threading;

using Microsoft.AspNetCore.Http;

internal static class RoleNameContainer
{
private const string WebAppHostNameHeaderName = "WAS-DEFAULT-HOSTNAME";
private const string WebAppHostNameEnvironmentVariable = "WEBSITE_HOSTNAME";

private static string roleName = string.Empty;

public static string RoleName
{
get => roleName;

set
{
if (value != roleName)
Copy link
Member

Choose a reason for hiding this comment

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

why not using Interlocked.CompareExchange for comparison and exchange?

Copy link
Member Author

Choose a reason for hiding this comment

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

Simple answer is, i didn't know the correct way to use CompareExchange.
Now that I have better test coverage, i'll take another look at this. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

If you know something I don't, please share. :)

From what I'm reading, CompareExchange will always replace the ref variable with an incoming value.
In my use case, I only need to replace the ref variable when the incoming value does not match.
Specifically, when a VIP SWAP occurs, we need to update the role name.
My understanding is, replacing this with a CompareExchange will always replace the ref variable with the incoming value regardless of if it's new.

I believe CompareExchange doesn't help me in this use case.
But please correct me if i'm wrong.

{
Interlocked.Exchange(ref roleName, value);
}
}
}

/// <summary>
/// Gets or sets suffix of website name. This must be changed when running in non public Azure region.
/// Default value (Public Cloud): ".azurewebsites.net"
/// For US Gov Cloud: ".azurewebsites.us"
/// For Azure Germany: ".azurewebsites.de".
/// </summary>
public static string HostNameSuffix { get; set; } = ".azurewebsites.net";

public static void SetFromEnvironmentVariable(out bool isAzureWebApp)
{
var value = Environment.GetEnvironmentVariable(WebAppHostNameEnvironmentVariable);
ParseAndSetRoleName(value);

isAzureWebApp = !string.IsNullOrEmpty(value);
}

public static void Set(IHeaderDictionary requestHeaders)
{
string headerValue = requestHeaders[WebAppHostNameHeaderName];
ParseAndSetRoleName(headerValue);
}

private static void ParseAndSetRoleName(string input)
{
if (string.IsNullOrEmpty(input))
{
// do nothing
}
else if (input.EndsWith(HostNameSuffix, StringComparison.OrdinalIgnoreCase))
{
RoleName = input.Substring(0, input.Length - HostNameSuffix.Length);
}
else
{
RoleName = input;
}
}
}
}
111 changes: 18 additions & 93 deletions ....AspNetCore/TelemetryInitializers/AzureAppServiceRoleNameFromHostNameHeaderInitializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
namespace Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers
{
using System;

using Microsoft.ApplicationInsights.AspNetCore.Extensibility.Implementation.Tracing;
using Microsoft.ApplicationInsights.AspNetCore.Implementation;
using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.DataContracts;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.ApplicationInsights.Extensibility.Implementation.Tracing;
using Microsoft.AspNetCore.Http;

/// <summary>
/// A telemetry initializer that will gather Azure Web App Role Environment context information to
Expand All @@ -23,42 +23,19 @@
/// </remarks>
public class AzureAppServiceRoleNameFromHostNameHeaderInitializer : ITelemetryInitializer
{
private const string WebAppHostNameHeaderName = "WAS-DEFAULT-HOSTNAME";
private const string WebAppHostNameEnvironmentVariable = "WEBSITE_HOSTNAME";
private readonly IHttpContextAccessor httpContextAccessor;
private string roleName;
private bool isAzureWebApp;
private readonly bool isAzureWebApp;

/// <summary>
/// Initializes a new instance of the <see cref="AzureAppServiceRoleNameFromHostNameHeaderInitializer" /> class.
/// </summary>
/// <param name="httpContextAccessor">Accessor to provide HttpContext if available.</param>
public AzureAppServiceRoleNameFromHostNameHeaderInitializer(IHttpContextAccessor httpContextAccessor)
: this(httpContextAccessor, ".azurewebsites.net")
{
}

/// <summary>
/// Initializes a new instance of the <see cref="AzureAppServiceRoleNameFromHostNameHeaderInitializer" /> class.
/// </summary>
/// <param name="httpContextAccessor">Accessor to provide HttpContext if available.</param>
/// <param name="webAppSuffix">WebApp name suffix.</param>
public AzureAppServiceRoleNameFromHostNameHeaderInitializer(IHttpContextAccessor httpContextAccessor, string webAppSuffix)
public AzureAppServiceRoleNameFromHostNameHeaderInitializer(string webAppSuffix = ".azurewebsites.net")
{
this.httpContextAccessor = httpContextAccessor ?? throw new ArgumentNullException(nameof(httpContextAccessor));
this.WebAppSuffix = webAppSuffix;

try
{
var result = Environment.GetEnvironmentVariable(WebAppHostNameEnvironmentVariable);
this.isAzureWebApp = !string.IsNullOrEmpty(result);

if (!string.IsNullOrEmpty(result) && result.EndsWith(this.WebAppSuffix, StringComparison.OrdinalIgnoreCase))
{
result = result.Substring(0, result.Length - this.WebAppSuffix.Length);
}

this.roleName = result;
RoleNameContainer.HostNameSuffix = webAppSuffix;
RoleNameContainer.SetFromEnvironmentVariable(out bool isAzureWebApp);
this.isAzureWebApp = isAzureWebApp;
}
catch (Exception ex)
{
Expand All @@ -72,7 +49,16 @@ public AzureAppServiceRoleNameFromHostNameHeaderInitializer(IHttpContextAccessor
/// For US Gov Cloud: ".azurewebsites.us"
/// For Azure Germany: ".azurewebsites.de".
/// </summary>
public string WebAppSuffix { get; set; }
public string WebAppSuffix
{
get => RoleNameContainer.HostNameSuffix;

set
{
RoleNameContainer.HostNameSuffix = value;
RoleNameContainer.SetFromEnvironmentVariable(out _);
TimothyMothra marked this conversation as resolved.
Show resolved Hide resolved
}
}

/// <summary>
/// Populates RoleName from the request telemetry associated with the http context.
Expand Down Expand Up @@ -100,73 +86,12 @@ public void Initialize(ITelemetry telemetry)
return;
}

string roleName = string.Empty;
var context = this.httpContextAccessor.HttpContext;

if (context != null)
{
lock (context)
{
var request = context.Features.Get<RequestTelemetry>();

if (request != null)
{
if (string.IsNullOrEmpty(request.Context.Cloud.RoleName))
{
if (this.TryGetRoleNameFromHeader(context, out roleName))
{
request.Context.Cloud.RoleName = roleName;
}
}
else
{
roleName = request.Context.Cloud.RoleName;
}
}
else
{
if (!this.TryGetRoleNameFromHeader(context, out roleName))
{
roleName = this.roleName;
}
}
}
}

if (string.IsNullOrEmpty(roleName))
{
// Fallback to value from ENV variable.
roleName = this.roleName;
}

telemetry.Context.Cloud.RoleName = roleName;
telemetry.Context.Cloud.RoleName = RoleNameContainer.RoleName;
}
catch (Exception ex)
{
AspNetCoreEventSource.Instance.LogAzureAppServiceRoleNameFromHostNameHeaderInitializerWarning(ex.ToInvariantString());
}
}

private bool TryGetRoleNameFromHeader(HttpContext context, out string roleName)
{
roleName = string.Empty;

if (context.Request?.Headers != null)
{
string headerValue = context.Request.Headers[WebAppHostNameHeaderName];
if (!string.IsNullOrEmpty(headerValue))
{
if (headerValue.EndsWith(this.WebAppSuffix, StringComparison.OrdinalIgnoreCase))
{
headerValue = headerValue.Substring(0, headerValue.Length - this.WebAppSuffix.Length);
}

roleName = headerValue;
return true;
}
}

return false;
}
}
}
31 changes: 28 additions & 3 deletions NETCORE/test/Microsoft.ApplicationInsights.AspNetCore.Tests/HostingDiagnosticListenerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,28 @@
{
using System;
using System.Collections.Concurrent;
using System.ComponentModel;
using System.Diagnostics;
using System.Globalization;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading.Tasks;

using Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners;
using Microsoft.ApplicationInsights.AspNetCore.Implementation;
using Microsoft.ApplicationInsights.AspNetCore.Tests;
using Microsoft.ApplicationInsights.AspNetCore.Tests.Helpers;
using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.Common;
using Microsoft.ApplicationInsights.DataContracts;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.ApplicationInsights.Extensibility.Implementation;
using Microsoft.ApplicationInsights.Extensibility.W3C;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Primitives;

using Xunit;
using Xunit.Abstractions;

using AspNetCoreMajorVersion = Microsoft.ApplicationInsights.AspNetCore.Tests.AspNetCoreMajorVersion;

public class HostingDiagnosticListenerTest : IDisposable
{
private const string HttpRequestScheme = "http";
Expand Down Expand Up @@ -1211,6 +1213,29 @@ public void RequestTelemetryIsNotProactivelySampledOutIfFeatureFlagIfOff(AspNetC
}
}

[Theory]
[Trait("Trait", "RoleName")]
[InlineData(AspNetCoreMajorVersion.One)]
[InlineData(AspNetCoreMajorVersion.Two)]
[InlineData(AspNetCoreMajorVersion.Three)]
public void VerifyRequestsUpdateRoleNameContainer(AspNetCoreMajorVersion aspNetCoreMajorVersion)
{
HttpContext context = CreateContext(HttpRequestScheme, HttpRequestHost, "/Test", method: "POST");

RoleNameContainer.HostNameSuffix = ".azurewebsites.net";

using (var hostingListener = CreateHostingListener(aspNetCoreMajorVersion))
{
context.Request.Headers["WAS-DEFAULT-HOSTNAME"] = "a.b.c.azurewebsites.net";
HandleRequestBegin(hostingListener, context, 0, aspNetCoreMajorVersion);
Assert.Equal("a.b.c", RoleNameContainer.RoleName);

context.Request.Headers["WAS-DEFAULT-HOSTNAME"] = "d.e.f.azurewebsites.net";
HandleRequestBegin(hostingListener, context, 0, aspNetCoreMajorVersion);
Assert.Equal("d.e.f", RoleNameContainer.RoleName);
}
}

private void HandleRequestBegin(HostingDiagnosticListener hostingListener, HttpContext context, long timestamp, AspNetCoreMajorVersion aspNetCoreMajorVersion)
{
if (aspNetCoreMajorVersion == AspNetCoreMajorVersion.Two)
Expand Down
Loading