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

Cannot use .AuditTo with SpanId or TraceId #515

Closed
Kolthor opened this issue Jan 30, 2024 · 6 comments · Fixed by #535
Closed

Cannot use .AuditTo with SpanId or TraceId #515

Kolthor opened this issue Jan 30, 2024 · 6 comments · Fixed by #535

Comments

@Kolthor
Copy link

Kolthor commented Jan 30, 2024

Bug Report

Please clearly describe what the SQL Sink is doing incorrectly:

When using .AuditTo.MSSqlServer and including the new SpanId and TraceId standard columns, events which contains either a SpanId or TraceId breaks Serilog.Sinks.MSSqlServer, with the exception message:

Failed to emit a log event. (No mapping exists from object type System.Diagnostics.ActivitySpanId to a known managed provider native type.)

or:

Failed to emit a log event. (No mapping exists from object type System.Diagnostics.ActivityTraceId to a known managed provider native type.)

This exception does not occur when using .WriteTo.MSSqlServer, and both columns are populated in the table as expected.

The problem is that .AuditTo uses SqlLogEventWriter, which uses SqlCommand.AddParameter when adding the SpanId and TraceId:

command.AddParameter(Invariant($"@P{index}"), field.Value);

and the values returned from StandardColumnDataGenerator.GetColumnsAndValues() are the structs System.Diagnostics.ActivitySpanId and System.Diagnostics.ActivityTraceId respectively.

.WriteTo is not affected because it uses SqlBulkBatchWriter, which maps a SqlBulkCopy to a DataTable, where the DataColumns are specified as strings, and the structs are converted when they are added.

I believe calling .ToString() on logEvent.TraceId and logEvent.SpanId in StandardColumnDataGenerator.GetColumnsAndValues() should fix the issue:

case StandardColumn.TraceId:
return new KeyValuePair<string, object>(_columnOptions.TraceId.ColumnName, logEvent.TraceId);
case StandardColumn.SpanId:
return new KeyValuePair<string, object>(_columnOptions.SpanId.ColumnName, logEvent.SpanId);

Please clearly describe the expected behavior:

Both columns are populated in the table, with no exceptions or missing events.

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

  • Serilog: 3.1.1
  • Serilog.Sinks.MSSqlServer: 6.5.1
  • Serilog.Extensions.Logging: 8.0.0

Target framework and operating system:

[x] .NET 6
[ ] .NET Framework 4.8
[ ] .NET Framework 4.7
[ ] .NET Framework 4.6

OS: Windows 11 Version 22H2 (OS Build 22621.3007)

Provide a simple reproduction of your Serilog configuration code:

// Serilog.Sinks.MSSqlServer ColumnOptions
var columnOptions = new ColumnOptions();
columnOptions.Store.Remove(StandardColumn.Properties);
columnOptions.Store.Add(StandardColumn.LogEvent);
columnOptions.Store.Add(StandardColumn.TraceId);
columnOptions.Store.Add(StandardColumn.SpanId);

// Configure Serilog
builder.Services.AddLogging(builder =>
	builder.AddSerilog(
		logger: new LoggerConfiguration().Enrich.FromLogContext()
		                                 .MinimumLevel.Is(Serilog.Events.LogEventLevel.Debug)
		                                 .AuditTo.MSSqlServer(@"Server=.\SqlExpress;Database=__SerilogTest;Trusted_Connection=True;Encrypt=False;",
		                                                      new MSSqlServerSinkOptions
		                                                      {
			                                                      TableName = "EventLog",
			                                                      AutoCreateSqlDatabase = true,
			                                                      AutoCreateSqlTable = true,
		                                                      },
		                                                      columnOptions: columnOptions
		                                 ).CreateLogger(),
		dispose: true
	).Configure(options => options.ActivityTrackingOptions = ActivityTrackingOptions.TraceId | ActivityTrackingOptions.SpanId)
);

Provide a simple reproduction of your Serilog configuration file, if any:

N/A

Provide a simple reproduction of your application code:

https://github.com/Kolthor/SerilogTest

@ckadluba
Copy link
Member

Hello @Kolthor!

Thank you for reporting this issue and for the thorough analysis. I will look into itnas soon as I can. Hopefully within the next 2 weeks.

Cheers,
Christian

@vui611
Copy link
Contributor

vui611 commented Apr 30, 2024

Hi @ckadluba Is there any progress on this ticket? Based on what @Kolthor described, it seems to be a very easy fix, doesn't it?
Regards

@ckadluba
Copy link
Member

Sorry for the delay. I have not enough bandwidth for testing the proposed fix at the moment. But we are always happy if someone helps us out with a PR. We could review this quickly and merge if it is good.

vui611 added a commit to vui611/serilog-sinks-mssqlserver that referenced this issue Apr 30, 2024
When include 2 standard columns SpanId and TraceId, AuditTo will throw an exception. The quick solution is adding ToString() for these columns.
@vui611 vui611 mentioned this issue Apr 30, 2024
@vui611
Copy link
Contributor

vui611 commented Apr 30, 2024

@ckadluba Thank you for your fast response.
Please check the quick PR here :) #535

@ckadluba
Copy link
Member

@vui611 Thank you very much for the PR. It was just merged into the dev branch. 🫶

@Kolthor Thanks a lot for reporting this and for the detailed bug analysis.

A dev version of the sink was built and published on nuget.

https://www.nuget.org/packages/Serilog.Sinks.MSSqlServer/6.6.1-dev-00077

It would be great if you could take it for a spin and test the fix with this version or with the current code in the dev branch.

Naturally the fix will also go into the next regular release (when we merge dev into main). But since there are one or two significant open bugs, I would like to wait until we have them fixed too.

@vui611
Copy link
Contributor

vui611 commented Apr 30, 2024

@ckadluba I can confirm the version 6.6.1-dev-00077 works well now. I can use AuditTo() without any exception and 2 columns TraceId, SpanId are written to the database.
Thank you.

Screenshot 2024-05-01 at 9 47 51 am

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants