Skip to content
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.

Sign up for GitHub

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

Jump to bottom

update HttpSemanticConventions for Instrumentation.AspNetCore #4537

Merged
merged 29 commits into from
Jun 21, 2023
Merged

update HttpSemanticConventions for Instrumentation.AspNetCore #4537

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c60216f
update http semantic conventions for aspnetcore
TimothyMothra Jun 1, 2023
8b892ac
Merge branch 'main' into 4484_aspnetcore
TimothyMothra Jun 1, 2023
9b5c800
prefix local calls with this
TimothyMothra Jun 1, 2023
d47b1f7
Merge branch '4484_aspnetcore' of https://github.com/TimothyMothra/op…
TimothyMothra Jun 1, 2023
9215a07
pr feedback
TimothyMothra Jun 6, 2023
f78aedf
comment
TimothyMothra Jun 7, 2023
b1fc737
update
TimothyMothra Jun 7, 2023
4de47b4
Merge branch 'main' into 4484_aspnetcore
TimothyMothra Jun 7, 2023
357af6a
Merge branch 'main' into 4484_aspnetcore
TimothyMothra Jun 7, 2023
f2203e3
split tests into two files to solve concurrency failures
TimothyMothra Jun 7, 2023
352f939
Merge branch '4484_aspnetcore' of https://github.com/TimothyMothra/op…
TimothyMothra Jun 7, 2023
d03a1a5
Merge branch 'main' into 4484_aspnetcore
TimothyMothra Jun 9, 2023
14250da
fix attribute name
TimothyMothra Jun 9, 2023
cbd15c7
comment
TimothyMothra Jun 9, 2023
f2716a2
remove unused attribute
TimothyMothra Jun 9, 2023
96ca6b0
Merge branch 'main' into 4484_aspnetcore
vishweshbankwar Jun 12, 2023
85a9120
added another test for the dup scenario
TimothyMothra Jun 13, 2023
86ab41c
Merge branch 'main' into 4484_aspnetcore
TimothyMothra Jun 13, 2023
4da2eb7
Merge branch 'main' into 4484_aspnetcore
TimothyMothra Jun 13, 2023
808646b
changelog
TimothyMothra Jun 14, 2023
406b24f
Merge branch '4484_aspnetcore' of https://github.com/TimothyMothra/op…
TimothyMothra Jun 14, 2023
8796539
Merge branch 'main' into 4484_aspnetcore
TimothyMothra Jun 14, 2023
99eabb8
comments
TimothyMothra Jun 16, 2023
0933da2
update url
TimothyMothra Jun 16, 2023
8f7934c
Merge branch 'main' into 4484_aspnetcore
TimothyMothra Jun 16, 2023
2a6d88c
update changelog
TimothyMothra Jun 16, 2023
bf807eb
fix attribute: server.port
TimothyMothra Jun 19, 2023
3020b62
Merge branch 'main' into 4484_aspnetcore
TimothyMothra Jun 19, 2023
e02b8c7
Merge branch 'main' into 4484_aspnetcore
utpilla Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/OpenTelemetry.Api/Internal/SemanticConventions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,5 +110,19 @@ internal static class SemanticConventions
public const string AttributeExceptionType = "exception.type";
public const string AttributeExceptionMessage = "exception.message";
public const string AttributeExceptionStacktrace = "exception.stacktrace";

// NEW v1.21.0 Http Semantic Conventions
public const string AttributeClientSocketPort = "client.socket.port";
public const string AttributeHttpRequestMethod = "http.request.method";
public const string AttributeHttpResponseStatusCode = "http.response.status_code";
public const string AttributeNetworkProtocolVersion = "network.protocol.version";
public const string AttributeServerAddress = "server.address";
public const string AttributeServerName = "server.name";
TimothyMothra marked this conversation as resolved.
Show resolved Hide resolved
public const string AttributeServerPort = "server.port";
TimothyMothra marked this conversation as resolved.
Show resolved Hide resolved
public const string AttributeUrlFull = "url.full";
public const string AttributeUrlPath = "url.path";
public const string AttributeUrlScheme = "url.scheme";
public const string AttributeUrlQuery = "url.query";
public const string AttributeUserAgentOriginal = "user_agent.original";
}
}
91 changes: 72 additions & 19 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,29 +197,65 @@ 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/semantic-conventions/blob/main/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);
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);
}
}
}

Expand Down Expand Up @@ -247,12 +283,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)
{
Expand Down Expand Up @@ -429,7 +473,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.
Expand All @@ -439,10 +483,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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 context.Connection.RemotePort represent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 server.port

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the value of context.Connection.RemotePort, the documentation states:

Gets or sets the port of the remote target
https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.connectioninfo.remoteport?view=aspnetcore-7.0

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

if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old))
{
activity.SetTag(SemanticConventions.AttributeNetPeerPort, context.Connection.RemotePort);
}

}

bool validConversion = GrpcTagHelper.TryGetGrpcStatusCodeFromActivity(activity, out int status);
if (validConversion)
Expand Down
40 changes: 32 additions & 8 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts...

  • Just reading this work I found "New" and "Old" pretty confusing. Why not Legacy vs Stable or something along those lines?

  • This is probably an issue for some users:

    opentelemetry-dotnet/src/OpenTelemetry.Api/Internal/HttpSemanticConventionHelper.cs

    Line 53 in 5d321ce

    var envVarValue = Environment.GetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN");

    We should be using IConfiguration to get the switch.

  • Shouldn't the environment key have HTTP in the name like OTEL_HTTP_SEMCONV_STABILITY_OPT_IN?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Just reading this work I found "New" and "Old" pretty confusing. Why not Legacy vs Stable or something along those lines?

+1, I had similar concern here #4538 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Just reading this work I found "New" and "Old" pretty confusing. Why not Legacy vs Stable or something along those lines?

How strongly do you feel about this? :)
We can change the enum, but New and Old shipped as part of OpenTelemetry.Api v1.5.0.
It's internal so we CAN change it.
But this would require re-releasing the Api library. (ie Instrumentation.AspNetCore would require an OpenTelemetry.Api v1.5.1).

Also, this is meant to be short lived. When these instrumentation libraries go stable this enum and helper class will be removed.

  1. We should be using IConfiguration to get the switch.

Can you please share an example of this?

  1. Shouldn't the environment key have HTTP in the name like OTEL_HTTP_SEMCONV_STABILITY_OPT_IN?

That makes sense to me, but the spec defines OTEL_SEMCONV_STABILITY_OPT_IN

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change the enum, but New and Old shipped as part of OpenTelemetry.Api v1.5.0.
It's internal so we CAN change it.
But this would require re-releasing the Api library. (ie Instrumentation.AspNetCore would require an OpenTelemetry.Api v1.5.1).

The instrumentation libraries do not use this enum from OpenTelemetry.Api. They link to the file so each of the libraries have their own copy of it. You should be able to make the necessary changes without worrying about version of OpenTelemetry.Api.

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/semantic-conventions/blob/main/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.AttributeServerName, 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))
Expand Down
Loading