Windows EventLog for NetStandard2.0 - new NLog.WindowsEventLog package #2547

snakefoot · 2018-01-27T00:10:27Z

Depending on preview/pre-release nuget-package:

https://www.nuget.org/packages/System.Diagnostics.EventLog/

Expecting to be part of the NetCore compatibility pack:

https://blogs.msdn.microsoft.com/dotnet/2017/11/16/announcing-the-windows-compatibility-pack-for-net-core/

codecov · 2018-01-27T11:48:24Z

Codecov Report

Merging #2547 into master will decrease coverage by <1%.
The diff coverage is 50%.

@@           Coverage Diff           @@
##           master   #2547    +/-   ##
=======================================
- Coverage      81%     81%   -<1%     
=======================================
  Files         326     326            
  Lines       24222   24242    +20     
  Branches     3068    3073     +5     
=======================================
- Hits        19621   19604    -17     
- Misses       3773    3809    +36     
- Partials      828     829     +1

304NotModified · 2018-01-28T19:28:04Z

nice!

I think this should be in NLog 5?

snakefoot · 2018-01-28T19:54:57Z

I think this should be in NLog 5?

It is a seperate Nuget-package, not dependent on any specific version of NLog.

snakefoot · 2018-01-29T06:42:53Z

Think people doing NetCore optimized web-applications on the Windows-Platform would love to have this feature.

304NotModified · 2018-01-29T07:20:23Z

Yes, great feature!

304NotModified · 2018-02-06T19:49:14Z

src/NLog/Targets/Target.cs

@@ -669,6 +669,9 @@ protected void MergeEventProperties(LogEventInfo logEvent)
        /// <returns>String representing log event.</returns>
        protected string RenderLogEvent(Layout layout, LogEventInfo logEvent)
        {
+            if (layout == null || logEvent == null)
+                return string.Empty;


I think it would be nice to write a warning (maybe debug level) to the internal logger in this case, what do you think?

Not sure I like messages per logevent.

true, but not logging anything without any logs is also not that nice. Internal log Trace level otherwise?

Well some Layouts are expected to be blank/null, so I think the important layout logging should be handled by the individual targets.

OK, thanks for the input

304NotModified · 2018-02-06T19:48:33Z

src/NLog/Targets/EventLogTarget.cs

@@ -437,11 +448,10 @@ private void CreateEventSourceIfNeeded(string fixedSource, bool alwaysThrowError
            catch (Exception exception)
            {
                InternalLogger.Error(exception, "Error when connecting to EventLog.");
-                if (alwaysThrowError || exception.MustBeRethrown())
+                if (alwaysThrowError || LogManager.ThrowExceptions)


this is a bugfix?

also this one, copy code or it should be public

Don't like to pollute with public extension methods on non-NLog-objects. Have now introduced a new public method LogManager.MustBeRethrown(Exception)

304NotModified · 2018-02-06T19:48:16Z

src/NLog/Targets/EventLogTarget.cs

                var value = RenderLogEvent(EntryType, logEvent);

                EventLogEntryType eventLogEntryType;
+#if NETSTANDARD2_0
+                if (Enum.TryParse(value, true, out eventLogEntryType))


could we move this to the EnumHelpers?

the line below there is the EnumHelpers? ;)

The NetStandard2.0 EventLog-assembly is not part of the Nlog.dll, it is a separate package and cannot access internal stuff (unless I open for it like with unit-tests).

cannot access internal stuff (unless I open for it like with unit-tests).

Maybe that isn't a bad idea, what do you think? Never done that with "public" assemblies, but guess it works?

I don't like replacing a hack with another hack. The correct solution would be to make an official API. But I can copy-paste the code for this. Though code-search in the solution is going to be funny.

OK, please copy-paste then for these cases, thx!

Now performed copy-paste

304NotModified · 2018-02-06T19:47:54Z

src/NLog/Targets/EventLogTarget.cs


-
+            short category = 0;
+            if (!string.IsNullOrEmpty(renderCategory) && !short.TryParse(renderCategory, NumberStyles.Integer, CultureInfo.InvariantCulture, out category))


why did we remove the RenderShort and RenderInt? Because there aren't public?

Yes not public

Proposal: or make public, or duplicate the code into this package :)

It's easier to maintain then

please share your thoughts

I was thinking everything would be changed, when NLog 6.0 comes and explodes everything into smaller pieces. This PR is actually a small sample of the challenges that will come when the bomb explodes.

yep, and it's easier to structural fix it with NLog 6 if we copy-paste now the methods, agree?

Have now replaced internal LayoutHelpers with public ConversionHelpers.

304NotModified · 2018-02-06T19:50:54Z

Meta data Looks good :)

304NotModified · 2018-02-12T12:22:37Z

src/NLog/Common/ConversionHelpers.cs

+        /// <param name="resultValue">Output value</param>
+        /// <param name="defaultValue">Default value</param>
+        /// <returns>Returns failure if the input value could not be parsed</returns>
+        public static bool TryParseInt16(string inputValue, out short resultValue, short defaultValue = default(short))


is there a reason why we use int16 etc, while short is recommend by MS?

304NotModified · 2018-02-12T12:23:10Z

src/NLog/Common/ConversionHelpers.cs

+    /// <summary>
+    /// String Conversion Helpers
+    /// </summary>
+    public static class ConversionHelpers


any idea why this isn't a git rename?

probable because it was render... and now tryparse...

Could you explain why this is changed? IMO the render... was nice, also all rendering and conversing (and internal logging) would be handled consistently.

Think the target should decide if critical error and if logging is needed

304NotModified · 2018-02-12T18:07:59Z

Imo Consistent internal logging is also important

The helpers should help and make rendering easier, including error handling. In this pr they are tryparse wrappers.

snakefoot · 2018-02-12T19:16:37Z

I prefer the flexibility. What if the input doesn't come from a Layout and LogEventInfo ? What if needing to log multiple parameters when conversion fails?

304NotModified · 2018-02-19T20:15:57Z

I prefer the flexibility.

That's also possible, by not using the render helpers. Also that's not the case here?

I have introduced the helpers because internal logging was forgotten and that gives too less insights when something when wrong.

I propose to copy paste the current internal helpers, and keep them internal. In the future we could discuss a public API in which we are both happy with :)

ghost · 2018-05-24T18:41:14Z

This feature will be really useful. I'm wondering if this Nuget package will enable the EventLog target even if using NLog with ASP.NET Core (ie calling the ILoggingBuilder.UseNLog() in Program.cs)? Thank you for your work.

304NotModified · 2018-05-24T19:09:12Z

This feature will be really useful. I'm wondering if this Nuget package will enable the EventLog target even if using NLog with ASP.NET Core

Yes, but it's Windows only.

304NotModified · 2018-05-24T19:15:49Z

I will release this hopefully today!

304NotModified · 2018-05-25T16:17:23Z

merged!

snakefoot · 2018-05-25T19:07:59Z

@vlef Has been released: https://www.nuget.org/packages/NLog.WindowsEventLog/

ghost · 2018-05-25T20:18:04Z

Great, thanks guys. However, I keep getting "Target cannot be found: 'EventLog'" when it's parsing my nuget.config. Is there anything else I should be doing other than adding the new nuget package NLog.WindowsEventLog to the project?

snakefoot · 2018-05-25T20:20:46Z

Must run netcore2. But maybe you need add assembly at the top of your NLog.config:

<?xml version="1.0" encoding="utf-8" ?>
<nlog xmlns="http://www.nlog-project.org/schemas/NLog.xsd" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" >
  <extensions>
    <add assembly="NLog.WindowsEventLog" />
  </extensions>
  <targets>

idurston · 2018-05-29T13:55:02Z

Adding that extension in the config got me past the Target cannot be found error...but now I keep getting:

2018-05-29 08:53:26.7090 Trace AsyncWrapper(Name=eventlog): Writing 1 events (Timer)
2018-05-29 08:53:26.7090 Trace AsyncWrapper(Name=webservice): Writing 1 events (Timer)
2018-05-29 08:53:26.9850 Error Error has been raised. Exception: System.TypeLoadException: Could not load type 'NLog.Common.ConversionHelpers' from assembly 'NLog, Version=4.0.0.0, Culture=neutral, PublicKeyToken=5120e14c03d0593c'.
at NLog.Targets.EventLogTarget.GetEntryType(LogEventInfo logEvent)
at NLog.Targets.EventLogTarget.Write(LogEventInfo logEvent)
at NLog.Targets.Target.Write(AsyncLogEventInfo logEvent)

I updated NLog to 4.5.5. Any help is appreciated...

danstur · 2018-05-29T14:22:08Z

The NLog.Common.ConversionHelpers is not part of the released NLog 4.5.5 nuget package, the class is still NLog.Internal.EnumHelpers in the released sources.

This means the current package can not work. We'd need an NLog 4.5.6 from the current master to fix that problem.

snakefoot · 2018-05-29T14:45:44Z

@304NotModified & @idurston & @danstur Yes this PR was merged AFTER the NLog 4.5.5 release-cut (for some reason). I guess it needs to be re-releasesd with dependency on NLog 4.5.6 (And the NLog.WindowsEventLog 4.5.5 should be unlisted)

304NotModified · 2018-05-29T15:21:11Z

I think I have to release all (NLog and this package) under 4.5.6, isn't?

304NotModified · 2018-05-29T15:21:46Z

PS: I've unlisted the current package.

snakefoot · 2018-05-29T15:43:37Z

Could you delay the NLog 4.5.6 some hours ? as I would like to fix a bug in JsonLayout

…

Sent from my Samsung device

