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

remove correlationIdLookupHelper. #636

Merged
merged 18 commits into from
Apr 12, 2018
Merged

remove correlationIdLookupHelper. #636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5a4bbfb
remove correlationIdLookupHelper.
TimothyMothra Apr 10, 2018
411e0a4
Merge branch 'develop' into tilee/remove_correlationidlookuphelper
cijothomas Apr 11, 2018
1c2fe70
change to allow nullable IApplicationIdProvider
TimothyMothra Apr 12, 2018
05c0b27
change to the configuration
TimothyMothra Apr 12, 2018
bb4620c
merge branch 'develop'
TimothyMothra Apr 12, 2018
09c1283
Merge branch 'develop' into tilee/remove_correlationidlookuphelper
TimothyMothra Apr 12, 2018
6084c4f
update TelemetryConfig setup
TimothyMothra Apr 12, 2018
9533849
changing to TelemetryConfiguration injection
TimothyMothra Apr 12, 2018
3c218f2
Merge branch 'develop' into tilee/remove_correlationidlookuphelper
TimothyMothra Apr 12, 2018
fd1cdc8
added tests
TimothyMothra Apr 12, 2018
6244f9c
nullable ApplicationIdProvider.
TimothyMothra Apr 12, 2018
6e2c1c4
merge local
TimothyMothra Apr 12, 2018
b3ad343
Revert "added tests"
TimothyMothra Apr 12, 2018
dc4edfc
revert
TimothyMothra Apr 12, 2018
92f8c14
include missing using
TimothyMothra Apr 12, 2018
e1ff218
Merge branch 'tilee/remove_correlationidlookuphelper' of https://gith…
TimothyMothra Apr 12, 2018
3eaab02
Merge branch 'develop' into tilee/remove_correlationidlookuphelper
TimothyMothra Apr 12, 2018
554705d
update changelog
TimothyMothra Apr 12, 2018
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
375 changes: 0 additions & 375 deletions ...cationInsights.AspNetCore/DiagnosticListeners/Implementation/CorrelationIdLookupHelper.cs
View file
Open in desktop

This file was deleted.

19 changes: 9 additions & 10 deletions ...cationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.ApplicationInsights.AspNetCore.Common;
using Microsoft.ApplicationInsights.AspNetCore.Extensions;
using Microsoft.ApplicationInsights.DataContracts;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.ApplicationInsights.Extensibility.Implementation;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
Expand All @@ -28,20 +29,19 @@ internal class HostingDiagnosticListener : IApplicationInsightDiagnosticListener
public static bool IsAspNetCore20 = typeof(WebHostBuilder).GetTypeInfo().Assembly.GetName().Version.Major >= 2;

private readonly TelemetryClient client;
private readonly ICorrelationIdLookupHelper correlationIdLookupHelper;
private readonly string sdkVersion;
private readonly IApplicationIdProvider applicationIdProvider;
private readonly string sdkVersion = SdkVersionUtils.GetVersion();
private const string ActivityCreatedByHostingDiagnosticListener = "ActivityCreatedByHostingDiagnosticListener";

/// <summary>
/// Initializes a new instance of the <see cref="T:HostingDiagnosticListener"/> class.
/// </summary>
/// <param name="client"><see cref="TelemetryClient"/> to post traces to.</param>
/// <param name="correlationIdLookupHelper">A store for correlation ids that we don't have to query it everytime.</param>
public HostingDiagnosticListener(TelemetryClient client, ICorrelationIdLookupHelper correlationIdLookupHelper)
/// <param name="applicationIdProvider">Provider for resolving application Id to be used by Correlation.</param>
public HostingDiagnosticListener(TelemetryClient client, IApplicationIdProvider applicationIdProvider)
Copy link
Contributor

Choose a reason for hiding this comment

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

Inject TelemetryConfiguration here, and obtain appid provider from the Configuration, so that even if there is no AppId provider(occurs if user removes it from DI), this wouldn't result in exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cijothomas Are you sure we want to do that?
My thought is that only the NetFramework Modules will be expecting a TelemetryConfiguration.
All of your NetCore Modules are using DependencyInjection, and it would make sense to keep that consistent.

As an alternative; I can make this parameter null by default or make a second ctor without the IApplicationIdProvider parameter, to support DependencyInjection.

I assume consistency would be prefered, but I will defer to your judgement here :)

