Skip to content

Updated SwqlStudio.csproj to use Serilog for logging#275

Open
danjagnow wants to merge 5 commits intomasterfrom
feature/logging
Open

Updated SwqlStudio.csproj to use Serilog for logging#275
danjagnow wants to merge 5 commits intomasterfrom
feature/logging

Conversation

@danjagnow
Copy link
Copy Markdown
Contributor

@danjagnow danjagnow commented Mar 31, 2021

Updated SwqlStudio.csproj to eliminate the SolarWinds.Logging dependency (and a transitive dependency on log4net. It is configured instead to use Microsoft.Extensions.Logging and Serilog for logging. The log file path is no longer configurable via the App.config file in this approach. Updated the installer to add the new dependencies, including transitive dependencies. The SolarWinds.Logging and log4net assemblies are still deployed since SolarWinds.InformationService.Contract still depends on them. This is not ready for merging.

@danjagnow danjagnow self-assigned this Mar 31, 2021
@danjagnow
Copy link
Copy Markdown
Contributor Author

This PR is just a request for comment. Switching up the logging like this makes it easier to swap out logging approaches in the future, and it would allow us to remove the SolarWinds.Logging abstraction, but it also means a larger MSI installer because of the many transitive dependencies involved. If this approach makes sense, we'd want to remove SolarWinds.Logging and log4net uses throughout the solution before merging anything. If not, we should just close this PR.

internal partial class MainForm : Form, IApplicationService
{
private static readonly SolarWinds.Logging.Log log = new SolarWinds.Logging.Log();
private static readonly ILogger<MainForm> log = Program.LoggerFactory.CreateLogger<MainForm>();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The absence of true dependency injection makes these initializations a bit uglier than they would otherwise be.

@tdanner
Copy link
Copy Markdown
Contributor

tdanner commented Mar 31, 2021

This seems reasonable to me. The extra DLLs are a mild nuisance, but not a big deal.

@danjagnow
Copy link
Copy Markdown
Contributor Author

This seems reasonable to me. The extra DLLs are a mild nuisance, but not a big deal.

OK, let me do a little work and turn this into a real PR, then.

Updated SwqlStudio.csproj to eliminate the SolarWinds.Logging dependency (and a transitive dependency on log4net.  It is configured instead to use Microsoft.Extensions.Logging and Serilog for logging.  The log file path is no longer configurable via the App.config file in this approach.  Updated the installer to add the new dependencies, including transitive dependencies.  The SolarWinds.Logging and log4net assemblies are still deployed since SolarWinds.InformationService.Contract still depends on them.  This is not ready for merging.
Removed SolarWinds.Logging references from SwisPowerShell.  It was only referenced in the CustomCertificateValidator class for a single log entry, and logging was never configured.
Updated SolarWinds.InformationService.Contract to use Microsoft.Extensions.Logging instead of SolarWinds.Logging.  Updated classes in SwqlStudio and SwisPowerShell accordingly.  Note that SwisPowerShell uses null loggers after this change.
Removed SolarWinds.Logging.csproj from the solution.
Updated the installer, removing SolarWinds.Logging.dll and log4net.dll and adding the new dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants