Enable ILogger by default by cijothomas · Pull Request #853 · microsoft/ApplicationInsights-aspnetcore

Cijo Thomas (cijothomas) · 2019-03-20T20:52:41Z

Fix Issue #854

Take dependency of Microsoft.Extensions.Logging.ApplicationInsights for NETSTANDARD2.0
Enables ApplicationInsightsILogger Provider by default for Warning or above logs.
Removes the default DebugLogger - its irrelevant now, as Ilogger logs are now automatically captured.
Add FunctionalTests to validate ILogger integration.

I ran Unit Tests locally.

For significant contributions please make sure you have completed the following items:

Changes in public surface reviewed
Design discussion issue #
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.
The PR will trigger build, unit tests, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.
If you want to to re-run the build/tests, the easiest way is to simply Close and Re-Open this same PR. (Just click 'close pull request' followed by 'open pull request' buttons at the bottom of the PR)
Please follow [these] (https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/develop/Readme.md) instructions to build and test locally.

…nd enable ILogger Provider by default for Warning or above logs.

…ades

Cijo Thomas (cijothomas) · 2019-03-21T00:32:24Z

Sergey Kanzhelev (@SergeyKanzhelev) Pavel Krymets (@pakrym) Ramjot Singh (@RamjotSingh) Please review when you get a chance! thanks in advance.

Ramjot Singh (RamjotSingh) · 2019-03-21T01:14:54Z

+
+                     // By default, all logs Warning or above is captured.
+                     // AddFilter is additive
+                     loggingBuilder.AddFilter<Microsoft.Extensions.Logging.ApplicationInsights.ApplicationInsightsLoggerProvider>("Default", LogLevel.Warning);


I wonder if we are messing with the ILogger infra here. This can already be done from the config which is read as part of ILogger setup. Hiding this statement this deep in SDK might be non-intuitive.

I'd be interested in David Fowler (@davidfowl)'s opinion here. Ideally, custom middleware authors would wire up all of the services requisite for their expected behavior. If someone uses AddApplicationInsights, for instance, we'd presume all trace, exception - everything, by default - goes to Application Insights, and that any service wire-ups required for that goal to be "easy" or even "invisible" to the developer.

Since users updatng to this new SDK version has not explicitly made a decision to send ILogger logs to AI, i'd like to just sent Warning or above to AI. Without this, we could end up sending too much logs to AI. (negative surprises when billing comes). Since the runtime itself logs using ILogger, we have experienced user complaints about too much AI logs. (#603)

I'd open to all suggestions here. We want to balance between 'easy/auto capture of ILogger logs' and 'too much noise'.

By default, only "Warning" or above LogLevel logs are sent to ApplicationInsightsProvider for all categories. But this can be easily configured by user either in code or using appsettings.json.
I added more description here: #854

…rought by Microsoft.Extensions.Logging.ApplicationInsights

…th position

…oft/ApplicationInsights-aspnetcore into cithomas/iloggerbydefault

Cijo Thomas (cijothomas) · 2019-03-28T19:02:02Z

Sergey Kanzhelev (@SergeyKanzhelev) can you review?

Sergey Kanzhelev (SergeyKanzhelev)

generally looks OK

Sergey Kanzhelev (SergeyKanzhelev) · 2019-03-28T21:19:57Z

    <PackageReference Include="Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel" Version="2.10.0-beta2" />
-    <PackageReference Include="Microsoft.AspNetCore.Hosting" Version="1.0.2" />
-    <PackageReference Include="Microsoft.Extensions.Configuration" Version="1.0.2" />
+    <PackageReference Include="Microsoft.AspNetCore.Hosting" Version="1.0.2" />           


will Microsoft.AspNetCore.Hosting.Abstractions be enough?

Likely yes, but i don't want to make un-related changes to this PR. Will revisit all package references in a separate PR.

