Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS #5197

ngruson · 2023-12-21T19:01:50Z

Fixes open-telemetry/opentelemetry-dotnet-contrib#1731

If the HTTP request method is not known to instrumentation, it MUST set the http.request.method attribute to _OTHER.

Changes

Added KnownHttpMethods property to options classes. Populated from environment variables or programmatically.
Respect KnownHttpMethods in AspnetCore and Http instrumentation
Added unit tests

codecov · 2023-12-21T20:24:06Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (6250307) 83.38% compared to head (1d08a17) 83.11%.
Report is 30 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    open-telemetry/opentelemetry-dotnet#5197      +/-   ##
==========================================
- Coverage   83.38%   83.11%   -0.28%     
==========================================
  Files         297      271      -26     
  Lines       12531    11987     -544     
==========================================
- Hits        10449     9963     -486     
+ Misses       2082     2024      -58

Flag	Coverage Δ
unittests	`?`
unittests-Instrumentation-Experimental	`25.21% <93.25%> (?)`
unittests-Instrumentation-Stable	`25.23% <93.25%> (?)`
unittests-Solution-Experimental	`83.08% <93.25%> (?)`
unittests-Solution-Stable	`83.11% <93.25%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...reInstrumentationMeterProviderBuilderExtensions.cs	`100.00% <100.00%> (ø)`
...eInstrumentationTracerProviderBuilderExtensions.cs	`100.00% <100.00%> (ø)`
...ry.Instrumentation.AspNetCore/AspNetCoreMetrics.cs	`100.00% <100.00%> (ø)`
...tation.AspNetCore/Implementation/HttpInListener.cs	`89.43% <100.00%> (-0.15%)`	⬇️
....Instrumentation.Http/HttpClientInstrumentation.cs	`100.00% <100.00%> (ø)`
...ntInstrumentationMeterProviderBuilderExtensions.cs	`100.00% <100.00%> (ø)`
...tInstrumentationTracerProviderBuilderExtensions.cs	`100.00% <100.00%> (ø)`
...elemetry.Instrumentation.Http/HttpClientMetrics.cs	`100.00% <100.00%> (ø)`
...tp/Implementation/HttpHandlerDiagnosticListener.cs	`70.86% <100.00%> (ø)`
...plementation/HttpWebRequestActivitySource.netfx.cs	`80.93% <100.00%> (+0.15%)`	⬆️
... and 2 more

... and 32 files with indirect coverage changes

github-actions · 2023-12-29T03:16:56Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

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

src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs

CodeBlanch · 2024-01-08T23:21:31Z

src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs

@@ -125,6 +125,9 @@ public class HttpClientTraceInstrumentationOptions
    /// </remarks>
    public bool RecordException { get; set; }

+    internal List<string> KnownHttpMethods { get; set; } =


Is this being set from OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS anywhere? I don't see a ctor accepting IConfiguration on this class so it is possible some prep works needs to be done to hook that up. LMK if you would like some help with that.

Correct, that was still a to do.
I now made some changes to read from env variables as well and added a unit test for it.
Can you review?

A question I had:
When OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS is set to an empty string or something illogical like fooBar, http.request.method is still set to GET for example and not to _OTHER.
Do you think this is correct behaviour?

I pushed some changes to the branch. Sorry about that I needed to see how the code was working and then I just started messing around with it 😓

Refactored everything into RequestMethodHelper (instead of options classes). Reason for that is OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS should also apply to metrics. I didn't want to introduce metrics options classes where we don't have them and also didn't want the same code in 4 options classes 🤣 I think now it has better separation of concerns.

Your question above about fooBar. I took a look at the spec and it seems like OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS should fully override the default list. The way it was coded you could only pair down the list of known things. I switched it up so we respect whatever is specified by OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS. I would like @vishweshbankwar to sound off on this. He has more expertise in the area. But he is out until 1/15 so we'll have to wait a bit to get his review.

…opentelemetry-dotnet into 5013-http-known-methods

TimothyMothra · 2024-01-13T00:03:51Z

src/Shared/RequestMethodHelper.cs

+#if NET8_0_OR_GREATER
+    public void SetHttpClientActivityDisplayName(Activity activity, string method)
+#else
+    public void SetHttpClientActivityDisplayName(Activity activity, string method)
+#endif


Why is the #if needed here? Maybe an earlier change that was removed?

TimothyMothra · 2024-01-13T00:07:44Z

src/OpenTelemetry.Instrumentation.Http/OpenTelemetry.Instrumentation.Http.csproj

    <Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" />
-    <Compile Include="$(RepoRoot)\src\Shared\Shims\NullableAttributes.cs" Link="Includes\Shims\NullableAttributes.cs" />
+    <Compile Include="$(RepoRoot)\src\Shared\Options\*.cs" Link="Includes\Options\%(Filename).cs" />


nit: this could be simplified

Suggested change

<Compile Include="$(RepoRoot)\src\Shared\Options\*.cs" Link="Includes\Options\%(Filename).cs" />

<Compile Include="$(RepoRoot)\src\Shared\Options\*.cs" LinkBase="Includes\Options" />

vishweshbankwar · 2024-01-17T16:45:39Z

I think we should not add this for now. OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS isn't supported in .NET8.0. @JamesNK Is there a plan to add support for overriding the known methods in .NET implementation?

JamesNK · 2024-01-17T23:37:17Z

Not at the moment. No one has asked for it yet.

If OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS was set, how would that information be passed to .NET? Would OTEL look for the env var and then call some .NET configuration API to config the known methods?

vishweshbankwar · 2024-01-18T16:10:51Z

Not at the moment. No one has asked for it yet.

If OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS was set, how would that information be passed to .NET? Would OTEL look for the env var and then call some .NET configuration API to config the known methods?

If a config option would be added to .NET (assuming OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS cannot be directly supported within ASP.NET Core), Users would have to directly configure that option in the supported version of .NET. From the instrumentation library side, we would not support OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS environment variable.

github-actions · 2024-01-26T03:16:50Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

vishweshbankwar · 2024-01-30T17:01:19Z

Closing this PR: See #5197 (comment)

Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS

81790f5

ngruson requested a review from a team December 21, 2023 19:01

ngruson added 9 commits December 21, 2023 20:02

Added http unit tests

22aad50

Removed unnecessary using statement

2cf08b2

added inline data to tests

5e4635e

Removed unnecessary using statement

7bf90e5

Removed unnecessary using statement

047b4b7

Removed unnecessary using statement

3bc8c2d

Merge branch 'main' into 5013-http-known-methods

283f818

Fixed unit test http netfx

5e6d36c

Fixed netfx tests

688602f

github-actions bot added Stale Issues and pull requests which have been flagged for closing due to inactivity and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels Dec 29, 2023

ngruson and others added 2 commits January 2, 2024 21:28

Merge branch 'main' into 5013-http-known-methods

92c4df1

Merge branch 'main' into 5013-http-known-methods

40dec15

CodeBlanch reviewed Jan 8, 2024

View reviewed changes

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

CodeBlanch reviewed Jan 8, 2024

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs Outdated Show resolved Hide resolved

CodeBlanch reviewed Jan 8, 2024

View reviewed changes

CodeBlanch added the area:instrumentation label Jan 8, 2024

ngruson and others added 6 commits January 9, 2024 13:15

Processed review comments

f8e1d6d

Merge branch '5013-http-known-methods' of https://github.com/ngruson/…

058e5ba

…opentelemetry-dotnet into 5013-http-known-methods

Removed blank line

9d62ca6

Tweaks.

f89c183

Merge branch 'main' into 5013-http-known-methods

e50836b

Code review and refactoring.

8e2928b

TimothyMothra reviewed Jan 13, 2024

View reviewed changes

Tweaks

1d08a17

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jan 26, 2024

vishweshbankwar closed this Jan 30, 2024

vishweshbankwar mentioned this pull request May 14, 2024

[Instrumentation.AspNetCore, Instrumentation.Http] Revert support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS open-telemetry/opentelemetry-dotnet-contrib#1754

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS #5197

Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS #5197

ngruson commented Dec 21, 2023

codecov bot commented Dec 21, 2023 •

edited

Loading

github-actions bot commented Dec 29, 2023

CodeBlanch Jan 8, 2024

ngruson Jan 9, 2024

ngruson Jan 9, 2024

CodeBlanch Jan 10, 2024

TimothyMothra Jan 13, 2024

TimothyMothra Jan 13, 2024

vishweshbankwar commented Jan 17, 2024

JamesNK commented Jan 17, 2024

vishweshbankwar commented Jan 18, 2024

github-actions bot commented Jan 26, 2024

vishweshbankwar commented Jan 30, 2024

	<Compile Include="$(RepoRoot)\src\Shared\Options\*.cs" Link="Includes\Options\%(Filename).cs" />
	<Compile Include="$(RepoRoot)\src\Shared\Options\*.cs" LinkBase="Includes\Options" />

Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS #5197

Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS #5197

Conversation

ngruson commented Dec 21, 2023

Changes

codecov bot commented Dec 21, 2023 • edited Loading

Codecov Report

github-actions bot commented Dec 29, 2023

CodeBlanch Jan 8, 2024

Choose a reason for hiding this comment

ngruson Jan 9, 2024

Choose a reason for hiding this comment

ngruson Jan 9, 2024

Choose a reason for hiding this comment

CodeBlanch Jan 10, 2024

Choose a reason for hiding this comment

TimothyMothra Jan 13, 2024

Choose a reason for hiding this comment

TimothyMothra Jan 13, 2024

Choose a reason for hiding this comment

vishweshbankwar commented Jan 17, 2024

JamesNK commented Jan 17, 2024

vishweshbankwar commented Jan 18, 2024

github-actions bot commented Jan 26, 2024

vishweshbankwar commented Jan 30, 2024

codecov bot commented Dec 21, 2023 •

edited

Loading