update HttpSemanticConventions for Instrumentation.Http (part2) by TimothyMothra · Pull Request #4639 · open-telemetry/opentelemetry-dotnet

TimothyMothra · 2023-07-06T20:34:00Z

Design discussion issue #4484
Follow up to #4538

Previous PR updated Net Core implementation.
This PR makes the same changes for Net Fx.

Changes

update Instrumentation.Http HttpWebRequestActivitySource.netfx.cs to emit new attributes
- http.method -> htttp.request.method
- net.peer.name -> server.address
- net.peer.port ->server.port
- http.scheme -> REMOVED
- http.url -> url.full
- http.flavor -> network.protocol.version
- http.status.code -> http.response.status.code
remove "url.scheme" from other classes.
This was introduced in the previous PR. This is for server spans only, not client.
Updated tests. This class was large, so I copied the original test class to make it easier to separate the scenarios.
For review, pay attention to the helper methods at the bottom of the class near Line 200 VerifyActivityStartTags() and VerifyActivityStopTags()
- Added HttpWebRequestActivitySourceTestsNew.netfx.cs to test the new attributes.
  This class can replace HttpWebRequestActivitySourceTests.netfx.cs when this library is GA.
- Added HttpWebRequestActivitySourceTestsDupe.netfx.cs to test emitting both new and old attributes.
  This class can be deleted when this library is GA.

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)

codecov · 2023-07-07T18:18:11Z

Codecov Report

Merging #4639 (36475e2) into main (d2b269a) will increase coverage by 0.04%.
The diff coverage is 96.55%.

❗ Current head 36475e2 differs from pull request most recent head 743a311. Consider uploading reports for the commit 743a311 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4639      +/-   ##
==========================================
+ Coverage   84.94%   84.99%   +0.04%     
==========================================
  Files         314      314              
  Lines       12689    12706      +17     
==========================================
+ Hits        10779    10799      +20     
+ Misses       1910     1907       -3

Impacted Files	Coverage Δ
...tp/Implementation/HttpHandlerDiagnosticListener.cs	`68.42% <ø> (+0.59%)`	⬆️
...ementation/HttpHandlerMetricsDiagnosticListener.cs	`75.75% <ø> (+2.22%)`	⬆️
...plementation/HttpWebRequestActivitySource.netfx.cs	`80.86% <96.55%> (+0.62%)`	⬆️

... and 6 files with indirect coverage changes

vishweshbankwar

LGTM - needs test https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs

…emetry-dotnet into 4484_http2

TimothyMothra · 2023-07-12T00:11:35Z

test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsDupe.netfx.cs

+        private static void VerifyActivityStartTags(string netPeerName, int? netPeerPort, string method, string url, Activity activity)
+        {
+            // New
+            Assert.NotNull(activity.TagObjects);
+            Assert.Equal(method, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod));
+            if (netPeerPort != null)
+            {
+                Assert.Equal(netPeerPort, activity.GetTagValue(SemanticConventions.AttributeServerPort));
+            }
+
+            Assert.Equal(netPeerName, activity.GetTagValue(SemanticConventions.AttributeServerAddress));
+
+            Assert.Equal(url, activity.GetTagValue(SemanticConventions.AttributeUrlFull));
+
+            // Old
+            Assert.NotNull(activity.TagObjects);
+            Assert.Equal(method, activity.GetTagValue(SemanticConventions.AttributeHttpMethod));
+            if (netPeerPort != null)
+            {
+                Assert.Equal(netPeerPort, activity.GetTagValue(SemanticConventions.AttributeNetPeerPort));
+            }
+
+            Assert.Equal(netPeerName, activity.GetTagValue(SemanticConventions.AttributeNetPeerName));
+
+            Assert.Equal(url, activity.GetTagValue(SemanticConventions.AttributeHttpUrl));
+        }
+
+        private static void VerifyActivityStopTags(int statusCode, Activity activity)
+        {
+            // New
+            Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpResponseStatusCode));
+
+            // Old
+            Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpStatusCode));
+        }


These are the new validation methods

TimothyMothra · 2023-07-12T00:11:55Z

test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsNew.netfx.cs

+        private static void VerifyActivityStartTags(string netPeerName, int? netPeerPort, string method, string url, Activity activity)
+        {
+            Assert.NotNull(activity.TagObjects);
+            Assert.Equal(method, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod));
+            if (netPeerPort != null)
+            {
+                Assert.Equal(netPeerPort, activity.GetTagValue(SemanticConventions.AttributeServerPort));
+            }
+
+            Assert.Equal(netPeerName, activity.GetTagValue(SemanticConventions.AttributeServerAddress));
+
+            Assert.Equal(url, activity.GetTagValue(SemanticConventions.AttributeUrlFull));
+        }
+
+        private static void VerifyActivityStopTags(int statusCode, Activity activity)
+        {
+            Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpResponseStatusCode));
+        }


These are the new validation methods

utpilla · 2023-07-12T00:12:53Z

test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsDupe.netfx.cs

Copying the entire test file doesn't look right. We really only need to test the new/dupe attributes for just the happy-path scenario. Could you check the attributes only for the happy-path case?

This is the same approach we took for the AspNetCore library.

I can work on a new test that just evaluates the happy-path.

I need more time for this. The attribute changes affect 3 different methods and there's not one test scenario that calls all three. I should have new tests by the end of today (Wed).

A separate class is going to be necessary.
I tried adding a new test to the existing class, but because this is working with static objects, it breaks the other tests in this class.

Discussed with Utkarsh, going to keep the separate test classes to avoid impacting the existing tests.
New classes will scope down to a single test.

utpilla · 2023-07-12T00:13:31Z

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs

-                activity.SetTag(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host);
-                if (!request.RequestUri.IsDefaultPort)
+                // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md
+                if (Options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old))


These could be extracted into boolean variables to avoid checking them repeatedly. This can be done in another PR.

The pattern used in the other libraries won't work here because this is a static class.

Interesting. I'm not sure why this class uses a static instance of the options.

However, you should still be able to extract them into static bool variables in this case and use them instead of checking if the enum has a flag repeatedly.

Not just a static instance of the options, the entire class is static.

I changed the Options field to a property so I can use the setter to evaluate the booleans in this commit: b91509e

utpilla · 2023-07-12T00:36:55Z

Need to update the existing CHANGELOG entry with this PR. It looks like we missed to add the previous PR in the CHANGELOG entry:

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md

Lines 5 to 8 in f95c689

    
           * 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 
        
             which attributes are emitted by setting the environment variable 
        
             `OTEL_SEMCONV_STABILITY_OPT_IN`.

TimothyMothra · 2023-07-13T17:51:49Z

I've corrected the links to the spec (new repo) in all comments and changelog.
Please re-review.

update HttpSemanticConventions for Instrumentation.Http (net fx)

c0b2e57

TimothyMothra requested a review from a team July 6, 2023 20:34

Timothy Mothra added 3 commits July 6, 2023 15:08

Merge branch 'main' into 4484_http2

3397105

fix

0bcfdf3

remove url.scheme as this is for server, not client

0768089

Timothy Mothra and others added 4 commits July 7, 2023 13:24

Merge branch 'main' into 4484_http2

977dbb3

Merge branch 'main' into 4484_http2

d66f7f9

Merge branch 'main' into 4484_http2

4d0a5c7

Merge branch 'main' into 4484_http2

14ecda8

vishweshbankwar approved these changes Jul 11, 2023

View reviewed changes

Timothy Mothra Lee added 5 commits July 11, 2023 15:36

tests

29b64ef

Merge branch 'main' into 4484_http2

51999c4

remove blink line

f9e7b14

Merge branch '4484_http2' of https://github.com/TimothyMothra/opentel…

88f817e

…emetry-dotnet into 4484_http2

update test comment

c2ee1dd

TimothyMothra commented Jul 12, 2023

View reviewed changes

utpilla reviewed Jul 12, 2023

View reviewed changes

Timothy Mothra Lee added 5 commits July 12, 2023 09:40

refactor to use booleans instead of HasFlag

b91509e

update changelog

b326f3c

remove extra tests

80adf0d

Merge branch 'main' into 4484_http2

e5a54f1

correct link to spec in comments and changelog

edcc16d

Timothy Mothra Lee added 2 commits July 13, 2023 11:02

update more comments

d0ff7e3

fix build error

36475e2

utpilla approved these changes Jul 13, 2023

View reviewed changes

Merge branch 'main' into 4484_http2

743a311

utpilla merged commit 5c347ac into open-telemetry:main Jul 13, 2023

fastfend mentioned this pull request Jan 9, 2024

[Sentry.OpenTelemetry] SentrySpanProcessor does not ignore Sentry requests getsentry/sentry-dotnet#3028

Closed

Conversation

TimothyMothra commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Merge requirement checklist

Uh oh!

codecov bot commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vishweshbankwar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

utpilla Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

utpilla commented Jul 12, 2023

Uh oh!

TimothyMothra commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TimothyMothra commented Jul 6, 2023 •

edited

Loading

codecov bot commented Jul 7, 2023 •

edited

Loading

utpilla Jul 12, 2023 •

edited

Loading