Skip to content
This repository has been archived by the owner on Jun 1, 2024. It is now read-only.

Extend LoggerConfiguration.Elasticsearch with additional attributes #150

Merged
merged 2 commits into from
Feb 25, 2018

Conversation

letargis
Copy link

What issue does this PR address?
#149

Does this PR introduce a breaking change?
No

@mivano
Copy link
Contributor

mivano commented Feb 1, 2018

Thanks. Adding parameters, even optional, is a breaking change. And although I see why you want this, I m a bit afraid we are making this method longer and longer while we actually introduced an object to avoid this.
@nblumhardt any tips how to handle this? I see more request for actually wanting to manage the config of the sink in the appSetting and adding the options to the method seems to be the only way.

@letargis
Copy link
Author

letargis commented Feb 6, 2018

So, @mivano, @nblumhardt what will be the conclusion?
For me it looks, that LoggerConfiguration.Elasticsearch method with multiple parameters has the only purpose to support appSettings configuration. So it is natural to extend it with all parameters, which could be configured via config file.
Otherwise, I should use LoggerConfiguration.Elasticsearch with object parameter and manage settings myself. This is strange, because Elasticsearch sink supports appConfig settings.

@mivano
Copy link
Contributor

mivano commented Feb 10, 2018

@nblumhardt any tips/suggestions for this?

@nblumhardt
Copy link
Contributor

Howdy! In the past, where we've had to do this, the process has been:

  1. Create a new overload with all of the existing parameters (excluding any obsolete ones) plus the new parameters
  2. Make the old method forward to the new one (i.e. implement it by calling new method)
  3. Since this still won't play nicely with overload resolution:
    • Remove all of the default values from the old method
    • Mark the old method as [EditorBrowsable(EditorBrowsableState.Never)]
  4. If there's no valid scenario in which every single parameter on the old method could be validly supplied (i.e. some are mutually-exclusive), [Obsolete] the old method

This should avoid all source and binary breaking changes. It's at the cost of keeping a method with the old signature around, but it's proven to be unobtrusive so far.

See, for example: https://github.com/serilog/serilog-sinks-file/blob/dev/src/Serilog.Sinks.File/FileLoggerConfigurationExtensions.cs#L59

HTH, let me know if you need more details.

@letargis
Copy link
Author

I've created new method and modified old one as suggested by @nblumhardt . Moreover, I've added some additional parameters which could be configured via config file.

@mivano mivano merged commit a30be1a into serilog-contrib:dev Feb 25, 2018
@mivano
Copy link
Contributor

mivano commented Feb 25, 2018

Sorry, it took a while but merged now. Thanks for the PR!

@letargis letargis deleted the extend-appsettings branch February 26, 2018 14:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants