search

serilog-contrib / serilog-sinks-elasticsearch

A Serilog sink that writes events to Elasticsearch
Apache License 2.0
434 stars 197 forks source link

FailureSink change #334

Open zewar96 opened 4 years ago

zewar96 commented 4 years ago

Does this issue relate to a new feature or an existing bug?

What version of Serilog.Sinks.Elasticsearch is affected? Please list the related NuGet package. 8.1.0

What is the target framework and operating system? See target frameworks & net standard matrix.

Please describe the current behavior? Currently LoggerConfigurationElasticSearchExtensions.cs lists FailureSink as ILogEventSink. This prevents being able to create a failover logger from appSettings.json.

Please describe the expected behavior? In looking at how Serilog.Sinks.Async works, wouldn't it make more sense to define FailoverSink as Action FailureSink instead so that the configuration via appSettings would work the same way?

This is how https://github.com/serilog/serilog-settings-configuration#nested-configuration-sections recommends that we use nested configurations.

Open for any other ideas on how to load the FailureSink via config, but the current implementation does not work well from configurations

mivano commented 4 years ago

A better alignment with the configuration system would be much appreciated. would be a breaking change I think, so we should bump major and document this well. Care for a PR?

Kamil-Zakiev commented 3 years ago

I've got issues setting up a failure sink from appsettings.json since it requires a class name but I want to specify default console sink, as @zewar96 suggests