Configuration in sink is incompatible with Azure Functions V2

[ObadahFahmy]

I have been developing an Azure Function on the V2 Functions library, the function uses our internal logging library (built off of serilog and this sink specifically). I've run into an issue where initializing the log causes a ConfigurationErrorsException to be thrown. I traced the exception down to this issue in the Azure Functions CLI Repo.

A comment on the Issue says "In 2.0, we have moved to the new ASP.NET Core configuration model. We intend to support binding to an IConfiguration instance that will provide a similar API, but in the meantime, the workaround is to read environment variables as opposed to relying on the ConfigurationManager and its APIs."

Looking through the source code I can see a few areas where the Configuration Manager is being used that would throw an exception during initialization they are as follows:

• LoggerConfigurationMSSqlServerExtensions.cs Line 67-68
  MSSqlServerConfigurationSection serviceConfigSection = ConfigurationManager.GetSection("MSSqlServerSettingsSection") as MSSqlServerConfigurationSection;
• LoggerConfigurationMSSqlServerExtensions.cs Line 80, 109 connectionString = GetConnectionString(connectionString);
var cs = ConfigurationManager.ConnectionStrings[nameOrConnectionString];

At present the library is un-usable in V2 functions.

[nblumhardt]

Thanks for the note 👍

Ideally, we should drop any references to ConfigurationManager from the .NET Core build of the sink - perhaps by multi-targeting we can conditionally compile these out?

Any help/PRs appreciated. Cheers!

[MV10]

I may tackle this one, it just became a blocking issue migrating our V1 Functions to V2.

(Edit: I deleted an earlier comment stating that Functions V1 were dependent on Microsoft.Extensions.Configuration 1.0.0 ... that was incorrect.)

[MV10]

@nblumhardt I've had a chance to go through the code in detail. Multi-targeting is going to be a challenge because ConfigurationManager assumptions and dependencies are pretty extensive. As you probably realize, configuration makes up most of this project's codebase.

Inline conditionals would generally result in two complete side-by-side copies of a couple of top-level classes that could be used by both targets, and it would require quite a few supporting classes that would be completely target-specific (for both targets). Additionally, given that the preferred M.E.Config 2 approach is strongly-typed settings via IOptions<T>, an M.E.Config update amounts to a rewrite (admittedly not a huge chore given the relative simplicity of the project).

Since inline conditionals would be so messy, my first inclination was to restructure the project with separate sets of folders specific to each target, but that's a pretty major overhaul (especially considering git considers file-moves as delete/add changes that breaks change-history relationships). I'm still willing to tackle it, but it may actually be preferable (and certainly easier) to start a whole new .NET Core -friendly version and leave this one as a .NET Framework legacy project.

Not to editorialize too much, but MS has clearly turned their backs on static-style libraries like ConfigurationManager -- if you ask me, it's pretty much obsolete, even if MS isn't willing to freak out their enterprise customers by openly deprecating the .NET Framework... yet.

Which way would you rather see this go?

If you agree that a new project is appropriate, maybe you set up a new repo and structure, then I'll fork from there?

[nblumhardt]

@MV10 thanks for the details 👍

We've so far found it a pretty significant challenge to keep this sink functioning, since most of the Serilog contributors seem to use purpose-built log storage and not SQL Server. While I agree that a clean start would be nice, I am not sure about the chances of a second MSSQL sink getting enough maintenance attention to be viable, unless you're making long-term investments yourself in logging to SQL Server?

I guess my initial thoughts tend towards preferring the refactoring, but only because of the maintenance problem. What do you think?

[MV10]

@nblumhardt If you're more comfortable with refactoring (and ok losing the git history), I'll proceed with those changes.

I just deleted a long reply -- my only real concern is that I think ConfigurationManager is a dead-end, but ultimately I also think a lot of .NET Standard / Core libraries will end up multi-targeted so .NET Framework as one-target-among-many is probably not overhead worth worrying about (that part of the change is easy to set up).

In terms of maintenance, there is a very good chance I'm aboard for the long-haul. Personally I have a startup effort that will generate lots of SQL logging, and I'm sold on Serilog to the point that anything else we start will use it. Plus my day job is Solution Architect for a very (very) large financial firm and I intend to try pushing our aging log4net infrastructure to Serilog. (I'm actually surprised the SQL sink isn't more popular with the enterprise... we have billions of rows logged in SQL via log4net today.)

[nblumhardt]

Awesome, sounds good @MV10 - will be fantastic having some help to get this sink into better shape!

I agree with your assessment of ConfigurationManager's bleak future, but it also will be around a long time, and will keep getting use in large organisations, so worth keeping support here if possible.

I didn't mean to imply that the MSSQL sink is infrequently used - it's been downloaded a quarter of a million times - it's just that among the more active Serilog contributors, it's not been a frequent choice so far.

we have billions of rows logged in SQL

Yikes! :-)

[MV10]

@nblumhardt The code is done (though I have more testing to do) in the sense that it compiles and the tests all pass, but there's a design issue I'm not comfortable about.

I changed the LoggerConfiguration extension to accept an optional IConfiguration parameter. It uses that to look for connection strings and to add any custom columns, just like the ConfigurationManager version does.

Before I rolled up my sleeves to tackle testing, I decided to take a break and consider changes to the README file. The ReadFrom.AppSettings() section is really only applicable to .NET Framework and ConfigurationManager, and as you know, I just finished updating ReadFrom.Configuration() to reference M.E.C v2, so I started adding a section about that for .NET Standard 2 libraries and .NET Core apps.

I realized there is a problem. Since a sink can only be configured once, by definition configuration-by-code and calling ReadFrom.Configuration() are mutually exclusive. This means using M.E.C v2 with ReadFrom.Configuration() makes it impossible to specify custom columns in the SQL sink.

In the static ConfigurationManager world, there is no flexibility regarding the configuration source: it's always XML from either app.config or web.config, period. And regardless of whether ReadFrom.AppSettings() or config-by-code calls the LoggerConfiguration extension to initialize the sink, the static ConfigurationManager is still always available and can be used to read any custom column information.

But IConfiguration isn't "universally available" in the static sense, and ReadFrom.Configuration() can only populate the initialization parameters defined in the WriteTo settings. There's no way to tell it to populate the IConfiguration parameter so that the sink init code has an opportunity to check for custom columns.

I suspect this will become a recurring theme as other sinks migrate to Microsoft.Extensions.Configuration support (unless adding config-based features on the fly is uncommon, I don't have broad experience with the various sinks).

My first inclination is to suggest poor-man's DI by modifying Serilog.Settings.Configuration to automatically populate an IConfiguration parameter whenever one exists. It feels like a hack, but I suppose it might be a legit feature since that package is entirely M.E.C-oriented. I took a quick peek at the code but there's a lot going on there (my gut feel is that it would be a big change to CallConfigurationMethods ... if the idea makes sense to anyone else in the first place).

I hope all that makes sense. I'm open to suggestions.

[MV10]

My proposed fix above turns out to be slightly more complicated than I thought last night, but I think it will work.

Related: serilog/serilog-settings-configuration#97

[jducobu]

@MV10,

Is this pakage contains these changes ? Otherwise, please resend a new nuget package ?

[MV10]

@jducobu I think it does. I don't have any control over the packaging but I think the dev packages are generated automatically.

@nblumhardt Nick is that right?

[jducobu]

On my experience, only the main branch is packaged in alpha, beta version ...
I wait the response of @nblumhardt.

[nblumhardt]

Hi- looks like the version info in the dev branch is incorrect - 5.1.2-dev-00201, released 21 days ago, should be a 5.1.3-* build. Fixing now 👍

[jducobu]

Thanks @nblumhardt