Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

ILogger implementation for ApplicationInsights #239

Merged
merged 21 commits into from
Dec 31, 2018
Merged

ILogger implementation for ApplicationInsights #239

merged 21 commits into from
Dec 31, 2018

Conversation

RamjotSingh
Copy link
Contributor

@RamjotSingh RamjotSingh commented Dec 7, 2018

Original discussion at - microsoft/ApplicationInsights-aspnetcore#749

C# Console program sample

Packages installed

<ItemGroup>
    <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="2.1.0" />
    <PackageReference Include="Microsoft.Extensions.Logging" Version="2.1.0" />
    <PackageReference Include="Microsoft.Extensions.Logging.ApplicationInsights" Version="2.8.1-build39946" />
    <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="2.1.0" />
  </ItemGroup>
class Program
    {
        static void Main(string[] args)
        {
            // Create DI container.
            IServiceCollection services = new ServiceCollection();
            
            // Add the logging pipelines to use. We are using Console and AI and configuring them both.
            services.AddLogging(loggingBuilder =>
            {
                loggingBuilder.AddApplicationInsights("--YourAIKeyHere--");

                loggingBuilder.AddConsole();
            });

            // Build ServiceProvider.
            IServiceProvider serviceProvider = services.BuildServiceProvider();

            ILogger<Program> logger = serviceProvider.GetRequiredService<ILogger<Program>>();

            // Begin a new scope. This is optional. Epecially in case of AspNetCore request info is already
            // present in scope.
            using (logger.BeginScope(new Dictionary<string, object> { { "Method", nameof(Main) } }))
            {
                logger.LogInformation("Logger is working");
            }
        }
    }

@RamjotSingh
Copy link
Contributor Author

Trying to collect all the issues which are logged across various repos. Please feel free to reference other issues here.

microsoft/ApplicationInsights-aspnetcore#491
microsoft/ApplicationInsights-aspnetcore#749
microsoft/ApplicationInsights-aspnetcore#783

@lmolkova
Copy link
Member

lmolkova commented Dec 7, 2018

@RamjotSingh we have an ongoing conversation with Asp.Net Core team on the high-level design of this feature . There are many things to consider: scopes support and meaning in AppInsights, sampling, common ids.

We also have ILogger implementation in AspNEtCore repo: https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/ed794ae3a0150715d76432c7417aa93c334126af/src/Microsoft.ApplicationInsights.AspNetCore/Logging/Implementation/ApplicationInsightsLogger.cs

So could we hold this on until we finalize the design?

cc @SergeyKanzhelev

@RamjotSingh
Copy link
Contributor Author

@lmolkova Oh ok that's cool. I thought that this was sitting on cold plate so I ended up picking it up. The ILogger implementation I have done is actually based on what @SergeyKanzhelev wrote in his private branch (which was in turn based on AI-ASPNetCore repo). If you feel this is getting picked up I can close the PR for now.

Let me know what you think.

@cijothomas
Copy link
Contributor

@RamjotSingh Thanks for your effort here - we definitely want to take it. We may change implementation later based on discussions with asp.net core team, but moving this to logging is the right thing to do now!
Will review this shortly

@cijothomas
Copy link
Contributor

Once we merge this and ship a new nuget package from this repo (Microsoft.ApplicationInsights.ILoggerProvider or Microsoft.Extensions.Logging.ApplicationInsights), the extension methods on application insights asp.net core sdk can be marked obsolete to be removed in the next major version bump. It'll still contain the duplicate code for ApplicationInsightsLogger etc.

User who want to use ILogger with ApplicationInsights should use this new package, and new extension methods.

