-
Notifications
You must be signed in to change notification settings - Fork 765
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 additional tags to ASP.NET Core metrics #3247
Changes from all commits
22bd3a3
a75ddb1
aaf0d14
f43b1a9
d34da2b
ad4fade
9257e9e
52325fd
4b4fcbe
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 |
---|---|---|
|
@@ -17,6 +17,9 @@ | |
using System.Diagnostics; | ||
using System.Diagnostics.Metrics; | ||
using Microsoft.AspNetCore.Http; | ||
#if NETCOREAPP | ||
using Microsoft.AspNetCore.Routing; | ||
#endif | ||
using OpenTelemetry.Trace; | ||
|
||
namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation | ||
|
@@ -51,16 +54,60 @@ public override void OnStopActivity(Activity activity, object payload) | |
return; | ||
} | ||
|
||
string host; | ||
|
||
if (context.Request.Host.Port is null or 80 or 443) | ||
{ | ||
host = context.Request.Host.Host; | ||
} | ||
else | ||
{ | ||
host = context.Request.Host.Host + ":" + context.Request.Host.Port; | ||
} | ||
|
||
TagList tags; | ||
|
||
// We need following directive as | ||
// RouteEndpoint is not available in netstandard2.0 and netstandard2.1 | ||
#if NETCOREAPP | ||
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 have a 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. Actually, now I'm wondering why we need a 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. This directive only applies to 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. could you add a comment why this compiler directive is relevant. ? 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. @cijothomas - Added - Please review. 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/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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not have that information available during OnStop event. |
||
var target = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; | ||
|
||
// 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 TagList | ||
if (!string.IsNullOrEmpty(target)) | ||
{ | ||
tags = new TagList | ||
{ | ||
{ SemanticConventions.AttributeHttpFlavor, context.Request.Protocol }, | ||
{ SemanticConventions.AttributeHttpScheme, context.Request.Scheme }, | ||
{ SemanticConventions.AttributeHttpMethod, context.Request.Method }, | ||
{ SemanticConventions.AttributeHttpHost, host }, | ||
{ SemanticConventions.AttributeHttpTarget, target }, | ||
{ SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() }, | ||
}; | ||
} | ||
else | ||
{ | ||
tags = new TagList | ||
{ | ||
{ SemanticConventions.AttributeHttpFlavor, context.Request.Protocol }, | ||
{ SemanticConventions.AttributeHttpScheme, context.Request.Scheme }, | ||
{ SemanticConventions.AttributeHttpMethod, context.Request.Method }, | ||
{ SemanticConventions.AttributeHttpHost, host }, | ||
{ SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() }, | ||
}; | ||
} | ||
#else | ||
tags = new TagList | ||
{ | ||
{ SemanticConventions.AttributeHttpMethod, context.Request.Method }, | ||
{ SemanticConventions.AttributeHttpScheme, context.Request.Scheme }, | ||
{ SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode }, | ||
{ SemanticConventions.AttributeHttpFlavor, context.Request.Protocol }, | ||
{ 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. 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/pull/3247/files#r866970771 same comment. we can follow up to fix the string alloc. |
||
}; | ||
|
||
#endif | ||
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.
for a follow up - see if we avoid this string alloc, by using cache/other technique?