feat: Enhance telemetry with operation context properties #237
Conversation
nblumhardt
left a comment
nblumhardt
left a comment
There was a problem hiding this comment.
Thanks for taking a look!
I think this could work, but only if an enricher (ILogEventEnricher) is used to attach the OperationName property from Activity.Current - nothing else will currently set OperationName.
An enricher could be avoided by setting the operation name in the sink's Emit(LogEvent) method using ConditionalWeakTable, but this would require internally wrapping Serilog's batched sink implementation via LoggerSinkConfiguration.Wrap() so that the Emit() call is the one that happens on the application thread at the point the event is created, not the asynchronous/background thread EmitBatch..() methods.
....ApplicationInsights/Sinks/ApplicationInsights/TelemetryConverters/TelemetryConverterBase.cs
Show resolved
Hide resolved
hangy
force-pushed
the
serilog-tracing-support
branch
from
9994dc5 to
c00d3f9
Compare
You're right. I'll take a look at it. I guess there are several camps when using this Sink. Personally, I don't really need the |
| return loggerConfiguration.Sink(new ApplicationInsightsSink(telemetryClient, telemetryConverter), | ||
| restrictedToMinimumLevel, levelSwitch); | ||
| } | ||
| var wrapper = LoggerSinkConfiguration.Wrap( |
There was a problem hiding this comment.
@nblumhardt Is this the correct usage of the wrapper, as you suggested? It seems to work in this project's unit tests (Activity.Current is available), but this test setup seems to circumvent batching, anyways.
There was a problem hiding this comment.
Sorry @hangy, I had this wrong, assuming the App Insights sink was batched using the newer mechanism. No wrapping is needed - the sink already implements ILogEventSink.Emit() so the change can be made there directly and this can be reverted.
| logEvent.AddOrUpdateProperty( | ||
| new LogEventProperty( | ||
| TelemetryConverterBase.OperationNameProperty, | ||
| new ScalarValue(activity.OperationName))); |
There was a problem hiding this comment.
Unfortunately sinks can't/shouldn't enrich events on the way through, as they run in parallel and this can cause inconsistent results/threading issues for other sinks. This was what I had in mind when referring to ConditionalWeakTable, which can be used to attach properties to an object behind-the-scenes using a GC-friendly CLR trick.
|
Looking at #239 and the complexity involved here, I wonder if we're better off putting the new behavior added in 4.1 behind an opt-in configuration option? Currently it seems like fixing this reliably will take a while, so having the sink ignore |
2 participants
By adding support for
OperationNameproperty fromserilog-tracing, I think this should fix #236