-
Notifications
You must be signed in to change notification settings - Fork 823
Simple ASP.NET Core instrumentation capturing request count metric #2159
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,53 @@ | ||||||||||||||
// <copyright file="AspNetCoreMetrics.cs" company="OpenTelemetry Authors"> | ||||||||||||||
// Copyright The OpenTelemetry Authors | ||||||||||||||
// | ||||||||||||||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||
// you may not use this file except in compliance with the License. | ||||||||||||||
// You may obtain a copy of the License at | ||||||||||||||
// | ||||||||||||||
// http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||
// | ||||||||||||||
// Unless required by applicable law or agreed to in writing, software | ||||||||||||||
// distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||
// See the License for the specific language governing permissions and | ||||||||||||||
// limitations under the License. | ||||||||||||||
// </copyright> | ||||||||||||||
|
||||||||||||||
using System; | ||||||||||||||
using System.Diagnostics.Metrics; | ||||||||||||||
using System.Reflection; | ||||||||||||||
using OpenTelemetry.Instrumentation.AspNetCore.Implementation; | ||||||||||||||
|
||||||||||||||
namespace OpenTelemetry.Instrumentation.AspNetCore | ||||||||||||||
{ | ||||||||||||||
/// <summary> | ||||||||||||||
/// Asp.Net Core Requests instrumentation. | ||||||||||||||
/// </summary> | ||||||||||||||
internal class AspNetCoreMetrics : IDisposable | ||||||||||||||
{ | ||||||||||||||
internal static readonly AssemblyName AssemblyName = typeof(HttpInListener).Assembly.GetName(); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to be defensive here in case any exception happened https://social.msdn.microsoft.com/Forums/windowsapps/en-US/ba19700e-8f63-411d-9097-63a37c7e8a4d/assemblygetname-throws-securityexception?forum=netfxtoolsdev. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not blocking this PR, in general we might want to be defensive while having static initialization logic - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea good point. This should be fixed elsewhere as well - here's another example opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs Lines 31 to 36 in a4d8b35
I can grab this in a follow up. |
||||||||||||||
internal static readonly string InstrumentationName = AssemblyName.Name; | ||||||||||||||
internal static readonly string InstrumentationVersion = AssemblyName.Version.ToString(); | ||||||||||||||
|
||||||||||||||
private readonly DiagnosticSourceSubscriber diagnosticSourceSubscriber; | ||||||||||||||
private readonly Meter meter; | ||||||||||||||
|
||||||||||||||
/// <summary> | ||||||||||||||
/// Initializes a new instance of the <see cref="AspNetCoreMetrics"/> class. | ||||||||||||||
/// </summary> | ||||||||||||||
public AspNetCoreMetrics() | ||||||||||||||
{ | ||||||||||||||
this.meter = new Meter(InstrumentationName, InstrumentationVersion); | ||||||||||||||
this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpInMetricsListener("Microsoft.AspNetCore", this.meter), null); | ||||||||||||||
this.diagnosticSourceSubscriber.Subscribe(); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// <inheritdoc/> | ||||||||||||||
public void Dispose() | ||||||||||||||
{ | ||||||||||||||
this.diagnosticSourceSubscriber?.Dispose(); | ||||||||||||||
this.meter?.Dispose(); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
// <copyright file="HttpInMetricsListener.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Diagnostics.Metrics; | ||
using Microsoft.AspNetCore.Http; | ||
using OpenTelemetry.Trace; | ||
|
||
namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation | ||
{ | ||
internal class HttpInMetricsListener : ListenerHandler | ||
{ | ||
private readonly PropertyFetcher<HttpContext> stopContextFetcher = new PropertyFetcher<HttpContext>("HttpContext"); | ||
private readonly Meter meter; | ||
|
||
private Counter<long> httpServerRequestCount; | ||
|
||
public HttpInMetricsListener(string name, Meter meter) | ||
: base(name) | ||
{ | ||
this.meter = meter; | ||
|
||
// TODO: | ||
// In the future, this instrumentation should produce the http.server.duration metric which will likely be represented as a histogram. | ||
// See: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-server | ||
// | ||
// Histograms are not yet supported by the SDK. | ||
// | ||
// For now we produce a count metric called http.server.request_count just for demonstration purposes. | ||
// This metric is not defined by the in the semantic conventions. | ||
this.httpServerRequestCount = meter.CreateCounter<long>("http.server.request_count", null, "The number of HTTP requests processed."); | ||
} | ||
|
||
public override void OnStopActivity(Activity activity, object payload) | ||
{ | ||
HttpContext context = this.stopContextFetcher.Fetch(payload); | ||
if (context == null) | ||
{ | ||
AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), nameof(this.OnStopActivity)); | ||
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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/open-telemetry/opentelemetry-dotnet/blob/metrics/src/OpenTelemetry.Exporter.Prometheus/PrometheusExporterMetricsHttpServer.cs#L114 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yea the comment could probably be more clear - this comment was left over from my original PR from awhile back. I meant suppressing invocations of the In a follow up PR I'll expand the test app to export via Prometheus and OTLP and validate where things stand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we haven't resurrected the middleware version of Prometheus. The in-proc server using HttpListener is resurrected, and it already does suppress. (but since there is no instrumentation for it, it shouldn't get captured anyway). |
||
// If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. | ||
if (context.Request.Path.HasValue && context.Request.Path.Value.Contains("metrics")) | ||
{ | ||
return; | ||
} | ||
|
||
// TODO: This is just a minimal set of attributes. See the spec for additional attributes: | ||
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-server | ||
var tags = new KeyValuePair<string, object>[] | ||
{ | ||
new KeyValuePair<string, object>(SemanticConventions.AttributeHttpMethod, context.Request.Method), | ||
new KeyValuePair<string, object>(SemanticConventions.AttributeHttpScheme, context.Request.Scheme), | ||
new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode), | ||
new KeyValuePair<string, object>(SemanticConventions.AttributeHttpFlavor, context.Request.Protocol), | ||
}; | ||
|
||
this.httpServerRequestCount.Add(1, tags); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
// <copyright file="MeterProviderBuilderExtensions.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
using System; | ||
using OpenTelemetry.Instrumentation.AspNetCore; | ||
|
||
namespace OpenTelemetry.Metrics | ||
{ | ||
/// <summary> | ||
/// Extension methods to simplify registering of ASP.NET Core request instrumentation. | ||
/// </summary> | ||
public static class MeterProviderBuilderExtensions | ||
{ | ||
/// <summary> | ||
/// Enables the incoming requests automatic data collection for ASP.NET Core. | ||
/// </summary> | ||
/// <param name="builder"><see cref="MeterProviderBuilder"/> being configured.</param> | ||
/// <returns>The instance of <see cref="MeterProviderBuilder"/> to chain the calls.</returns> | ||
public static MeterProviderBuilder AddAspNetCoreInstrumentation( | ||
this MeterProviderBuilder builder) | ||
{ | ||
if (builder == null) | ||
{ | ||
throw new ArgumentNullException(nameof(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 | ||
|
||
var instrumentation = new AspNetCoreMetrics(); | ||
builder.AddSource(AspNetCoreMetrics.InstrumentationName); | ||
return builder.AddInstrumentation(() => instrumentation); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
until we add Ext.Hosting support, we can simply add the meterprovider as a singleton in the services, to ensure it gets disposed at end.