Simple ASP.NET Core instrumentation capturing request count metric#2159
Conversation
Codecov Report
@@ Coverage Diff @@
## metrics #2159 +/- ##
===========================================
- Coverage 75.15% 74.53% -0.63%
===========================================
Files 216 219 +3
Lines 6910 6969 +59
===========================================
+ Hits 5193 5194 +1
- Misses 1717 1775 +58
|
|
Adding @shirhatti the Program Manager from Asp.Net Core diagnostics for review. |
| } | ||
|
|
||
| // 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.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/metrics/src/OpenTelemetry.Exporter.Prometheus/PrometheusExporterMetricsHttpServer.cs#L114
Traces would be suppressed, similar to this.
There was a problem hiding this comment.
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 /metrics endpoint exposed via the ASP.NET Core middleware - pretty sure this would cause an activity to be generated. At least that's what I recall when running this with the old metrics code and exporting to prometheus, though I haven't tried out prometheus now that we've resurrected all the code.
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.
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).
We can resurrect the asp.net core middleware based one and add the suppress flag there, so activity wont be recorded from that operation.
| /// </summary> | ||
| internal class AspNetCoreMetrics : IDisposable | ||
| { | ||
| internal static readonly AssemblyName AssemblyName = typeof(HttpInListener).Assembly.GetName(); |
There was a problem hiding this comment.
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.
Not blocking this PR, in general we might want to be defensive while having static initialization logic - TypeInitializationException could be very hard to troubleshoot.
There was a problem hiding this comment.
Yea good point. This should be fixed elsewhere as well - here's another example
I can grab this in a follow up.
| } | ||
|
|
||
| // TODO: Add IServiceCollection.AddOpenTelemetryMetrics extension method | ||
| var providerBuilder = Sdk.CreateMeterProviderBuilder() |
There was a problem hiding this comment.
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.
|
LGTM as our initial version! Thank you. |
3 participants
Fixes #.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes