Handling of `Serilog.Events.LogEvent` in `LokiBatchFormatter` is not thread-safe

[k-wojcik]

Which version of Serilog.Sinks.Grafana.Loki are you using?

v8.2.0

Which version of .NET are you using?

net6.0

Describe the bug

Handling of Serilog.Events.LogEvent in LokiBatchFormatter is not thread-safe. Method AddLevelAsPropertySafely modifies Serilog.Events.LogEvent.Properties (Dictionary<string, LogEventPropertyValue>) and causes problems with simultaneous aceess to it from other threads (sinks).

https://github.com/serilog-contrib/serilog-sinks-grafana-loki/blob/master/src/Serilog.Sinks.Grafana.Loki/LokiBatchFormatter.cs#L157-L158

https://github.com/serilog/serilog/issues/1154#issuecomment-387895863 Serilog's LogEvent is not thread-safe for readers concurrent with writers; concurrent reads are fine, but only in the absence of writes

To Reproduce

Configure at least one sink with async processing alongside Serilog.Sinks.Grafana.Loki, for example I use sinks:

• Serilog.Sinks.Async + Serilog.Sinks.File,
• Serilog.Sinks.Seq

Expected behavior

The original value of Serilog.Events.LogEvent.Properties should never be modified in sink.

Log/SelfLog output or exception with stacktrace

System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.Dictionary`2.Enumerator.MoveNext()
   at Serilog.Formatting.Compact.CompactJsonFormatter.FormatEvent(LogEvent logEvent, TextWriter output, JsonValueFormatter valueFormatter)
   at Serilog.Sinks.Seq.ConstrainedBufferedFormatter.Format(LogEvent logEvent, TextWriter output, Boolean writePlaceholders)

Application or code sample, which could be used to reproduce a bug

No response

Additional context

No response

I have read the documentation

• [X] I have read Common Problems section in project wiki

[ramonesz297]

Faced the same issue, but decided to write own version, as this implementation has problems with performance (especially memory allocations).

One of optimization was to avoid any modifications of original LogEvent, that fixed the issue.

I do not think that my version can do all this package can, as my implementation target net6, net8 only, and (may be ) do not handle all cases, but for my purposes, where allocation is critical it is ok.

Anyway, maybe some ideas can be useful and applicable to this package as well.

[mishamyte]

Thanks for sharing, @ramonesz297.

Yeah, I agree about the perf-related things, they are noted. I'm also not happy with some features, that brought complexity and perf downgrade, so thinking about their future in the next release.

Hope your version will be useful for people, who struggle with some current behavior and characteristics

[oliver-unifii]

I'm evaluating this nuget and this issue alone makes it unsafe for any production environment, unfortunately.

[mishamyte]

@oliver-unifii sorry to hear that and hope you will find the alternative solution, which will fit your requirements