Add additional tags to ASP.NET Core metrics #3247
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3247 +/- ##
==========================================
+ Coverage 85.36% 85.49% +0.12%
==========================================
Files 261 261
Lines 9395 9410 +15
==========================================
+ Hits 8020 8045 +25
+ Misses 1375 1365 -10
|
There was a problem hiding this comment.
LGTM
Just checking what Prometheus .NET client does:
- https://github.com/prometheus-net/prometheus-net/blob/ff337560e66af5dd45070014d6e554febb599f16/Prometheus.AspNetCore/HttpMetrics/HttpRequestMiddlewareBase.cs#L264-L277
- https://github.com/prometheus-net/prometheus-net/blob/ff337560e66af5dd45070014d6e554febb599f16/Prometheus.AspNetCore/HttpMetrics/HttpRequestMiddlewareBase.cs#L195
Seems to allow users to map known route values as dimensions. We may want to do similar at some point.
|
|
||
| TagList tags; | ||
|
|
||
| #if NETCOREAPP |
There was a problem hiding this comment.
We have a net6.0 and netstandard2.1 build of this instrumentation. Does this directive apply to those targets?
There was a problem hiding this comment.
Actually, now I'm wondering why we need a netstandard2.1 target given that we have a netcoreapp3.1 target.
There was a problem hiding this comment.
This directive only applies to netcoreapp3.1 and net6.0. It does not apply to netstandard2.1or netstandard2.0
There was a problem hiding this comment.
could you add a comment why this compiler directive is relevant. ?
There was a problem hiding this comment.
@cijothomas - Added - Please review.
There was a problem hiding this comment.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs#L243 -- could you check this part.. it does use reflection to get the releveant info on routes...
There was a problem hiding this comment.
We do not have that information available during OnStop event.
| { SemanticConventions.AttributeHttpMethod, context.Request.Method }, | ||
| { SemanticConventions.AttributeHttpHost, host }, | ||
| { SemanticConventions.AttributeHttpTarget, target }, | ||
| { SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode }, |
There was a problem hiding this comment.
nit: Spec says this should be string, but I think StatusCode is an int here.
| } | ||
| else | ||
| { | ||
| host = context.Request.Host.Host + ":" + context.Request.Host.Port; |
There was a problem hiding this comment.
for a follow up - see if we avoid this string alloc, by using cache/other technique?
https://github.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/add-aspnetcore-metrics-additional-dimensions
| { SemanticConventions.AttributeHttpScheme, context.Request.Scheme }, | ||
| { SemanticConventions.AttributeHttpMethod, context.Request.Method }, | ||
| { SemanticConventions.AttributeHttpHost, host }, | ||
| { SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() }, |
There was a problem hiding this comment.
https://github.com/open-telemetry/opentelemetry-dotnet/pull/3247/files#r866970771 same comment. we can follow up to fix the string alloc.
Fixes #3083
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