If I make changes here, would you like me to make the same changes in OperationCorrelationTelemetryInitializer?

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it’s about somebody forgot to add it, not somebody removed it. In some custom scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SergeyKanzhelev, But the TelemetryConfiguration is also being constructed by DependencyInjection.
Maybe I'm not understanding a user custom config scenario. :)

Would something like this be an appropriate alternative...

public HostingDiagnosticListener(TelemetryClient client, IApplicationIdProvider applicationIdProvider, TelemetryConfiguration telemetryConfiguration)
{
     this.applicationIdProvider = applicationIdProvider ?? telemetryConfiguration.ApplicationIdProvider;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@cijothomas explained this to me offline. I'm cleaning this up now.
Thanks

{
this.client = client;
this.correlationIdLookupHelper = correlationIdLookupHelper;
this.sdkVersion = SdkVersionUtils.VersionPrefix + SdkVersionUtils.GetAssemblyVersion();
this.client = client ?? throw new ArgumentNullException(nameof(client));
this.applicationIdProvider = applicationIdProvider ?? throw new ArgumentNullException(nameof(applicationIdProvider));
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets NOT do this check, and if provider is null, don't add correlation headers.

}

/// <inheritdoc/>
Expand Down Expand Up @@ -236,10 +236,9 @@ private void SetAppIdInResponseHeader(HttpContext httpContext, RequestTelemetry
!string.IsNullOrEmpty(requestTelemetry.Context.InstrumentationKey) &&
(!responseHeaders.ContainsKey(RequestResponseHeaders.RequestContextHeader) || HttpHeadersUtilities.ContainsRequestContextKeyValue(responseHeaders, RequestResponseHeaders.RequestContextTargetKey)))
{
string correlationId = null;
if (this.correlationIdLookupHelper.TryGetXComponentCorrelationId(requestTelemetry.Context.InstrumentationKey, out correlationId))
if (this.applicationIdProvider.TryGetApplicationId(requestTelemetry.Context.InstrumentationKey, out string applicationId))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do this only if applicationIdprovider is NOT null.

{
HttpHeadersUtilities.SetRequestContextKeyValue(responseHeaders, RequestResponseHeaders.RequestContextTargetKey, correlationId);
HttpHeadersUtilities.SetRequestContextKeyValue(responseHeaders, RequestResponseHeaders.RequestContextTargetKey, applicationId);
}
}
}
Expand Down
16 changes: 0 additions & 16 deletions ...ationInsights.AspNetCore/DiagnosticListeners/Implementation/ICorrelationIdLookupHelper.cs
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion src/Microsoft.ApplicationInsights.AspNetCore/ExceptionTrackingMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public ExceptionTrackingMiddleware(RequestDelegate next, TelemetryClient client)
{
this.next = next;
this.telemetryClient = client;
this.sdkVersion = SdkVersionUtils.VersionPrefix + SdkVersionUtils.GetAssemblyVersion();
this.sdkVersion = SdkVersionUtils.GetVersion();
}

