serilog / serilog-settings-configuration

A Serilog configuration provider that reads from Microsoft.Extensions.Configuration
Apache License 2.0
455 stars 129 forks source link

Consider supporting a streamlined WriteTo syntax #49

Open nblumhardt opened 7 years ago

nblumhardt commented 7 years ago

"WriteTo", and other configuration elements that accept arguments, use a syntax like:

  "WriteTo": [
       {"Name": "File", "Args": {"path": "%APPDATA%\\log.txt" }}
   ]

"WriteTo" accepts an array so that it's easy to extrapolate from configuring one instance of a sink, to multiple instances:

  "WriteTo": [
       {"Name": "File", "Args": {"path": "%APPDATA%\\log.txt" }},
       {"Name": "File", "Args": {"path": "%APPDATA%\\another-log.txt" }}
   ]

The syntax is a bit unwieldy. If only one instance of each sink is used, "WriteTo" could accept an object, and there would be no (invalid) duplication of property names:

  "WriteTo": {
       "File": {"path": "%APPDATA%\\log.txt" }
   }

Behind the scenes, I think we could implement this today alongside the current syntax, by considering "WriteTo" elements with numeric names to be the verbose array syntax, and otherwise assume the compact syntax is being used. (I say this without having spiked it out, so there could be obstacles in the way...)

The downside of the change would be more complexity (attempting to accept a new syntax while not breaking existing configurations), and confusion while documentation, examples, etc. move across.

Any thoughts?

skomis-mm commented 7 years ago

I'm on simplifying syntax and making it more streamlined 👍 I've played around with proposed syntax and made some list of pros and cons.

"WriteTo": {
  "File": { "path": "log1.txt" },
  "RollingFile": { "pathFormat": "log2.txt" }
},

The only problem with this are sinks of the same kind. To add another instance of the File sink we cannot add:

"WriteTo:File3": {
  "File": { "path": "log3.txt" }
},

because we can not distinguish it with antecedent configuration. But we can use expanded syntax:

"WriteTo:File3": {
  "Name": "File",
  "Args": { "path": "log3.txt" }
},

which is equivalent to:

"WriteTo": {
  "File": { "path": "log3.txt" },
  "File3": { "Name": "File", "Args": { "path": "log3.txt" } }
}

And we can see that we may have some ambiguity: is File3 sink's section name or name of some extension method with the Name and Args arguments?

Pros:

Cons:


Now let propose intermediate variation on this:

"WriteTo": [
  { "File": { "path": "log1.txt" } },
  { "RollingFile": { "pathFormat": "log2.txt" } },
],

I just dropped Name and Args. Let add another File sink:

"WriteTo": [
  { "File": { "path": "log1.txt" } },
  { "File": { "path": "log3.txt" } },
  { "RollingFile": { "pathFormat": "log2.txt" } },
],

And this will also work now:

"WriteTo:File3": {
  "File": { "path": "log3.txt" }
},

If I'll try to add another sink with expanded syntax like this:

"WriteTo": [
  { "File": { "path": "log1.txt" } },
  { "File": { "path": "log3.txt" } },
  { "RollingFile": { "pathFormat": "log2.txt" } },
  { "Name": "LiterateConsole" }
],

it will not introduce ambiguity because Name's section value can only be the string, but not some subsection which could be mapped to arguments of a some Name's extension method.

Pros:

Cons:

I did not mention implementation for both. The changes will be minimal to support either of them or mix of them. I'm leaning towards intermediate version since it brings desired less verbosity in relation to the current syntax with fewer design changes.

What do you think?

nblumhardt commented 7 years ago

Agree with the pro/con analysis.

The additional alternative is interesting - it definitely strikes a good balance between the two. It's an improvement over the current syntax, but still does look quite "brackety" compared with the naive/simplistic version.

Just getting all possible variations out there, without thought to implementation effort or feasibility, we could also consider this style:

"WriteTo": {
  "File": { "path": "log1.txt" },
  "RollingFile": [
    { "pathFormat": "log2.txt" },
    { "pathFormat": "log3.txt" }
  ]
}

Here the one-instance version is the simple object-style option, but when multiple instances of a sink are required, we accept an array of parameter sets.

Borrowing from your earlier pro/con list, I think this would be:

Pros:

Cons:

Thoughts?

skomis-mm commented 7 years ago

I'm really like it. If not taking into account current syntax third one has no inconsistencies of the first version when introducing new sinks of the same kind, and it is natural, compact and intuitive. So the remaining cons can be considered minor with low risk to break something:

"WriteTo": {
  // old syntax with implicit (index) instance name, 
  // no ambiguities since `0` is not a valid method name
  "0": { "Name": "File", "Args": { "path": "log1.txt" } },

  // old syntax with explicit instance name, may introduce ambiguities - the only one
  "File1": { "Name": "File", "Args": { "path": "log2.txt" } },

  // old syntax for parameterless method with implicit (index) instance name,
  // no ambiguities since `ColoredConsole` is string value
  "1": "ColoredConsole",

  // old syntax for parameterless method with explicit instance name,
  // no ambiguities since `Trace` is string value
  "TraceInstance": "Trace",

  // new compact syntax for single instance
  "File": { "path": "log3.txt" },

  // new compact syntax for multiple instances with implicit (indexes) instance names
  "RollingFile": [
    { "pathFormat": "log3-{Date}.txt" },
    { "pathFormat": "log4-{Date}.txt" }
  ],

  // new compact syntax for multiple instances with explicit instance names
  "RollingFile:Instance1": { "pathFormat": "log1-{Date}.txt" },
  "RollingFile:Instance2": { "pathFormat": "log2-{Date}.txt" },

  // new compact syntax for parameterless method
  "LiterateConsole": {}
},

So we can stick with it 👍


Ok, some update. This one will not work:

"LiterateConsole": { }

since its not Microsoft.Extensions.Configuration-friendly now. Its api just ignores such section with empty subsection (or object with no properties, see this). The workaround will be:

"LiterateConsole": ""

Probably a small inconvenience.. But the same applies to other providers which require that each configuration can be flattened to key=value pair.

Thoughts?

nblumhardt commented 7 years ago

The workaround is bearable, but could be surprising. Booleans (if they're supported?) might look less odd:

  "LiterateConsole": true

Parameterless methods still seem like a bit of a trap for new users.

I wonder if this has been discussed already in the m.e.Configuration repo? I can't find any earlier ticket mentioning it. Having "X": {} interpreted internally as equivalent to "X": "" seems like it could work... Do you think it's worth proposing?

skomis-mm commented 7 years ago

The boolean semantics is interesting (and supported of course). And actually may be useful for suppressing output, for example via environment variables:

set Serilog:WriteTo:LiterateConsole=false ; // or 0

json's look less natural then {} . I checked other providers and here what I found. For ini it also doesn't work:

[Serilog:WriteTo:LiterateConsole]

But, interesting thing with xml provider:

<settings>
   <serilog>
       <writeTo>
           <literateConsole></literateConsole>
       <writeTo />
   </serilog>
</settings>

is works. So yes, I think some edge cases in Configuration packages may be polished more. More over, they recently have added new api - IConfigurationSection.Exists() which will be awkward to see false while having 'LiterateConsole': {}. And for json format its pretty natural to see such configuration. They can do more for it like they did with arrays. And we could support both {} and <Boolean>. May worth to propose this 👍


Update. This one will not work:

<writeTo>
      <literateConsole />
<writeTo />

because of this. Seems like it's by design and previous config <literateConsole></literateConsole> considered as empty string value. Not sure now if this possible proposal can make it through.

MV10 commented 6 years ago

I ran into the array syntax problem mentioned in #109, and that reminded me that I wanted to read this discussion more closely. In the spirit of "simple is best," the closest we can get to a plain list with clear syntax seems ideal.

Ironically, I'm partial to the "brackety" version that uses one containing array because there is no "hidden" syntax. If you see this example, you know everything there is about using WriteTo:

"WriteTo": [
  { "File": { "path": "log1.txt" } },
  { "File": { "path": "log3.txt" } },
  { "RollingFile": { "pathFormat": "log2.txt" } },
  { "RollingFile": { "pathFormat": "log4.txt" } }
]

When people have to deal with arrays in the current syntax, it appears to be confusing to many, given the number of issues I've seen where explicit array-number manipulation is the "hidden" answer (or like #109, questions about dealing with arrays outright). The idea of an array for the arg lists of multiple instances is another case of hidden syntax -- if you suddenly realize you need two sinks of the same type, it isn't obvious that you can add an array.

I think you both probably understand this, but array indices and unique keys are irrelevant to making WriteTo work, except as a trivial implementation detail. Configuration expressions require an encompassing array (as in the JSON above) but there's no reason to retain that internally or to burden the user with figuring out uniqueness criteria and related syntax. That means issue #109 just ceases to be a problem -- an additional config source doesn't have to know anything about the WriteTo array structure used by other possible config sources.

In terms of backwards compatibility, I think it should be easy to recognize arrays in the current Name and Args format to load old-style configs equally transparently.

The discussion around LiterateConsole and other parameterless methods isn't surprising. Config boils down to key-value pairs. No value, no pair. I'm not sure how M.E.Config reacts to a value of null (which is a valid value in the 2014 JSON RFC) but to me that's the most consistent representation of a method without arguments. (I'll try it after I actually post this, the formerly-super-badass workstation I inherited from the wife does a lot of Surprise Rebooting these days...)

MV10 commented 6 years ago

FWIW, M.E.Config does accept null as a valid "value" (as in "LiterateConsole": null).

MV10 commented 6 years ago

Tagging #102 as another issue with WriteTo-array confusion...

desmondgc commented 5 years ago

My hope is for a solution that eliminates array indexes. My specific use case is that I want to override the Seq apiKey from environment variables. Currently I can do this: Serilog__WriteTo__1__Args__apiKey=my_secret_key but the inclusion of the array index makes it brittle.

tzographos commented 5 years ago

Another proposal would be to "reference" the sinks inside the WriteTo array, thus converting WriteTo to an object. For example:


  "Serilog": {
    "WriteTo": {
      "Names": ["Console", "File1", "File2"],
      "Console": {
        "Args": {
          "restrictedToMinimumLevel": "Information",
          "outputTemplate": "blah blah"
        }
      },
      "File1": {
        "Args": {
          "restrictedToMinimumLevel": "Debug",
          "path": "blah blah 1",
          "outputTemplate": "blah blah"
        }
      },
      "File2": {
        "Args": {
          "restrictedToMinimumLevel": "Debug",
          "path": "blah blah 2",
          "outputTemplate": "blah blah"
        }
      }
    }
  }
}
kovyfive commented 4 years ago

SerilogWriteTo1ArgsapiKey

Thanks, that saved my life here! Can recommend this to everyone who is in trouble. Arrays start with 0 here.

JonruAlveus commented 3 years ago

Hi. I’ve come across the array indexing confusion as well and I think @nblumhardt solution seems the cleanest solution to me. It would really help with logging to different sinks in different environments. Is there any movement on this? I’d be happy to help out if I can, not sure I’d know where to start though!

nblumhardt commented 3 years ago

No movement yet! Would be great if someone can write up an RFC-style proposal (perhaps in a new ticket) so that we can nail the design down - from that point hopefully it will be easier for someone (else?) to pick up the implementation side.