[http] Add http.client.request.duration metric and .NET Framework support #4870

matt-hensley · 2023-09-20T13:28:54Z

Fixes #4764
Towards #4484

Changes

Adds the new http.client.request.duration metric in the HttpClient Instrumentation library.
Adds support for http.client.duration & http.client.request.duration metrics on .NET Framework. Unlike Add HttpClient metrics for .NET Framework #4768, this new implementation does not require tracing to be enabled for metrics generation to work.

Tests have been updated to cover:

Metrics and tracing enabled
Only metrics enabled
Only tracing enabled
Metrics and tracing disabled

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)

…ensley/opentelemetry-dotnet into net462httpclientmetricsv3

…ensley/opentelemetry-dotnet into net462httpclientmetricsv3 # Conflicts: # src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs

…equestActivitySource.netfx.cs Co-authored-by: Johannes Tax <[email protected]>

…ensley/opentelemetry-dotnet into net462httpclientmetricsv3

…equestActivitySource.netfx.cs Co-authored-by: Johannes Tax <[email protected]>

…ensley/opentelemetry-dotnet into net462httpclientmetricsv3

test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs

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

codecov · 2023-09-27T23:17:39Z

Codecov Report

Merging #4870 (f2e864b) into main (8ea6bb9) will increase coverage by 0.23%.
The diff coverage is 97.61%.

@@            Coverage Diff             @@
##             main    #4870      +/-   ##
==========================================
+ Coverage   82.95%   83.19%   +0.23%     
==========================================
  Files         294      294              
  Lines       12206    12279      +73     
==========================================
+ Hits        10125    10215      +90     
+ Misses       2081     2064      -17

Flag	Coverage Δ
unittests	`83.19% <97.61%> (+0.23%)`	⬆️

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

Files	Coverage Δ
...elemetry.Instrumentation.Http/HttpClientMetrics.cs	`100.00% <100.00%> (ø)`
...ementation/HttpHandlerMetricsDiagnosticListener.cs	`97.61% <100.00%> (+20.47%)`	⬆️
...rumentation.Http/MeterProviderBuilderExtensions.cs	`100.00% <100.00%> (ø)`
...umentation.Http/TracerProviderBuilderExtensions.cs	`100.00% <100.00%> (ø)`
...plementation/HttpWebRequestActivitySource.netfx.cs	`82.93% <96.96%> (+2.06%)`	⬆️

... and 8 files with indirect coverage changes

CodeBlanch · 2023-09-27T23:34:32Z

@vishweshbankwar @alanwest @Kielek This ended up being a collaboration between me and @matt-hensley so I'm not going to self-review. Please take a look when you have time.

TimothyMothra · 2023-09-28T21:41:11Z

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

+    internal static HttpClientInstrumentationOptions TracingOptions
    {
-        get => options;
+        get => tracingOptions;
        set
        {
-            options = value;
+            tracingOptions = value;

-            emitOldAttributes = value.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old);
-            emitNewAttributes = value.HttpSemanticConvention.HasFlag(HttpSemanticConvention.New);
+            tracingEmitOldAttributes = value.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old);
+            tracingEmitNewAttributes = value.HttpSemanticConvention.HasFlag(HttpSemanticConvention.New);
+        }
+    }
+
+    internal static HttpClientMetricInstrumentationOptions MetricsOptions
+    {
+        get => metricsOptions;
+        set
+        {
+            metricsOptions = value;
+
+            metricsEmitOldAttributes = value.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old);
+            metricsEmitNewAttributes = value.HttpSemanticConvention.HasFlag(HttpSemanticConvention.New);
        }
    }


The fact that old & new attributes is separate for Tracing and Metrics seems wrong to me. The HttpSemanticConvention comes from an environment variable. It should be set once for the whole process.

Ya it is weird. I think though if our plan is to remove the new/old/dupe stuff soon we should just live with it for a bit 😄

Having the two does allow you to do some interesting things. Imagine this...

using var tracerProvider = Sdk.CreateTracerProviderBuilder() .AddHttpClientInstrumentation() .AddOtlpExporter() .ConfigureServices(s => s.AddSingleton<IConfiguration>(new ConfigurationBuilder() .AddInMemoryCollection(new Dictionary<string, string> { ["OTEL_SEMCONV_STABILITY_OPT_IN"] = "http" }) .Build())) .Build(); using var meterProvider = Sdk.CreateTracerProviderBuilder() .AddHttpClientInstrumentation() .AddPrometheusExporter() .ConfigureServices(s => s.AddSingleton<IConfiguration>(new ConfigurationBuilder() .AddInMemoryCollection(new Dictionary<string, string> { ["OTEL_SEMCONV_STABILITY_OPT_IN"] = "http/dup" }) .Build())) .Build();

What that would do is send "new" semantic conventions via OTLP for traces and send "dupe" semantic conventions via Prometheus for metrics. That is because we support IConfiguration for each provider not just envvars for a process.

I think though if our plan is to remove the new/old/dupe stuff soon we should just live with it for a bit 😄

👍

TimothyMothra · 2023-09-28T22:20:24Z

test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs

+            if (semanticConvention != null && semanticConvention.Value.HasFlag(HttpSemanticConvention.New))
+            {
+                Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpRequestMethod && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpMethod]);
+                Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeServerAddress && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerName]);


Not a blocker for this PR, but this has a bad smell.
Because the json file wasn't updated, it's using the old attribute names from the test case to validate the new attributes.
When we finally ship stable, the json should be updated to have only the New attributes.

When we finally ship stable, the json should be updated to have only the New attributes.

👍

test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs

vishweshbankwar · 2023-10-02T16:27:49Z

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

+                }
+            }
+
+            HttpClientDuration.Record(durationMs, tags);


We need to use different histogram based on the configured option

metricsEmitOldAttributes - > http.client.duration
metricsEmitNewAttributes -> http.client.request.duration

I think this PR is focused on adding metrics to NetFx, where my PR was adding the NEW metric to the library. I don't think we should try to accomplish both goals in a single PR. :)

@TimothyMothra I think it is ok to do this here as your PR only updates HttpHandlerMetricsDiagnosticListener. But we need two different Histograms for this to be correct.

If the style is "dupe" what happens? You get both old + new histograms each with the full set of tags? Or you get old histogram with the old tags and new histogram with the new tags?

you get old histogram with the old tags and new histogram with the new tags

This^

These HttpClient tests currently are exercising both the .NET Framework & .NET/Core style instrumentations. If we make this change here the tests will start failing for the .NET/Core style. I'm thinking we should tackle both on @TimothyMothra's PR?

Could we remove the new conventions from here then?

If the style is "dupe" what happens? You get both old + new histograms each with the full set of tags? Or you get old histogram with the old tags and new histogram with the new tags?

We would emit old metric using old histogram and old attributes (http.client.duration + miliseconds) AND new metric using new histogram and new attributes (http.client.request.duration + seconds).

Could we remove the new conventions from here then?

I just pulled @TimothyMothra's work from #4891 into this PR. Seemed like more work to remove it all and then put it back in.

I just pulled @TimothyMothra's work from #4891 into this PR.

😲

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

test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs

…lerMetricsDiagnosticListener.cs Co-authored-by: Vishwesh Bankwar <[email protected]>

…ensley/opentelemetry-dotnet into net462httpclientmetricsv3

vishweshbankwar · 2023-10-02T23:17:21Z

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs

+    internal static readonly string MeterVersion = AssemblyName.Version.ToString();
+    internal static readonly Meter Meter = new(MeterName, MeterVersion);
+    private static readonly Histogram<double> HttpClientDuration = Meter.CreateHistogram<double>("http.client.duration", "ms", "Measures the duration of outbound HTTP requests.");
+    private static readonly Histogram<double> HttpClientRequestDuration = Meter.CreateHistogram<double>("http.client.request.duration", "s", "Measures the duration of outbound HTTP requests.");


nit: we are creating an instrument here even in when it will not be used (based on the option selected by user).

Agree, but I wasn't worried about it. For example if user is using only tracing they'll have a Meter + Histogram today that is just sitting there of no use to them. So what's one more thingy? 🤣 Keeping them static readonlys is nice for hot path perf when they are used though. Plus I figured when @TimothyMothra cleans this up we'll go back to just a single histogram in play. Decided to just keep it dumb/simple, basically.

vishweshbankwar

LGTM - We need to work on changelog and readme

Changelog needs to clearly indicate added support for metrics in HttpWebRequest

Readme needs to be updated https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.Http/README.md#list-of-metrics-produced

I am ok with merging this as is and do follow up for items above (In favor of making progress).

Thanks @matt-hensley and @CodeBlanch!

CodeBlanch · 2023-10-02T23:52:43Z

@vishweshbankwar

Changelog needs to clearly indicate added support for metrics in HttpWebRequest

Just FYI it will work on .NET Framework for either HttpClient or HttpWebRequest. For that reason I thought it was well enough to just mention support being added "for .NET Framework" on the CHANGELOG. But if you want to make it more detailed feel free 😄

vishweshbankwar · 2023-10-03T00:00:08Z

@vishweshbankwar

Changelog needs to clearly indicate added support for metrics in HttpWebRequest

Just FYI it will work on .NET Framework for either HttpClient or HttpWebRequest. For that reason I thought it was well enough to just mention support being added "for .NET Framework" on the CHANGELOG. But if you want to make it more detailed feel free 😄

👍 I meant to clarify this. It may not be clear to users what is the difference between those two metrics. We should consolidate it with details here. @TimothyMothra will work on the follow up.

matt-hensley and others added 30 commits September 14, 2023 20:35

snapshot

17d67e0

null actvitiy is tracing is disabled

800a71d

setup metics without tracing

4a9239e

only emit when there is a listener

d7b2472

modify tests for netfx

46de1a9

always capture response status code

4e445b5

Merge branch 'main' into net462httpclientmetricsv3

f0ca432

undo conditional extraction

bed7b2b

activity duration or stopwatch

ba04ac0

activity duration or stopwatch

48c354a

Merge branch 'net462httpclientmetricsv3' of https://github.com/matt-h…

872ce63

…ensley/opentelemetry-dotnet into net462httpclientmetricsv3

activity may be null here

6999167

duration -> durationMs for now

42e9022

StartTimestamp handling

527106f

adopt taglist

e31f687

using directive is unnecessary

01840c0

using directive is unnecessary

9ef2915

Merge branch 'net462httpclientmetricsv3' of https://github.com/matt-h…

47fc955

…ensley/opentelemetry-dotnet into net462httpclientmetricsv3 # Conflicts: # src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs

remove URL attribute

d9ca497

status enum to int

308ca8f

remove ternary

480940e

early return if tracing and metrics are not enabled

27ad5ab

set activity

025c02d

Update src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebR…

dfaa8f7

…equestActivitySource.netfx.cs Co-authored-by: Johannes Tax <[email protected]>

Merge branch 'net462httpclientmetricsv3' of https://github.com/matt-h…

ad10683

…ensley/opentelemetry-dotnet into net462httpclientmetricsv3

Update src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebR…

8bd64ef

…equestActivitySource.netfx.cs Co-authored-by: Johannes Tax <[email protected]>

Update src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebR…

6ca5c9d

…equestActivitySource.netfx.cs Co-authored-by: Johannes Tax <[email protected]>

Merge branch 'net462httpclientmetricsv3' of https://github.com/matt-h…

61a01f7

…ensley/opentelemetry-dotnet into net462httpclientmetricsv3

flag name change

b4dd250

remove Moq from test

bca1361

matt-hensley and others added 2 commits September 26, 2023 13:41

modify test to ensure duration was written

0ce3ced

Fixes, tweaks, and cleanup.

5218b65

CodeBlanch reviewed Sep 27, 2023

View reviewed changes

test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs Show resolved Hide resolved

CodeBlanch reviewed Sep 27, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs Outdated Show resolved Hide resolved

CodeBlanch mentioned this pull request Sep 28, 2023

HttpSemanticConventions - new metric in HttpClient Instrumentation #4891

Closed

4 tasks

TimothyMothra reviewed Sep 28, 2023

View reviewed changes

test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs Outdated Show resolved Hide resolved

Code review.

e70629b