src/ILogger/ApplicationInsightsLogger.cs Outdated Show resolved Hide resolved
src/ILogger/ILogger.csproj Show resolved Hide resolved
src/ILogger/ILogger.csproj Outdated Show resolved Hide resolved
src/ILogger/ApplicationInsightsLoggerExtensions.cs Outdated Show resolved Hide resolved
src/ILogger/ApplicationInsightsLogger.cs Outdated Show resolved Hide resolved
Logging.sln Show resolved Hide resolved
src/ILogger/ApplicationInsightsLoggerExtensions.cs Outdated Show resolved Hide resolved
@davidfowl
Copy link

davidfowl commented Dec 18, 2018

I'm hope it would look something like this:

var services = new ServiceCollection();            
services.AddLogging(loggingBuilder =>
{
    loggingBuilder.AddApplicationInsights("--YourAIKeyHere--");
});

var serviceProvider = services.BuildServiceProvider();
var loggerFactory = serviceProvider.GetRequiredService<ILoggerFactory>();
var logger = loggerFactory.CreateLogger("MyCategory");

using (logger.BeginScope("Test"))
{
    logger.LogInformation("Logger is working");
}

in 3.0 it'll look like this:

var loggerFactory = LoggerFactory.Create(builder => 
{
    builder.AddApplicationInsights("--YourAIKeyHere--");
});

var logger = loggerFactory.CreateLogger("MyCategory");

using (logger.BeginScope("Test"))
{
    logger.LogInformation("Logger is working");
}

More advanced options should look like this:

var loggerFactory = LoggerFactory.Create(builder => 
{
    builder.AddApplicationInsights(options => 
    {
        options.InstrumentationKey  = "--YourAIKeyHere--";
    });
});

var logger = loggerFactory.CreateLogger("MyCategory");
using (logger.BeginScope("Test"))
{
    logger.LogInformation("Logger is working");
}

Generic Host (3.0 APIs):

 var host = Host.CreateHostBuilder()
            .ConfigureLogging(logging =>
            {
                logging.AddApplicationInsights("--YourAIKeyHere--");
            })
            .Build();

host.RunAsync();

@pakrym
Copy link

pakrym commented Dec 19, 2018

LGTM

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

left few comments.. Most importantly - i cannot see how a user would setup application insights to log only logs above a given level ? Loglevel seems missing..
Other than that (and few minor comments), looks good!

src/ILogger/ApplicationInsightsLoggerOptions.cs Outdated Show resolved Hide resolved
src/ILogger/ApplicationInsightsLoggingBuilderExtensions.cs Outdated Show resolved Hide resolved
src/ILogger/ApplicationInsightsLoggingBuilderExtensions.cs Outdated Show resolved Hide resolved
src/ILogger/ApplicationInsightsLoggingBuilderExtensions.cs Outdated Show resolved Hide resolved
test/ILogger.NetStandard.Tests/ILoggerIntegrationTests.cs Outdated Show resolved Hide resolved
src/ILogger/ApplicationInsightsLogger.cs Show resolved Hide resolved
src/ILogger/ApplicationInsightsLogger.cs Show resolved Hide resolved
@RamjotSingh
Copy link
Contributor Author

RamjotSingh commented Dec 20, 2018

@cijothomas Here is how someone can configure ApplicationInsights to only log above a specific level.

               loggingBuilder.AddFilter((logLevel) =>
                {
                    if (logLevel > LogLevel.Information)
                    {
                        return true;
                    }
                    else
                    {
                        return false;
                    }
                }).AddApplicationInsights();

I think this is what @pakrym talked about when referring to Filtering.

@davidfowl
Copy link

Yes filtering is built into the factory as a first class citizen.

@cijothomas cijothomas closed this Dec 21, 2018
@cijothomas cijothomas reopened this Dec 21, 2018
@lmolkova
Copy link
Member

lmolkova commented Dec 31, 2018

@RamjotSingh we fixed build issue (#243) and are going to ship it in next beta release. Would you mind re-enabling analyzers nugets as a separate PR?

@lmolkova
Copy link
Member

@RamjotSingh thanks a lot for the contribution! 🥇

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ilogger all issues related to Ilogger provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants