Skip to content

Add routing metrics #48637

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

Merged
merged 9 commits into from
Jun 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing.Matching;
using Microsoft.AspNetCore.Routing.ShortCircuit;
using Microsoft.AspNetCore.Testing;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Primitives;
Expand All @@ -21,6 +22,7 @@ public class EndpointRoutingShortCircuitBenchmark
[GlobalSetup]
public void Setup()
{
var routingMetrics = new RoutingMetrics(new TestMeterFactory());
var normalEndpoint = new Endpoint(context => Task.CompletedTask, new EndpointMetadataCollection(), "normal");

_normalEndpointMiddleware = new EndpointRoutingMiddleware(
Expand All @@ -30,6 +32,7 @@ public void Setup()
new BenchmarkEndpointDataSource(),
new DiagnosticListener("benchmark"),
Options.Create(new RouteOptions()),
routingMetrics,
context => Task.CompletedTask);

var shortCircuitEndpoint = new Endpoint(context => Task.CompletedTask, new EndpointMetadataCollection(new ShortCircuitMetadata(200)), "shortcircuit");
Expand All @@ -41,8 +44,8 @@ public void Setup()
new BenchmarkEndpointDataSource(),
new DiagnosticListener("benchmark"),
Options.Create(new RouteOptions()),
routingMetrics,
context => Task.CompletedTask);

}

[Benchmark]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
Expand Down Expand Up @@ -31,6 +31,7 @@
<Compile Include="..\..\test\UnitTests\Matching\TreeRouterMatcherBuilder.cs">
<Link>Matching\TreeRouterMatcherBuilder.cs</Link>
</Compile>
<Compile Include="$(SharedSourceRoot)Metrics\TestMeterFactory.cs" LinkBase="shared" />
<Compile Include="..\..\test\UnitTests\TestObjects\TestServiceProvider.cs" Link="Matching\TestServiceProvider.cs" />
</ItemGroup>

Expand All @@ -39,6 +40,7 @@
<Reference Include="Microsoft.AspNetCore.Http" />
<Reference Include="Microsoft.AspNetCore.Routing" />
<Reference Include="Microsoft.Extensions.DependencyInjection" />
<Reference Include="Microsoft.Extensions.Diagnostics" />
<Reference Include="Microsoft.Extensions.Logging" />

<Compile Include="$(SharedSourceRoot)BenchmarkRunner\*.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public static IServiceCollection AddRoutingCore(this IServiceCollection services
//
services.TryAddSingleton<TemplateBinderFactory, DefaultTemplateBinderFactory>();
services.TryAddSingleton<RoutePatternTransformer, DefaultRoutePatternTransformer>();
services.TryAddSingleton<RoutingMetrics>();

// Set RouteHandlerOptions.ThrowOnBadRequest in development
services.TryAddEnumerable(ServiceDescriptor.Transient<IConfigureOptions<RouteHandlerOptions>, ConfigureRouteHandlerOptions>());
Expand Down
26 changes: 20 additions & 6 deletions src/Http/Routing/src/EndpointRoutingMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Cors.Infrastructure;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Metadata;
using Microsoft.AspNetCore.Routing.Matching;
using Microsoft.AspNetCore.Routing.ShortCircuit;
using Microsoft.Extensions.Logging;
Expand All @@ -22,6 +23,7 @@ internal sealed partial class EndpointRoutingMiddleware
private readonly ILogger _logger;
private readonly EndpointDataSource _endpointDataSource;
private readonly DiagnosticListener _diagnosticListener;
private readonly RoutingMetrics _metrics;
private readonly RequestDelegate _next;
private readonly RouteOptions _routeOptions;
private Task<Matcher>? _initializationTask;
Expand All @@ -33,13 +35,15 @@ public EndpointRoutingMiddleware(
EndpointDataSource rootCompositeEndpointDataSource,
DiagnosticListener diagnosticListener,
IOptions<RouteOptions> routeOptions,
RoutingMetrics metrics,
RequestDelegate next)
{
ArgumentNullException.ThrowIfNull(endpointRouteBuilder);

_matcherFactory = matcherFactory ?? throw new ArgumentNullException(nameof(matcherFactory));
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
_diagnosticListener = diagnosticListener ?? throw new ArgumentNullException(nameof(diagnosticListener));
_metrics = metrics;
_next = next ?? throw new ArgumentNullException(nameof(next));
_routeOptions = routeOptions.Value;

Expand Down Expand Up @@ -98,6 +102,7 @@ private Task SetRoutingAndContinue(HttpContext httpContext)
if (endpoint == null)
{
Log.MatchFailure(_logger);
_metrics.MatchFailure();
}
else
{
Expand All @@ -107,12 +112,21 @@ private Task SetRoutingAndContinue(HttpContext httpContext)
Write(_diagnosticListener, httpContext);
}

Log.MatchSuccess(_logger, endpoint);

if (_logger.IsEnabled(LogLevel.Debug)
&& endpoint.Metadata.GetMetadata<FallbackMetadata>() is not null)
if (_logger.IsEnabled(LogLevel.Debug) || _metrics.MatchSuccessCounterEnabled)
{
Log.FallbackMatch(_logger, endpoint);
var isFallback = endpoint.Metadata.GetMetadata<FallbackMetadata>() is not null;

Log.MatchSuccess(_logger, endpoint);

if (isFallback)
{
Log.FallbackMatch(_logger, endpoint);
}

// It shouldn't be possible for a route to be matched via the route matcher and not have a route.
// Just in case, add a special (missing) value as the route tag to metrics.
var route = endpoint.Metadata.GetMetadata<IRouteDiagnosticsMetadata>()?.Route ?? "(missing)";
_metrics.MatchSuccess(route, isFallback);
}

var shortCircuitMetadata = endpoint.Metadata.GetMetadata<ShortCircuitMetadata>();
Expand Down Expand Up @@ -290,7 +304,7 @@ public static void MatchSkipped(ILogger logger, Endpoint endpoint)
[LoggerMessage(6, LogLevel.Information, "The endpoint '{EndpointName}' is being short circuited without running additional middleware or producing a response.", EventName = "ShortCircuitedEndpoint")]
public static partial void ShortCircuitedEndpoint(ILogger logger, Endpoint endpointName);

[LoggerMessage(7, LogLevel.Debug, "Matched endpoint '{EndpointName}' is a fallback endpoint.", EventName = "FallbackMatch", SkipEnabledCheck = true)]
[LoggerMessage(7, LogLevel.Debug, "Matched endpoint '{EndpointName}' is a fallback endpoint.", EventName = "FallbackMatch")]
public static partial void FallbackMatch(ILogger logger, Endpoint endpointName);
}
}
1 change: 1 addition & 0 deletions src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
<Reference Include="Microsoft.AspNetCore.Authorization" />
<Reference Include="Microsoft.AspNetCore.Http.Extensions" />
<Reference Include="Microsoft.AspNetCore.Routing.Abstractions" />
<Reference Include="Microsoft.Extensions.Diagnostics.Abstractions" />
<Reference Include="Microsoft.Extensions.ObjectPool" />
<Reference Include="Microsoft.Extensions.Options" />
</ItemGroup>
Expand Down
43 changes: 43 additions & 0 deletions src/Http/Routing/src/RoutingMetrics.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.Metrics;
using Microsoft.Extensions.Diagnostics.Metrics;

namespace Microsoft.AspNetCore.Routing;

internal sealed class RoutingMetrics
{
public const string MeterName = "Microsoft.AspNetCore.Routing";

private readonly Meter _meter;
private readonly Counter<long> _matchSuccessCounter;
private readonly Counter<long> _matchFailureCounter;

public RoutingMetrics(IMeterFactory meterFactory)
{
_meter = meterFactory.Create(MeterName);

_matchSuccessCounter = _meter.CreateCounter<long>(
"routing-match-success",
description: "Number of requests that successfully matched to an endpoint.");

_matchFailureCounter = _meter.CreateCounter<long>(
"routing-match-failure",
description: "Number of requests that failed to match to an endpoint. An unmatched request may be handled by later middleware, such as the static files or authentication middleware.");
}

public bool MatchSuccessCounterEnabled => _matchSuccessCounter.Enabled;

public void MatchSuccess(string route, bool isFallback)
{
_matchSuccessCounter.Add(1,
new KeyValuePair<string, object?>("route", route),
new KeyValuePair<string, object?>("fallback", isFallback));
}

public void MatchFailure()
{
_matchFailureCounter.Add(1);
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to be clear in the docs that there are lots of requests handled by middleware that will be reported as route match failures. E.g. static files, auth callbacks, etc. I wonder if that will make this counter more noise than value.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@geeknoid @tekian Is this a counter you want?

Copy link
Member

Choose a reason for hiding this comment

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

We'll need to be clear in the docs that there are lots of requests handled by middleware that will be reported as route match failures. E.g. static files, auth callbacks, etc. I wonder if that will make this counter more noise than value.

Can you show an example of this? I'm not sure how desirable this is. We need to verify this for all sorts of applications. (blazor to apis)

Copy link
Member Author

@JamesNK JamesNK Jun 10, 2023

Choose a reason for hiding this comment

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

I thought about this counter more, and I think we should keep it.

  • It doesn't cost anything to have it. Implementation is simple, and the counter doesn't hurt perf if no one listens. If someone doesn't care about it, then they won't listen.
  • If people see a counter that reports route matching success, they'll expect a counter that reports failure. Provides symmetry.
  • Potential confusion about what the counter reports and what it means can be explained in the description. The description can say that unmatched requests may be handled later by other middleware, such as static files or authentication callbacks. The description could even recommend http-server-unhandled-requests if you want a counter for requests that go nowhere.

I'll make these changes and then merge this PR soon. If I don't hear feedback, I'll assume people are okay with this. It's not an end to discussion; I don't want to hold up metrics progress towards done.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.AspNetCore.Routing.Patterns;
using Microsoft.AspNetCore.Routing.TestObjects;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Diagnostics.Metrics;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Options;
using Moq;
Expand Down Expand Up @@ -354,6 +355,7 @@ private IServiceProvider CreateServices(MatcherFactory matcherFactory)
services.AddSingleton<MatcherFactory>(matcherFactory);
}

services.AddMetrics();
services.AddLogging();
services.AddOptions();
services.AddRouting();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Routing.Matching;
using Microsoft.AspNetCore.Routing.TestObjects;
using Microsoft.AspNetCore.Testing;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Logging.Testing;
Expand Down Expand Up @@ -90,7 +91,6 @@ public async Task Invoke_BackCompatGetRouteValue_ValueUsedFromEndpointFeature()
{
// Arrange
var httpContext = CreateHttpContext();

var middleware = CreateMiddleware();

// Act
Expand Down Expand Up @@ -282,6 +282,7 @@ private EndpointRoutingMiddleware CreateMiddleware(
logger ??= new Logger<EndpointRoutingMiddleware>(NullLoggerFactory.Instance);
matcherFactory ??= new TestMatcherFactory(true);
listener ??= new DiagnosticListener("Microsoft.AspNetCore");
var metrics = new RoutingMetrics(new TestMeterFactory());

var middleware = new EndpointRoutingMiddleware(
matcherFactory,
Expand All @@ -290,6 +291,7 @@ private EndpointRoutingMiddleware CreateMiddleware(
new DefaultEndpointDataSource(),
listener,
Options.Create(new RouteOptions()),
metrics,
next);

return middleware;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@
<Reference Include="Microsoft.AspNetCore.Routing" />
<Reference Include="Microsoft.AspNetCore.Http.Results" />
<Reference Include="Microsoft.Extensions.DependencyInjection" />
<Reference Include="Microsoft.Extensions.Diagnostics" />
<Reference Include="Microsoft.Extensions.Hosting.Abstractions" />
<Reference Include="Microsoft.Extensions.Logging" />
<Reference Include="Microsoft.Extensions.WebEncoders" />

<Compile Include="$(SharedSourceRoot)Metrics\TestMeterFactory.cs" LinkBase="shared" />
</ItemGroup>

</Project>
Loading