Fix couple 2.0-beta issues #317
Conversation
…ebug logging if instrumentation key exists, Disable debug logger if other loggers are being added to ILoggerFactory
|
Also fixes null reference exception while determining application version if ran from unit tests: #310 |
|
@pakrym pakrym @DamianEdwards for #314 does this mean that if you have an ikey you don't get any output in the console when debugging? this would mean that all of the VS tooling for local search/etc would stop working once you've set an ikey. or am i interpreting this wrong and that output would still go out? or does this only turn off debug Traces when configured, normal debug output still occurs? |
| { | ||
| this.telemetryConfiguration = telemetryConfiguration; | ||
| this.httpContextAccessor = httpContextAccessor; | ||
| this.enableAuthSnippet = serviceOptions.Value.EnableAuthenticationTrackingJavaScript; |
There was a problem hiding this comment.
No, I don't think so.
There was a problem hiding this comment.
The IOptions infrastructure will ensure a value is created.
| @@ -16,7 +16,8 @@ public ApplicationInsightsServiceOptions() | |||
| this.EnableQuickPulseMetricStream = true; | |||
| this.EnableAdaptiveSampling = true; | |||
| this.EnableDebugLogger = true; | |||
| this.ApplicationVersion = Assembly.GetEntryAssembly().GetName().Version.ToString(); | |||
| this.EnableAuthenticationTrackingJavaScript = false; | |||
| this.ApplicationVersion = Assembly.GetEntryAssembly()?.GetName().Version.ToString(); | |||
There was a problem hiding this comment.
Just curious but when can GetEntryAssembly return null?
There was a problem hiding this comment.
When running in xunit test.
| @@ -55,5 +56,11 @@ public ApplicationInsightsServiceOptions() | |||
| /// Gets or sets a value indicating whether a logger would be registered automatically in debug mode. | |||
| /// </summary> | |||
| public bool EnableDebugLogger { get; set; } | |||
|
|
|||
| /// <summary> | |||
| /// Gets or sets a value indicating whether a JavaScript to track current authenticated user would be printed along with main | |||
There was a problem hiding this comment.
nitpick grammar: "Gets or sets a value indicating whether a JavaScript snippet to track the current authenticated user should be printed along with the main ApplicationInsights tracking script."
|
@gardnerjr we would only disable automatic debug time logger in that case so you don't accidentally write tons of traces directly to Azure. All other integration features would work as expected. |
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds an ApplicationInsights logger that is enabled for <see cref="LogLevel"/>s of minLevel or higher. | ||
| /// </summary> | ||
| /// <param name="factory"></param> | ||
| /// <param name="client">The instance of <see cref="TelemetryClient"/> to use for logging.</param> | ||
| /// <param name="serviceProvider">The instance of <see cref="IServiceProvider"/> to use for logging.</param> |
There was a problem hiding this comment.
The param comment here and below should be updated to reflect the new type being passed since the service provider isn't being used for logging.
| var debugLoggerControl = serviceProvider.GetService<DebugLoggerControl>(); | ||
| if (debugLoggerControl != null) | ||
| { | ||
| debugLoggerControl.EnableDebugLogger = false; |
There was a problem hiding this comment.
Just curious so I understand this, when DebugLoggerControl is created the flag is defaulted to true but then here when adding App Insights it is set to false, can you explain the flow for enabling and disabling the debug logger?
There was a problem hiding this comment.
Idea here is to enable debug time logger only when there is no other logger added explicitly. So when adding debug logger we include debugLoggerControl.EnableDebugLogger into it's filter statement https://github.com/Microsoft/ApplicationInsights-aspnetcore/pull/317/files#diff-ba05ccca641cf8f4dbb9accc9dc93780R37 and when you call loggetFactory.AddApplicationInsights EnableDebugLogger get's set to false and debug logger disabled.
| _environment = environment; | ||
| } | ||
|
|
||
| public void Initialize(ITelemetry telemetry) |
| /// </summary> | ||
| public AspNetCoreEnvironmentTelemetryInitializer(IHostingEnvironment environment) | ||
| { | ||
| _environment = environment; |
There was a problem hiding this comment.
nitpick: prefer StyleCop compliance "this.environment" vs. "_environment".
| }; | ||
| } | ||
|
|
||
| private class IdentityStub : IIdentity |
There was a problem hiding this comment.
Please put doc comments on new code.
|
Done |
Add AspNetCore environment initializer. Fixes: #312
Authenticated user tracking script support. Fixes: #311
Do not output client javascript if telemetry is disabled. Fixes: #313
Prevent debug logging if instrumentation key exists. Fixes: #314
Disable debug logger if other loggers are being added to ILoggerFactory. Fixes: #315
@SergeyKanzhelev @dnduffy