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

Refactoring and Performance Optimizations #598

Merged
merged 6 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,24 @@ csharp_style_var_for_built_in_types = true:warning
csharp_style_var_when_type_is_apparent = true:warning
csharp_using_directive_placement = outside_namespace:warning

# Naming rules for non-public fields
dotnet_naming_rule.private_members_with_underscore.symbols = private_fields
dotnet_naming_rule.private_members_with_underscore.style = prefix_underscore
dotnet_naming_rule.private_members_with_underscore.severity = error
dotnet_naming_symbols.private_fields.applicable_kinds = field
dotnet_naming_symbols.private_fields.applicable_accessibilities = private,protected
dotnet_naming_symbols.private_fields.applicable_accessibilities = private, protected
dotnet_naming_style.prefix_underscore.capitalization = camel_case
dotnet_naming_style.prefix_underscore.required_prefix = _

# Naming rules for const or static public fields
dotnet_naming_rule.public_fields_pascal_case.symbols = public_constant_static_fields
dotnet_naming_rule.public_fields_pascal_case.style = pascal_case
dotnet_naming_rule.public_fields_pascal_case.severity = error
dotnet_naming_symbols.public_constant_static_fields.applicable_kinds = field
dotnet_naming_symbols.public_constant_static_fields.applicable_accessibilities = public
dotnet_naming_symbols.public_constant_static_fields.required_modifiers = const, static
dotnet_naming_style.pascal_case.capitalization = pascal_case

dotnet_sort_system_directives_first = true
dotnet_style_require_accessibility_modifiers = always:error

9 changes: 5 additions & 4 deletions .github/ISSUE_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,18 @@ If you are opening a feature request, you can ignore this template. Bug reports

>> List the names and versions of all Serilog packages used in the project:

- Serilog:
- Serilog.Sinks.MSSqlServer:
- Serilog:
- Serilog.Sinks.MSSqlServer:
- (configuration, etc.)

>> Target framework and operating system:

[ ] .NET 6
[ ] .NET 9
[ ] .NET 8
[ ] .NET Framework 4.8
[ ] .NET Framework 4.7
[ ] .NET Framework 4.6
OS:
OS:

>> Provide a *simple* reproduction of your Serilog configuration code:

Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ jobs:
run: ./RunPerfTests.ps1 -Filter "*QuickBenchmarks*"
shell: pwsh

- name: Upload perf test results artifact
uses: actions/upload-artifact@v4
with:
name: perftestresults
path: artifacts\perftests

- name: Get last commit message
id: last_commit
if: success() && github.ref == 'refs/heads/main'
Expand Down
7 changes: 7 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
# 8.0.1
* Refactoring and performance optimizations in batched and audit sink
* Create perftest result on release
* Updated issue template
* Updated editorconfig
* Added specific documentation about when SQL SELECT permission is not required

