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.
| /// </summary> | ||
| /// <param name="factory"></param> | ||
| /// <param name="filter"></param> | ||
| /// <param name="serviceProvider">The instance of <see cref="IServiceProvider"/> to use for service resolution.</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.
| this ILoggerFactory factory, | ||
| IServiceProvider serviceProvider, | ||
| Func<string, LogLevel, bool> 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. |
SergeyKanzhelev
left a comment
SergeyKanzhelev
left a comment
There was a problem hiding this comment.
thanks! It would be nice if you can also update the summary for the methods Use and Add. Mention that if you need change configuration - you need to do it before instantiating the TelemetryClient.
| 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?
8 participants
To avoid duplicated telemetries if
AddApplicationInsightsTelemetrywas called whileUseAddApplicationInsightsis inProgram.Main/cc @SergeyKanzhelev @Dmitry-Matveev @dnduffy