Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for mixed parameters and/or TimeSpan parsing #9

Closed
JFesta opened this issue Sep 8, 2017 · 6 comments
Closed

Support for mixed parameters and/or TimeSpan parsing #9

JFesta opened this issue Sep 8, 2017 · 6 comments

Comments

@JFesta
Copy link

JFesta commented Sep 8, 2017

In my application I want to write my logs using Serilog.Sinks.RollingFile. Additionally, as I need to configure some things (e.g. the pathFormat or the minimum logging level) depending on what environment I'm deploying it, I tried to use Serilog.Settings.AppSettings to read parameters from App.config.

Everything was working fine, until I had to use RollingFile's parameter flushToDiskInterval, which happens to be a TimeSpan.
I added a new setting that was like:

<add key="serilog:write-to:RollingFile.flushToDiskInterval" value="10000" />

Guessing that the value would be parsed as a time in milliseconds. It didn't work as Serilog, fairly enough, throws an exception when trying to parse the parameter.

I didn't find in the internet if and how TimeSpans are handled. Are they even implemented? It could be very useful.

This brings to another problem. I thought that I could just use ReadFrom.AppSettings() to get what I need from App.config, and modify some other parameter afterwards, like the already mentioned flushToDiskInterval. Generally speaking, I wanted to load some parameters from App.config and some others programmatically.
Problem is, I didn't found any way to access those parameters, or even the logger itself, after it has been created through Read.FromAppSettings().
This forced me to stop using Serilog.Settings.AppSettings and to implement my own logic: now I read all the needed parameters programmatically from App.Config manually, and it's not a good solution:

config.WriteTo.RollingFile( System.Configuration.ConfigurationManager.AppSettings["serilog:write-to:RollingFile.pathFormat"], ... );

It would be awesome to have at least one of these two features:

  1. TimeSpan parsing
  2. An overload to Read.FromAppSettings() that has a key-value set as argument through which the caller can specify new parameters (which override parameters with the same name), allowing for an hybrid configuration where some parameters are specified programmatically and cannot be modified, others can be modified through App.config.

I'm assuming both features are not yet implemented as I didn't found them, but I would be happy to know how to use them if they are already implemented, as I failed to find anything on the internet.

@nblumhardt
Copy link
Member

Thanks for the note. Just on item #1, I don't think Convert.ChangeType() works for TimeSpan from string, but a PR to add a few more type conversions would be great if anyone is able to check it out.

The second point is a pretty deep part of the design of this package; one thing we could do here is let ReadFrom.AppSettings accept a set of key-value-pair "defaults" and "overrides" that would be mixed in with the content of the XML file. It would be a bit "stringy" to work with, but the values could then at least be computed programmatically... Thoughts on this option?

@tsimbalar
Copy link
Member

For the timespan parsing, I would say it's wrong to accept ms as a unit by default.

FYI the TimeSpan.ToString() returns :

> TimeSpan.FromDays(2).ToString()
"2.00:00:00"
> TimeSpan.FromHours(5).ToString()
"05:00:00"
> TimeSpan.FromMinutes(2).ToString()
"00:02:00"
> TimeSpan.FromSeconds(15).ToString()
"00:00:15"
> TimeSpan.FromMilliseconds(25).ToString()
"00:00:00.0250000"
> TimeSpan.FromTicks(17).ToString()
"00:00:00.0000017"

And TimeSpan.Parse(string input) accepts the same kind of strings.

So I would say that we should accept values that are valid strings for TimeSpan.Parse() as valid values for a parameter of type TimeSpan

For the second point
For now, the AppSettings parsing handles environment variable expansion, meaning that in

<add key="serilog:enrich:with-property:MachineName" value="%COMPUTERNAME%" /> 

%COMPUTERNAME% is replaced by the value of the environment variable.

Maybe adding an overload of ReadFrom.AppSettings(Dictionary<string,string> defaultEnvironmentVariable) could help ?

You would then pass for instance