public async Task Invoke(HttpContext httpContext)
Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ namespace Microsoft.Extensions.DependencyInjection
using Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers;
using Microsoft.ApplicationInsights.DependencyCollector;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.ApplicationInsights.Extensibility.Implementation.ApplicationId;
using Microsoft.ApplicationInsights.WindowsServer;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
Expand Down Expand Up @@ -165,7 +166,7 @@ public static IServiceCollection AddApplicationInsightsTelemetry(this IServiceCo
services.AddSingleton<ITelemetryModule, AzureInstanceMetadataTelemetryModule>();
services.AddSingleton<TelemetryConfiguration>(provider => provider.GetService<IOptions<TelemetryConfiguration>>().Value);

services.AddSingleton<ICorrelationIdLookupHelper>(provider => new CorrelationIdLookupHelper(() => provider.GetService<IOptions<TelemetryConfiguration>>().Value));
services.AddSingleton<IApplicationIdProvider>(provider => new ApplicationInsightsApplicationIdProvider());
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be great to document how one can override this provider with the DictionaryApplicationIdProvider with the set of predefined values and a fallback to the standard one. Ask @cijothomas where all configuration scenarios are listed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use this overload services.AddSingleton<IApplicationIdProvider, ApplicationInsightsApplicationIdProvider>() , so it'll be easy for end user to remove this provider.


services.AddSingleton<TelemetryClient>();

Expand Down
3 changes: 1 addition & 2 deletions ...rosoft.ApplicationInsights.AspNetCore/Logging/Implementation/ApplicationInsightsLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ internal class ApplicationInsightsLogger : ILogger
private readonly TelemetryClient telemetryClient;
private readonly Func<string, LogLevel, bool> filter;
private readonly ApplicationInsightsLoggerOptions options;
private readonly string sdkVersion;
private readonly string sdkVersion = SdkVersionUtils.GetVersion();

/// <summary>
/// Creates a new instance of <see cref="ApplicationInsightsLogger"/>
Expand All @@ -29,7 +29,6 @@ public ApplicationInsightsLogger(string name, TelemetryClient telemetryClient, F
this.telemetryClient = telemetryClient;
this.filter = filter;
this.options = options;
this.sdkVersion = SdkVersionUtils.VersionPrefix + SdkVersionUtils.GetAssemblyVersion();
}

/// <inheritdoc />
Expand Down
12 changes: 5 additions & 7 deletions src/Microsoft.ApplicationInsights.AspNetCore/Microsoft.ApplicationInsights.AspNetCore.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,15 @@
<PackageReference Include="System.Threading.Tasks.Analyzers" Version="1.2.0-beta2">
<PrivateAssets>All</PrivateAssets>
</PackageReference>
<PackageReference Include="Microsoft.ApplicationInsights" Version="2.6.0-beta2" />
<PackageReference Include="Microsoft.ApplicationInsights.DependencyCollector" Version="2.6.0-beta2-build14769" />
<PackageReference Include="Microsoft.ApplicationInsights.PerfCounterCollector" Version="2.6.0-beta2-build14769" />
<PackageReference Include="Microsoft.ApplicationInsights.WindowsServer" Version="2.6.0-beta2-build14769" />
<PackageReference Include="Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel" Version="2.6.0-beta2" />
<PackageReference Include="Microsoft.ApplicationInsights" Version="2.6.0-beta3" />
<PackageReference Include="Microsoft.ApplicationInsights.DependencyCollector" Version="2.6.0-beta3" />
<PackageReference Include="Microsoft.ApplicationInsights.PerfCounterCollector" Version="2.6.0-beta3" />
<PackageReference Include="Microsoft.ApplicationInsights.WindowsServer" Version="2.6.0-beta3" />
<PackageReference Include="Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel" Version="2.6.0-beta3" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'net451' OR '$(TargetFramework)' == 'net46' ">
<PackageReference Include="System.Net.Http" Version="4.3.1" />
<PackageReference Include="Microsoft.ApplicationInsights.PerfCounterCollector" Version="2.6.0-beta2-build14769" />
<PackageReference Include="Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel" Version="2.6.0-beta2" />
<Reference Include="System" />
<Reference Include="Microsoft.CSharp" />
</ItemGroup>
Expand Down
10 changes: 9 additions & 1 deletion src/Microsoft.ApplicationInsights.AspNetCore/SdkVersionUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,15 @@ internal class SdkVersionUtils
public const string VersionPrefix = "aspnet5c:";
#endif

internal static string GetAssemblyVersion()
/// <summary>
/// Get the Assembly Version with SDK prefix.
/// </summary>
internal static string GetVersion()
{
return VersionPrefix + GetAssemblyVersion();
}

private static string GetAssemblyVersion()
{
return typeof(SdkVersionUtils).GetTypeInfo().Assembly.GetCustomAttributes<AssemblyInformationalVersionAttribute>()
.First()
Expand Down
28 changes: 8 additions & 20 deletions ...tionInsights.AspNetCore/TelemetryInitializers/OperationCorrelationTelemetryInitializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,28 @@
namespace Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers
{
using System;
using System.Net.Http.Headers;
using Microsoft.ApplicationInsights.AspNetCore.Common;
using Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners;
using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.DataContracts;
using Microsoft.ApplicationInsights.Extensibility.Implementation;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Primitives;

/// <summary>
/// A telemetry initializer that will set the correlation context for all telemetry items in web application.
/// </summary>
internal class OperationCorrelationTelemetryInitializer : TelemetryInitializerBase
{
private ICorrelationIdLookupHelper correlationIdLookupHelper = null;
private readonly IApplicationIdProvider applicationIdProvider;

/// <summary>
/// Initializes a new instance of the <see cref="OperationCorrelationTelemetryInitializer"/> class.
/// </summary>
/// <param name="httpContextAccessor">Accessor for retrieving the current HTTP context.</param>
/// <param name="correlationIdLookupHelper">A store for correlation ids that we don't have to query it everytime.</param>
public OperationCorrelationTelemetryInitializer(
IHttpContextAccessor httpContextAccessor, ICorrelationIdLookupHelper correlationIdLookupHelper) : base(httpContextAccessor)
/// <param name="applicationIdProvider">Provider for resolving application Id to be used by Correlation.</param>
public OperationCorrelationTelemetryInitializer(IHttpContextAccessor httpContextAccessor, IApplicationIdProvider applicationIdProvider) : base(httpContextAccessor)
{
if (correlationIdLookupHelper == null)
{
throw new ArgumentNullException(nameof(correlationIdLookupHelper));
}
this.correlationIdLookupHelper = correlationIdLookupHelper;
this.applicationIdProvider = applicationIdProvider ?? throw new ArgumentNullException(nameof(applicationIdProvider));
Copy link
Contributor

Choose a reason for hiding this comment

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

can we avoid this exception logic here and instead check if appidprovider is null in Initialize?

}

/// <summary>
Expand All @@ -38,17 +31,13 @@ public OperationCorrelationTelemetryInitializer(
/// <param name="platformContext">Http context</param>
/// <param name="requestTelemetry">Request telemetry object associated with the current request.</param>
/// <param name="telemetry">Telemetry item to initialize.</param>
protected override void OnInitializeTelemetry(
HttpContext platformContext,
RequestTelemetry requestTelemetry,
ITelemetry telemetry)
protected override void OnInitializeTelemetry(HttpContext platformContext, RequestTelemetry requestTelemetry, ITelemetry telemetry)
{
HttpRequest currentRequest = platformContext.Request;
if (currentRequest?.Headers != null && string.IsNullOrEmpty(requestTelemetry.Source))
{
string headerCorrelationId = HttpHeadersUtilities.GetRequestContextKeyValue(currentRequest.Headers, RequestResponseHeaders.RequestContextSourceKey);
string headerCorrelationId = HttpHeadersUtilities.GetRequestContextKeyValue(currentRequest.Headers, RequestResponseHeaders.RequestContextSourceKey);

string appCorrelationId = null;
// If the source header is present on the incoming request, and it is an external component (not the same ikey as the one used by the current component), populate the source field.
if (!string.IsNullOrEmpty(headerCorrelationId))
{
Expand All @@ -57,8 +46,7 @@ protected override void OnInitializeTelemetry(
{
requestTelemetry.Source = headerCorrelationId;
}
else if (this.correlationIdLookupHelper.TryGetXComponentCorrelationId(requestTelemetry.Context.InstrumentationKey, out appCorrelationId) &&
appCorrelationId != headerCorrelationId)
else if (this.applicationIdProvider.TryGetApplicationId(requestTelemetry.Context.InstrumentationKey, out string applicationId) && applicationId != headerCorrelationId)
Copy link
Contributor

Choose a reason for hiding this comment

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

only if provider is not null

{
requestTelemetry.Source = headerCorrelationId;
}
Expand Down
108 changes: 0 additions & 108 deletions test/Microsoft.ApplicationInsights.AspNetCore.Tests/CorrelationIdLookupHelperTest.cs
View file
Open in desktop

This file was deleted.

6 changes: 2 additions & 4 deletions test/Microsoft.ApplicationInsights.AspNetCore.Tests/ExceptionTrackingMiddlewareTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ public class ExceptionTrackingMiddlewareTest
[Fact]
public void InvokeTracksExceptionThrownByNextMiddlewareAsHandledByPlatform()
{
var middleware = new HostingDiagnosticListener(CommonMocks.MockTelemetryClient(telemetry => this.sentTelemetry = telemetry),
CommonMocks.MockCorrelationIdLookupHelper());
var middleware = new HostingDiagnosticListener(CommonMocks.MockTelemetryClient(telemetry => this.sentTelemetry = telemetry), CommonMocks.GetMockApplicationIdProvider());

middleware.OnHostingException(null, null);

Expand All @@ -28,8 +27,7 @@ public void InvokeTracksExceptionThrownByNextMiddlewareAsHandledByPlatform()
[Fact]
public void SdkVersionIsPopulatedByMiddleware()
{
var middleware = new HostingDiagnosticListener(CommonMocks.MockTelemetryClient(telemetry => this.sentTelemetry = telemetry),
CommonMocks.MockCorrelationIdLookupHelper());
var middleware = new HostingDiagnosticListener(CommonMocks.MockTelemetryClient(telemetry => this.sentTelemetry = telemetry), CommonMocks.GetMockApplicationIdProvider());

middleware.OnHostingException(null, null);

Expand Down
Loading