remove correlationIdLookupHelper. #636

TimothyMothra · 2018-04-10T17:32:29Z

This is a continuation of the work started in BaseSDK: [New] Interface for ApplicationId Lookup (IApplicationIdProvider)

This ultimately fixes issue WebSDK:853 where the Endpoint for CorrelationId could not be customized.

RequestTelemetryModule and DependencyTelemetryModule will now rely on a configuration level ApplicationIdProvider to resolve Ids used for correlation. This provider will be configured with default settings on Nuget install.

I ran Unit Tests locally.

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

Changes in public surface reviewed
Design discussion issue #
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.
The PR will trigger build, unit tests, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.
If you want to to re-run the build/tests, the easiest way is to simply Close and Re-Open this same PR. (Just click 'close pull request' followed by 'open pull request' buttons at the bottom of the PR)
Please follow [these] (https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/develop/Readme.md) instructions to build and test locally.

use applicationIdprovider. code cleanup

SergeyKanzhelev · 2018-04-10T22:35:53Z

src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsExtensions.cs

@@ -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());


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.

cijothomas

Lets chat about making the AppliationIdProvider optional?

cijothomas · 2018-04-11T03:44:15Z

src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsExtensions.cs

@@ -163,7 +164,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());


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

cijothomas · 2018-04-11T03:45:00Z

...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs

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


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

cijothomas · 2018-04-11T03:45:28Z

...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs

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


Do this only if applicationIdprovider is NOT null.

cijothomas · 2018-04-11T03:50:00Z

...icationInsights.AspNetCore/TelemetryInitializers/OperationCorrelationTelemetryInitializer.cs

-                throw new ArgumentNullException(nameof(correlationIdLookupHelper));
-            }
-            this.correlationIdLookupHelper = correlationIdLookupHelper;
+            this.applicationIdProvider = applicationIdProvider ?? throw new ArgumentNullException(nameof(applicationIdProvider));


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

cijothomas · 2018-04-11T03:50:33Z

...icationInsights.AspNetCore/TelemetryInitializers/OperationCorrelationTelemetryInitializer.cs

@@ -57,8 +46,7 @@ internal class OperationCorrelationTelemetryInitializer : TelemetryInitializerBa
                    {
                        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)


only if provider is not null

TimothyMothra · 2018-04-12T17:27:10Z

TODO:

~~Should wait until 2.6.0-beta3 is officially published before merging this PR.~~
~~What is the correct way to customize TelemetryConfiguration defaults?~~
We need a configuration story for DictionaryApplicationIdProvider
Unit tests for null configuration scenario

cijothomas

Add a unittest to ensure that lack of AppId provider does not cause crash.
Add a unit test to ensure that user can simply add a new AppIdImplementation to DI, and Configuration will pick it up.

eg for removebutdontcrash test
https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/fecf25acfa69e86e6aaa649ef50409e10b8ec1be/test/Microsoft.ApplicationInsights.AspNetCore.Tests/Extensions/ApplicationInsightsExtensionsTests.cs#L544

eg for overridable appid provider test
https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/fecf25acfa69e86e6aaa649ef50409e10b8ec1be/test/Microsoft.ApplicationInsights.AspNetCore.Tests/Extensions/ApplicationInsightsExtensionsTests.cs#L531

cijothomas · 2018-04-12T19:26:21Z

...icrosoft.ApplicationInsights.AspNetCore/Implementation/TelemetryConfigurationOptionsSetup.cs

        {
            this.applicationInsightsServiceOptions = applicationInsightsServiceOptions.Value;
            this.initializers = initializers;
            this.modules = modules;
            this.telemetryProcessorFactories = telemetryProcessorFactories;
            this.telemetryModuleConfigurators = telemetryModuleConfigurators;
            this.telemetryChannel = serviceProvider.GetService<ITelemetryChannel>();
+            this.applicationIdProvider = applicationIdProvider;


lets do it like done for telemetry channel in the line above, so that if user removes the AppIdProvider from DI, it wont throw.

cijothomas · 2018-04-12T19:27:29Z

...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs

-        /// <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">Nullable Provider for resolving application Id to be used by Correlation.</param>
+        public HostingDiagnosticListener(TelemetryClient client, IApplicationIdProvider applicationIdProvider)


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.

@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?

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

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

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

bump version.

This reverts commit fd1cdc8.

…ub.com/Microsoft/ApplicationInsights-aspnetcore into tilee/remove_correlationidlookuphelper

cijothomas

Please add changelog before merge

remove correlationIdLookupHelper.

5a4bbfb

use applicationIdprovider. code cleanup

SergeyKanzhelev approved these changes Apr 10, 2018

View reviewed changes

Merge branch 'develop' into tilee/remove_correlationidlookuphelper

411e0a4

cijothomas suggested changes Apr 11, 2018

View reviewed changes

cijothomas closed this Apr 12, 2018

cijothomas reopened this Apr 12, 2018

change to allow nullable IApplicationIdProvider

1c2fe70

TimothyMothra added 3 commits April 12, 2018 10:34

change to the configuration

05c0b27

merge branch 'develop'

bb4620c

Merge branch 'develop' into tilee/remove_correlationidlookuphelper

09c1283

cijothomas suggested changes Apr 12, 2018

View reviewed changes

TimothyMothra added 2 commits April 12, 2018 13:59

update TelemetryConfig setup

6084c4f

changing to TelemetryConfiguration injection

9533849

microsoft deleted a comment from cijothomas Apr 12, 2018

TimothyMothra added 9 commits April 12, 2018 14:52

Merge branch 'develop' into tilee/remove_correlationidlookuphelper

3c218f2

added tests

fd1cdc8

nullable ApplicationIdProvider.

6244f9c

bump version.

merge local

6e2c1c4

Revert "added tests"

b3ad343

This reverts commit fd1cdc8.

revert

dc4edfc

include missing using

92f8c14

Merge branch 'tilee/remove_correlationidlookuphelper' of https://gith…

e1ff218

…ub.com/Microsoft/ApplicationInsights-aspnetcore into tilee/remove_correlationidlookuphelper

Merge branch 'develop' into tilee/remove_correlationidlookuphelper

3eaab02

cijothomas approved these changes Apr 12, 2018

View reviewed changes

update changelog

554705d

cijothomas merged commit 0d3e709 into develop Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove correlationIdLookupHelper. #636

remove correlationIdLookupHelper. #636

TimothyMothra commented Apr 10, 2018

SergeyKanzhelev Apr 10, 2018

cijothomas left a comment

cijothomas Apr 11, 2018

cijothomas Apr 11, 2018

cijothomas Apr 11, 2018

cijothomas Apr 11, 2018

cijothomas Apr 11, 2018

TimothyMothra commented Apr 12, 2018 •

edited

Loading

cijothomas left a comment •

edited

Loading

cijothomas Apr 12, 2018

cijothomas Apr 12, 2018

TimothyMothra Apr 12, 2018

SergeyKanzhelev Apr 12, 2018

TimothyMothra Apr 12, 2018

TimothyMothra Apr 12, 2018

cijothomas left a comment

remove correlationIdLookupHelper. #636

remove correlationIdLookupHelper. #636

Conversation

TimothyMothra commented Apr 10, 2018

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyMothra commented Apr 12, 2018 • edited Loading

cijothomas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

TimothyMothra commented Apr 12, 2018 •

edited

Loading

cijothomas left a comment •

edited

Loading