MVC use concrete types for DiagnosticListener

[benaadams]

Pattern:

Under Diagnostics namespace

Event Type is same name as Event Name

Type has a EventName string const so what to listen to is exposed without having to check the source (and is a const so can be used in switch and Attributes).

Type has its data exposed as readonly properties.

Type implements IReadOnlyList<KeyValuePair<string, object>> so the data can be iterated over.

Type implements IEnumerable<KeyValuePair<string, object>> so the data can be enumerated over,

Interfaces are explicit so they don't interfere with reflection/serialization (i.e. Count being seen as a data item)

Each event has its own type so pattern matching can be used to differentiate events.

e.g.

namespace Microsoft.AspNetCore.Mvc.Diagnostics
{
    public sealed partial class AfterAction : IEnumerable<KeyValuePair<string, object>>, IReadOnlyList<KeyValuePair<string, object>>
    {
        public const string EventName = "Microsoft.AspNetCore.Mvc.AfterAction";
        
        public AfterAction(ActionDescriptor actionDescriptor, HttpContext httpContext, RouteData routeData);
        
        public ActionDescriptor ActionDescriptor { get; }
        public HttpContext HttpContext { get; }
        public RouteData RouteData { get ; }

        int IReadOnlyCollection<KeyValuePair<string, object>>.Count { get; }
        KeyValuePair<string, object> IReadOnlyList<KeyValuePair<string, object>>.this[int index];

        IEnumerator<KeyValuePair<string, object>> IEnumerable<KeyValuePair<string, object>>.GetEnumerator(););
    }
}

/cc @rynowak @davidfowl @NickCraver

[benaadams]

Are still anonymous diagnostics in other places like Hosting, but will see how this goes down first before seeing about those.

[NickCraver]

This would be a huge improvement - constants as event names make both the adapter case and general usage (e.g. a case in an IObserver) much better. The strong typing is very welcomed for anyone taking the reference and makes the whole thing sane (over loads of reflection today).

If anyone's curious about context: https://github.com/dotnet/corefx/issues/18114 is the original issue pushing for this, but closed since the teams weren't going to make any more breaking changes. With 3.0, it seems like hopefully that's back on the table.

[rynowak]

@benaadams - this is dope, I like the design of this.

[rynowak]

@ajcvickers - is it work comparing the design of this with https://github.com/aspnet/EntityFrameworkCore/tree/9026770af72683ba3cf6c2a8125fa7a8d602703f/src/EFCore/Diagnostics ?

I notice that you're using the same payloads for logging and diagnostics source.

[rynowak]

Ooops. This is a really unfortunate event name. I think we should call this type something like ActionFilterOnActionExecuted on so on for the other filter-related events.

[benaadams]

Is that everything that has IFilterMetadata property?

e.g. AfterOnException => AfterFilterOnException

[rynowak]

I would call it ExceptionFilterAfterOnException. So the scheme is <filter kind> [Before|After] Method

[NickCraver]

I'm just curious around what constitutes a breaking change on these APIs w.r.t. event names.

Since the property names on the object are going from camelCase to proper C# Pascal case for properties, isn't it already breaking? Or is the latest binder reflection case-insensitive and that keeps working? I'm just confused on the event names being breaking when the property casing isn't...if we're firing the old events but they just don't work, maybe it would be better to change and break the event names? Just tossing that thought out there.

[rynowak]

Since the property names on the object are going from camelCase to proper C# Pascal case for properties, isn't it already breaking? Or is the latest binder reflection case-insensitive and that keeps working?

It depends on the consumer. The consumer that I'm aware of is app-insights which I think is still using the Microsoft.Extensions.DiagnosticSourceAdapter package - which is case-insensitive.

Technically speaking, top level entries are defined as case-insensitive, but what really matters to me is whether we break people that have real usage.

If this is a breaking change that impacts existing usage of diagnostic source, then I would lean towards using lowercase properties. This is too late in the development cycle to be cavalier about breaking changes.

@lmolkova - any insights here?

[NickCraver]

Given the very few people using these (as far as I'm aware) outside diagnostic adapter, IMO we should change them now while making changes and have them reflect the casing that should be on a public type API. Even if it's breaking, as long as DiagnosticAdapter is case-insensitive.

[benaadams]

Aside: Just to make note on my understanding of the status of this PR. I've made the requested changes and @rynowak is picking up class naming changes.

Also outstanding issue on Instrumentation apis #11730 (comment) to be picked up separately?

[rynowak]

There's still a breaking change here: https://github.com/aspnet/AspNetCore/pull/11730/files#diff-1f8dd5fa9cfd7a11a18340f90b1706d7R18

[benaadams]

There's still a breaking change here:

Surely not with the magic of DI and all that 😉

Reverted

[lmolkova]

I'll look more into this PR later today.

Regarding this:

Microsoft.Extensions.DiagnosticSourceAdapter package - which is case-insensitive.

AppInsights is NOT using DiagnosticAdapter anymore and event handling is case-sensitive. We still have several work items to support ASP.NET Core 3.0 and can fit case-insensitive work if needed.

Also

very few people using these (as far as I'm aware) outside diagnostic adapter

After we introduced Activities, it became hard and inefficient to use DiagnosticAdapter because of aspnet/EventNotification#67 (check out this dotnet/extensions#1895 as an example I just found)

We also removed adapter because of perf - DiagnosticsSource.IsEnabled became twice as fast. And the ability to filter out events in IsEnabled (which brought more improvements) is not supported by the adapter.

So, I'd argue DiagnosticAdapter is the default way is not a good assumption.

[rynowak]

I'll look more into this PR later today.

Regarding this:

Microsoft.Extensions.DiagnosticSourceAdapter package - which is case-insensitive.

AppInsights is NOT using DiagnosticAdapter anymore and event handling is case-sensitive. We still have several work items to support ASP.NET Core 3.0 and can fit case-insensitive work if needed.

Ok great. Is there somewhere you want me to file this?

After we introduced Activities, it became hard and inefficient to use DiagnosticAdapter because of aspnet/EventNotification#67 (check out this aspnet/Extensions#1895 as an example I just found)

We also removed adapter because of perf - DiagnosticsSource.IsEnabled became twice as fast. And the ability to filter out events in IsEnabled (which brought more improvements) is not supported by the adapter.

So, I'd argue DiagnosticAdapter is the default way is not a good assumption.

Also great from my POV. I'd like to use deprecate the adapter and stop shipping it in a future release. It hasn't really proved to be essential for diagnostic source.