Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Design : first iteration of what Combined settings should look like #5

Open
tsimbalar opened this issue Oct 23, 2017 · 8 comments
Open
Assignees

Comments

@tsimbalar
Copy link
Contributor

tsimbalar commented Oct 23, 2017

Based on earlier discussions :

And prototypes from:

@MV10
Copy link

MV10 commented Aug 28, 2018

I've read the linked threads but I'm not seeing how this is any different than what is already available from the huge selection of Microsoft.Extensions.Settings.XYZ packages. They combine into one giant final config. Was this maybe a plan to do the same thing with the older Framework XML config?

Or did I just catch up to an out-of-date experiment?

@tsimbalar
Copy link
Contributor Author

I have failed to keep working on this U_U.

The goal was to be able to express configuration both in C# code and in config files, in a way that a single sink can be configured half in code / half in config file, which is not currently possible (i.e. you cannot override a method call param used in C# from a config file).

The idea was to merge together in a given order :

  • C# code based on expressions
  • appSettings/xml config
  • other like Microsoft.Settings*

Let's see if I find time to properly polish it :P

@MV10
Copy link

MV10 commented Aug 30, 2018

Interesting. I'd like to hear more about the planned approach.

On a possibly-related note, I've been trying to reason my way through something I think of as a unified config execution system. Once any given config source is parsed, the reflection and invocation processes ought to be more or less the same. We already have some deviation between .NET Framework and .NET Core (for example, serilog/serilog-settings-configuration#122 (comment)). With a standardized base, config loaders would work sort of like a plug-in stack -- more of the "builder" pattern, really.

It sounds as if the goal here is similar. It would be nice to figure this out without reinventing several flavors of config-ingestion.

The other possibly-related thing I've been considering is automated config reloading. In .NET Core config there is the "options pattern" which semi-automatically reloads config changes. However, Microsoft does a poor job clarifying this is highly dependent on injection of scoped services (config is reloaded for each scope-change). In both Framework and Core it's possible to subscribe to file system changes, and environment reads are fast enough they could be tested at regular intervals. I could see that as part of a core config execution layer as well.

Is any of this making sense in the context of the work here, or is it too far afield?

(I leave for vacation today, so I'm still interested even if I disappear for a week or so.)

@tsimbalar
Copy link
Contributor Author

Basically, it's kind of a mixture of :

  • it's painful to have to maintain both appSettings and Configuration
  • Microsoft.Configuration.* is nice, but there are a few caveats and we may not want to take a hard dependency on it
  • we already have ReadFrom.KeyValuePairs that exposes key-value settings in a format that is quite specific to Serilog
  • there is a need for hybrid config : define config in code ... but allow to override some params of what are really methods calls in the end ...

The general idea was therefore to have all "Settings providers" end up generating key-value pairs in the Serilog-recognized format ... Possible providers being :

  • appSettings / XML
  • appSettings.json / Microsoft.Configuration.*
  • actual method calls (through ExpressionTrees)

and then being to merge all those, the way ConfigurationBuilders do it , in a last one wins sort of style.

There have been discussions about not wanting to take on a dependency on Microsoft.Config here : serilog/serilog#1038 (comment)

So yes, standardization (see https://xkcd.com/927/) is part of the idea. The config reloading is not really the focus here though :-)

For someone who is on vacation, I find you extremely involved in GitHub issues and PRs ;)

@MV10
Copy link

MV10 commented Sep 5, 2018

For someone who is on vacation, I find you extremely involved in GitHub issues and PRs ;)

Haha, my wife felt the same way...

Thanks for the clarification, and the example provided by this test looks very cool.

https://github.com/serilog/serilog-settings-combined/blob/dev/test/Serilog.Settings.Combined.Tests/CombinedSettingsMixTests.cs#L23

I wasn't really suggesting a hard dependency on M.E.C but rather pointing out the flexibility in that system. I actually wound up here because I was searching for ReadFrom.KeyValuePairs syntax (I've only used it for unit tests) and landed on one of the threads that sparked this repo. But yes, that's the same concept.

Even though this is very cool, it gives me the feeling of going to extreme measures to service a simple-sounding requirement. I know this would be waaaay out of scope for this repo but just thinking out loud, one of the other M.E.C conventions that might be nice to borrow (some day... Serilog v3...) is config by POCOs. Again, not proposing any dependencies, just the concept. Then config is relatively trivial. But I know that would amount to completely overhauling configuration.

Definitely watching with interest! In particular the SQL sink has a bunch of config points that probably never change that could benefit by being baked into code (table definition...).

@MV10
Copy link

MV10 commented Sep 7, 2018

The general idea was therefore to have all "Settings providers" end up generating key-value pairs in the Serilog-recognized format

Definitely sold on the concept at this point (especially if your idea in serilog/serilog#1178 happens), but I'm still The New Guy around here and haven't seen how big ideas get set into motion. Would such a significant change to the settings pipeline and providers get serious consideration?

I ask because I'd love to tackle parts of this now that I have time to focus on Serilog again. I was already lamenting that the various settings packages are left to implement the reflect-and-invoke process internally. Hindsight being 20/20, I think the ideal scenario is what you've described -- settings packages (including this one) ingest and translate various config sources to the baseline KVP format -- then Serilog itself invokes the resulting combined KVP list. Is that last part also under consideration, by any chance?

It's sort of ironic that you mentioned avoiding hard M.E.C dependencies given that I'd recently created some in the SQL sink to deal with the many complexities of the ColumnOptions class (of course, the .NET Framework variant has equivalent appSettings XML dependencies). Once everything clicked about how KVP could represent a baseline format I realized the headaches which come with that multi-targeting split would just go away.

It's also a little ironic that the SQL sink has a miniature version of combined settings: you can pass a ColumnOptions object in code, and simultaneously pass an IConfigurationSection representing ColumnOptions and the config will overlay anything in the code version. (Of course, this is still config-in-code in some sense because you have to pass the config collection yourself in the same call.)

I mention all this stuff about the SQL sink's use of ColumnOptions because the arg-matching and invocation step of any final implementation of these ideas probably needs a way to populate a LoggerConfiguration argument with either a subset of the KVPs (equivalent to a config section in M.E.C) or at a minimum, a reference to the entire KVP list. Actually, the SQL sink has use-cases for both: the entire config is useful to resolve connection strings by name stored outside of the Serilog section. There are similar questions I've fielded about complex parameters on the App Insights sink.

I know that part is a bit further out than you're probably looking right now, but this seems like a good place to mention a concern that crosses many boundaries in the Serilog ecosystem.

@lloydjatkinson
Copy link

Is this still being looked into?

@nblumhardt
Copy link
Contributor

Not currently - would still be very interesting to look into, but low on bandwidth :-)

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

No branches or pull requests

4 participants