Skip to content

Add Counters for Endpoints #21675

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add Counters for Endpoints #21675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/Http/Routing/src/EndpointMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public Task Invoke(HttpContext httpContext)
}
}

EndpointMiddlewareEventSource.Log.ExecutingEndpoint(endpoint.DisplayName);
Log.ExecutingEndpoint(_logger, endpoint);

try
Expand All @@ -62,10 +63,13 @@ public Task Invoke(HttpContext httpContext)
}
catch (Exception exception)
{
EndpointMiddlewareEventSource.Log.ExecutedEndpoint(endpoint.DisplayName);
EndpointMiddlewareEventSource.Log.FailedEndpoint(endpoint.DisplayName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Log.ExecutedEndpoint(_logger, endpoint);
return Task.FromException(exception);
}

EndpointMiddlewareEventSource.Log.ExecutedEndpoint(endpoint.DisplayName);
Log.ExecutedEndpoint(_logger, endpoint);
return Task.CompletedTask;
}
Expand All @@ -77,6 +81,12 @@ static async Task AwaitRequestTask(Endpoint endpoint, Task requestTask, ILogger
try
{
await requestTask;
EndpointMiddlewareEventSource.Log.ExecutedEndpoint(endpoint.DisplayName);
}
catch
{
EndpointMiddlewareEventSource.Log.FailedEndpoint(endpoint.DisplayName);
throw;
}
finally
{
Expand Down
101 changes: 101 additions & 0 deletions src/Http/Routing/src/EndpointMiddlewareEventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Concurrent;
using System.Diagnostics.Tracing;
using System.Threading;

namespace Microsoft.AspNetCore.Routing
{
internal class EndpointMiddlewareEventSource : EventSource
{
public static readonly EndpointMiddlewareEventSource Log = new EndpointMiddlewareEventSource();

private readonly ConcurrentDictionary<string, EndpointCounter> _endpointCounters = new ConcurrentDictionary<string, EndpointCounter>();

internal EndpointMiddlewareEventSource()
: base("Microsoft.AspNetCore.Routing")
{

}

[Event(1, Level = EventLevel.Informational)]
public void ExecutingEndpoint(string endpoint)
{
var endpointCounter = _endpointCounters.GetOrAdd(endpoint, key => new EndpointCounter(key, this));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@JamesNK JamesNK May 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidfowl Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

endpointCounter.Executing();
WriteEvent(3, endpoint);
}

[Event(2, Level = EventLevel.Informational)]
public void ExecutedEndpoint(string endpoint)
{
var endpointCounter = _endpointCounters.GetOrAdd(endpoint, key => new EndpointCounter(key, this));
endpointCounter.Executed();
WriteEvent(2, endpoint);
}

[Event(3, Level = EventLevel.Error)]
public void FailedEndpoint(string endpoint)
{
var endpointCounter = _endpointCounters.GetOrAdd(endpoint, key => new EndpointCounter(key, this));
endpointCounter.Failed();
WriteEvent(3, endpoint);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: one empty line too much.


private class EndpointCounter
{
private readonly IncrementingPollingCounter _requestsPerSecondCounter;
private readonly PollingCounter _totalRequestsCounter;
private readonly PollingCounter _failedRequestsCounter;
private readonly PollingCounter _currentRequestsCounter;

private long _totalRequests;
private long _currentRequests;
private long _failedRequests;

public EndpointCounter(string endpoint, EventSource eventSource)
{
_requestsPerSecondCounter = new IncrementingPollingCounter($"{endpoint}:requests-per-second", eventSource, () => _totalRequests)
{
DisplayName = $"{endpoint}:Request Rate",
DisplayRateTimeScale = TimeSpan.FromSeconds(1)
};

_totalRequestsCounter = new PollingCounter($"{endpoint}:total-requests", eventSource, () => _totalRequests)
{
DisplayName = $"{endpoint}:Total Requests",
};

_currentRequestsCounter = new PollingCounter($"{endpoint}:current-requests", eventSource, () => _currentRequests)
{
DisplayName = $"{endpoint}:Current Requests"
};

_failedRequestsCounter = new PollingCounter($"{endpoint}:failed-requests", eventSource, () => _failedRequests)
{
DisplayName = $"{endpoint}:Failed Requests"
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove empty line

}

public void Executing()
{
Interlocked.Increment(ref _totalRequests);
Interlocked.Increment(ref _currentRequests);
}

public void Executed()
{
Interlocked.Decrement(ref _currentRequests);
}

public void Failed()
{
Interlocked.Increment(ref _failedRequests);
}
}
}
}