Optimize parsing of NLog Properties and LogLevel about elmah.io.nlog

[snakefoot]

Something like this. I have not tested the code:

public class ElmahIoTarget : TargetWithLayout
{
        public Layout Hostname { get; set; } = "${event-properties:hostname:whenEmpty=${machinename}}";

       protected override void Write(LogEventInfo logEvent)
       {
             string hostName = RenderLogEvent(Hostname, logEvent);
       }
}

[snakefoot]

Btw. you should also consider to activate OptimizeBufferReuse (Below code change will also protect you from any future changes to the NLog default-Layout).

        private readonly string DefaultLayout;
        private bool _usingDefaultLayout;

        public ElmahIoTarget()
        {
             OptimizeBufferReuse = true;
             DefaultLayout = Layout?.ToString();
        }

        public ElmahIoTarget(IElmahioAPI client)
               :this()
        {
             _client = client;
        }

        protected override void InitializeTarget()
        {
             _usingDefaultLayout = Layout == null || Layout.ToString() == DefaultLayout;
        }

        protected override void Write(LogEventInfo logEvent)
        {
              var title = _usingDefaultLayout ? logEvent.FormattedMessage : RenderLogEvent(Layout, logEvent);
        }

[snakefoot]

Oh yeah. And then always RenderLogEvent(ApplicationLayout, logEvent), right?

Yes.

[ThomasArdal]

Those are some really interesting suggestions actually. I think that I may want to move the first part to a CreateMessage-method or something like that, to avoid the ref parameters. Thanks as always dude!

[ThomasArdal]

I've implemented your suggestion to LevelToSeverity. I have also switched from calling ToString().ToLower() to using StringComparison.OrdinalIgnoreCase which is a much cleaner way of writing the code. I will wait with the multiple iterations of Properties for now. I don't really like to look at code with ref parameters, but will think about how to reduce the number of times we need to iterate through all properties.

[ThomasArdal]

[snakefoot]

Instead of doing this:

if (logEvent.Properties == null || !logEvent.Properties.Any()) return null;

Then I really recommend this:

if (!logEvent.HasProperties) return null;

• private List PropertiesToData(LogEventInfo logEvent)
• internal static int? Integer(this LogEventInfo logEvent, string name)
• internal static string String(this LogEventInfo logEvent, string name)

Also strange that your two extensions methods performs two sequential iterations. One to check, and one to extract. Also strange that they are identical except the one is doing int.TryParse. Why not let one call the other?

[ThomasArdal]

Awesome. Hadn't even noticed HasProperties. Fixed!

[ThomasArdal]

Totally forgot to comment on your suggestion about replacing IList<Item> with ICollection<Item>. I see how this would be beneficial, but CreateMessage is auto-generated code. I might be able to achieve it by changing the swagger JSON on the API, but want to keep this as stable as possible 😄

[snakefoot]

@ThomasArdal Any reason why you don't implement all these as individual NLog Layout property-members on the Target:

• application,
• source,
• hostname - Ex: ${event-properties:hostname:whenEmpty=${machinename}}
• user,
• method,
• version,
• url,
• type,
• statuscode

Maybe a new option could be added to the NLog ${event-properties} to be case-insensitive on lookup. Or maybe just have multiple lookup values. Ex. "hostname" + "Hostname" + "HostName".

[ThomasArdal]

When looking at this: NLog/NLog#2995 isn't that exactly what you are proposing?

[snakefoot]

When looking at this: NLog/NLog#2995 isn't that exactly what you are proposing?

Not sure I understand. By using Layout-properties as I'm suggesting here in this issue, then you get the ValueFormatter automatically.

[ThomasArdal]

Would it be too much trouble showing the code for a single property then? Not sure I understand 😄

[ThomasArdal]

Something like this: https://github.com/elmahio/elmah.io.nlog/blob/master/src/Elmah.Io.NLog/ElmahIoTarget.cs?

[snakefoot]

@ThomasArdal Something like this...

Yes just like that. Maybe change to these gnarly expression to help against case-sensitive:

        public Layout HostnameLayout { get; set; } = "${event-properties:hostname:whenEmpty=${event-properties:Hostname:whenEmpty=${event-properties:HostName:whenEmpty=${machinename}}}}";

        public Layout SourceLayout { get; set; } = "${event-properties:source:whenEmpty=${event-properties:Source:whenEmpty=${logger}}}";

        public Layout ApplicationLayout { get; set; } = "${event-properties:application:whenEmpty=${event-properties:Application}}";

#if NET45
        public Layout UserLayout { get; set; } = "${event-properties:user:whenEmpty=${event-properties:User:whenEmpty=${identity:authType=false:isAuthenticated=false}}}";
#else
        public Layout UserLayout { get; set; } = "${event-properties:user:whenEmpty=${event-properties:User}}";
#endif

        public Layout MethodLayout { get; set; } = "${event-properties:method:whenEmpty=${event-properties:Method}}";

        public Layout VersionLayout { get; set; } = "${event-properties:version:whenEmpty=${event-properties:Version}}";

        public Layout UrlLayout { get; set; } = "${event-properties:url:whenEmpty=${event-properties:Url:whenEmpty=${event-properties:URL}}}";

        public Layout TypeLayout { get; set; } = "${event-properties:type:whenEmpty=${event-properties:Type}}";

        public Layout StatusCodeLayout { get; set; } = "${event-properties:statuscode:whenEmpty=${event-properties:Statuscode:whenEmpty=${event-properties:StatusCode}}}";

Remember to fix these also to use RenderLogEvent:

Source = Source(logEvent),
Application = ApplicationName(logEvent),

And maybe change this line:

_usingDefaultLayout = Layout?.ToString() == DefaultLayout;

To this line:

_usingDefaultLayout = Layout == null || Layout.ToString() == DefaultLayout;

[ThomasArdal]

Good suggestions. Source, Application and StatusCode do call RenderLogEvent. But in case that doesn't return a value, the methods picks some other values that I couldn't spot how to express as a layout renderer.

[snakefoot]

@ThomasArdal Have updated my suggestion so SourceLayout has fallback to ${logger}

Cannot see any logic is happening for ApplicationName. Maybe my eyes have become old.

[ThomasArdal]

👍 on ${logger}.

ApplicationName returns the value of the Application property if nothing is found in properties.

[snakefoot]

@ThomasArdal Still recommend that you only allocate the StringBuilder for IEnumerable-objects. But if you really like the ValueFormatter, then you can do this:

        private List<Item> PropertiesToData(LogEventInfo logEvent)
        {
            if (!logEvent.HasProperties) return null;

            var items = new List<Item>();
            StringBuilder sb = null;
            var valueFormatter = ConfigurationItemFactory.Default.ValueFormatter;
            foreach (var obj in logEvent.Properties)
            {
                if (obj.Value != null)
                {
                     string text;
                     if (obj.Value is string)
                     {
                          text = (string)obj.Value;
                     }
                     else
                     {
                         sb = sb ?? new StringBuilder();
                         sb.Length = 0;  // Reuse StringBuilder
                         valueFormatter.FormatValue(obj.Value, null, CaptureType.Normal, null, sb);
                         text = sb.ToString();
                    }
                    items.Add(new Item { Key = obj.Key.ToString(), Value = text });
                }
            }
            return items;
      }

[ThomasArdal]

Looks nice! Making the last changes now. This is going to be great 😄

[snakefoot]

About having both Application-property and ApplicationLayout-property, then should consider doing this:

public string Application { get => (ApplicationLayout as SimpleLayout)?.Text; set => ApplicationLayout = value; }

public Layout ApplicationLayout { get; set; } = "${event-properties:application:whenEmpty=${event-properties:Application}}";

[ThomasArdal]

Oh yeah. And then always RenderLogEvent(ApplicationLayout, logEvent), right?