Leverage 'DiagnosticSources' for collecting data instead of custom 'DiagnosticContext'

[julealgon]

I just stumbled upon this native library from Microsoft that is eerily similar to what IDiagnosticContext is doing for Serilog

My proposal would be for authors to reconsider keeping a custom abstraction in place and instead migrate over to the native approach by dropping IDiagnosticContext altogether in favor of DiagnosticSources.

As an example, AspNetCore could be fully instrumented using this approach. Here are some of the events it fires that could be captured:
https://github.com/aspnet/Hosting/blob/e4350945c56d69523fe36bf33c58b955db3d8985/src/Microsoft.AspNetCore.Hosting/Internal/HostingApplicationDiagnostics.cs#L18

I found out about this system through this issue on AWS XRay repository

I believe trying to reuse standard abstractions like activities and DiagnosticSources and DiagnosticListeners would benefit the community more than coming up with fully custom abstractions like DiagnosticContext which are specific to Serilog. It would also help bring awareness to these systems to more people and push then to leverage them more (I was completely unaware it existed).

This is related to both of these in the end of the day:

• Combine 'ILogEventEnricher' abstraction with 'IDiagnosticContext' serilog-aspnetcore#222
• Expose an interface for DiagnosticContext collection flow serilog-aspnetcore#223

[sungam3r]

What about destructureObjects feature?

[sungam3r]

My proposal would be for authors to reconsider keeping a custom (1) abstraction in place and instead migrate over to the native (2) approach.

It seems funny to me that from my point of view these two words should be reversed because from a design point of view, the opposite is happening for this package - replacing the native abstraction with the external one.

[julealgon]

@sungam3r

What about destructureObjects feature?

That could be implemented in the transformation phase of getting the data collected by the DiagnosticSource and creating the Serilog log event. It is a completely separate concern than just collecting the data.

It seems funny to me that from my point of view these two words should be reversed because from a design point of view, the opposite is happening for this package - replacing the native abstraction with the external one.

For all intents and purposes, I consider System.* and Microsoft.* classes "native", and everything else external. Surely, if you look from your library's perspective then of course, DiagnosticContext is native and DiagnosticSource is an external dependency, but I don't particularly see how that's a useful way to look at it. You don't go out of your way to implement a Serilog.DateAndTime type that is fully incompatible with System.DateTime for obvious reasons, and those same obvious reasons apply here IMHO.

By moving in the opposite direction (recreating things that already exist in the framework) you are just making your library less general purpose and in the long term harder to integrate with.

I wouldn't be surprised if more people knew about IDiagnosticContext than DiagnosticSource (like myself a few hours ago) and I strongly feel this actually hurts the community.

[sungam3r]

By moving in the opposite direction (recreating things that already exist in the framework) you are just making your library less general purpose and in the long term harder to integrate with.

This is a deceptive statement. It relies on the confidence that the framework developers have foreseen all use cases for a feature and provide a user-friendly interface for all consumers. In practice, this is rarely the case, which is why alternative implementations appear that are somehow integrated into the main framework. Then comes the process of evolution. Implementations compete with each other.

And also pay attention to when DiagnosticSource appeared and in which frameworks it is available. Even MS has not yet migrated all their .NET packages to this API.

[julealgon]

This is a deceptive statement. It relies on the confidence that the framework developers have foreseen all use cases for a feature and provide a user-friendly interface for all consumers.

I'm aware that they are sometimes not perfect: nothing really is. The point I'm making is that, instead of coming up with something completely isolated from scratch that mimics what already exists, we should strive to improve upon what already is there. If something is lacking in the native abstraction, we should report and work with Microsoft on the needed improvements whenever possible, and when that fails, we should try to base our solution on the base solution.

DiagnosticContext has zero relation right now to DiagnosticSource, even though they perform something extremely similar.

...alternative implementations appear that are somehow integrated into the main framework.

Any examples besides Newtonsoft.Json here? And that's actually not a great example since there was mostly zero support for Json in the main framework to begin with.

Then comes the process of evolution. Implementations compete with each other.

This is fine between libraries, everybody wants to get the best possible results. But to me doing that against native abstractions is counter-productive: you are just fracturing and weakening the community overall.

And also pay attention to when DiagnosticSource appeared and in which frameworks it is available. Even MS has not yet migrated all their .NET packages to this API.

I know it is a new(ish) API. It was introduced in .Net Core 1.0 (so not that new). In this discussion, to me, it doesn't matter which came first: regardless, Serilog should adapt itself to native framework types when they become available.

I honestly don't understand part of this resistance. If I was the owner I'd be ecstatic to know the framework started to use types that are incredibly similar to my own and would rush to adapt to them in hopes that everybody gets more use out of them. Once again you seem to be favoring the opposite here: go the full customization route and create unique abstractions that only Serilog uses.

If you are attempting to vendor-lock people into Serilog... them I guess this is a fine strategy: make it impossible to migrate away from it by forcing its abstractions on as many places as possible in consumer code.

[sungam3r]

I honestly don't understand part of this resistance.
you seem to be favoring the opposite here: go the full customization route and create unique abstractions that only Serilog uses.

Let's just say my point of view is not to accept any one vision. I understand that the task can be done this way and that, and both ways have pros and cons. I do not have much love for MS abstractions, but I also have no dislike for them. And the fact that they are the authors of the .NET Core does not add much points in their favor.

If you are attempting to vendor-lock people into Serilog...

Not at all. By the way, MS is also a vendor :) so the whole question is which vendor you like best.

[skomis-mm]

Hi @julealgon ,
I think this abstractions have a bit different purposes.

System.Diagnostics.(DiagnosticSource/Activity/ActivitySource) is a tracing API that is different from logging.
IDiagnosticSource here is about collecting properties within some scope (e.g. ASP.NET Core request scope) specifically to enrich logging events at the end of the scope, not to trace itself with this data. The motivation described here:

The package includes middleware for smarter HTTP request logging. The default request logging implemented by ASP.NET Core is noisy, with multiple events emitted per request. The included middleware condenses these into a single event that carries method, path, status code, and timing information.

IDiagnosticsContext.Set can be used to enrich from the current trace / Activity. Or you can create standalone ILogEventEnricher implementation like Serilog.Enrichers.Span and enrich what you need from the trace data.

[nblumhardt]

I don't think this is likely to yield productive steps forward right now, but thanks for all the thoughts!