search

serilog / serilog-extensions-logging

Serilog provider for Microsoft.Extensions.Logging
Apache License 2.0
307 stars 97 forks source link

`ISupportExternalScope` enrichment #251

Closed pavel-faltynek closed 1 month ago

pavel-faltynek commented 1 month ago

Is it intended that the code utilizing scopes provided by IExternalScopeProvider is modifying the SerilogLoggerProvider's current scope chain via creating (and not disposing) SerilogLoggerScope every time a LogEvent is enriched?

https://github.com/serilog/serilog-extensions-logging/blob/3c329978b208b15a5bc80f1204cdb14b64f5effe/src/Serilog.Extensions.Logging/Extensions/Logging/SerilogLoggerProvider.cs#L81

Does not feel correct at first sight. It captures provider's current source and replaces it by itself, which seems to be intended to reverse on Dispose() (which is never called). Should it even manipulate the provider's scope chain at all? Shouldn't it just enrich the LogEvent via scopes from external source without performing any side effects?

pavel-faltynek commented 1 month ago

Anyway, conversion to using var might probably work (even if it temporarily changes the provider's scope chain). As the provider's scope is async-local, it might be also resistant to concurrency issues.

nblumhardt commented 1 month ago

Thanks for checking this out, @pavel-faltynek, I think this is an issue 👍

We should be able to pull most of EnrichAndCreateScopeItem() out into a static method that avoids tinkering with the current scope or allocating.

I can take a look at this, but let me know if you're keen to dig in, all help appreciated.

pavel-faltynek commented 1 month ago

Ok, tried in https://github.com/serilog/serilog-extensions-logging/pull/252, could you please check it does make sense to you?

nblumhardt commented 1 month ago

252 should resolve this; a new -dev package will be published shortly.