Open tsimbalar opened 2 weeks ago
Hey @tsimbalar!
I've also been thinking a bit about IDiagnosticContext
lately; not exactly from the same angle, but maybe there's some convergence to find; please excuse the brain dump :-)
IDiagnosticContext
has a lot of overlap with Activity.Current
— Activity
has or is getting parallel methods to the ones on IDiagnosticContext
, and when an ASP.NET Core request is traced, the properties from the activity end up naturally on that final "request completion" span.
In an app that's recording traces, Activity.Current?.SetTag(...)
etc. is therefore just about enough to fulfill most of the current role of IDiagnosticContext
.
In an app that isn't tracing, e.g. using the Serilog request logging middleware instead (still a much easier option to reason about in some circumstances), it'd be nice if we could maintain IDiagnosticContext
as a way to call those underlying Activity
APIs, since ASP.NET Core will (currently) create and start activities anyway.
In this version, instead of enriching from some separate diagnostic context container, the request logging middleware would enrich the final request completion event from the tags on Activity.Current
.
Also in this version of the world, the kind of enriching you're interested in that this ticket discusses would be implemented as an Enrich.FromCurrentActivity()
kind of thing on your root logger configuration, and wouldn't be specific to ASP.NET Core (though FromCurrentActivity()
might accept a filter on the activity source name so that only ASP.NET Core request activities are used this way).
There's a bit of subtlety, though - IDiagnosticContext
is intended to record properties on the root "activity" representing a request. Sometimes, during a request, Activity.Current
might be a nested one representing some child operation. This will generally only be the case when tracing is active and an activity is recorded, however, so the user does have some control over this, and an Activity.Current
implementation of IDiagnosticContext
for the logging scenario could be designed to work either way - either enriching the current activity, or enriching the (root) activity representing ASP.NET Core's handling of a request.
I'm not anticipating that the whole .NET world will go all-in on tracing over logging for this scenario, but it'd be nice if Serilog's support for ASP.NET Core could be reasonably consistent and compatible across the two approaches. I've spent the year working on an integration between Serilog and System.Diagnostics.Activity, if you haven't seen how that looks in the ASP.NET Core case this is probably the best minimal example.
Anyway, hope this is some food for thought :-)
Is your feature request related to a problem? Please describe. I am happy using
IDiagnosticContext
to "add stuff" that will end up being available in the final Serilog/end of request log event ... but I'm a bit annoyed that all those properties I have attached are not available for other instances of log events generated within the same HTTP Request.I guess what I'm looking for is a
LogContext.Push
where I wouldn't have to.Dispose()
, and would last until the end of the current HTTP Request.Describe the solution you'd like I understand this is not a behavior we'd want to enable by default (this would be a breaking change)... but maybe we'd like to explicit be able to
Enrich.FromDiagnosticContext()
, i.e. whenever I am emitting aLogEvent
, look if there is anything inside the currentDiagnosticContext
of the current HttpRequest, and attach those as well.Describe alternatives you've considered Maybe I could somehow add another place that is "within Http Request" where I'd store things for the scope of the request's duration, and have an
Enricher
that would read from it 🤔Comments At this point I'm pretty sure it's technically doable (within this library, or separately with custom code) ... but I'm also trying to figure out where it's actually a good idea in the first place, or there are good reasons NOT to do it 😅