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

Consider supporting a streamlined WriteTo syntax #49

Open
nblumhardt opened this issue Apr 12, 2017 · 13 comments
Open

Consider supporting a streamlined WriteTo syntax #49

nblumhardt opened this issue Apr 12, 2017 · 13 comments

Comments

@nblumhardt
Copy link
Member

"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
Copy link
Contributor

skomis-mm commented Apr 12, 2017

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:

  • much less verbose than current syntax
  • more intuitive with simple configurations
  • more environment-friendly: paths to elements are much shorter, have strong names (no indexes) and they are closer to what we have in appsettings

Cons:

  • more shift with the existing configuration syntax (when considering to support both)
  • less consistent and intuitive when introducing new instances of sinks of the same kind, may introduce ambiguities thus can be breaking

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:

  • less verbose than current syntax
  • more consistent with the existing syntax and more intuitive when adding new sinks of the same kind

Cons:

  • less environment-friendly: paths are longer, can contain indexes on its names (when using within arrays)

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
Copy link
Member Author

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:

  • much less verbose than current syntax
  • more intuitive with simple configurations
  • more environment-friendly: paths to elements are much shorter, have strong names (no indexes unless multiple instances are added) and they are closer to what we have in appsettings
  • reasonably consistent and intuitive when introducing new instances of sinks of the same kind (rule would be: if one instance, pass an object, if multiple instances, pass an array of objects)

Cons:

  • more shift with the existing configuration syntax (when considering to support both)
  • may introduce ambiguities thus can be breaking

Thoughts?

@skomis-mm
Copy link
Contributor

skomis-mm commented Apr 13, 2017

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
Copy link
Member Author

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
Copy link
Contributor

skomis-mm commented Apr 14, 2017

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
Copy link
Contributor

MV10 commented Aug 29, 2018

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
Copy link
Contributor

MV10 commented Aug 29, 2018

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

@MV10
Copy link
Contributor

MV10 commented Sep 30, 2018

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

@nblumhardt nblumhardt mentioned this issue Oct 7, 2018
@desmondgc
Copy link

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
Copy link

tzographos commented Oct 20, 2019

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
Copy link

kovyfive commented Dec 6, 2019

Serilog__WriteTo__1__Args__apiKey

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

@JonruAlveus
Copy link

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
Copy link
Member Author

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.

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

No branches or pull requests

7 participants