Skip to content
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

Allow to filter requests when collecting ASP.NET Core metrics. #3323

Closed
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 @@ -10,5 +10,5 @@ OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.Record
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.RecordException.set -> void
OpenTelemetry.Metrics.MeterProviderBuilderExtensions
OpenTelemetry.Trace.TracerProviderBuilderExtensions
static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder) -> OpenTelemetry.Metrics.MeterProviderBuilder
static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null) -> OpenTelemetry.Metrics.MeterProviderBuilder
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.Record
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.RecordException.set -> void
OpenTelemetry.Metrics.MeterProviderBuilderExtensions
OpenTelemetry.Trace.TracerProviderBuilderExtensions
static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder) -> OpenTelemetry.Metrics.MeterProviderBuilder
static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null) -> OpenTelemetry.Metrics.MeterProviderBuilder
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.Record
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.RecordException.set -> void
OpenTelemetry.Metrics.MeterProviderBuilderExtensions
OpenTelemetry.Trace.TracerProviderBuilderExtensions
static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder) -> OpenTelemetry.Metrics.MeterProviderBuilder
static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null) -> OpenTelemetry.Metrics.MeterProviderBuilder
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.Record
OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions.RecordException.set -> void
OpenTelemetry.Metrics.MeterProviderBuilderExtensions
OpenTelemetry.Trace.TracerProviderBuilderExtensions
static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder) -> OpenTelemetry.Metrics.MeterProviderBuilder
static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null) -> OpenTelemetry.Metrics.MeterProviderBuilder
static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null) -> OpenTelemetry.Trace.TracerProviderBuilder
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,11 @@ internal class AspNetCoreMetrics : IDisposable
/// <summary>
/// Initializes a new instance of the <see cref="AspNetCoreMetrics"/> class.
/// </summary>
public AspNetCoreMetrics()
/// <param name="options">ASP.NET Core Request configuration options.</param>
public AspNetCoreMetrics(AspNetCoreInstrumentationOptions options)
{
this.meter = new Meter(InstrumentationName, InstrumentationVersion);
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInMetricsListener("Microsoft.AspNetCore", this.meter), null);
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInMetricsListener("Microsoft.AspNetCore", this.meter, options), null);
this.diagnosticSourceSubscriber.Subscribe();
}

Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
can still be used with .NET5.0 apps.
([#3147](https://github.com/open-telemetry/opentelemetry-dotnet/issues/3147))

* Respect `AspNetCoreInstrumentationOptions.Filter` when collecting ASP.NET Core metrics.

## 1.0.0-rc9.3

Released 2022-Apr-15
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ internal class AspNetCoreInstrumentationEventSource : EventSource
public static AspNetCoreInstrumentationEventSource Log = new();

[NonEvent]
public void RequestFilterException(Exception ex)
public void RequestFilterException(string handlerName, Exception ex)
{
if (this.IsEnabled(EventLevel.Error, EventKeywords.All))
{
this.RequestFilterException(ex.ToInvariantString());
this.RequestFilterException(handlerName, ex.ToInvariantString());
}
}

Expand All @@ -43,16 +43,16 @@ public void NullPayload(string handlerName, string eventName)
this.WriteEvent(1, handlerName, eventName);
}

[Event(2, Message = "Request is filtered out.", Level = EventLevel.Verbose)]
public void RequestIsFilteredOut(string eventName)
[Event(2, Message = "Request is filtered out from handler '{0}'.", Level = EventLevel.Verbose)]
public void RequestIsFilteredOut(string handlerName, string eventName)
Copy link
Author

Choose a reason for hiding this comment

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

I didn't bump the event versions. It doesn't seem like we care about ETW consumers that cache manifests.

{
this.WriteEvent(2, eventName);
this.WriteEvent(2, handlerName, eventName);
}

[Event(3, Message = "Filter threw exception. Request will not be collected. Exception {0}.", Level = EventLevel.Error)]
public void RequestFilterException(string exception)
[Event(3, Message = "Filter from handler '{0}' threw exception. Request will not be collected. Exception {1}.", Level = EventLevel.Error)]
public void RequestFilterException(string handlerName, string exception)
{
this.WriteEvent(3, exception);
this.WriteEvent(3, handlerName, exception);
}

[NonEvent]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,15 @@ public override void OnStartActivity(Activity activity, object payload)
{
if (this.options.Filter?.Invoke(context) == false)
{
AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(activity.OperationName);
AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInListener), activity.OperationName);
activity.IsAllDataRequested = false;
activity.ActivityTraceFlags &= ~ActivityTraceFlags.Recorded;
return;
}
}
catch (Exception ex)
{
AspNetCoreInstrumentationEventSource.Log.RequestFilterException(ex);
AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInListener), ex);
activity.IsAllDataRequested = false;
activity.ActivityTraceFlags &= ~ActivityTraceFlags.Recorded;
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@
// limitations under the License.
// </copyright>

using System;
using System.Diagnostics;
using System.Diagnostics.Metrics;
using Microsoft.AspNetCore.Http;
#if NETCOREAPP
using Microsoft.AspNetCore.Routing;
#endif
using OpenTelemetry.Internal;
using OpenTelemetry.Trace;

namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation
Expand All @@ -29,12 +31,16 @@ internal class HttpInMetricsListener : ListenerHandler
private readonly PropertyFetcher<HttpContext> stopContextFetcher = new("HttpContext");
private readonly Meter meter;
private readonly Histogram<double> httpServerDuration;
private readonly AspNetCoreInstrumentationOptions options;

public HttpInMetricsListener(string name, Meter meter)
public HttpInMetricsListener(string name, Meter meter, AspNetCoreInstrumentationOptions options)
: base(name)
{
Guard.ThrowIfNull(options);

this.meter = meter;
this.httpServerDuration = meter.CreateHistogram<double>("http.server.duration", "ms", "measures the duration of the inbound HTTP request");
this.options = options;
}

public override void OnStopActivity(Activity activity, object payload)
Expand All @@ -46,6 +52,20 @@ public override void OnStopActivity(Activity activity, object payload)
return;
}

try
{
if (this.options.Filter?.Invoke(context) == false)
{
AspNetCoreInstrumentationEventSource.Log.RequestIsFilteredOut(nameof(HttpInMetricsListener), activity.OperationName);
return;
}
}
catch (Exception ex)
{
AspNetCoreInstrumentationEventSource.Log.RequestFilterException(nameof(HttpInMetricsListener), ex);
return;
}

// TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this.
// Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too).
// If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// limitations under the License.
// </copyright>

using System;
using OpenTelemetry.Instrumentation.AspNetCore;
using OpenTelemetry.Internal;

Expand All @@ -28,23 +29,47 @@ public static class MeterProviderBuilderExtensions
/// Enables the incoming requests automatic data collection for ASP.NET Core.
/// </summary>
/// <param name="builder"><see cref="MeterProviderBuilder"/> being configured.</param>
/// <param name="configureAspNetCoreInstrumentationOptions">ASP.NET Core Request configuration options.</param>
/// <returns>The instance of <see cref="MeterProviderBuilder"/> to chain the calls.</returns>
public static MeterProviderBuilder AddAspNetCoreInstrumentation(
this MeterProviderBuilder builder)
this MeterProviderBuilder builder,
Action<AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions = null)
{
Guard.ThrowIfNull(builder);

// TODO: Implement an IDeferredMeterProviderBuilder

// TODO: Handle AspNetCoreInstrumentationOptions
// Filter - makes sense for metric instrumentation
// Enrich - do we want a similar kind of functionality for metrics?
// RecordException - probably doesn't make sense for metric instrumentation
// EnableGrpcAspNetCoreSupport - this instrumentation will also need to also handle gRPC requests
// EnableGrpcAspNetCoreSupport - this instrumentation will also need to handle gRPC requests

if (builder is IDeferredMeterProviderBuilder deferredMeterProviderBuilder)
{
return deferredMeterProviderBuilder.Configure((sp, builder) =>
{
AddAspNetCoreInstrumentation(builder, sp.GetOptions<AspNetCoreInstrumentationOptions>(), configureAspNetCoreInstrumentationOptions);
});
}

return AddAspNetCoreInstrumentation(builder, new AspNetCoreInstrumentationOptions(), configureAspNetCoreInstrumentationOptions);
}

var instrumentation = new AspNetCoreMetrics();
internal static MeterProviderBuilder AddAspNetCoreInstrumentation(
this MeterProviderBuilder builder,
AspNetCoreMetrics instrumentation)
{
builder.AddMeter(AspNetCoreMetrics.InstrumentationName);
return builder.AddInstrumentation(() => instrumentation);
}

private static MeterProviderBuilder AddAspNetCoreInstrumentation(
MeterProviderBuilder builder,
AspNetCoreInstrumentationOptions options,
Action<AspNetCoreInstrumentationOptions> configure = null)
{
configure?.Invoke(options);
return AddAspNetCoreInstrumentation(
Copy link
Author

Choose a reason for hiding this comment

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

This is where the discrepancy is between traces and metrics - in traces we will new-up HttpInLIstener here and pass it to the instrumentation (AspNetCoreMetrics in this case). Here it is a little bit more complicated, because metrics listener also expects the Meter instance, which is disposable. Currently AspNetCoreMetrics will maintain meter's lifecycle, and I didn't want to push it out to here, which will be hard to do, given that this is a static POP by itself. So I just pass in options to the instrumentation class, and it passes them to the listener. Should not be a big deal, just one thing to note.

builder,
new AspNetCoreMetrics(options));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,45 @@ public async Task RequestMetricIsCaptured()
Assert.Equal(6, attributes.Length);
}

/// <summary>
/// Verifies that <see cref="AspNetCoreInstrumentationOptions.Filter"/> is respected when collecting
/// ASP.NET Core request metrics.
/// </summary>
/// <returns>A <see cref="Task"/> that represents an asynchronous test operation.</returns>
[Fact]
public async Task RequestMetricIsFiltered()
{
var metricItems = new List<Metric>();

this.meterProvider = Sdk.CreateMeterProviderBuilder()
.AddAspNetCoreInstrumentation(options =>
{
options.Filter =
context => !context.Request.Path.Value.StartsWith("/api/");
})
.AddInMemoryExporter(metricItems)
.Build();

using (var client = this.factory.CreateClient())
{
var response = await client.GetAsync("/api/values");
response.EnsureSuccessStatusCode();
}

// We need to let End callback execute as it is executed AFTER response was returned.
// In unit tests environment there may be a lot of parallel unit tests executed, so
// giving some breezing room for the End callback to complete
await Task.Delay(TimeSpan.FromSeconds(1));

this.meterProvider.Dispose();

var requestMetrics = metricItems
.Where(item => item.Name == "http.server.duration")
.ToArray();

Assert.Empty(requestMetrics);
}

public void Dispose()
{
this.meterProvider?.Dispose();
Expand Down