-------- Original message -------- From: Julian Verdurmen <notifications@github.com> Date: 29/05/2018 17:21 (GMT+01:00) To: NLog/NLog <NLog@noreply.github.com> Cc: Rolf Kristensen <sweaty1@hotmail.com>, Mention <mention@noreply.github.com> Subject: Re: [NLog/NLog] Windows EventLog for NetStandard2.0 - new NLog.WindowsEventLog package (#2547) I think I have to release all (NLog and this package) under 4.5.6, ins;t? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2547 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AK-fnPwcb7389oINpMuqwFIwOPyyXs44ks5t3WdygaJpZM4RvC7A>.

304NotModified · 2018-05-29T15:47:27Z

OK!

snakefoot · 2018-05-29T17:44:33Z

@304NotModified Have now created #2747

304NotModified · 2018-05-29T21:33:43Z

4.5.6 is now on nuget.org: https://www.nuget.org/packages/NLog.WindowsEventLog

(currently indexing on nuget.org)

also a RTM of the dependency is released this evening: (that will be in 4.5.7)

danstur · 2018-05-30T07:18:40Z

I can confirm that using the 4.5.6 NuGets solves the problem.

304NotModified · 2018-05-30T18:46:30Z

@danstur Thanks for the confirmation!

snakefoot force-pushed the NetCoreEventLog branch 7 times, most recently from 17f255b to d72434f Compare January 27, 2018 11:30

304NotModified added this to the 5.0 (new) milestone Jan 28, 2018

304NotModified added the feature label Jan 28, 2018

304NotModified self-requested a review January 29, 2018 07:19

304NotModified changed the title ~~Windows EventLog for NetStandard2.0~~ Windows EventLog for NetStandard2.0 - new NLog.WindowsEventLog package Feb 6, 2018

304NotModified reviewed Feb 6, 2018

View reviewed changes

304NotModified modified the milestones: 5.0 (new), 4.5 Feb 6, 2018

snakefoot force-pushed the NetCoreEventLog branch 2 times, most recently from 39edcd4 to a609d88 Compare February 6, 2018 21:22

304NotModified reviewed Feb 12, 2018

View reviewed changes

Windows EventLog for NetStandard2.0

1490f3b

snakefoot force-pushed the NetCoreEventLog branch from f3d6db7 to 1490f3b Compare May 16, 2018 20:13

Update dependency System.Diagnostics.EventLog

e34a6f3

304NotModified approved these changes May 24, 2018

View reviewed changes

304NotModified mentioned this pull request May 24, 2018

Better list for targets, layoutrenderers & layouts (including platform support) NLog/NLog.github.io#68

Closed

31 tasks

Fix Nuget install of OpenCover

0940338

304NotModified merged commit cc6e653 into NLog:master May 25, 2018

304NotModified mentioned this pull request May 29, 2018

Release 4.5.6 #2746

Merged

3 tasks

304NotModified modified the milestones: 4.5, 4.5.6 May 30, 2018

snakefoot mentioned this pull request Aug 15, 2018

IRawValue + dbNullLayoutRenderer #2849

Merged

5 tasks

snakefoot deleted the NetCoreEventLog branch April 4, 2020 17:37



		short category = 0;
		if (!string.IsNullOrEmpty(renderCategory) && !short.TryParse(renderCategory, NumberStyles.Integer, CultureInfo.InvariantCulture, out category))

Windows EventLog for NetStandard2.0 - new NLog.WindowsEventLog package #2547

Windows EventLog for NetStandard2.0 - new NLog.WindowsEventLog package #2547

Conversation

snakefoot commented Jan 27, 2018 • edited

codecov bot commented Jan 27, 2018 • edited

Codecov Report

304NotModified commented Jan 28, 2018

snakefoot commented Jan 28, 2018

snakefoot commented Jan 29, 2018 • edited

304NotModified commented Jan 29, 2018

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

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

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

snakefoot Feb 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snakefoot Feb 6, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snakefoot Feb 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snakefoot Feb 6, 2018 • edited

Choose a reason for hiding this comment

304NotModified commented Feb 6, 2018

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

304NotModified commented Feb 12, 2018

snakefoot commented Feb 12, 2018 via email • edited

304NotModified commented Feb 19, 2018

ghost commented May 24, 2018

304NotModified commented May 24, 2018

304NotModified commented May 24, 2018

304NotModified commented May 25, 2018

snakefoot commented May 25, 2018

ghost commented May 25, 2018

snakefoot commented May 25, 2018

idurston commented May 29, 2018

danstur commented May 29, 2018

snakefoot commented May 29, 2018 • edited

304NotModified commented May 29, 2018 • edited

304NotModified commented May 29, 2018

snakefoot commented May 29, 2018 via email

304NotModified commented May 29, 2018

snakefoot commented May 29, 2018

304NotModified commented May 29, 2018 • edited

danstur commented May 30, 2018

304NotModified commented May 30, 2018

snakefoot commented Jan 27, 2018 •

edited

codecov bot commented Jan 27, 2018 •

edited

snakefoot commented Jan 29, 2018 •

edited

snakefoot Feb 6, 2018 •

edited

snakefoot Feb 6, 2018 •

edited

snakefoot Feb 6, 2018 •

edited

snakefoot Feb 6, 2018 •

edited

snakefoot commented Feb 12, 2018 via email •

edited

snakefoot commented May 29, 2018 •

edited

304NotModified commented May 29, 2018 •

edited

304NotModified commented May 29, 2018 •

edited