Skip ActivityData capture for NLog, since doing Layout capture #379
Conversation
snakefoot
force-pushed
the
net6
branch
2 times, most recently
from
cf1fd62 to
084a6c3
Compare
snakefoot
force-pushed
the
net6
branch
2 times, most recently
from
439efad to
f48620b
Compare
There was a problem hiding this comment.
Thank you for covering this @snakefoot
This also needs some work on Serilog which i'll do in a separate PR.
It requires updating to Serilog 3.1 and updating our base TFM from net461 to 462.
AFAIK log4net appenders are synchronous so we should be okay there.
|
run docs-build |
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. |
|
|
||
| if (options?.IncludeHost is null or true) doc.Host = GetHost(initialCache); | ||
| if (options?.IncludeProcess is null or true) doc.Process = GetProcess(initialCache); | ||
| if (options?.IncludeUser is null or true) doc.User = GetUser(); | ||
| if (options?.IncludeTraceId is null or true) | ||
| SetActivityData(doc); |
There was a problem hiding this comment.
The new ActivityExtensions.cs introduced here could be also used within SetActivityData - it has basically the same logic.
There was a problem hiding this comment.
@Mpdreamz Should I make the suggested change in this pull-request? It will complicate a little since ActivityExtensions.cs is right now isolated for the NLog, and will require shared code between projects (new public API)
There was a problem hiding this comment.
Let's do it in a follow up PR 👍
It would be nice if they used the same logic across logging implementations.
ToString already calls ToHexString however we currently have different fallbacks if the format is not W3C or if it's null or empty.
I guess there are two directions:
|
|
That enricher is deprecated and so I rather update to a more recent serilog :) |
|
Actually @snakefoot I think we are good for Serilog: Dispatch calls Emit() which is where we do the Similar to why we are good for extensions logging. |
|
Serilog has the ability (Like NLog) to render on background-thread. See also: serilog/serilog-sinks-async#93 But yes extensions logging is good. |
|
Yeah there really is no sense in using us with Will add an explicit note to our docs to not use us wrapped in the async sync. |
|
Ahhh sorry,
|
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)