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

Read from IConfiguration without SectionKey #289

Closed
krafs opened this issue Nov 22, 2021 · 11 comments · Fixed by #362
Closed

Read from IConfiguration without SectionKey #289

krafs opened this issue Nov 22, 2021 · 11 comments · Fixed by #362

Comments

@krafs
Copy link

krafs commented Nov 22, 2021

I'm using Serilog.Settings.Configuration to configure Serilog, and I came across an issue.
If I have an IConfiguration that consists of the Serilog-section, how can I configure Serilog with it?
ConfigurationLoggerConfigurationExtensions.ConfigurationSection only takes an IConfigurationSection, whilst I have an IConfiguration. And even if I did have a section - it's marked as obsolete.
ConfigurationLoggerConfigurationExtensions.Configuration takes an IConfiguration, but it requires an IConfiguration that is one step back, so it can use the sectionKey to get into it.

Now, I understand that you can't just add an overload to the abovementioned method, because an overload without the sectionkey already defaults to the key being "Serilog".
Or is there perhaps some other way that I've missed? Apart from "just provide an IConfiguration that isn't already zoomed in to the Serilog section" :)

@nblumhardt
Copy link
Member

I had a quick look and can't spot a way to achieve this currently. Whipping up your own IConfiguration implementation that wraps the one you're holding could be a quick unblocker, though.

Longer term we might look at accepting a null section name - but, it's a bit of a niche case, so not entirely likely we'd add this.

Any more info about why/how you're hitting this requirement? Thanks!

@krafs
Copy link
Author

krafs commented Nov 25, 2021

I've inherited a code base where IConfigurations are distributed already-zoomed-into.
Yeah, I've considered making my own IConfiguration-wrapper for stepping back a step into the outer config section. Definitely possible, but would prefer a native solution.
It's probably also possible to get ahold of a different IConfiguration in my specific case.

Still, is there a reason I'm not able to give Serilog exactly what it needs? It doesn't feel right to provide it with the entire outer configuration block. And if there are reasons, why is that the default?

@nblumhardt
Copy link
Member

@krafs thanks for the reply. Perhaps you're right - it could just be historical accident at this point.

If you're interested in investigating further and putting a PR together, it seems reasonable to take a closer look 👍

@skomis-mm
Copy link
Contributor

skomis-mm commented Nov 26, 2021

@krafs , @nblumhardt the reason why IConfigurationSection parameters were obsoleted is to support IConfiguration sinks arguments, such as Serilog.Sinks.MSSqlServer's (in order to get root ConnectionStrings section etc). See #148 and related PRs/issues.

This is why root configuration is needed, so it is probably "by design" issue.

@nblumhardt
Copy link
Member

Thanks for the note @skomis-mm!

I think where this gets confusing is that IConfigurationSection implements IConfiguration anyway, so the API we have here doesn't really enforce the kind of usage that the MSSQL sink requires (one can still pass a nested IConfiguration to ReadFrom.Configuration(), as long as it has a sub-key for Serilog, but it doesn't have to be the root).

Unless I'm misunderstanding the situation, I think the desire to pass both the root configuration and the one to read from as the same argument might be problematic - going back in time, would we consider separating them to allow something like this?

    .ReadFrom.Configuration(configuration.GetSection("MySerilog"), root: configuration)

where the second root parameter would be optional and really only required for MSSQL. 🤔

@skomis-mm
Copy link
Contributor

I think where this gets confusing is that IConfigurationSection implements IConfiguration anyway, so the API we have here doesn't really enforce the kind of usage that the MSSQL sink requires (one can still pass a nested IConfiguration to ReadFrom.Configuration()

Indeed.. 🤔

I beleive the intent was to have overloads of .ReadFrom.Configuration(..) accepting only IConfigurationRoot type. But IConfigurationRoot is not UX friendly because of need to cast to that type in asp.net/generic host scenarios (HostBuilderContext/WebHostBuilderContext expose IConfiguration type). Because of this IConfiguration is used like a root configuration.

.ReadFrom.Configuration(configuration.GetSection("MySerilog"), root: configuration)

Looks good to me. 👍
Probably move it to .ReadFrom.ConfigurationSection(..) overloads?
And since this is a new overload, we can accept IConfiguration as root parameter and add runtime check if it is really IConfiguratonRoot.

Thoughts?

@nblumhardt
Copy link
Member

Seems like a reasonable set of trade-offs 👍

@krafs
Copy link
Author

krafs commented Dec 3, 2021

I feel like the best fit would have been as an overload for .ReadFrom.Configuration, but seeing as the preferred signature would look something like .ReadFrom.Configuration(IConfiguration serilogConfig, IConfiguration root = null) that won't work, as it would conflict with existing overloads.

If we do add it to .ReadFrom.ConfigurationSection, how about using IConfiguration instead of IConfigurationSection? So a signature like e.g. .ReadFrom.ConfigurationSection(IConfiguration configuration)?

@nblumhardt
Copy link
Member

I'm not 100% sure it would actually conflict - if we just add the IConfiguration root = null argument to the existing method, the calls would look like:

    // Section name defaults to "Serilog", argument is assumed to be at the root, everything
    // works as it does today
    .ReadFrom.Configuration(configuration)

    // Passed-in configuration is the Serilog section, root not available
    .ReadFrom.Configuration(configuration, sectionName: null)

    // Passed-in configuration is the Serilog section, root supplied for MSSQL and any other
    // sinks that need it (probably quite uncommon)
    .ReadFrom.Configuration(configuration, sectionName: null, root: ...)

Passing a null sectionName is slightly strange but not unprecedented 🤔

@krafs
Copy link
Author

krafs commented Dec 8, 2021

Ah, right. Today, all overloads throw ArgumentNullException if the provided sectionName is null. But changing null to instead indicate that the given IConfiguration is the serilog section would work. As you say - slightly strange, but seems perfectly fine given the circumstances :)

I was going to suggest making sectionName nullable, but seems like the project doesn't have nullable reference types enabled anyway, so I guess that's not an issue.

@skomis-mm
Copy link
Contributor

👍 I'll come up with something based on your suggestions

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

Successfully merging a pull request may close this issue.

3 participants