-
Notifications
You must be signed in to change notification settings - Fork 123
Conversation
use applicationIdprovider. code cleanup
@@ -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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets chat about making the AppliationIdProvider optional?
@@ -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()); |
There was a problem hiding this comment.
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.
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)); |
There was a problem hiding this comment.
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.
@@ -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)) |
There was a problem hiding this comment.
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.
throw new ArgumentNullException(nameof(correlationIdLookupHelper)); | ||
} | ||
this.correlationIdLookupHelper = correlationIdLookupHelper; | ||
this.applicationIdProvider = applicationIdProvider ?? throw new ArgumentNullException(nameof(applicationIdProvider)); |
There was a problem hiding this comment.
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?
@@ -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) |
There was a problem hiding this comment.
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
TODO:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
{ | ||
this.applicationInsightsServiceOptions = applicationInsightsServiceOptions.Value; | ||
this.initializers = initializers; | ||
this.modules = modules; | ||
this.telemetryProcessorFactories = telemetryProcessorFactories; | ||
this.telemetryModuleConfigurators = telemetryModuleConfigurators; | ||
this.telemetryChannel = serviceProvider.GetService<ITelemetryChannel>(); | ||
this.applicationIdProvider = applicationIdProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets do it like done for telemetry channel in the line above, so that if user removes the AppIdProvider from DI, it wont throw.
/// <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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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
bump version.
This reverts commit fd1cdc8.
…ub.com/Microsoft/ApplicationInsights-aspnetcore into tilee/remove_correlationidlookuphelper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add changelog before merge
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.
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.