Ramjot Singh (RamjotSingh) · 2019-03-28T01:17:41Z

            if (options == null)
            {
-                options = Options.Create(new ApplicationInsightsLoggerOptions());
+                options = Options.Create(new Microsoft.ApplicationInsights.AspNetCore.Logging.ApplicationInsightsLoggerOptions());


This should not be needed. IOptions give a default instance if one does not exist.

I guess you can ignore this too considering this is old code.

Ramjot Singh (RamjotSingh) · 2019-03-28T01:39:52Z

+
+                // NetStandard2.0 has a package reference to Microsoft.Extensions.Logging.ApplicationInsights, and
+                // enables ApplicationInsightsLoggerProvider by default.                
+#if NETSTANDARD2_0


If I install in Net461 project which one gets picked up? NetStandard or NetFX? My sinking suspicision is that it will pick up NetFX. We can choose to live with it but we should know it (and maybe put a comment here).

NetFX will always "win" over netstandard2.0

Ramjot Singh (RamjotSingh) · 2019-03-28T21:37:34Z

-  <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="2.0.0" />
-  <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="2.0.0" />
+  <PackageReference Include="Microsoft.Extensions.Configuration" Version="2.1.0" />
+  <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="2.1.0" />  


This should also be coming from Microsoft.Extensions.Logging.AI so you can skip it here.

Likely yes, but i don't want to make un-related changes to this PR. Will revisit all package references in a separate PR.

…crosoft.Extensions.Configuration.Json

…oft/ApplicationInsights-aspnetcore into cithomas/iloggerbydefault

Take dependency of Microsoft.Extensions.Logging.ApplicationInsights a…

b2eaeec

…nd enable ILogger Provider by default for Warning or above logs.

Cijo Thomas (cijothomas) commented Mar 20, 2019

View reviewed changes

Comment thread ...tionInsights.AspNetCore/Logging/Implementation/ApplicationInsightsLoggerFactoryExtensions.cs Outdated

Cijo Thomas (cijothomas) commented Mar 20, 2019

View reviewed changes

Comment thread src/Microsoft.ApplicationInsights.AspNetCore/Microsoft.ApplicationInsights.AspNetCore.csproj

Cijo Thomas (cijothomas) added 3 commits March 20, 2019 14:49

conditional package reference to avoid conflicting version and downgr…

a68dce4

…ades

test project version fix

723266a

tryingto fix version downgrades

7707193

Cijo Thomas (cijothomas) closed this Mar 20, 2019

Cijo Thomas (cijothomas) reopened this Mar 20, 2019

added a unit test

0d92bc5

Cijo Thomas (cijothomas) added 2 commits March 20, 2019 17:44

default is warning or above

6bceda4

modify readme to show build status

858d115

Ramjot Singh (RamjotSingh) suggested changes Mar 21, 2019

View reviewed changes

Pavel Krymets (pakrym) reviewed Mar 21, 2019

View reviewed changes

Comment thread ...t.ApplicationInsights.AspNetCore/Logging/Implementation/ApplicationInsightsLoggerProvider.cs

Pavel Krymets (pakrym) reviewed Mar 21, 2019

View reviewed changes

Comment thread src/Microsoft.ApplicationInsights.AspNetCore/Microsoft.ApplicationInsights.AspNetCore.csproj Outdated

Cijo Thomas (cijothomas) added 5 commits March 21, 2019 11:17

remove reference to Microsoft.Extensions.Logging.Abstrations as its b…

db171c0

…rought by Microsoft.Extensions.Logging.ApplicationInsights

remove attempt to prevent duplication. ensure filter ruleis addedto 0…

79a99d4

…th position

Merge branch 'cithomas/iloggerbydefault' of https://github.com/Micros…

54670ef

…oft/ApplicationInsights-aspnetcore into cithomas/iloggerbydefault

modify obsolete message.

e0804a9

Conflict resolve

2515b78

Pavel Krymets (pakrym) reviewed Mar 28, 2019

View reviewed changes

Comment thread src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsExtensions.cs Outdated

Added FuncTests for Ilogger integration

9861437

Sergey Kanzhelev (SergeyKanzhelev) approved these changes Mar 28, 2019

View reviewed changes

Add changelog description

c6de48b

Ramjot Singh (RamjotSingh) approved these changes Mar 28, 2019

View reviewed changes

Cijo Thomas (cijothomas) changed the title ~~{WIP }Take dependency of Microsoft.Extensions.Logging.ApplicationInsights~~ Enable ILogger by default Mar 29, 2019

Cijo Thomas (cijothomas) added 3 commits March 29, 2019 15:53

remove Microsoft.Extensions.Configuration ref as its is brought by Mi…

9dab9de

…crosoft.Extensions.Configuration.Json

Merge branch 'cithomas/iloggerbydefault' of https://github.com/Micros…

8c2877e

…oft/ApplicationInsights-aspnetcore into cithomas/iloggerbydefault

Merge branch 'develop' into cithomas/iloggerbydefault

a7ff908

Cijo Thomas (cijothomas) merged commit 04ec946 into develop Mar 29, 2019

Cijo Thomas (cijothomas) deleted the cithomas/iloggerbydefault branch March 29, 2019 23:35

Cijo Thomas (cijothomas) mentioned this pull request May 31, 2019

VS Debugger/Azure Web Apps - application insights logger LogLevel is not set correctly #603

Closed

Cijo Thomas (cijothomas) mentioned this pull request Nov 15, 2019

Enable ILogger provider for apps targetting .NET Framework microsoft/ApplicationInsights-dotnet#1312

Closed

Conversation

Cijo Thomas (cijothomas) commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cijo Thomas (cijothomas) commented Mar 21, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cijo Thomas (cijothomas) commented Mar 28, 2019

Uh oh!

Sergey Kanzhelev (SergeyKanzhelev) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Cijo Thomas (cijothomas) commented Mar 20, 2019 •

edited

Loading