elastic / ecs-dotnet

https://www.elastic.co/guide/en/ecs-logging/dotnet/current/setup.html
Apache License 2.0
108 stars 54 forks source link

Skip ActivityData capture for NLog, since doing Layout capture #379

Closed snakefoot closed 1 month ago

snakefoot commented 3 months ago

When enabling writing on background-thread, then one should not capture the activity-traceid of the background-thread.

I guess Serilog has the same issue, since it relies on its enricher-capture (similar to NLog layout capture)

This is ofcourse a breaking change, since adding new property to existing interface IEcsDocumentCreationOptions. Maybe change to use enum-flag-values?

Alternative only perform automatic capture for Elastic.Extensions.Logging? (Skip helping NLog / Serilog / Log4net)

Mpdreamz commented 1 month ago

run docs-build

Mpdreamz commented 1 month ago

This is ofcourse a breaking change, since adding new property to existing interface IEcsDocumentCreationOptions. Maybe change to use enum-flag-values?

I am not concerned breaking this in a non major update.

Yes its breaking but the chances someone updating a shipper/formatter without updating Elastic.CommonSchema are slim IMO.

snakefoot commented 1 month ago

@Mpdreamz This also needs some work on Serilog which i'll do in a separate PR.

I guess there are two directions:

Mpdreamz commented 1 month ago

That enricher is deprecated and so I rather update to a more recent serilog :)

Mpdreamz commented 1 month ago

Actually @snakefoot I think we are good for Serilog:

https://github.com/serilog/serilog/blob/4ef3a8772c2dee065a5c6d5ac951f405b6c7ced4/src/Serilog/Core/Logger.cs#L427-L429

Dispatch calls Emit() which is where we do the Activity inspection so its on the same callstack.

Similar to why we are good for extensions logging.

snakefoot commented 1 month ago

Serilog has the ability (Like NLog) to render on background-thread. See also: https://github.com/serilog/serilog-sinks-async/issues/93

But yes extensions logging is good.

Mpdreamz commented 1 month ago

Yeah there really is no sense in using us with serilog-sinks-async, we already push our events over a channel that is being consumed in a background thread.

Will add an explicit note to our docs to not use us wrapped in the async sync.

Mpdreamz commented 1 month ago

Ahhh sorry,

Elastic.CommonSchema.Serilog and using it with an async file sink is something we need to support.

Elastic.Serilog.Sinks should never be wrapped in an async sink.