Add Counters for Endpoints

[Kahbazi]

Fixes #19027

[gfoidl]

Are tests needed?

[gfoidl]

Nit: one empty line too much.

[gfoidl]

Nit: remove empty line

[gfoidl]

May it be "better" to handle the executed within the failed?
So that here is just one method call, and in the internals of Log.FailedEndpoint this case gets handled and has the potential to adapt it later if needed.

[Kahbazi]

Are tests needed?

Absolutely. I will add tests after the team approved the design.

[JamesNK]

Looking up a dictionary multiple times (executing, executed, failed) for every call would have a performance impact. This code is on a very hot path.

I don't know the best way to mitigate but EventSource.OnEventCommand provides a hook to decide to enable counters. We should only be initializing counters when needed.

[JamesNK]

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelEventSource.cs

Lines 314 to 319 in 5a0c097

	protected override void OnEventCommand(EventCommandEventArgs command)
	{
	if (command.Command == EventCommand.Enable)
	{
	// This is the convention for initializing counters in the RuntimeEventSource (lazily on the first enable command).
	// They aren't disabled afterwards...

[halter73]

The problem is that even if the event source is disabled, we would still need to do the dictionary lookup to ensure EndpointCounter._totalRequests/_currentRequests/_failedRequests are all accurate when the event source becomes enabled.

We can still override OnEventCommand to lazily initialize the PollingCounters, but we'd have to iterate _endpointCounters dictionary when the event source becomes enabled to do so.

[JamesNK]

Ah, right. We could add counters as metadata or a property on the endpoint. However endpoints are intended to be immutable and Ryan would murder us all 😄

[halter73]

I was thinking the same thing. Problem with a property is that Endpoint is defined in another assembly and InternalsVisibleTo is bad. Putting it in Metadata doesn't work because it is immutable and Ryan would murder us. Retrieving the EndpointCounter from Metadata would involve a dictionary lookup anyway.

[JamesNK]

Yes.

There would be some other challenges, like an application's endpoints can change. For example, dropping a new Razor file into a directory would change that endpoint data source to include the new endpoint and the matcher would recompute the DFA graph. When that happens we'd need to keep the endpoint+counter instances together. Not difficult, but something to take into account.

[Kahbazi]

What should happen if someone wants to SetEndpoint without Routing Middleware. Do they need to provide the counter data as well?

Correct me if I'm wrong, but Routing matcher only gives RouteEndpoint not Endpoint, and If we are going with getting counters from Routing matcher, can we instead add a property to RouteEndpoint and since it's in the same assembly as there's no need to InternalVisibleTo.

[JamesNK]

Routing matching usual matches RouteEndpoint but can return Endpoint from a policy.

That's a good point about SetEndpoint. Matching is the primary way of setting endpoints, but not the only way. Matching providing the counter type wouldn't work.

[BrennanConroy]

@davidfowl Thoughts?

[BrennanConroy]

@noahfalk This is an example of where we want dimensions for metrics to avoid the dictionary workaround.

[BrennanConroy]

Thanks for trying this out @Kahbazi, we're going to wait for dimensions support in counters before doing this.