new Dictionary<string, string>{
  {"FOO", "bar"},
  {"BAZ", "qux"},
}

and

<add key="serilog:enrich:with-property:MachineName" value="%COMPUTERNAME%" /> 
<add key="serilog:enrich:with-property:Foo" value="%FOO%" /> 
<add key="serilog:enrich:with-property:Baz" value="%BAZ%" /> 

and stuff between the % would first be looked for in current environment variables, then in defaultEnvironmentVariables... Allowing to specify values in code with the rules we want, and optionnally overriding them by environment variables at run time.

Would that cover this use case ?

@nblumhardt
Copy link
Member

+1 on supporting the syntax understood by TimeSpan.Parse 👍

The variable substitution puzzle is getting interesting :-)

Just to put one other idea out there, already discussed in the #7 level switch thread:

<add key="serilog:enrich:with-property:Foo" value="$foo" /> 

where:

new Dictionary<string, object>{
  {"foo", 42},
  {"bar", "qux"},
}

I.e., instead of string substitution, we could implement object references.

The litmus test would be whether it'd work seamlessly with:

var ls = new LoggingLevelSwitch(LogEventLevel.Information);
Log.Logger = new LoggerConfiguration()
    .MinimumLevel.ControlledBy(ls)
    .ReadFrom.Configuration(new Dictionary<string, object>{
        ["ls"] = ls
    })
    .CreateLogger();

With config containing:

<add key="serilog:write-to:Seq.controlLevelSwitch" value="$ls" /> 

It's dangerous designing these kinds of things piecemeal, but it would also be nice if we could fit multiple features cleanly into the one simple "variables" framework.

(Against this style we might consider the argument that pushing variables down from the code into the config is almost the reverse of Serilog's typically code-first design - although we've failed to come up with a code-centric solution to the mixed/overridden configuration requirement so far.)

@tsimbalar
Copy link
Member

@JFesta Regarding TimeSpan ... it turns out that it is already supported, and relies on the format that is accepted by TimeSpan.Parse(string) ...

According to MSDN :

The s parameter contains a time interval specification in the form:

[ws][-]{ d | [d.]hh:mm[:ss[.ff]] }[ws]

So the proper way to write, say, 30 seconds, is 00:00:30.

It is not officially documented in Serilog, but I've just issued a PR (serilog/serilog#1030) with passing tests like this :

[Theory]
[InlineData("3.14:21:18.986", 3 /*days*/, 14 /*hours*/, 21 /*min*/, 18 /*sec*/, 986 /*ms*/)]
[InlineData("4", 4, 0, 0, 0, 0)] // minimal : days
[InlineData("2:0", 0, 2, 0, 0, 0)] // minimal hours
[InlineData("0:5", 0, 0, 5, 0, 0)] // minimal minutes
[InlineData("0:0:7", 0, 0, 0, 7, 0)] // minimal seconds
[InlineData("0:0:0.2", 0, 0, 0, 0, 200)] // minimal milliseconds
public void TimeSpanValuesCanBeParsed(string input, int expDays, int expHours, int expMin, int expSec, int expMs)
{
    var expectedTimeSpan = new TimeSpan(expDays, expHours, expMin, expSec, expMs);
    var actual = SettingValueConversions.ConvertToType(input, typeof(TimeSpan));

    Assert.IsType<TimeSpan>(actual);
    Assert.Equal(expectedTimeSpan, actual);
}

@tsimbalar
Copy link
Member

@JFesta regarding

An overload to Read.FromAppSettings() that has a key-value set as argument through which the caller can specify new parameters (which override parameters with the same name), allowing for an hybrid configuration where some parameters are specified programmatically and cannot be modified, others can be modified through App.config.

would proposal serilog/serilog#1038 help ?

@tsimbalar
Copy link
Member

Closing this one for now.

  • The TimeSpan part is covered
  • The other part is covered in pending issues in different spots.

Can be reopened if needed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants