search

serilog-mssql / serilog-sinks-mssqlserver

A Serilog sink that writes events to Microsoft SQL Server and Azure SQL
Apache License 2.0
283 stars 148 forks source link

Add SqlInsertStatementWriter which uses INSERT statements instead of SqlBulkCopy #510

Closed BrettJaner closed 9 months ago

BrettJaner commented 10 months ago

Addresses issue (#509)

Summary of changes:

  1. Add property called UseSqlBulkCopy to MSSqlServerSinkOptions defaulting to true
  2. Update configuration implementations in order to read UseSqlBulkCopy from appsettings.json
  3. Add SqlInsertStatementWriter which uses INSERT statements (highly based off of the SqlLogEventWriter used by the MSSqlServerAuditSink)
    • Implements both ISqlBulkBatchWriter and ISqlLogEventWriter
    • Removed SqlLogEventWriter as it is now redundant
  4. Add SqlInsertStatementWriterTests
ckadluba commented 9 months ago

Hi @BrettJaner!

Thank you for the contribution. It is very welcome.

Can you please update the documentation in README.md to relfect your changes?

BrettJaner commented 9 months ago

Can you please update the documentation in README.md to relfect your changes?

Hi @ckadluba, I've updated the README.md.

BrettJaner commented 9 months ago

After reviewing my code for possible breaking changes, I realized I missed the configuration implementation (eg. ability to specify UseSqlBulkCopy in appsettings.json). I've since updated the PR (4a725ea). Please let me know if anything looks off.

ckadluba commented 9 months ago

@BrettJaner Can you please add also one integration test using the new SqlInsertBatchWriter? You'll find examples in the Misc directory, for instance this one: https://github.com/serilog-mssql/serilog-sinks-mssqlserver/blob/dev/test/Serilog.Sinks.MSSqlServer.Tests/Misc/OpenTelemetryColumnsTests.cs.

BrettJaner commented 9 months ago

@BrettJaner Can you please add also one integration test using the new SqlInsertBatchWriter?

I've added integration tests to switch between the two batch writers (00cc7e9).