-
Notifications
You must be signed in to change notification settings - Fork 773
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
update HttpSemanticConventions for Instrumentation.AspNetCore #4537
Changes from 25 commits
c60216f
8b892ac
9b5c800
d47b1f7
9215a07
f78aedf
b1fc737
4de47b4
357af6a
f2203e3
352f939
d03a1a5
14250da
cbd15c7
f2716a2
96ca6b0
85a9120
86ab41c
4da2eb7
808646b
406b24f
8796539
99eabb8
0933da2
8f7934c
2a6d88c
bf807eb
3020b62
e02b8c7
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 |
---|---|---|
|
@@ -2,6 +2,11 @@ | |
|
||
## Unreleased | ||
|
||
* Updated [Http Semantic Conventions](https://github.com/open-telemetry/semantic-conventions/blob/main/specification/trace/semantic_conventions/http.md). | ||
TimothyMothra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* This library can emit either old, new, or both attributes. Users can control | ||
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 don't need a sub-bullet here. This change is simple enough to be put under the same bullet. |
||
which attributes are emitted by setting the environment variable | ||
`OTEL_SEMCONV_STABILITY_OPT_IN`. | ||
|
||
## 1.5.0-beta.1 | ||
|
||
Released 2023-Jun-05 | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -197,29 +197,66 @@ public void OnStartActivity(Activity activity, object payload) | |||||||||
var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/"; | ||||||||||
activity.DisplayName = path; | ||||||||||
|
||||||||||
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md | ||||||||||
if (request.Host.HasValue) | ||||||||||
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md | ||||||||||
if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeNetHostName, request.Host.Host); | ||||||||||
if (request.Host.HasValue) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeNetHostName, request.Host.Host); | ||||||||||
|
||||||||||
if (request.Host.Port is not null && request.Host.Port != 80 && request.Host.Port != 443) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeNetHostPort, request.Host.Port); | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
if (request.Host.Port is not null && request.Host.Port != 80 && request.Host.Port != 443) | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method); | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpScheme, request.Scheme); | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpTarget, path); | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUri(request)); | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); | ||||||||||
|
||||||||||
if (request.Headers.TryGetValue("User-Agent", out var values)) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeNetHostPort, request.Host.Port); | ||||||||||
var userAgent = values.Count > 0 ? values[0] : null; | ||||||||||
if (!string.IsNullOrEmpty(userAgent)) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpUserAgent, userAgent); | ||||||||||
} | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method); | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpScheme, request.Scheme); | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpTarget, path); | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUri(request)); | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); | ||||||||||
|
||||||||||
if (request.Headers.TryGetValue("User-Agent", out var values)) | ||||||||||
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md | ||||||||||
if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) | ||||||||||
{ | ||||||||||
var userAgent = values.Count > 0 ? values[0] : null; | ||||||||||
if (!string.IsNullOrEmpty(userAgent)) | ||||||||||
if (request.Host.HasValue) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeServerAddress, request.Host.Host); | ||||||||||
|
||||||||||
if (request.Host.Port is not null && request.Host.Port != 80 && request.Host.Port != 443) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeServerPort, request.Host.Port); | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
if (request.QueryString.HasValue) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpUserAgent, userAgent); | ||||||||||
// QueryString should be sanitized. see: https://github.com/open-telemetry/opentelemetry-dotnet/issues/4571 | ||||||||||
activity.SetTag(SemanticConventions.AttributeUrlQuery, request.QueryString.Value); | ||||||||||
vishweshbankwar marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
} | ||||||||||
|
||||||||||
activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, request.Method); | ||||||||||
activity.SetTag(SemanticConventions.AttributeUrlScheme, request.Scheme); | ||||||||||
activity.SetTag(SemanticConventions.AttributeUrlPath, path); | ||||||||||
activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); | ||||||||||
|
||||||||||
if (request.Headers.TryGetValue("User-Agent", out var values)) | ||||||||||
{ | ||||||||||
var userAgent = values.Count > 0 ? values[0] : null; | ||||||||||
if (!string.IsNullOrEmpty(userAgent)) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeUserAgentOriginal, userAgent); | ||||||||||
} | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
|
@@ -247,12 +284,20 @@ public void OnStopActivity(Activity activity, object payload) | |||||||||
|
||||||||||
var response = context.Response; | ||||||||||
|
||||||||||
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); | ||||||||||
if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); | ||||||||||
} | ||||||||||
|
||||||||||
if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); | ||||||||||
} | ||||||||||
|
||||||||||
#if !NETSTANDARD2_0 | ||||||||||
if (this.options.EnableGrpcAspNetCoreSupport && TryGetGrpcMethod(activity, out var grpcMethod)) | ||||||||||
{ | ||||||||||
AddGrpcAttributes(activity, grpcMethod, context); | ||||||||||
this.AddGrpcAttributes(activity, grpcMethod, context); | ||||||||||
} | ||||||||||
else if (activity.Status == ActivityStatusCode.Unset) | ||||||||||
{ | ||||||||||
|
@@ -429,7 +474,7 @@ private static bool TryGetGrpcMethod(Activity activity, out string grpcMethod) | |||||||||
} | ||||||||||
|
||||||||||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||||||||||
private static void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext context) | ||||||||||
private void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext context) | ||||||||||
{ | ||||||||||
// The RPC semantic conventions indicate the span name | ||||||||||
// should not have a leading forward slash. | ||||||||||
|
@@ -439,10 +484,19 @@ private static void AddGrpcAttributes(Activity activity, string grpcMethod, Http | |||||||||
activity.SetTag(SemanticConventions.AttributeRpcSystem, GrpcTagHelper.RpcSystemGrpc); | ||||||||||
if (context.Connection.RemoteIpAddress != null) | ||||||||||
{ | ||||||||||
// TODO: This attribute was changed in v1.13.0 https://github.com/open-telemetry/opentelemetry-specification/pull/2614 | ||||||||||
activity.SetTag(SemanticConventions.AttributeNetPeerIp, context.Connection.RemoteIpAddress.ToString()); | ||||||||||
} | ||||||||||
|
||||||||||
activity.SetTag(SemanticConventions.AttributeNetPeerPort, context.Connection.RemotePort); | ||||||||||
if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeNetPeerPort, context.Connection.RemotePort); | ||||||||||
} | ||||||||||
|
||||||||||
if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) | ||||||||||
{ | ||||||||||
activity.SetTag(SemanticConventions.AttributeClientSocketPort, context.Connection.RemotePort); | ||||||||||
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. Are we clear on what this tag is meant for? And what does this value 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. I reviewed the spec and confirmed this was the wrong attribute. will change 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. Regarding the value of
This is not changed, it was mapped to the former attribute. opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs Lines 491 to 494 in 2a6d88c
|
||||||||||
} | ||||||||||
|
||||||||||
bool validConversion = GrpcTagHelper.TryGetGrpcStatusCodeFromActivity(activity, out int status); | ||||||||||
if (validConversion) | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -82,20 +82,44 @@ public override void OnEventWritten(string name, object payload) | |||
|
||||
TagList tags = default; | ||||
|
||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpMethod, context.Request.Method)); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); | ||||
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md | ||||
if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) | ||||
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. Some thoughts...
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.
+1, I had similar concern here #4538 (comment) 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.
How strongly do you feel about this? :) Also, this is meant to be short lived. When these instrumentation libraries go stable this enum and helper class will be removed.
Can you please share an example of this?
That makes sense to me, but the spec defines OTEL_SEMCONV_STABILITY_OPT_IN 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.
The instrumentation libraries do not use this Note: This does not have to be done in this PR. |
||||
{ | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpMethod, context.Request.Method)); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); | ||||
|
||||
if (context.Request.Host.HasValue) | ||||
{ | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetHostName, context.Request.Host.Host)); | ||||
|
||||
if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) | ||||
{ | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); | ||||
} | ||||
} | ||||
} | ||||
|
||||
if (context.Request.Host.HasValue) | ||||
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md | ||||
if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) | ||||
{ | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetHostName, context.Request.Host.Host)); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); | ||||
|
||||
if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) | ||||
if (context.Request.Host.HasValue) | ||||
{ | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeServerAddress, context.Request.Host.Host)); | ||||
|
||||
if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) | ||||
{ | ||||
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeServerPort, context.Request.Host.Port)); | ||||
} | ||||
} | ||||
} | ||||
|
||||
#if NET6_0_OR_GREATER | ||||
var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; | ||||
if (!string.IsNullOrEmpty(route)) | ||||
|
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.
// replaces: "net.peer.port" and "net.host.port"