Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable to enrich logs from what is currently in IDiagnosticContext #384

Open
tsimbalar opened this issue Oct 10, 2024 · 1 comment
Open

Comments

@tsimbalar
Copy link
Member

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 a LogEvent, look if there is anything inside the current DiagnosticContext 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 😅

@nblumhardt
Copy link
Member

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.CurrentActivity 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 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants