update HttpSemanticConventions for Instrumentation.AspNetCore #4537

TimothyMothra · 2023-06-01T21:43:36Z

Design discussion issue #4484

Http Semantic Convention is introducing breaking changes.
This PR makes the change in the Instrumentation.AspNetCore library.

Changes

add new attributes to SemanticConventions
update Instrumentation.AspNetCore classes HttpInListener and HttpInMetricsListener to emit new attributes.
- net.host.name -> server.address
- net.host.port -> server.port
- NOT COLLECTED -> url.query
- http.method -> http.request.method
- http.scheme -> url.scheme
- http.target -> url.path
- http.url -> REMOVED
- http.flavor -> network.protocol.version
- http.user_agent -> user_agent.original
- http.status_code -> http.response.status_code

Please provide a brief description of the changes here.

Merge requirement checklist

CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

@vishweshbankwar please review.

…entelemetry-dotnet into 4484_aspnetcore

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

codecov · 2023-06-07T20:04:34Z

Codecov Report

Merging #4537 (e02b8c7) into main (7333a55) will decrease coverage by 0.07%.
The diff coverage is 80.35%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4537      +/-   ##
==========================================
- Coverage   85.13%   85.06%   -0.07%     
==========================================
  Files         312      312              
  Lines       12532    12564      +32     
==========================================
+ Hits        10669    10688      +19     
- Misses       1863     1876      +13

Impacted Files	Coverage Δ
...AspNetCore/Implementation/HttpInMetricsListener.cs	`64.00% <50.00%> (-11.00%)`	⬇️
...tation.AspNetCore/Implementation/HttpInListener.cs	`90.32% <94.73%> (+0.07%)`	⬆️

... and 2 files with indirect coverage changes

…entelemetry-dotnet into 4484_aspnetcore

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

src/OpenTelemetry.Api/Internal/SemanticConventions.cs

...Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs

vishweshbankwar

LGTM overall with minor suggestion.

…entelemetry-dotnet into 4484_aspnetcore

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

src/OpenTelemetry.Api/Internal/SemanticConventions.cs

...nstrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs

utpilla · 2023-06-16T01:53:13Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

+
+            if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New))
+            {
+                activity.SetTag(SemanticConventions.AttributeClientSocketPort, context.Connection.RemotePort);


Are we clear on what this tag is meant for? And what does this value context.Connection.RemotePort represent?

I reviewed the spec and confirmed this was the wrong attribute. will change to server.port

v1.20.0
https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md#http-client

v1.21.0
https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md#http-server-semantic-conventions

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);

}

CodeBlanch · 2023-06-19T19:15:41Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs

-                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))


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?

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)

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.

We should be using IConfiguration to get the switch.

Can you please share an example of this?

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

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.

utpilla · 2023-06-21T01:50:20Z

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

@@ -2,6 +2,11 @@

 ## Unreleased

+* Updated [Http Semantic Conventions](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md).
+  * This library can emit either old, new, or both attributes. Users can control


We don't need a sub-bullet here. This change is simple enough to be put under the same bullet.

utpilla · 2023-06-21T02:32:56Z

Merging this PR to unblock progress.

As discussed in the SIG meeting today. @TimothyMothra would work on using IConfiguration and updating the enum in follow-up PRs.

utpilla · 2023-06-21T02:35:12Z

src/OpenTelemetry.Api/Internal/SemanticConventions.cs

+        public const string AttributeHttpResponseStatusCode = "http.response.status_code"; // replaces: "http.status_code" (AttributeHttpStatusCode)
+        public const string AttributeNetworkProtocolVersion = "network.protocol.version"; // replaces: "http.flavor" (AttributeHttpFlavor)
+        public const string AttributeServerAddress = "server.address"; // replaces: "net.host.name" (AttributeNetHostName)
+        public const string AttributeServerPort = "server.port"; // replaces: "net.host.port" (AttributeNetHostPort)


// replaces: "net.peer.port" and "net.host.port"

update http semantic conventions for aspnetcore

c60216f

TimothyMothra requested a review from a team as a code owner June 1, 2023 21:43

TimothyMothra added 3 commits June 1, 2023 14:43

Merge branch 'main' into 4484_aspnetcore

8b892ac

prefix local calls with this

9b5c800

Merge branch '4484_aspnetcore' of https://github.com/TimothyMothra/op…

d47b1f7

…entelemetry-dotnet into 4484_aspnetcore

vishweshbankwar reviewed Jun 5, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs Outdated Show resolved Hide resolved

vishweshbankwar reviewed Jun 5, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs Outdated Show resolved Hide resolved

pr feedback

9215a07

TimothyMothra added 6 commits June 7, 2023 13:31

comment

f78aedf

update

b1fc737

Merge branch 'main' into 4484_aspnetcore

4de47b4

Merge branch 'main' into 4484_aspnetcore

357af6a

split tests into two files to solve concurrency failures

f2203e3

Merge branch '4484_aspnetcore' of https://github.com/TimothyMothra/op…

352f939

…entelemetry-dotnet into 4484_aspnetcore

vishweshbankwar reviewed Jun 8, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs Show resolved Hide resolved

Merge branch 'main' into 4484_aspnetcore

d03a1a5

vishweshbankwar reviewed Jun 9, 2023

View reviewed changes

src/OpenTelemetry.Api/Internal/SemanticConventions.cs Outdated Show resolved Hide resolved

TimothyMothra and others added 4 commits June 9, 2023 10:00

fix attribute name

14250da

comment

cbd15c7

remove unused attribute

f2716a2

Merge branch 'main' into 4484_aspnetcore

96ca6b0

vishweshbankwar reviewed Jun 12, 2023

View reviewed changes

...Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs Show resolved Hide resolved

vishweshbankwar approved these changes Jun 12, 2023

View reviewed changes

TimothyMothra added 6 commits June 12, 2023 17:17

added another test for the dup scenario

85a9120

Merge branch 'main' into 4484_aspnetcore

86ab41c

Merge branch 'main' into 4484_aspnetcore

4da2eb7

changelog

808646b

Merge branch '4484_aspnetcore' of https://github.com/TimothyMothra/op…

406b24f

…entelemetry-dotnet into 4484_aspnetcore

Merge branch 'main' into 4484_aspnetcore

8796539

utpilla reviewed Jun 15, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md Outdated Show resolved Hide resolved

utpilla reviewed Jun 15, 2023

View reviewed changes

src/OpenTelemetry.Api/Internal/SemanticConventions.cs Outdated Show resolved Hide resolved

TimothyMothra added 4 commits June 15, 2023 17:25

comments

99eabb8

update url

0933da2

Merge branch 'main' into 4484_aspnetcore

8f7934c

update changelog

2a6d88c

utpilla approved these changes Jun 16, 2023

View reviewed changes

...nstrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs Show resolved Hide resolved

utpilla reviewed Jun 16, 2023

View reviewed changes

CodeBlanch reviewed Jun 19, 2023

View reviewed changes

TimothyMothra added 2 commits June 19, 2023 14:55

fix attribute: server.port

bf807eb

Merge branch 'main' into 4484_aspnetcore

3020b62

utpilla reviewed Jun 21, 2023

View reviewed changes

Merge branch 'main' into 4484_aspnetcore

e02b8c7

utpilla merged commit 8e5e7dd into open-telemetry:main Jun 21, 2023
29 checks passed

utpilla reviewed Jun 21, 2023

View reviewed changes

TimothyMothra deleted the 4484_aspnetcore branch June 21, 2023 17:00

TimothyMothra mentioned this pull request Jun 21, 2023

update HttpSemanticConventions for Instrumentation.AspNetCore (part2) #4606

Merged

4 tasks

vishweshbankwar mentioned this pull request Jun 29, 2023

[Instrumentations] Use IConfiguration for semantic conventions compatibility check #4627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update HttpSemanticConventions for Instrumentation.AspNetCore #4537

update HttpSemanticConventions for Instrumentation.AspNetCore #4537

TimothyMothra commented Jun 1, 2023 •

edited

Loading

codecov bot commented Jun 7, 2023 •

edited

Loading

vishweshbankwar left a comment

utpilla Jun 16, 2023

TimothyMothra Jun 19, 2023

TimothyMothra Jun 19, 2023

CodeBlanch Jun 19, 2023

reyang Jun 19, 2023

TimothyMothra Jun 19, 2023

utpilla Jun 21, 2023

utpilla Jun 21, 2023

utpilla commented Jun 21, 2023

utpilla Jun 21, 2023

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

update HttpSemanticConventions for Instrumentation.AspNetCore #4537

update HttpSemanticConventions for Instrumentation.AspNetCore #4537

Conversation

TimothyMothra commented Jun 1, 2023 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Jun 7, 2023 • edited Loading

Codecov Report

vishweshbankwar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla commented Jun 21, 2023

Choose a reason for hiding this comment

TimothyMothra commented Jun 1, 2023 •

edited

Loading

codecov bot commented Jun 7, 2023 •

edited

Loading