Make UseAI and AddAI calls idempotent #419
Conversation
|
This is required for the Azure light-up scenarios to work correctly and thus is needed in 2.1.0 |
|
|
||
| private static bool IsApplicationInsightsAdded(IServiceCollection services) | ||
| { | ||
| // We treat ApplicationInsightsInitializer as a marker that AI services were addede to service collection |
| using System.Collections.Generic; | ||
|
|
||
| /// <summary> | ||
| /// Class to control default debug logger and disable it if logger was added to <see cref="ILoggerFactory"/> explicetely. |
There was a problem hiding this comment.
Also, the description no longer makes sense for this class.
| lock (callbacks) | ||
| { | ||
| foreach (var callback in callbacks) | ||
| { |
There was a problem hiding this comment.
Should this really be in the same place where you register new loggers?
Shouldn't there be two methods: e.g. AddLoggerCallback and NotifyLoggers?
It looks like we've just re-invented a MulticastDelegate. Perhaps an event would be better?
[Edit]
OK, I see how this works now, but it's quirky. I think the doc-comments deserve a little explanation. The registered callback is called only when subsequent loggers are added.
| /// <param name="factory"></param> | ||
| /// <param name="filter"></param> | ||
| /// <param name="serviceProvider">The instance of <see cref="IServiceProvider"/> to use for service resolution.</param> | ||
| /// <param name="loggerAddedCallback">The callback that get's executed when another ApplicationInsights logger is added.</param> |
There was a problem hiding this comment.
typo: "gets" should not have an apostrophe.
| private static bool IsApplicationInsightsAdded(IServiceCollection services) | ||
| { | ||
| // We treat ApplicationInsightsInitializer as a marker that AI services were addede to service collection | ||
| return services.Any(service => service.ServiceType == typeof(ApplicationInsightsInitializer)); |
There was a problem hiding this comment.
return services.OfType<ApplicationInsightsInitializer>().Any();There was a problem hiding this comment.
It's not what it does.
| @@ -50,11 +50,27 @@ public static ILoggerFactory AddApplicationInsights(this ILoggerFactory factory, | |||
| IServiceProvider serviceProvider, | |||
| Func<string, LogLevel, bool> filter) | |||
| { | |||
| return factory.AddApplicationInsights(serviceProvider, filter, () => { }); | |||
There was a problem hiding this comment.
Can't you use 'null' instead of creating an empty delegate? Then it can be a default parameter. Handle null in AddLoggerCallback with callback?.Invoke()
|
|
||
| public ApplicationInsightsLoggerCallbacks() | ||
| { | ||
| callbacks = new List<Action>(); |
There was a problem hiding this comment.
Could be inline with the field definition, then the constructor evaporates.
|
It looks like this fix will make UseAI and AddAI idempotent in the case on single-threaded execution. |
|
@Dmitry-Matveev we are not concerned with multi threaded scenario here, configuration part of application is inherently one threaded. |
| services.AddSingleton<ITelemetryModule, DependencyTrackingTelemetryModule>(); | ||
| if (!IsApplicationInsightsAdded(services)) | ||
| { | ||
| services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>(); |
There was a problem hiding this comment.
Does the error from this method should be traced?
To avoid duplicated telemetries if
AddApplicationInsightsTelemetrywas called whileUseAddApplicationInsightsis inProgram.Main/cc @SergeyKanzhelev @Dmitry-Matveev @dnduffy