# 8.0.0
* Updated to .NET 8
* Updated nearly all dependencies
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@ CREATE TABLE [Logs] (

At a minimum, writing log entries requires SELECT and INSERT permissions for the log table. (SELECT is required because the sink's batching behavior uses bulk inserts which reads the schema before the write operations begin).

If the audit version of the sink is used or the sink option [UseSqlBulkCopy](#usesqlbulkcopy) is set to `true`, only INSERT statements are used and no SELECT permission is required.

SQL permissions are a very complex subject. Here is an example of one possible solution (valid for SQL 2012 or later):

```
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ namespace Serilog.Sinks.MSSqlServer.Dependencies
{
internal class SinkDependencies
{
public IDataTableCreator DataTableCreator { get; set; }
public ISqlCommandExecutor SqlDatabaseCreator { get; set; }
public ISqlCommandExecutor SqlTableCreator { get; set; }
public ISqlBulkBatchWriter SqlBulkBatchWriter { get; set; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ internal static SinkDependencies Create(
var sqlConnectionStringBuilderWrapper = new SqlConnectionStringBuilderWrapper(
connectionString, sinkOptions.EnlistInTransaction);
var sqlConnectionFactory = new SqlConnectionFactory(sqlConnectionStringBuilderWrapper);
var sqlCommandFactory = new SqlCommandFactory();
var dataTableCreator = new DataTableCreator(sinkOptions.TableName, columnOptions);
var sqlCreateTableWriter = new SqlCreateTableWriter(sinkOptions.SchemaName,
sinkOptions.TableName, columnOptions, dataTableCreator);
Expand All @@ -47,21 +48,16 @@ internal static SinkDependencies Create(

var sinkDependencies = new SinkDependencies
{
DataTableCreator = dataTableCreator,
SqlDatabaseCreator = new SqlDatabaseCreator(
sqlCreateDatabaseWriter, sqlConnectionFactoryNoDb),
sqlCreateDatabaseWriter, sqlConnectionFactoryNoDb, sqlCommandFactory),
SqlTableCreator = new SqlTableCreator(
sqlCreateTableWriter, sqlConnectionFactory),
SqlBulkBatchWriter = sinkOptions.UseSqlBulkCopy
? (ISqlBulkBatchWriter)new SqlBulkBatchWriter(
sinkOptions.TableName, sinkOptions.SchemaName, columnOptions.DisableTriggers,
sqlConnectionFactory, logEventDataGenerator)
: (ISqlBulkBatchWriter)new SqlInsertStatementWriter(
sinkOptions.TableName, sinkOptions.SchemaName,
sqlConnectionFactory, logEventDataGenerator),
sqlCreateTableWriter, sqlConnectionFactory, sqlCommandFactory),
SqlBulkBatchWriter = new SqlBulkBatchWriter(
sinkOptions.TableName, sinkOptions.SchemaName, columnOptions.DisableTriggers,
dataTableCreator, sqlConnectionFactory, logEventDataGenerator),
SqlLogEventWriter = new SqlInsertStatementWriter(
sinkOptions.TableName, sinkOptions.SchemaName,
sqlConnectionFactory, logEventDataGenerator)
sqlConnectionFactory, sqlCommandFactory, logEventDataGenerator)
};

return sinkDependencies;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// Copyright 2024 Serilog Contributors
//
// Copyright 2024 Serilog Contributors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//
// http://www.apache.org/licenses/LICENSE-2.0
//
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
Expand All @@ -30,11 +30,13 @@ public class MSSqlServerAuditSink : ILogEventSink, IDisposable
{
private readonly ISqlLogEventWriter _sqlLogEventWriter;

private bool _disposedValue;

/// <summary>
/// Construct a sink posting to the specified database.
///
/// Note: this is the legacy version of the extension method. Please use the new one using MSSqlServerSinkOptions instead.
///
///
/// </summary>
/// <param name="connectionString">Connection string to access the database.</param>
/// <param name="tableName">Name of the table to store the data in.</param>
Expand Down Expand Up @@ -113,7 +115,15 @@ public void Dispose()
/// <param name="disposing">True to release both managed and unmanaged resources; false to release only unmanaged resources.</param>
protected virtual void Dispose(bool disposing)
{
// This class needn't to dispose anything. This is just here for sink interface compatibility.
if (!_disposedValue)
{
if (disposing)
{
_sqlLogEventWriter.Dispose();
}

_disposedValue = true;
}
}

private static void ValidateParameters(MSSqlServerSinkOptions sinkOptions, ColumnOptions columnOptions)
Expand All @@ -134,11 +144,6 @@ private static void CheckSinkDependencies(SinkDependencies sinkDependencies)
throw new ArgumentNullException(nameof(sinkDependencies));
}

if (sinkDependencies.DataTableCreator == null)
{
throw new InvalidOperationException("DataTableCreator is not initialized!");
}

if (sinkDependencies.SqlTableCreator == null)
{
throw new InvalidOperationException("SqlTableCreator is not initialized!");
Expand Down
33 changes: 18 additions & 15 deletions src/Serilog.Sinks.MSSqlServer/Sinks/MSSqlServer/MSSqlServerSink.cs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// Copyright 2024 Serilog Contributors
//
// Copyright 2024 Serilog Contributors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//
// http://www.apache.org/licenses/LICENSE-2.0
//
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
Expand All @@ -14,7 +14,6 @@

using System;
using System.Collections.Generic;
using System.Data;
using System.Threading.Tasks;
using Serilog.Events;
using Serilog.Formatting;
Expand All @@ -29,8 +28,9 @@ namespace Serilog.Sinks.MSSqlServer
/// </summary>
public class MSSqlServerSink : IBatchedLogEventSink, IDisposable
{
private readonly MSSqlServerSinkOptions _sinkOptions;
private readonly ISqlBulkBatchWriter _sqlBulkBatchWriter;
private readonly DataTable _eventTable;
private readonly ISqlLogEventWriter _sqlLogEventWriter; // Used if sink option UseSqlBulkCopy is set to false

/// <summary>
/// The default database schema name.
Expand All @@ -53,7 +53,7 @@ public class MSSqlServerSink : IBatchedLogEventSink, IDisposable
/// Construct a sink posting to the specified database.
///
/// Note: this is the legacy version of the extension method. Please use the new one using MSSqlServerSinkOptions instead.
///
///
/// </summary>
/// <param name="connectionString">Connection string to access the database.</param>
/// <param name="tableName">Name of the table to store the data in.</param>
Expand Down Expand Up @@ -108,8 +108,9 @@ internal MSSqlServerSink(
ValidateParameters(sinkOptions);
CheckSinkDependencies(sinkDependencies);

_sinkOptions = sinkOptions;
_sqlBulkBatchWriter = sinkDependencies.SqlBulkBatchWriter;
_eventTable = sinkDependencies.DataTableCreator.CreateDataTable();
_sqlLogEventWriter = sinkDependencies.SqlLogEventWriter;

CreateDatabaseAndTable(sinkOptions, sinkDependencies);
}
Expand All @@ -119,7 +120,9 @@ internal MSSqlServerSink(
/// </summary>
/// <param name="batch">The events to emit.</param>
public Task EmitBatchAsync(IReadOnlyCollection<LogEvent> batch) =>
_sqlBulkBatchWriter.WriteBatch(batch, _eventTable);
_sinkOptions.UseSqlBulkCopy
? _sqlBulkBatchWriter.WriteBatch(batch)
: _sqlLogEventWriter.WriteEvents(batch);

/// <summary>
/// Called upon batchperiod if no data is in batch. Not used by this sink.
Expand Down Expand Up @@ -148,7 +151,7 @@ protected virtual void Dispose(bool disposing)
{
if (disposing)
{
_eventTable.Dispose();
_sqlLogEventWriter.Dispose();
}

_disposedValue = true;
Expand All @@ -170,11 +173,6 @@ private static void CheckSinkDependencies(SinkDependencies sinkDependencies)
throw new ArgumentNullException(nameof(sinkDependencies));
}

if (sinkDependencies.DataTableCreator == null)
{
throw new InvalidOperationException("DataTableCreator is not initialized!");
}

if (sinkDependencies.SqlTableCreator == null)
{
throw new InvalidOperationException("SqlTableCreator is not initialized!");
Expand All @@ -184,6 +182,11 @@ private static void CheckSinkDependencies(SinkDependencies sinkDependencies)
{
throw new InvalidOperationException("SqlBulkBatchWriter is not initialized!");
}

if (sinkDependencies.SqlLogEventWriter == null)
{
throw new InvalidOperationException("SqlLogEventWriter is not initialized!");
}
}

private void CreateDatabaseAndTable(MSSqlServerSinkOptions sinkOptions, SinkDependencies sinkDependencies)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
using System.Collections.Generic;
using System.Data;
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using Serilog.Events;

namespace Serilog.Sinks.MSSqlServer.Platform
{
internal interface ISqlBulkBatchWriter
internal interface ISqlBulkBatchWriter : IDisposable
{
Task WriteBatch(IEnumerable<LogEvent> events, DataTable dataTable);
Task WriteBatch(IEnumerable<LogEvent> events);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
using Serilog.Sinks.MSSqlServer.Platform.SqlClient;

namespace Serilog.Sinks.MSSqlServer.Platform
{
internal interface ISqlCommandFactory
{
ISqlCommandWrapper CreateCommand(ISqlConnectionWrapper sqlConnection);
ISqlCommandWrapper CreateCommand(string cmdText, ISqlConnectionWrapper sqlConnection);
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
using Serilog.Events;
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using Serilog.Events;

namespace Serilog.Sinks.MSSqlServer.Platform
{
internal interface ISqlLogEventWriter
internal interface ISqlLogEventWriter : IDisposable
{
void WriteEvent(LogEvent logEvent);

Task WriteEvents(IEnumerable<LogEvent> events);
}
}
Loading
Loading