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

Extension method specially designed for JSON #53

Closed
FantasticFiasco opened this issue Apr 22, 2017 · 7 comments
Closed

Extension method specially designed for JSON #53

FantasticFiasco opened this issue Apr 22, 2017 · 7 comments

Comments

@FantasticFiasco
Copy link
Contributor

I realize that having Serilog.Settings.Configuration to depend on the extension methods of the sinks is extremely clever, since out of the gate you will have support for almost all sink already published.

But building it on the extension methods can also, at least for me as a sink author, be fragile. I was not aware of Serilog.Settings.Configuration, and had no idea that the argument names of my extension methods where of such importance, and renaming one would be a breaking change.

I would like to propose a solution where sink authors could opt in for providing a configuration method that was specially designed for the JSON configuration. Is that something other sink authors have requested?

This new configuration method would get the JSON relevant to the sink in the form of a IConfigurationSection or similar, and handle the parsing themselves. That way I would be able define a good API both for those that configure the sink using C# and those that prefer JSON.

As an example, one of the arguments to the extension method creating the HTTP sink allows the consumer to specify an implementation on a HttpClient. That is fine in C#, but more problematic in JSON. I think I would have a better chance at creating a good JSON API if it was handled separately from the C# configuration.

@nblumhardt
Copy link
Member

Thanks for the feedback!

allows the consumer to specify an implementation on a HttpClient.

Specifying the type name will work here - the provider translates type names to Types for these cases.

argument names of my extension methods where of such importance, and renaming one would be a breaking change

This is already a breaking change in C#, since they can be provided by name:

    .WriteTo.SomeSink(foo: "bar")

sink authors could opt in for providing a configuration method that was specially designed for the JSON configuration. Is that something other sink authors have requested?

Not as yet, but this ticket could be a conduit for some discussion around this 👍

@FantasticFiasco
Copy link
Contributor Author

Specifying the type name will work here - the provider translates type names to Types for these cases.

Yes, but I wouldn't wan't to expose this to the consumers using the C# API, it would serve no benefit other than supporting JSON.

This is already a breaking change in C#, since they can be provided by name

I guess you are right about that

@FantasticFiasco
Copy link
Contributor Author

Closing it due to lack of interest.

@zebrakot
Copy link

zebrakot commented May 2, 2018

There is definitely interest. Is there a way currently to configure Stackify sink in Serilog's json configuration file/section?

@MV10
Copy link
Contributor

MV10 commented May 2, 2018

@FantasticFiasco @zebrakot

Good news! The feature described below is possible as of PR#100. It is currently available in the pre-release (dev) NuGet package.

I would like to propose a solution where sink authors could opt in for providing a configuration method that was specially designed for the JSON configuration. Is that something other sink authors have requested?

This new configuration method would get the JSON relevant to the sink in the form of a IConfigurationSection or similar, and handle the parsing themselves. That way I would be able define a good API both for those that configure the sink using C# and those that prefer JSON.

Sink authors can declare an optional IConfiguration argument or IConfigurationSection argument. (An example of where the full configuration would be useful is reading a separate ConnectionsStrings section to retrieve a named connection string.) I'll be using both of these in an upcoming PR on the SQL Server sink, in fact.

@FantasticFiasco
Copy link
Contributor Author

With this PR, is it possible for me to create a new extensions method with only the IConfiguration/IConfigurationSection argument, and Serilog.Settings.Configuration would pick that extension method when creating the sink instead of the already existing extension methods?

Or is this more of a complementary thing in that consumers can provide configuration in the optional argument, configuration that for some reason isn't described by any of the other arguments in the extension method?

@MV10
Copy link
Contributor

MV10 commented May 3, 2018

@FantasticFiasco Yes, that would work. Of course, since this is all open source, you could also just update the sinks of interest (as I'm doing with SQL)...

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

4 participants