Layoutable non-string values #1811

Open
304NotModified opened this Issue Dec 3, 2016 · 9 comments

Projects

None yet

2 participants

@304NotModified
Member
304NotModified commented Dec 3, 2016 edited

Current approach:

render, parse.

e.g.

Convert.ToInt64(this.MaxKilobytes.Render(logEvent), CultureInfo.InvariantCulture);

This isn't optimal.

Proposal, replace Layout with Layout<int> IntLayout, and let render do a parse.

related #1234

update: We have now also an implicit conversion from string to layout. For the int case it should be also from int to layout (fixed values), hence we need IntLayout or something like that.

@snakefoot
Contributor
snakefoot commented Dec 3, 2016 edited

Yes it would make more sense that the correct type was declared for the property, and then the XML-parsing of values was able to recognize Layout-logic and automatically render/parse to the correct property-value-type.

That is possible, but then the property value is not evaluated runtime.

If the configuration-logic was able to set the correct properties at startup, then the configuration-logic should also be able to set the correct properties on reload (Without using dedicated Layout-properties). Would also require some logic regarding perform Reinitialize on a running system.

@304NotModified
Member
304NotModified commented Dec 3, 2016 edited

What would be the type of the property then @snakefoot, int? I don't see how that would work :(

for e.g. we GDC, we don't know when to reload (we could make it, but that would be a breaking change for custom layout renderers)

@snakefoot
Contributor
snakefoot commented Dec 3, 2016 edited

Not sure I understand. You have some logic already that can re-apply the configuration. The configuration-logic reads string-values and then parses it so it matches the property-type (string/int/enum or whatever)

Now this re-apply configuration logic "just" need to recognize layout-format, and then render first to an evaluated-string, before again parsing it to the matching property-type.

@304NotModified
Member

example

class MyCustomLayoutRender
{
    
    Layout MaxCount  {get;set;} //"int"
}

var l = new MyCustomLayoutRender();
l.MaxCount  = "${gdc:maxCount}";

Currently we have to do a int parse after render.

  • If we change the property type to int, this won't work currently (e.g. GDC, we need "subscriptions" then)
  • We could make dedicated Layout types, like IntLayout, to "hide" the parse and do some smarter conversions/caching.
@snakefoot
Contributor
snakefoot commented Dec 3, 2016 edited

If configuring stuff from the code, there I have much stronger eco-system (.NET) than Layout-renderer can ever provide.

I was thinking the Layout-Property-Parsing-Logic made sense when using the XML-config, like most people are using:

<targets>
  <target xsi:type="EventLog" maximumKilobytes="${gdc:maxSize}" />
</targets>

But if wanting to do it from code then I would properly make something like this:

l.MaxCount = LayoutParser<int>.Read("${gdc:maxCount}");

Or maybe add some logic, so one could partial update the in-memory XML-configuration residing in NLog.

Trying to wrap every property within a Layout-object starts to smell like Enterprise-software. Would also hurt performance as retrieving the value would require a reference-jump to the Layout-object (CPU cache-miss).

@304NotModified
Member

well there we're multiple request for changing variables on runtime.

I think also 90% of the cases are from XML config, but we can't drop the 10%

@snakefoot
Contributor
snakefoot commented Dec 3, 2016 edited

well there we're multiple request for changing variables on runtime.

But by assigning Layout-strings directly to properties in .NET-code? Instead of just changing the NLog-XML-config or flipping the properties directly using .NET-code ?

If just wanting to modify a Layout-variable in the NLog-XML-Config and re-apply the NLog-XML-Config, then it can probably be done without changing all NET-properties to be Layout-properties.

Anyway, just ignore my rant, the solution has to be something that you like. But if going for Layout-properties, then please make them similar to this:

struct LayoutProperty<T>
{
     public T Value { get { return _recalc ? _value = Parse<T>(_layout.Render()) : _value; } set { _value = value; _recalc = false; }
     private T _value;
     private bool _recalc;  // Maybe change into TimeSpan _recalcInterval
     private readonly Layout _layout;
     public SetLayout(string value) { _layout = value; _recalc = !_layout.FixedValue; _value = _value = Parse<T>(_layout.Render());  }
}

Though the struct will be mutable, then access to Value will be cheap.

@304NotModified
Member
304NotModified commented Dec 3, 2016 edited

But by assigning Layout-strings directly to properties in .NET-code? Instead of just changing the NLog-XML-config or flipping the properties directly using .NET-code ?

filename = "${basedir}/file1.txt" ? ;)

struct LayoutProperty<T>

Well that doesn't work as I can't write a implicit conversion from T to LayoutProperty<T> (e.g. int to LayoutProperty<int>). That's unfortunately, as we have to make a lot LayoutProperties

@snakefoot
Contributor
snakefoot commented Dec 4, 2016 edited

filename = "${basedir}/file1.txt" ? ;)

Yes the filename is very special, but after working with reducing memory allocations in NLog and seeing the cost of rendering the filename for every filewrite. Then I think realtime Layout-properties should only be used when absolutely necessary, as the performance drop is pretty steep (Just like your advice before about not rendering maximumKilobytes for every write). Instead the Layout-logic should primary be applied when reapplying a NLog-XML-config update (ReInitialize)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment