Add support for OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS

[ngruson]

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]

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]

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.

[CodeBlanch]

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.

[ngruson]

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?

[ngruson]

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?

[CodeBlanch]

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.

[TimothyMothra]

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

[TimothyMothra]

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]

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]

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]

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]

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]

Closing this PR: See #5197 (comment)