-
Notifications
You must be signed in to change notification settings - Fork 762
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
Add AspNet metrics instrumentation #2985
Add AspNet metrics instrumentation #2985
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2985 +/- ##
==========================================
+ Coverage 84.09% 84.23% +0.13%
==========================================
Files 255 258 +3
Lines 9062 9090 +28
==========================================
+ Hits 7621 7657 +36
+ Misses 1441 1433 -8
|
new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode), | ||
}; | ||
|
||
this.httpServerDuration.Record(activity.Duration.TotalMilliseconds, tags); |
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.
We need to fix the issue of this depending on tracing being enabled for metrics to work. But this limitation already exists in aspnet core as well. It is okay to merge this for an initial release. This is better than nothing.
(We definitely need to remove the Metrics dependency on Tracing before stable.)
src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInMetricsListener.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInMetricsListener.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInMetricsListenerTests.cs
Outdated
Show resolved
Hide resolved
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.
LGTM. Left a comment about allocation. That could be done in a later PR.
test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInMetricsListenerTests.cs
Show resolved
Hide resolved
Co-authored-by: Michael Maxwell <mike.ian.maxwell@gmail.com>
/// <inheritdoc/> | ||
public void Dispose() | ||
{ | ||
this.meter?.Dispose(); |
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.
do we need ? check?
@vishweshbankwar Could you update the example asp.net app to leverage this? https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/examples/AspNet/Global.asax.cs |
Fixes #2556
Changes
This change enables
http.server.duration
metric collection noted in spec for ASP.NET applications. It relies on activity to calculate the duration however, we need to update this at a later stage to make it independent of trace/activity creation. Opened #2994For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes