Skip to content
This repository has been archived by the owner on Jun 10, 2020. It is now read-only.

Fix couple 2.0-beta issues #317

Merged
merged 7 commits into from
Jan 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
namespace Microsoft.ApplicationInsights.AspNetCore
{
using System.Globalization;
using System.Security.Principal;
using Microsoft.ApplicationInsights.AspNetCore.Extensions;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Options;

/// <summary>
/// This class helps to inject Application Insights JavaScript snippet into application code.
Expand All @@ -11,29 +15,54 @@ public class JavaScriptSnippet
/// <summary>JavaScript snippet.</summary>
private static readonly string Snippet = Resources.JavaScriptSnippet;

/// <summary>JavaScript authenticated user tracking snippet.</summary>
private static readonly string AuthSnippet = Resources.JavaScriptAuthSnippet;

/// <summary>Configuration instance.</summary>
private TelemetryConfiguration telemetryConfiguration;

/// <summary> Http context accessor.</summary>
private readonly IHttpContextAccessor httpContextAccessor;

/// <summary> Weather to print authenticated user tracking snippet.</summary>
private bool enableAuthSnippet;

/// <summary>
/// Initializes a new instance of the JavaScriptSnippet class.
/// </summary>
/// <param name="telemetryConfiguration">The configuration instance to use.</param>
public JavaScriptSnippet(TelemetryConfiguration telemetryConfiguration)
/// <param name="serviceOptions">Service options instance to use.</param>
/// <param name="httpContextAccessor">Http context accessor to use.</param>
public JavaScriptSnippet(TelemetryConfiguration telemetryConfiguration,
IOptions<ApplicationInsightsServiceOptions> serviceOptions,
IHttpContextAccessor httpContextAccessor)
{
this.telemetryConfiguration = telemetryConfiguration;
this.httpContextAccessor = httpContextAccessor;
this.enableAuthSnippet = serviceOptions.Value.EnableAuthenticationTrackingJavaScript;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can "Value" ever be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IOptions infrastructure will ensure a value is created.

}

/// <summary>
/// Gets a code snippet with instrumentation key initialized from TelemetryConfiguration.
/// </summary>
/// <returns>JavaScript code snippet with instrumentation key or empty if instrumentation key was not set for the applicaiton.</returns>
/// <returns>JavaScript code snippet with instrumentation key or empty if instrumentation key was not set for the application.</returns>
public string FullScript
{
get
{
if (!string.IsNullOrEmpty(this.telemetryConfiguration.InstrumentationKey))
if (!this.telemetryConfiguration.DisableTelemetry &&
!string.IsNullOrEmpty(this.telemetryConfiguration.InstrumentationKey))
{
return string.Format(CultureInfo.InvariantCulture, Snippet, this.telemetryConfiguration.InstrumentationKey);
string additionalJS = string.Empty;
IIdentity identity = httpContextAccessor?.HttpContext.User?.Identity;
if (enableAuthSnippet &&
identity != null &&
identity.IsAuthenticated)
{
string escapedUserName = identity.Name.Replace("\\", "\\\\");
additionalJS = string.Format(CultureInfo.InvariantCulture, AuthSnippet, escapedUserName);
}
return string.Format(CultureInfo.InvariantCulture, Snippet, this.telemetryConfiguration.InstrumentationKey, additionalJS);
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.ApplicationInsights.AspNetCore;
using Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners;
using Microsoft.ApplicationInsights.AspNetCore.Extensions;
using Microsoft.ApplicationInsights.AspNetCore.Logging;
using Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.AspNetCore.Hosting;
Expand Down Expand Up @@ -132,6 +133,7 @@ public static IServiceCollection AddApplicationInsightsTelemetry(this IServiceCo
services.AddSingleton<ITelemetryInitializer, UserAgentTelemetryInitializer>();
services.AddSingleton<ITelemetryInitializer, WebSessionTelemetryInitializer>();
services.AddSingleton<ITelemetryInitializer, WebUserTelemetryInitializer>();
services.AddSingleton<ITelemetryInitializer, AspNetCoreEnvironmentTelemetryInitializer>();

#if NET451
services.AddSingleton<ITelemetryModule, PerformanceCollectorModule>();
Expand All @@ -151,6 +153,7 @@ public static IServiceCollection AddApplicationInsightsTelemetry(this IServiceCo
services.AddSingleton<IStartupFilter, ApplicationInsightsStartupFilter>();

services.AddSingleton<JavaScriptSnippet>();
services.AddSingleton<DebugLoggerControl>();

services.AddOptions();
services.AddSingleton<IOptions<TelemetryConfiguration>, TelemetryConfigurationOptions>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious but when can GetEntryAssembly return null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running in xunit test.

}

/// <summary>
Expand Down Expand Up @@ -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 snippet to track the current authenticated user should
/// be printed along with the main ApplicationInsights tracking script.
/// </summary>
public bool EnableAuthenticationTrackingJavaScript { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace Microsoft.ApplicationInsights.AspNetCore
using System.Diagnostics;
using Extensions;
using Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners;
using Microsoft.ApplicationInsights.AspNetCore.Logging;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

Expand All @@ -22,16 +23,18 @@ internal class ApplicationInsightsInitializer : IObserver<DiagnosticListener>, I
public ApplicationInsightsInitializer(
IOptions<ApplicationInsightsServiceOptions> options,
IEnumerable<IApplicationInsightDiagnosticListener> diagnosticListeners,
TelemetryClient telemetryClient,
ILoggerFactory loggerFactory)
ILoggerFactory loggerFactory,
DebugLoggerControl debugLoggerControl,
TelemetryClient client)
{
this.diagnosticListeners = diagnosticListeners;
this.subscriptions = new List<IDisposable>();

// Add default logger factory for debug mode
if (options.Value.EnableDebugLogger)
// Add default logger factory for debug mode only if enabled and instrumentation key not set
if (options.Value.EnableDebugLogger && string.IsNullOrEmpty(options.Value.InstrumentationKey))
{
loggerFactory.AddApplicationInsights(telemetryClient, (s, level) => Debugger.IsAttached);
// Do not use extension method here or it will disable debug logger we currently adding
loggerFactory.AddProvider(new ApplicationInsightsLoggerProvider(client, (s, level) => debugLoggerControl.EnableDebugLogger && Debugger.IsAttached));
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using Microsoft.Extensions.Configuration;

namespace Microsoft.Extensions.Logging
{
using System;
using ApplicationInsights;
using ApplicationInsights.AspNetCore.Logging;
using Microsoft.Extensions.DependencyInjection;

/// <summary>
/// Extension methods for <see cref="ILoggerFactory"/> that allow adding Application Insights logger.
Expand All @@ -17,24 +17,25 @@ public static class ApplicationInsightsLoggerFactoryExtensions
/// <summary>
/// Adds an ApplicationInsights logger that is enabled for <see cref="LogLevel.Warning"/> or higher.
/// </summary>
public static ILoggerFactory AddApplicationInsights(this ILoggerFactory factory,
TelemetryClient client)
/// <param name="factory"></param>
/// <param name="serviceProvider">The instance of <see cref="IServiceProvider"/> to use for service resolution.</param>
public static ILoggerFactory AddApplicationInsights(this ILoggerFactory factory, IServiceProvider serviceProvider)
{
return factory.AddApplicationInsights(client, LogLevel.Warning);
return factory.AddApplicationInsights(serviceProvider, LogLevel.Warning);
}

/// <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 service resolution.</param>
/// <param name="minLevel">The minimum <see cref="LogLevel"/> to be logged</param>
public static ILoggerFactory AddApplicationInsights(
this ILoggerFactory factory,
TelemetryClient client,
IServiceProvider serviceProvider,
LogLevel minLevel)
{
factory.AddApplicationInsights(client, (category, logLevel) => logLevel >= minLevel);
factory.AddApplicationInsights(serviceProvider, (category, logLevel) => logLevel >= minLevel);
return factory;
}

Expand All @@ -43,12 +44,19 @@ public static ILoggerFactory AddApplicationInsights(
/// </summary>
/// <param name="factory"></param>
/// <param name="filter"></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 service resolution.</param>
public static ILoggerFactory AddApplicationInsights(
this ILoggerFactory factory,
TelemetryClient client,
IServiceProvider serviceProvider,
Func<string, LogLevel, bool> filter)
{
var client = serviceProvider.GetService<TelemetryClient>();
var debugLoggerControl = serviceProvider.GetService<DebugLoggerControl>();
if (debugLoggerControl != null)
{
debugLoggerControl.EnableDebugLogger = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank-you.

}

factory.AddProvider(new ApplicationInsightsLoggerProvider(client, filter));
return factory;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
namespace Microsoft.ApplicationInsights.AspNetCore.Logging
{
using Microsoft.Extensions.Logging;

/// <summary>
/// Class to control default debug logger and disable it if logger was added to <see cref="ILoggerFactory"/> explicetely.
/// </summary>
internal class DebugLoggerControl
{
public DebugLoggerControl()
{
EnableDebugLogger = true;
}

/// <summary>
/// This property gets set to <code>false</code> by <see cref="ApplicationInsightsLoggerFactoryExtensions.AddApplicationInsights"/>.
/// </summary>
public bool EnableDebugLogger { get; set; }
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/Microsoft.ApplicationInsights.AspNetCore/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="JavaScriptAuthSnippet" xml:space="preserve">
<value>appInsights.setAuthenticatedUserContext("{0}");</value>
</data>
<data name="JavaScriptSnippet" xml:space="preserve">
<value> &lt;script type="text/javascript"&gt;
var appInsights=window.appInsights||function(config){{
Expand All @@ -127,6 +130,7 @@

window.appInsights=appInsights;
appInsights.trackPageView();
{1}
&lt;/script&gt;
</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
namespace Microsoft.ApplicationInsights.AspNetCore.TelemetryInitializers
{
using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.AspNetCore.Hosting;

/// <summary>
/// <see cref="ITelemetryInitializer"/> implementation that stamps ASP.NET Core environment name
/// on telemetries.
/// </summary>
internal class AspNetCoreEnvironmentTelemetryInitializer: ITelemetryInitializer
{
private const string AspNetCoreEnvironmentPropertyName = "AspNetCoreEnvironment";
private readonly IHostingEnvironment environment;

/// <summary>
/// Initializes a new instance of the <see cref="AspNetCoreEnvironmentTelemetryInitializer"/> class.
/// </summary>
public AspNetCoreEnvironmentTelemetryInitializer(IHostingEnvironment environment)
{
this.environment = environment;
}

/// <inheritdoc />
public void Initialize(ITelemetry telemetry)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add doc comments.

{
if (environment != null && !telemetry.Context.Properties.ContainsKey(AspNetCoreEnvironmentPropertyName))
{
telemetry.Context.Properties.Add(AspNetCoreEnvironmentPropertyName, environment.EnvironmentName);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ private static void UpdateRequestTelemetryFromPlatformContext(RequestTelemetry r
var sessionCookieParts = ((string)sessionCookieValue).Split('|');
if (sessionCookieParts.Length > 0)
{
// Currently SessionContext takes in only SessionId.
// Currently SessionContext takes in only SessionId.
// The cookies has SessionAcquisitionDate and SessionRenewDate as well that we are not picking for now.
requestTelemetry.Context.Session.Id = sessionCookieParts[0];
}
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.ApplicationInsights.AspNetCore/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"summary": "Application Insights for ASP.NET Core web applications.",
"description": "Application Insights for ASP.NET Core web applications. See https://azure.microsoft.com/en-us/documentation/articles/app-insights-asp-net-five/ for more information. Privacy statement: https://go.microsoft.com/fwlink/?LinkId=512156",
"authors": [ "Microsoft" ],
"version": "2.0.0-beta1",
"version": "2.0.0-beta2",
"copyright": "Copyright © Microsoft. All Rights Reserved.",
"packOptions": {
"projectUrl": "http://go.microsoft.com/fwlink/?LinkId=392727",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using Microsoft.Extensions.Options;
using Xunit;
using Xunit;

[assembly: CollectionBehavior(DisableTestParallelization = true)]
namespace Microsoft.Extensions.DependencyInjection.Test
Expand All @@ -15,10 +14,13 @@ namespace Microsoft.Extensions.DependencyInjection.Test
using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.ApplicationInsights.AspNetCore.Extensions;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Hosting.Internal;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Internal;
using Microsoft.Extensions.Configuration;
using Xunit;
using Microsoft.Extensions.Options;

#if NET451
using ApplicationInsights.DependencyCollector;
using ApplicationInsights.Extensibility.PerfCounterCollector;
Expand All @@ -37,6 +39,7 @@ public static ServiceCollection GetServiceCollectionWithContextAccessor()
var services = new ServiceCollection();
IHttpContextAccessor contextAccessor = new HttpContextAccessor();
services.AddSingleton<IHttpContextAccessor>(contextAccessor);
services.AddSingleton<IHostingEnvironment>(new HostingEnvironment());
services.AddSingleton<DiagnosticListener>(new DiagnosticListener("TestListener"));
return services;
}
Expand Down
Loading