Sql Connection factory #609
Conversation
Signed-off-by: Nathan Hart <nhart12@gmail.com>
nhart12
force-pushed
the
sqlConnectionFactory
branch
from
f83de79 to
adf1543
Compare
| /// <param name="logEventFormatter">Supplies custom formatter for the LogEvent column, or null</param> | ||
| /// <returns>Logger configuration, allowing configuration to continue.</returns> | ||
| /// <exception cref="ArgumentNullException">A required parameter is null.</exception> | ||
| public static LoggerConfiguration MSSqlServer( |
There was a problem hiding this comment.
Maybe just add the func parameter to the existing constructor since we already have a number of constructors here (some obsolete even).
| IConfigurationSection columnOptionsSection, | ||
| IApplySystemConfiguration applySystemConfiguration, | ||
| IApplyMicrosoftExtensionsConfiguration applyMicrosoftExtensionsConfiguration) | ||
| { |
There was a problem hiding this comment.
Maybe rather check the connection string for null instead of having another overload of the method which is almost identical.
| public static LoggerConfiguration MSSqlServer( | ||
| this LoggerSinkConfiguration loggerConfiguration, | ||
| Func<SqlConnection> sqlConnectionFactory, | ||
| string initialCatalog, |
There was a problem hiding this comment.
Again: add to existing ctor with default null instead of a new ctor.
|
Thank you for the PR. I made some comments about adding new constructors. I'm yet not sure how to handle this. You added new constructors/methods as overloads with the new Func as a parameter but at some spots we already have a lot of differnt constructors and some are marked obsolete. So I tend to not adding new ones and rather add the Func as a default null parameter. The most important point is another thing though. I just learned about a new issue causing exceptions under heavy load (#607). I'd really like to fix that first before going forward with the PR. Generally I think your suggestion is a useful feature for the sink and we should add it. :) |
|
I thought about the new parameter and think it would be best to add it to the MSSqlServerSinkOptions class. This way we could remain binary compatible to older versions and not introduce yet another set of constructor overloads. One thing that speaks against adding it to the options class is, that this class has currently only configuration data which can also be read using the .NET configuration APIs (e.g. from appsettings.json). For the connection factory this would not be the case. It can of course only be set programatically. But I think that should not stop us from adding it to options. The options class should be the primary location for new sink parameters. |
|
The fix of issue #607 is implemented, so you can rebase and proceed with this PR. |
|
Thank you. I agree with your comments regarding the additional constructors.. Instead of rebasing this one I made another pr that is hopefully a bit simpler, just extends the existing options like you said to optionally allow providing an Action that can customize the SqlConnection on creation (instead of a function which would entirely take over creation) |
Idea for
#608