ILogger implementation that sends data to EventSource/EventListener #424

karolz-ms · 2016-05-10T18:02:12Z

This implementation follows the standard "use extension method to enable" pattern. Nothing is on by default.

A separate test assembly was created because there are two slightly different implementations, one for .NET Core and one for .NET 4.6

dnfclas · 2016-05-10T18:02:16Z

Hi @karolz-ms, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2016-05-10T18:10:11Z

@karolz-ms, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

Eilon · 2016-05-10T21:15:34Z

src/Microsoft.Extensions.Logging.EventSource/project.json

+      },
+      "imports": [ "dotnet" ]
+    },
+    "netcoreapp1.0": {


This should be a netstandard TFM, which is for library. netcoreapp is only for apps. E.g. see this other logger: https://github.com/aspnet/Logging/blob/dev/src/Microsoft.Extensions.Logging.Debug/project.json#L25-L36.

The reason why I went with netcoreapp is that when I am trying to consume Newtonsoft JSON from a project that targets netstandard, I am getting the following error:

Errors in C:\code\aspnet-logging-rc\src\Microsoft.Extensions.Logging.EventSource\project.json Package Newtonsoft.Json 8.0.3 is not compatible with netstandard1.3 (.NETStandard,Version=v1.3). Package Newtonsoft.Json 8.0.3 supports: - net20 (.NETFramework,Version=v2.0) - net35 (.NETFramework,Version=v3.5) - net40 (.NETFramework,Version=v4.0) - net45 (.NETFramework,Version=v4.5) - portable-dnxcore50+net45+win8+wp8+wpa81 (.NETPortable,Version=v0.0,Profile=net45+wp80+win8+wpa81+dnxcore50) - portable-net40+sl5+win8+wp8+wpa81 (.NETPortable,Version=v0.0,Profile=Profile328) One or more packages are incompatible with .NETStandard,Version=v1.3.

Now, I use Newtonsoft to serialize log data into JSON format. The serialization is quite simple, so I could do it manually and remove the dependency if overcoming the problem above is hard, but I would prefer to leverage Newtonsoft if possible

You still can't use netcoreapp, it's the wrong TFM. Use imports instead: https://github.com/aspnet/Mvc/blob/3d494fd7326d3e137fdb22b53f01c5a34866fbaa/src/Microsoft.AspNetCore.Mvc.ViewFeatures/project.json#L59-L61

(Mind you, this is all temporary; once a new build of Json.NET is out that supports netstandard, this will go away.)

karolz-ms · 2016-05-11T15:52:57Z

Yes, that has helped, thanks--I can compile and run tests fine with the new logger targeting netstandard.

net46 issue still remains...

davidfowl · 2016-05-14T07:13:20Z

src/Microsoft.Extensions.Logging.EventSource/EventSourceLogger.cs

+                return;
+
+            // See if they want the formatted message
+            if (LoggingEventSource.Instance.IsEnabled(System.Diagnostics.Tracing.EventLevel.Critical, LoggingEventSource.Keywords.FormattedMessage))


nit: Why are we using the full namespace for the EventLevel?

Not sure what was original reason, but we do not need it. Fixed.

karolz-ms · 2016-05-16T23:35:45Z

So @davidfowl : any ideas/recommendations on how to treat the .NET46 target?

Eilon · 2016-05-20T23:16:14Z

src/Microsoft.Extensions.Logging.EventSource/EventSourceLogger.cs

+using System.Diagnostics.Tracing;
+using System.Threading;
+using Newtonsoft.Json;
+using System.IO;


Sort & Remove.

(Apply to all files, as necessary. Also, ensure "System" namespaces are at the top - VS has a setting for this.)

Eilon · 2016-05-20T23:46:43Z

Added some comments 🕐

davidfowl · 2016-05-21T16:12:47Z

src/Microsoft.Extensions.Logging.EventSource/LoggingEventSource.cs

+    ///     }
+    /// }
+    /// </summary>
+    [EventSource(Name = "Microsoft-Extensions-Logging")]


How does this name show up?

This name represents a well-known ID of the EventSource/ETW provider that exposes ILogger data to the ETW world. It will be used to subscribe to ILogger data by consumers who use EventSource/ETW to capture diagnostic traces.

karolz-ms · 2016-05-23T00:34:17Z

@Eilon , I think I have addressed all the code formatting issues.

As to why things are implemented the way they are, I tried to explain. Let me know if you need more info or want to discuss in person; please also add @vancem to the conversation

davidfowl · 2016-05-23T06:15:02Z

src/Microsoft.Extensions.Logging.EventSource/EventSourceLogger.cs

+        public EventSourceLogger(string categoryName, int factoryID, LoggingEventSource eventSource, EventSourceLogger next)
+        {
+            CategoryName = categoryName;
+            Level = LoggingEventSource.LoggingDisabled;     // Default is to turn off logging


nit: Can you put the comment on top instead of on the side?

davidfowl · 2016-05-23T06:16:37Z

src/Microsoft.Extensions.Logging.EventSource/EventSourceLogger.cs

+
+        private string ToJson(IEnumerable<KeyValuePair<string, string>> keyValues)
+        {
+            StringWriter sw = new StringWriter();


nit: use more var

karolz-ms · 2016-05-27T15:59:40Z

After my last checkin

The “Travis” build failed complaining about references to types related to EventSource infrastructure (Keywords, Level). I strongly suspect this is a Mono issue
The “AppVeyor” build succeeded, although all tests (not just mine) failed due to “cannot load GCHandle” issue

After the build issues are resolved this request should be ready to merge. I believe I have addressed all issues raised during the review

iron9light · 2016-05-31T13:25:53Z

src/Microsoft.Extensions.Logging.EventSource/LoggingEventSource.cs

+        /// FormattedMessage() is called when ILogger.Log() is called. and the FormattedMessage keyword is active
+        /// This only gives you the human reasable formatted message. 
+        /// </summary>
+        [Event(1, Keywords = Keywords.FormattedMessage, Level = EventLevel.LogAlways)]


[Event(1, Keywords = Keywords.FormattedMessage, Level = EventLevel.LogAlways, Message = "{2} - {4}")]
How about adding a formatter like this?
Indeed, I tried it, but it does not work (Message = null). I'm using azure fabric and tried it in local.
Don't know why.

FormattedMessage is precisely what the producer of the data intended to expose. It is readily available from the event payload. I do not see an added benefit of using the Message for this event, it would be less flexible and redundant.

I'm using EventSource in Azure fabric. Azure fabric provide a ETW viewer (as a VS extension) for local fabric services. It will show Timestamp, Event name and Message. You need to click one row to see the detail info. Anyway, I agree with you.

….EventSource.Test assembly

…Logger is called

Avoids using static variable for TestListener initialization parameters (an idea from Vance Morrison)

Includes necessary test changes related to the fact that .NET 4.5 does not have ETW payload names in listener args.

Eilon · 2016-06-24T21:40:48Z

Looks good. I'm closing this because we'll end up merging #448, which has some formatting/administrative fixes (e.g. version number updates). Thanks!!!

dnfclas added the cla-required label May 10, 2016

dnfclas added cla-signed and removed cla-required labels May 10, 2016

Eilon reviewed May 10, 2016
View reviewed changes

davidfowl reviewed May 14, 2016
View reviewed changes

Eilon reviewed May 20, 2016
View reviewed changes

davidfowl reviewed May 21, 2016
View reviewed changes

cwe1ss mentioned this pull request May 23, 2016

EventSource implementation #328

Closed

davidfowl reviewed May 23, 2016
View reviewed changes

davidfowl mentioned this pull request May 28, 2016

Implement an official FileLogger #441

Closed

iron9light reviewed May 31, 2016
View reviewed changes

karolz-ms added 13 commits June 1, 2016 10:33

ILogger implementation that sends data to EventSource/EventListener

e1a55b2

Fix project to refer to netstandard, not netcoreapp

701cc71

Eliminate self-closing loops

26e7687

Code formatting (address feedback from Eilon)

758798d

Reduce number of LoggingEventSource.Instance references to minimum (1)

27fcea8

A few more code cleanups based on feedback from David Fowler

429e54f

Explicitly disable parallel execution in Microsoft.Extensions.Logging…

a3c93a8

….EventSource.Test assembly

Add a logger provider to the factory immediately after AddEventSource…

1958f5e

…Logger is called

Lock on private object inside LoggingEventSource

c83da3e

A better workaround for EventListener initialization

216f34d

Avoids using static variable for TestListener initialization parameters (an idea from Vance Morrison)

Target netstandard1.3 and net451

9704ce0

Includes necessary test changes related to the fact that .NET 4.5 does not have ETW payload names in listener args.

Don't need reference to System.Diagnostics.Tracing for .NET 4.5.1

c47ff44

Remove unused loggerFactory parameter

61b061f

Eilon closed this Jun 24, 2016

wiktork mentioned this pull request Mar 11, 2020

EventSourceLogger does not propagate state data during ActivityStart dotnet/runtime#33486

Open

pranavkm mentioned this pull request Mar 2, 2022

Possible memory leak in EventSource logger? dotnet/runtime#66072

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ILogger implementation that sends data to EventSource/EventListener #424

ILogger implementation that sends data to EventSource/EventListener #424

karolz-ms commented May 10, 2016

dnfclas commented May 10, 2016

dnfclas commented May 10, 2016

Eilon May 10, 2016

karolz-ms May 10, 2016

Eilon May 10, 2016

karolz-ms commented May 11, 2016

davidfowl May 14, 2016

karolz-ms May 16, 2016

karolz-ms commented May 16, 2016

Eilon May 20, 2016

Eilon May 20, 2016

karolz-ms May 23, 2016

Eilon commented May 20, 2016

davidfowl May 21, 2016

karolz-ms May 23, 2016

karolz-ms commented May 23, 2016

davidfowl May 23, 2016

karolz-ms May 23, 2016

davidfowl May 23, 2016

karolz-ms May 23, 2016

karolz-ms commented May 27, 2016

iron9light May 31, 2016 •

edited

Loading

karolz-ms May 31, 2016

iron9light May 31, 2016

Eilon commented Jun 24, 2016

ILogger implementation that sends data to EventSource/EventListener #424

ILogger implementation that sends data to EventSource/EventListener #424

Conversation

karolz-ms commented May 10, 2016

dnfclas commented May 10, 2016

dnfclas commented May 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karolz-ms commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karolz-ms commented May 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon commented May 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karolz-ms commented May 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karolz-ms commented May 27, 2016

iron9light May 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon commented Jun 24, 2016

iron9light May 31, 2016 •

edited

Loading