From 17d67e0fedebb1e1c62c4b388d840a3ff7a38732 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Thu, 14 Sep 2023 20:35:26 -0400 Subject: [PATCH 01/44] snapshot --- .../HttpWebRequestActivitySource.netfx.cs | 50 ++++++++++++------- .../MeterProviderBuilderExtensions.cs | 8 ++- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 670ce8f3613..d2aca83993d 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -17,6 +17,7 @@ #if NETFRAMEWORK using System.Collections; using System.Diagnostics; +using System.Diagnostics.Metrics; using System.Net; using System.Reflection; using System.Reflection.Emit; @@ -39,12 +40,15 @@ internal static class HttpWebRequestActivitySource internal static readonly AssemblyName AssemblyName = typeof(HttpWebRequestActivitySource).Assembly.GetName(); internal static readonly string ActivitySourceName = AssemblyName.Name + ".HttpWebRequest"; internal static readonly string ActivityName = ActivitySourceName + ".HttpRequestOut"; + internal static readonly string MeterInstrumentationName = AssemblyName.Name; internal static readonly Func> HttpWebRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name); internal static readonly Action HttpWebRequestHeaderValuesSetter = (request, name, value) => request.Headers.Add(name, value); private static readonly Version Version = AssemblyName.Version; private static readonly ActivitySource WebRequestActivitySource = new ActivitySource(ActivitySourceName, Version.ToString()); + private static readonly Meter WebRequestMeter = new Meter(MeterInstrumentationName, Version.ToString()); + private static readonly Histogram HttpClientDuration; private static HttpClientInstrumentationOptions options; @@ -87,6 +91,7 @@ static HttpWebRequestActivitySource() PerformInjection(); Options = new HttpClientInstrumentationOptions(); + HttpClientDuration = WebRequestMeter.CreateHistogram("http.client.duration", "ms", "Measures the duration of outbound HTTP requests."); } catch (Exception ex) { @@ -265,17 +270,6 @@ private static bool IsRequestInstrumented(HttpWebRequest request) private static void ProcessRequest(HttpWebRequest request) { - if (!WebRequestActivitySource.HasListeners() || !Options.EventFilterHttpWebRequest(request)) - { - // No subscribers to the ActivitySource or User provider Filter is - // filtering this request. - // Propagation must still be done in such cases, to allow - // downstream services to continue from parent context, if any. - // Eg: Parent could be the Asp.Net activity. - InstrumentRequest(request, Activity.Current?.Context ?? default); - return; - } - if (IsRequestInstrumented(request)) { // This request was instrumented by previous @@ -283,17 +277,23 @@ private static void ProcessRequest(HttpWebRequest request) return; } - var activity = WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); + // No subscribers to the ActivitySource or User provider Filter is + // filtering this request. + var skipTracing = !WebRequestActivitySource.HasListeners() || !Options.EventFilterHttpWebRequest(request); + var activity = skipTracing + ? new Activity("filtered") + : WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); var activityContext = Activity.Current?.Context ?? default; // Propagation must still be done in all cases, to allow // downstream services to continue from parent context, if any. // Eg: Parent could be the Asp.Net activity. InstrumentRequest(request, activityContext); + + // There is a listener but it decided not to sample the current request. if (activity == null) { - // There is a listener but it decided not to sample the current request. - return; + activity = new Activity("test"); } IAsyncResult asyncContext = writeAResultAccessor(request); @@ -340,7 +340,7 @@ private static void HookOrProcessResult(HttpWebRequest request) if (endCalledAccessor.Invoke(readAsyncContext) || readAsyncContext.CompletedSynchronously) { // We need to process the result directly because the read callback has already fired. Force a copy because response has likely already been disposed. - ProcessResult(readAsyncContext, null, writeAsyncContextCallback.Activity, resultAccessor(readAsyncContext), true); + ProcessResult(readAsyncContext, null, writeAsyncContextCallback.Activity, resultAccessor(readAsyncContext), true, request, -1); return; } @@ -349,8 +349,15 @@ private static void HookOrProcessResult(HttpWebRequest request) asyncCallbackModifier(readAsyncContext, callback.AsyncCallback); } - private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncCallback, Activity activity, object result, bool forceResponseCopy) + private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncCallback, Activity activity, object result, bool forceResponseCopy, HttpWebRequest request, long startTimestamp) { + HttpClientDuration.Record(activity.Duration.TotalMilliseconds); + + if (activity.DisplayName == "detached") + { + return; + } + // We could be executing on a different thread now so restore the activity if needed. if (Activity.Current != activity) { @@ -1122,12 +1129,21 @@ public AsyncCallbackWrapper(HttpWebRequest request, Activity activity, AsyncCall public AsyncCallback OriginalCallback { get; } + public long StartTimestamp { get; } + public void AsyncCallback(IAsyncResult asyncResult) { object result = resultAccessor(asyncResult); if (result is Exception || result is HttpWebResponse) { - ProcessResult(asyncResult, this.OriginalCallback, this.Activity, result, false); + ProcessResult( + asyncResult, + this.OriginalCallback, + this.Activity, + result, + forceResponseCopy: false, + this.Request, + this.StartTimestamp); } this.OriginalCallback?.Invoke(asyncResult); diff --git a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs index 38d375436b0..f15950fa0de 100644 --- a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs @@ -52,9 +52,13 @@ public static MeterProviderBuilder AddHttpClientInstrumentation( // Filter - makes sense for metric instrumentation // Enrich - do we want a similar kind of functionality for metrics? // RecordException - probably doesn't make sense for metric instrumentation - +#if NETFRAMEWORK + builder.AddMeter(HttpWebRequestActivitySource.MeterInstrumentationName); +#else builder.AddMeter(HttpClientMetrics.InstrumentationName); - return builder.AddInstrumentation(sp => new HttpClientMetrics( + builder.AddInstrumentation(sp => new HttpClientMetrics( sp.GetRequiredService>().CurrentValue)); +#endif + return builder; } } From 800a71d01af1c3cdce04b9c80545a1ab21204d49 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Thu, 14 Sep 2023 21:01:02 -0400 Subject: [PATCH 02/44] null actvitiy is tracing is disabled --- .../HttpWebRequestActivitySource.netfx.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index d2aca83993d..7aab57df73b 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -281,7 +281,7 @@ private static void ProcessRequest(HttpWebRequest request) // filtering this request. var skipTracing = !WebRequestActivitySource.HasListeners() || !Options.EventFilterHttpWebRequest(request); var activity = skipTracing - ? new Activity("filtered") + ? null : WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); var activityContext = Activity.Current?.Context ?? default; @@ -290,12 +290,6 @@ private static void ProcessRequest(HttpWebRequest request) // Eg: Parent could be the Asp.Net activity. InstrumentRequest(request, activityContext); - // There is a listener but it decided not to sample the current request. - if (activity == null) - { - activity = new Activity("test"); - } - IAsyncResult asyncContext = writeAResultAccessor(request); if (asyncContext != null) { @@ -313,7 +307,10 @@ private static void ProcessRequest(HttpWebRequest request) asyncCallbackModifier(asyncContext, callback.AsyncCallback); } - AddRequestTagsAndInstrumentRequest(request, activity); + if (activity != null) + { + AddRequestTagsAndInstrumentRequest(request, activity); + } } private static void HookOrProcessResult(HttpWebRequest request) From 4a9239e497cedc883d9b260fb3befaf321bb6c0a Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Thu, 14 Sep 2023 21:01:16 -0400 Subject: [PATCH 03/44] setup metics without tracing --- .../HttpWebRequestActivitySource.netfx.cs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 7aab57df73b..2568f91a010 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -348,10 +348,21 @@ private static void HookOrProcessResult(HttpWebRequest request) private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncCallback, Activity activity, object result, bool forceResponseCopy, HttpWebRequest request, long startTimestamp) { - HttpClientDuration.Record(activity.Duration.TotalMilliseconds); - - if (activity.DisplayName == "detached") + // Activity may be null if we are not tracing in these cases: + // 1. No listeners + // 2. Request was filtered out + // 3. Request was not sampled + if (activity == null) { + if (HttpClientDuration.Enabled) + { + var endTimestamp = Stopwatch.GetTimestamp(); + var duration = endTimestamp - startTimestamp; + var durationS = duration / Stopwatch.Frequency; + var durationMs = durationS * 1000; + HttpClientDuration.Record(durationMs); + } + return; } From d7b2472f3629344bc38dc1659b8aa23193683c7e Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Thu, 14 Sep 2023 21:40:51 -0400 Subject: [PATCH 04/44] only emit when there is a listener --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 2568f91a010..57319555b0d 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -411,6 +411,11 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC } activity.Stop(); + + if (HttpClientDuration.Enabled) + { + HttpClientDuration.Record(activity.Duration.TotalMilliseconds); + } } private static void PrepareReflectionObjects() From 46de1a9feddee4a67ba1d17e6441db1086d2f41d Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Thu, 14 Sep 2023 21:41:05 -0400 Subject: [PATCH 05/44] modify tests for netfx --- .../HttpClientTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 0ceeaa1eb42..620222a49af 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -159,9 +159,6 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut Assert.True(enrichWithExceptionCalled); } -#if NETFRAMEWORK - Assert.Empty(requestMetrics); -#else Assert.Single(requestMetrics); var metric = requestMetrics[0]; @@ -193,7 +190,11 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut var method = new KeyValuePair(SemanticConventions.AttributeHttpMethod, tc.Method); var scheme = new KeyValuePair(SemanticConventions.AttributeHttpScheme, "http"); var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, tc.ResponseCode == 0 ? 200 : tc.ResponseCode); +#if NETFRAMEWORK + var flavor = new KeyValuePair(SemanticConventions.AttributeHttpFlavor, "1.1"); +#else var flavor = new KeyValuePair(SemanticConventions.AttributeHttpFlavor, "2.0"); +#endif var hostName = new KeyValuePair(SemanticConventions.AttributeNetPeerName, tc.ResponseExpected ? host : "sdlfaldfjalkdfjlkajdflkajlsdjf"); var portNumber = new KeyValuePair(SemanticConventions.AttributeNetPeerPort, port); Assert.Contains(hostName, attributes); @@ -211,7 +212,6 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut Assert.DoesNotContain(statusCode, attributes); Assert.Equal(5, attributes.Length); } -#endif } [Fact] From 4e445b5a10825f415ed55c85d0ab5720fdbea7c0 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Sun, 17 Sep 2023 21:39:51 -0400 Subject: [PATCH 06/44] always capture response status code --- .../HttpWebRequestActivitySource.netfx.cs | 47 ++++++++++++------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 57319555b0d..c8cd295faa9 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -348,26 +348,14 @@ private static void HookOrProcessResult(HttpWebRequest request) private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncCallback, Activity activity, object result, bool forceResponseCopy, HttpWebRequest request, long startTimestamp) { + HttpStatusCode? httpStatusCode = null; + // Activity may be null if we are not tracing in these cases: // 1. No listeners // 2. Request was filtered out // 3. Request was not sampled - if (activity == null) - { - if (HttpClientDuration.Enabled) - { - var endTimestamp = Stopwatch.GetTimestamp(); - var duration = endTimestamp - startTimestamp; - var durationS = duration / Stopwatch.Frequency; - var durationMs = durationS * 1000; - HttpClientDuration.Record(durationMs); - } - - return; - } - // We could be executing on a different thread now so restore the activity if needed. - if (Activity.Current != activity) + if (activity != null && Activity.Current != activity) { Activity.Current = activity; } @@ -381,8 +369,9 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC else { HttpWebResponse response = (HttpWebResponse)result; + bool copyResponse = forceResponseCopy || (asyncCallback == null && isContextAwareResultChecker(asyncResult)); - if (forceResponseCopy || (asyncCallback == null && isContextAwareResultChecker(asyncResult))) + if (copyResponse) { // For async calls (where asyncResult is ContextAwareResult)... // If no callback was set assume the user is manually calling BeginGetResponse & EndGetResponse @@ -398,10 +387,12 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC }); AddResponseTags(responseCopy, activity); + httpStatusCode = responseCopy.StatusCode; } else { AddResponseTags(response, activity); + httpStatusCode = response.StatusCode; } } } @@ -414,7 +405,29 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC if (HttpClientDuration.Enabled) { - HttpClientDuration.Record(activity.Duration.TotalMilliseconds); + if (httpStatusCode.HasValue) + { + HttpClientDuration.Record( + activity.Duration.TotalMilliseconds, + new(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)), + new(SemanticConventions.AttributeHttpMethod, request.Method), + new(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), + new(SemanticConventions.AttributeHttpStatusCode, httpStatusCode.Value), + new(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri)), + new(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host), + new(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port)); + } + else + { + HttpClientDuration.Record( + activity.Duration.TotalMilliseconds, + new(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)), + new(SemanticConventions.AttributeHttpMethod, request.Method), + new(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), + new(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri)), + new(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host), + new(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port)); + } } } From bed7b2b2dc618c5a1fd70912ed0b9b18f7b85944 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 09:40:21 -0400 Subject: [PATCH 07/44] undo conditional extraction --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index c8cd295faa9..a3905c0f1fa 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -369,9 +369,8 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC else { HttpWebResponse response = (HttpWebResponse)result; - bool copyResponse = forceResponseCopy || (asyncCallback == null && isContextAwareResultChecker(asyncResult)); - if (copyResponse) + if (forceResponseCopy || (asyncCallback == null && isContextAwareResultChecker(asyncResult))) { // For async calls (where asyncResult is ContextAwareResult)... // If no callback was set assume the user is manually calling BeginGetResponse & EndGetResponse From ba04ac0d9d809571ac02aceaf81841ce6e4bc154 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 09:53:51 -0400 Subject: [PATCH 08/44] activity duration or stopwatch --- .../HttpWebRequestActivitySource.netfx.cs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index a3905c0f1fa..f894519ea98 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -404,10 +404,24 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC if (HttpClientDuration.Enabled) { + double duration = 0; + + if (activity != null) + { + duration = activity.Duration.TotalMilliseconds; + } + else + { + var endTimestamp = Stopwatch.GetTimestamp(); + var durationS = (endTimestamp - startTimestamp) / Stopwatch.Frequency; + var durationMs = durationS * 1000; + duration = durationMs; + } + if (httpStatusCode.HasValue) { HttpClientDuration.Record( - activity.Duration.TotalMilliseconds, + duration, new(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)), new(SemanticConventions.AttributeHttpMethod, request.Method), new(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), From 48c354afa88fde4664a70d9c0fc8c20c7e035bf9 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 09:53:51 -0400 Subject: [PATCH 09/44] activity duration or stopwatch --- .../HttpWebRequestActivitySource.netfx.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index a3905c0f1fa..66de2bada89 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -404,10 +404,24 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC if (HttpClientDuration.Enabled) { + double duration = 0; + + if (activity != null) + { + duration = activity.Duration.TotalMilliseconds; + } + else + { + var endTimestamp = Stopwatch.GetTimestamp(); + var durationS = (endTimestamp - startTimestamp) / Stopwatch.Frequency; + var durationMs = durationS * 1000; + duration = durationMs; + } + if (httpStatusCode.HasValue) { HttpClientDuration.Record( - activity.Duration.TotalMilliseconds, + duration, new(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)), new(SemanticConventions.AttributeHttpMethod, request.Method), new(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), @@ -419,7 +433,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC else { HttpClientDuration.Record( - activity.Duration.TotalMilliseconds, + duration, new(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)), new(SemanticConventions.AttributeHttpMethod, request.Method), new(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), From 699916730698964d0ea61f0defa1dcd999f66c6c Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 10:37:55 -0400 Subject: [PATCH 10/44] activity may be null here --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 66de2bada89..19daf337a92 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -400,7 +400,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC HttpInstrumentationEventSource.Log.FailedProcessResult(ex); } - activity.Stop(); + activity?.Stop(); if (HttpClientDuration.Enabled) { From 42e9022b42b2fa0a629537b2459c867f86fc68f5 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 15:03:42 -0400 Subject: [PATCH 11/44] duration -> durationMs for now --- .../HttpWebRequestActivitySource.netfx.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 19daf337a92..75624391610 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -404,24 +404,23 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC if (HttpClientDuration.Enabled) { - double duration = 0; + double durationMs = 0; if (activity != null) { - duration = activity.Duration.TotalMilliseconds; + durationMs = activity.Duration.TotalMilliseconds; } else { var endTimestamp = Stopwatch.GetTimestamp(); var durationS = (endTimestamp - startTimestamp) / Stopwatch.Frequency; - var durationMs = durationS * 1000; - duration = durationMs; + durationMs = durationS * 1000; } if (httpStatusCode.HasValue) { HttpClientDuration.Record( - duration, + durationMs, new(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)), new(SemanticConventions.AttributeHttpMethod, request.Method), new(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), @@ -433,7 +432,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC else { HttpClientDuration.Record( - duration, + durationMs, new(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)), new(SemanticConventions.AttributeHttpMethod, request.Method), new(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), From 527106f7128e42f5a5f587643f66bab7cfaa5871 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 15:06:03 -0400 Subject: [PATCH 12/44] StartTimestamp handling --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 75624391610..579c6f7dae4 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -295,7 +295,7 @@ private static void ProcessRequest(HttpWebRequest request) { // Flow here is for [Begin]GetRequestStream[Async]. - AsyncCallbackWrapper callback = new AsyncCallbackWrapper(request, activity, asyncCallbackAccessor(asyncContext)); + AsyncCallbackWrapper callback = new AsyncCallbackWrapper(request, activity, asyncCallbackAccessor(asyncContext), Stopwatch.GetTimestamp()); asyncCallbackModifier(asyncContext, callback.AsyncCallback); } else @@ -303,7 +303,7 @@ private static void ProcessRequest(HttpWebRequest request) // Flow here is for [Begin]GetResponse[Async] without a prior call to [Begin]GetRequestStream[Async]. asyncContext = readAResultAccessor(request); - AsyncCallbackWrapper callback = new AsyncCallbackWrapper(request, activity, asyncCallbackAccessor(asyncContext)); + AsyncCallbackWrapper callback = new AsyncCallbackWrapper(request, activity, asyncCallbackAccessor(asyncContext), Stopwatch.GetTimestamp()); asyncCallbackModifier(asyncContext, callback.AsyncCallback); } @@ -342,7 +342,7 @@ private static void HookOrProcessResult(HttpWebRequest request) } // Hook into the result callback if it hasn't already fired. - AsyncCallbackWrapper callback = new AsyncCallbackWrapper(writeAsyncContextCallback.Request, writeAsyncContextCallback.Activity, asyncCallbackAccessor(readAsyncContext)); + AsyncCallbackWrapper callback = new AsyncCallbackWrapper(writeAsyncContextCallback.Request, writeAsyncContextCallback.Activity, asyncCallbackAccessor(readAsyncContext), Stopwatch.GetTimestamp()); asyncCallbackModifier(readAsyncContext, callback.AsyncCallback); } @@ -1154,11 +1154,12 @@ public override void Clear() /// private sealed class AsyncCallbackWrapper { - public AsyncCallbackWrapper(HttpWebRequest request, Activity activity, AsyncCallback originalCallback) + public AsyncCallbackWrapper(HttpWebRequest request, Activity activity, AsyncCallback originalCallback, long startTimestamp) { this.Request = request; this.Activity = activity; this.OriginalCallback = originalCallback; + this.StartTimestamp = startTimestamp; } public HttpWebRequest Request { get; } From e31f687d5c666152dd837abf492556268cedd4e8 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 15:10:48 -0400 Subject: [PATCH 13/44] adopt taglist --- .../HttpWebRequestActivitySource.netfx.cs | 31 +++++++------------ 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 579c6f7dae4..eee120c6bc9 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -417,29 +417,20 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC durationMs = durationS * 1000; } + TagList tags = default; + tags.Add(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)); + tags.Add(SemanticConventions.AttributeHttpMethod, request.Method); + tags.Add(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme); + tags.Add(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri)); + tags.Add(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host); + tags.Add(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port); + if (httpStatusCode.HasValue) { - HttpClientDuration.Record( - durationMs, - new(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)), - new(SemanticConventions.AttributeHttpMethod, request.Method), - new(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), - new(SemanticConventions.AttributeHttpStatusCode, httpStatusCode.Value), - new(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri)), - new(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host), - new(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port)); - } - else - { - HttpClientDuration.Record( - durationMs, - new(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)), - new(SemanticConventions.AttributeHttpMethod, request.Method), - new(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), - new(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri)), - new(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host), - new(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port)); + tags.Add(SemanticConventions.AttributeHttpStatusCode, httpStatusCode.Value); } + + HttpClientDuration.Record(durationMs, tags); } } From 01840c0f6ee4b4438a93bb4989682b322028c452 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 15:15:48 -0400 Subject: [PATCH 14/44] using directive is unnecessary --- .../MeterProviderBuilderExtensions.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs index f15950fa0de..f3e42fbd59b 100644 --- a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs @@ -14,8 +14,6 @@ // limitations under the License. // -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Options; using OpenTelemetry.Instrumentation.Http; using OpenTelemetry.Instrumentation.Http.Implementation; using OpenTelemetry.Internal; From 9ef2915a828597ddbe4b5579b7b49d3924f2aa83 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 15:15:48 -0400 Subject: [PATCH 15/44] using directive is unnecessary --- .../MeterProviderBuilderExtensions.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs index f15950fa0de..a2b8fec0dc0 100644 --- a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs @@ -14,8 +14,11 @@ // limitations under the License. // +#if NETFRAMEWORK +#else using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; +#endif using OpenTelemetry.Instrumentation.Http; using OpenTelemetry.Instrumentation.Http.Implementation; using OpenTelemetry.Internal; From d9ca497284cb30ca493f824fec7f1bd2aa1f5d1e Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 15:45:37 -0400 Subject: [PATCH 16/44] remove URL attribute --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index eee120c6bc9..fa200fc5c10 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -421,7 +421,6 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC tags.Add(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)); tags.Add(SemanticConventions.AttributeHttpMethod, request.Method); tags.Add(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme); - tags.Add(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri)); tags.Add(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host); tags.Add(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port); From 308ca8f2418f733fd3939cf9577f0a2a9b6d3ad1 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 15:45:47 -0400 Subject: [PATCH 17/44] status enum to int --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index fa200fc5c10..6fb8c1fd930 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -426,7 +426,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC if (httpStatusCode.HasValue) { - tags.Add(SemanticConventions.AttributeHttpStatusCode, httpStatusCode.Value); + tags.Add(SemanticConventions.AttributeHttpStatusCode, (int)httpStatusCode.Value); } HttpClientDuration.Record(durationMs, tags); From 480940e4c2264a543e9107284dbdac2cef2341fc Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 20:15:32 -0400 Subject: [PATCH 18/44] remove ternary --- .../HttpWebRequestActivitySource.netfx.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 6fb8c1fd930..28f9d83dff5 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -277,12 +277,13 @@ private static void ProcessRequest(HttpWebRequest request) return; } - // No subscribers to the ActivitySource or User provider Filter is - // filtering this request. - var skipTracing = !WebRequestActivitySource.HasListeners() || !Options.EventFilterHttpWebRequest(request); - var activity = skipTracing - ? null - : WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); + Activity activity = null; + + if (!skipTracing) + { + WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); + } + var activityContext = Activity.Current?.Context ?? default; // Propagation must still be done in all cases, to allow From 27ad5ab2b8f53851c404f4e1ae9be4841e6f2457 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Mon, 18 Sep 2023 20:15:48 -0400 Subject: [PATCH 19/44] early return if tracing and metrics are not enabled --- .../HttpWebRequestActivitySource.netfx.cs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 28f9d83dff5..b6dafa0cd71 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -270,6 +270,21 @@ private static bool IsRequestInstrumented(HttpWebRequest request) private static void ProcessRequest(HttpWebRequest request) { + // No subscribers to the ActivitySource or User provider Filter is + // filtering this request. + var skipTracing = !WebRequestActivitySource.HasListeners() || !Options.EventFilterHttpWebRequest(request); + + if (skipTracing && !HttpClientDuration.Enabled) + { + // Tracing and metrics are not enabled, so we can skip generating signals + // Propagation must still be done in such cases, to allow + // downstream services to continue from parent context, if any. + // Eg: Parent could be the Asp.Net activity. + InstrumentRequest(request, Activity.Current?.Context ?? default); + return; + } + + if (IsRequestInstrumented(request)) { // This request was instrumented by previous From 025c02d238b4781686814b84d3c9c868ca3330b1 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Tue, 19 Sep 2023 09:52:13 -0400 Subject: [PATCH 20/44] set activity --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index b6dafa0cd71..6512f47dc2d 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -296,7 +296,7 @@ private static void ProcessRequest(HttpWebRequest request) if (!skipTracing) { - WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); + activity = WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); } var activityContext = Activity.Current?.Context ?? default; From dfaa8f7573750a79158f233d3918d58e989c4149 Mon Sep 17 00:00:00 2001 From: Matt Hensley <130569+matt-hensley@users.noreply.github.com> Date: Tue, 19 Sep 2023 10:18:12 -0400 Subject: [PATCH 21/44] Update src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs Co-authored-by: Johannes Tax --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index b6dafa0cd71..b6d48dc9f93 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -299,7 +299,7 @@ private static void ProcessRequest(HttpWebRequest request) WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); } - var activityContext = Activity.Current?.Context ?? default; + var activityContext = activity?.Context ?? default; // Propagation must still be done in all cases, to allow // downstream services to continue from parent context, if any. From 8bd64ef58c5aa682cbb9e51cd83ce62d40f7e344 Mon Sep 17 00:00:00 2001 From: Matt Hensley <130569+matt-hensley@users.noreply.github.com> Date: Tue, 19 Sep 2023 10:19:02 -0400 Subject: [PATCH 22/44] Update src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs Co-authored-by: Johannes Tax --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index b6d48dc9f93..5c0e573a127 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -353,7 +353,7 @@ private static void HookOrProcessResult(HttpWebRequest request) if (endCalledAccessor.Invoke(readAsyncContext) || readAsyncContext.CompletedSynchronously) { // We need to process the result directly because the read callback has already fired. Force a copy because response has likely already been disposed. - ProcessResult(readAsyncContext, null, writeAsyncContextCallback.Activity, resultAccessor(readAsyncContext), true, request, -1); + ProcessResult(readAsyncContext, null, writeAsyncContextCallback.Activity, resultAccessor(readAsyncContext), true, request, writeAsyncContextCallback.StartTimestamp); return; } From 6ca5c9dc467c7683d446c1ae9d14343d36a8fe1b Mon Sep 17 00:00:00 2001 From: Matt Hensley <130569+matt-hensley@users.noreply.github.com> Date: Tue, 19 Sep 2023 10:20:14 -0400 Subject: [PATCH 23/44] Update src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs Co-authored-by: Johannes Tax --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 5c0e573a127..5abc1f7ab63 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -270,11 +270,11 @@ private static bool IsRequestInstrumented(HttpWebRequest request) private static void ProcessRequest(HttpWebRequest request) { - // No subscribers to the ActivitySource or User provider Filter is + // There are subscribers to the ActivitySource and no user-provided filter is // filtering this request. - var skipTracing = !WebRequestActivitySource.HasListeners() || !Options.EventFilterHttpWebRequest(request); + var enableTracing = WebRequestActivitySource.HasListeners() && Options.EventFilterHttpWebRequest(request); - if (skipTracing && !HttpClientDuration.Enabled) + if (!enableTracing && !HttpClientDuration.Enabled) { // Tracing and metrics are not enabled, so we can skip generating signals // Propagation must still be done in such cases, to allow From b4dd250a3df1e3dc302b29a317f2870b51be4e7a Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Tue, 19 Sep 2023 11:40:37 -0400 Subject: [PATCH 24/44] flag name change --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 6b4243b96fb..d7eefcc90e3 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -294,7 +294,7 @@ private static void ProcessRequest(HttpWebRequest request) Activity activity = null; - if (!skipTracing) + if (enableTracing) { activity = WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); } From bca1361744d3657876efdf8025a3c5af61428363 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Tue, 19 Sep 2023 11:44:22 -0400 Subject: [PATCH 25/44] remove Moq from test --- .../HttpClientTests.cs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 620222a49af..6668b79b53f 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -20,7 +20,6 @@ #endif using System.Reflection; using System.Text.Json; -using Moq; using OpenTelemetry.Metrics; using OpenTelemetry.Tests; using OpenTelemetry.Trace; @@ -51,10 +50,10 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut out var host, out var port); - var processor = new Mock>(); tc.Url = HttpTestData.NormalizeValues(tc.Url, host, port); var metrics = new List(); + var activities = new List(); var meterProvider = Sdk.CreateMeterProviderBuilder() .AddHttpClientInstrumentation() @@ -71,7 +70,7 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut opt.EnrichWithException = (activity, exception) => { enrichWithExceptionCalled = true; }; opt.RecordException = tc.RecordException ?? false; }) - .AddProcessor(processor.Object) + .AddInMemoryExporter(activities) .Build()) { try @@ -110,8 +109,7 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut .Where(metric => metric.Name == "http.client.duration") .ToArray(); - Assert.Equal(5, processor.Invocations.Count); // SetParentProvider/OnStart/OnEnd/OnShutdown/Dispose called. - var activity = (Activity)processor.Invocations[2].Arguments[0]; + var activity = Assert.Single(activities); Assert.Equal(ActivityKind.Client, activity.Kind); Assert.Equal(tc.SpanName, activity.DisplayName); @@ -262,7 +260,6 @@ private static async Task CheckEnrichment(Sampler sampler, bool enrichExpected, bool enrichWithHttpRequestMessageCalled = false; bool enrichWithHttpResponseMessageCalled = false; - var processor = new Mock>(); using (Sdk.CreateTracerProviderBuilder() .SetSampler(sampler) .AddHttpClientInstrumentation(options => @@ -273,7 +270,6 @@ private static async Task CheckEnrichment(Sampler sampler, bool enrichExpected, options.EnrichWithHttpRequestMessage = (activity, httpRequestMessage) => { enrichWithHttpRequestMessageCalled = true; }; options.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) => { enrichWithHttpResponseMessageCalled = true; }; }) - .AddProcessor(processor.Object) .Build()) { using var c = new HttpClient(); From 5200afb7b3f284a8ad322f6dbdd818c2b53fa10e Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Tue, 19 Sep 2023 13:23:31 -0400 Subject: [PATCH 26/44] need to pick context off current activity for propagator to work --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index d7eefcc90e3..19f5e0e5f93 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -299,7 +299,7 @@ private static void ProcessRequest(HttpWebRequest request) activity = WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); } - var activityContext = activity?.Context ?? default; + var activityContext = Activity.Current?.Context ?? default; // Propagation must still be done in all cases, to allow // downstream services to continue from parent context, if any. From 4d15ca6d6d01e7a2b2752b1f703496e171169d00 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Tue, 19 Sep 2023 13:25:39 -0400 Subject: [PATCH 27/44] extra line --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 19f5e0e5f93..fad1057f4f8 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -284,7 +284,6 @@ private static void ProcessRequest(HttpWebRequest request) return; } - if (IsRequestInstrumented(request)) { // This request was instrumented by previous From dd07ca019de8d190cc0d6a5b85ff432c9b944f4c Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Tue, 19 Sep 2023 15:01:49 -0400 Subject: [PATCH 28/44] modify tests to handle metrics and/or tracing on/off --- .../HttpClientTests.cs | 105 ++++++++++++------ 1 file changed, 72 insertions(+), 33 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 6668b79b53f..5bda2de32db 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -34,6 +34,14 @@ public partial class HttpClientTests [Theory] [MemberData(nameof(TestData))] public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOutTestCase tc) + { + await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, true, true).ConfigureAwait(false); + await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, true, false).ConfigureAwait(false); + await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, false, true).ConfigureAwait(false); + await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, false, false).ConfigureAwait(false); + } + + private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.HttpOutTestCase tc, bool enableTracing, bool enableMetrics) { bool enrichWithHttpWebRequestCalled = false; bool enrichWithHttpWebResponseCalled = false; @@ -55,13 +63,21 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut var metrics = new List(); var activities = new List(); - var meterProvider = Sdk.CreateMeterProviderBuilder() - .AddHttpClientInstrumentation() + var meterProviderBuilder = Sdk.CreateMeterProviderBuilder(); + + if (enableMetrics) + { + meterProviderBuilder.AddHttpClientInstrumentation(); + } + + var meterProvider = meterProviderBuilder .AddInMemoryExporter(metrics) .Build(); + var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder(); - using (Sdk.CreateTracerProviderBuilder() - .AddHttpClientInstrumentation((opt) => + if (enableTracing) + { + tracerProviderBuilder.AddHttpClientInstrumentation((opt) => { opt.EnrichWithHttpWebRequest = (activity, httpRequestMessage) => { enrichWithHttpWebRequestCalled = true; }; opt.EnrichWithHttpWebResponse = (activity, httpResponseMessage) => { enrichWithHttpWebResponseCalled = true; }; @@ -69,9 +85,14 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut opt.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) => { enrichWithHttpResponseMessageCalled = true; }; opt.EnrichWithException = (activity, exception) => { enrichWithExceptionCalled = true; }; opt.RecordException = tc.RecordException ?? false; - }) + }); + } + + var tracerProvider = tracerProviderBuilder .AddInMemoryExporter(activities) - .Build()) + .Build(); + + using (tracerProvider) { try { @@ -109,19 +130,25 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut .Where(metric => metric.Name == "http.client.duration") .ToArray(); - var activity = Assert.Single(activities); + if (!enableTracing) + { + Assert.Empty(activities); + } + else + { + var activity = Assert.Single(activities); - Assert.Equal(ActivityKind.Client, activity.Kind); - Assert.Equal(tc.SpanName, activity.DisplayName); + Assert.Equal(ActivityKind.Client, activity.Kind); + Assert.Equal(tc.SpanName, activity.DisplayName); #if NETFRAMEWORK - Assert.True(enrichWithHttpWebRequestCalled); - Assert.False(enrichWithHttpRequestMessageCalled); - if (tc.ResponseExpected) - { - Assert.True(enrichWithHttpWebResponseCalled); - Assert.False(enrichWithHttpResponseMessageCalled); - } + Assert.True(enrichWithHttpWebRequestCalled); + Assert.False(enrichWithHttpRequestMessageCalled); + if (tc.ResponseExpected) + { + Assert.True(enrichWithHttpWebResponseCalled); + Assert.False(enrichWithHttpResponseMessageCalled); + } #else Assert.False(enrichWithHttpWebRequestCalled); Assert.True(enrichWithHttpRequestMessageCalled); @@ -132,29 +159,36 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut } #endif - // Assert.Equal(tc.SpanStatus, d[span.Status.CanonicalCode]); - Assert.Equal(tc.SpanStatus, activity.Status.ToString()); + // Assert.Equal(tc.SpanStatus, d[span.Status.CanonicalCode]); + Assert.Equal(tc.SpanStatus, activity.Status.ToString()); - if (tc.SpanStatusHasDescription.HasValue) - { - var desc = activity.StatusDescription; - Assert.Equal(tc.SpanStatusHasDescription.Value, !string.IsNullOrEmpty(desc)); - } + if (tc.SpanStatusHasDescription.HasValue) + { + var desc = activity.StatusDescription; + Assert.Equal(tc.SpanStatusHasDescription.Value, !string.IsNullOrEmpty(desc)); + } - var normalizedAttributes = activity.TagObjects.Where(kv => !kv.Key.StartsWith("otel.")).ToDictionary(x => x.Key, x => x.Value.ToString()); - var normalizedAttributesTestCase = tc.SpanAttributes.ToDictionary(x => x.Key, x => HttpTestData.NormalizeValues(x.Value, host, port)); + var normalizedAttributes = activity.TagObjects.Where(kv => !kv.Key.StartsWith("otel.")).ToDictionary(x => x.Key, x => x.Value.ToString()); + var normalizedAttributesTestCase = tc.SpanAttributes.ToDictionary(x => x.Key, x => HttpTestData.NormalizeValues(x.Value, host, port)); - Assert.Equal(normalizedAttributesTestCase.Count, normalizedAttributes.Count); + Assert.Equal(normalizedAttributesTestCase.Count, normalizedAttributes.Count); - foreach (var kv in normalizedAttributesTestCase) - { - Assert.Contains(activity.TagObjects, i => i.Key == kv.Key && i.Value.ToString().Equals(kv.Value, StringComparison.OrdinalIgnoreCase)); + foreach (var kv in normalizedAttributesTestCase) + { + Assert.Contains(activity.TagObjects, i => i.Key == kv.Key && i.Value.ToString().Equals(kv.Value, StringComparison.OrdinalIgnoreCase)); + } + + if (tc.RecordException.HasValue && tc.RecordException.Value) + { + Assert.Single(activity.Events.Where(evt => evt.Name.Equals("exception"))); + Assert.True(enrichWithExceptionCalled); + } } - if (tc.RecordException.HasValue && tc.RecordException.Value) + if (!enableMetrics) { - Assert.Single(activity.Events.Where(evt => evt.Name.Equals("exception"))); - Assert.True(enrichWithExceptionCalled); + Assert.Empty(requestMetrics); + return; } Assert.Single(requestMetrics); @@ -176,7 +210,12 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut var sum = metricPoint.GetHistogramSum(); Assert.Equal(1L, count); - Assert.Equal(activity.Duration.TotalMilliseconds, sum); + + if (enableTracing) + { + var activity = Assert.Single(activities); + Assert.Equal(activity.Duration.TotalMilliseconds, sum); + } var attributes = new KeyValuePair[metricPoint.Tags.Count]; int i = 0; From fc47558cc3f9641ff6da41f022e88ad4d4bae5f6 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Tue, 19 Sep 2023 15:02:06 -0400 Subject: [PATCH 29/44] null handling --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index fad1057f4f8..7ce9f4bac97 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -162,7 +162,7 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void AddResponseTags(HttpWebResponse response, Activity activity) { - if (activity.IsAllDataRequested) + if (activity != null && activity.IsAllDataRequested) { if (emitOldAttributes) { From 9ae0193bc4aed931cab1669d8e91b978459f16cc Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Tue, 19 Sep 2023 20:38:14 -0400 Subject: [PATCH 30/44] null check for safety, tests don't blow up here for some reason? --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 7ce9f4bac97..6acb90ee605 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -190,7 +190,7 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity) [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void AddExceptionTags(Exception exception, Activity activity) { - if (!activity.IsAllDataRequested) + if (activity == null || !activity.IsAllDataRequested) { return; } From dcdde2c17cc34ad0aac18ddd92d11bafc6f8f976 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Wed, 20 Sep 2023 09:15:14 -0400 Subject: [PATCH 31/44] remove note that netfx does not have metrics --- src/OpenTelemetry.Instrumentation.Http/README.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/README.md b/src/OpenTelemetry.Instrumentation.Http/README.md index 7ca9400d4b2..18b6192a105 100644 --- a/src/OpenTelemetry.Instrumentation.Http/README.md +++ b/src/OpenTelemetry.Instrumentation.Http/README.md @@ -67,9 +67,6 @@ public class Program #### Metrics -> **Note** -> Metrics are not available for .NET Framework. - The following example demonstrates adding `HttpClient` instrumentation with the extension method `.AddHttpClientInstrumentation()` on `MeterProviderBuilder` to a console application. This example also sets up the OpenTelemetry Console From a063be19f90b89bb884643bf3aa2120b6f9470aa Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Wed, 20 Sep 2023 09:17:15 -0400 Subject: [PATCH 32/44] changelog entry --- src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index a94fa9789ef..de1a7e6b309 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +* Added support for publishing `http.client.duration` metric on .NET Framework + ## 1.5.1-beta.1 Released 2023-Jul-20 From 7ce3c515c3b5a7b9923c5dd6e2bda2cd7d00fe98 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Fri, 22 Sep 2023 07:59:12 -0400 Subject: [PATCH 33/44] correct whitespace --- .../HttpClientTests.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 5bda2de32db..a90f92b8b08 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -150,13 +150,13 @@ private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.Ht Assert.False(enrichWithHttpResponseMessageCalled); } #else - Assert.False(enrichWithHttpWebRequestCalled); - Assert.True(enrichWithHttpRequestMessageCalled); - if (tc.ResponseExpected) - { - Assert.False(enrichWithHttpWebResponseCalled); - Assert.True(enrichWithHttpResponseMessageCalled); - } + Assert.False(enrichWithHttpWebRequestCalled); + Assert.True(enrichWithHttpRequestMessageCalled); + if (tc.ResponseExpected) + { + Assert.False(enrichWithHttpWebResponseCalled); + Assert.True(enrichWithHttpResponseMessageCalled); + } #endif // Assert.Equal(tc.SpanStatus, d[span.Status.CanonicalCode]); From 812ad91b88deed32b81621db507dd2913a0daf63 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Fri, 22 Sep 2023 08:01:15 -0400 Subject: [PATCH 34/44] reorder methods per linter --- .../HttpClientTests.cs | 80 +++++++++---------- 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index a90f92b8b08..b19ff2f11b6 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -41,6 +41,46 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, false, false).ConfigureAwait(false); } + [Fact] + public async Task DebugIndividualTestAsync() + { + var input = JsonSerializer.Deserialize( + @" + [ + { + ""name"": ""Response code: 399"", + ""method"": ""GET"", + ""url"": ""http://{host}:{port}/"", + ""responseCode"": 399, + ""responseExpected"": true, + ""spanName"": ""HTTP GET"", + ""spanStatus"": ""Unset"", + ""spanKind"": ""Client"", + ""spanAttributes"": { + ""http.scheme"": ""http"", + ""http.method"": ""GET"", + ""net.peer.name"": ""{host}"", + ""net.peer.port"": ""{port}"", + ""http.status_code"": ""399"", + ""http.flavor"": ""{flavor}"", + ""http.url"": ""http://{host}:{port}/"" + } + } + ] + ", + new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); + + var t = (Task)this.GetType().InvokeMember(nameof(this.HttpOutCallsAreCollectedSuccessfullyAsync), BindingFlags.InvokeMethod, null, this, HttpTestData.GetArgumentsFromTestCaseObject(input).First()); + await t.ConfigureAwait(false); + } + + [Fact] + public async Task CheckEnrichmentWhenSampling() + { + await CheckEnrichment(new AlwaysOffSampler(), false, this.url).ConfigureAwait(false); + await CheckEnrichment(new AlwaysOnSampler(), true, this.url).ConfigureAwait(false); + } + private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.HttpOutTestCase tc, bool enableTracing, bool enableMetrics) { bool enrichWithHttpWebRequestCalled = false; @@ -251,46 +291,6 @@ private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.Ht } } - [Fact] - public async Task DebugIndividualTestAsync() - { - var input = JsonSerializer.Deserialize( - @" - [ - { - ""name"": ""Response code: 399"", - ""method"": ""GET"", - ""url"": ""http://{host}:{port}/"", - ""responseCode"": 399, - ""responseExpected"": true, - ""spanName"": ""HTTP GET"", - ""spanStatus"": ""Unset"", - ""spanKind"": ""Client"", - ""spanAttributes"": { - ""http.scheme"": ""http"", - ""http.method"": ""GET"", - ""net.peer.name"": ""{host}"", - ""net.peer.port"": ""{port}"", - ""http.status_code"": ""399"", - ""http.flavor"": ""{flavor}"", - ""http.url"": ""http://{host}:{port}/"" - } - } - ] - ", - new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); - - var t = (Task)this.GetType().InvokeMember(nameof(this.HttpOutCallsAreCollectedSuccessfullyAsync), BindingFlags.InvokeMethod, null, this, HttpTestData.GetArgumentsFromTestCaseObject(input).First()); - await t.ConfigureAwait(false); - } - - [Fact] - public async Task CheckEnrichmentWhenSampling() - { - await CheckEnrichment(new AlwaysOffSampler(), false, this.url).ConfigureAwait(false); - await CheckEnrichment(new AlwaysOnSampler(), true, this.url).ConfigureAwait(false); - } - private static async Task CheckEnrichment(Sampler sampler, bool enrichExpected, string url) { bool enrichWithHttpWebRequestCalled = false; From 2cfc3e21bd7216794e857f97b1dd8e8e74955ed8 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Fri, 22 Sep 2023 10:35:09 -0400 Subject: [PATCH 35/44] putting the combinations in one test is causing intermittent failures --- .../HttpClientTests.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index b19ff2f11b6..621314a009d 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -36,8 +36,26 @@ public partial class HttpClientTests public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOutTestCase tc) { await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, true, true).ConfigureAwait(false); + } + + [Theory] + [MemberData(nameof(TestData))] + public async Task HttpOutCallsAreCollectedSuccessfullyTracesOnlyAsync(HttpTestData.HttpOutTestCase tc) + { await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, true, false).ConfigureAwait(false); + } + + [Theory] + [MemberData(nameof(TestData))] + public async Task HttpOutCallsAreCollectedSuccessfullyMetricsOnlyAsync(HttpTestData.HttpOutTestCase tc) + { await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, false, true).ConfigureAwait(false); + } + + [Theory] + [MemberData(nameof(TestData))] + public async Task HttpOutCallsAreCollectedSuccessfullyNoSignalsAsync(HttpTestData.HttpOutTestCase tc) + { await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, false, false).ConfigureAwait(false); } From 0f3799200e5b71d8e234c65628adaef5cf8965e3 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Fri, 22 Sep 2023 21:30:49 -0400 Subject: [PATCH 36/44] lift null checks --- .../HttpWebRequestActivitySource.netfx.cs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 6acb90ee605..c7fd24a8331 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -377,7 +377,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC try { - if (result is Exception ex) + if (activity != null && result is Exception ex) { AddExceptionTags(ex, activity); } @@ -400,12 +400,20 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC isWebSocketResponseAccessor(response), connectionGroupNameAccessor(response), }); - AddResponseTags(responseCopy, activity); + if (activity != null) + { + AddResponseTags(responseCopy, activity); + } + httpStatusCode = responseCopy.StatusCode; } else { - AddResponseTags(response, activity); + if (activity != null) + { + AddResponseTags(response, activity); + } + httpStatusCode = response.StatusCode; } } From 86c8ab061aa0a509ae39ae33092afab651209291 Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Fri, 22 Sep 2023 21:31:19 -0400 Subject: [PATCH 37/44] make this isn't hit when tracing is disabled --- .../Implementation/HttpWebRequestActivitySource.netfx.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index c7fd24a8331..628a7a9a60d 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -162,7 +162,9 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void AddResponseTags(HttpWebResponse response, Activity activity) { - if (activity != null && activity.IsAllDataRequested) + Debug.Assert(activity != null, "Activity must not be null"); + + if (activity.IsAllDataRequested) { if (emitOldAttributes) { @@ -190,7 +192,9 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity) [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void AddExceptionTags(Exception exception, Activity activity) { - if (activity == null || !activity.IsAllDataRequested) + Debug.Assert(activity != null, "Activity must not be null"); + + if (!activity.IsAllDataRequested) { return; } From 7704a825b327e44e62a2ac14a577caa49c54a61e Mon Sep 17 00:00:00 2001 From: Matt Hensley <130569+matt-hensley@users.noreply.github.com> Date: Mon, 25 Sep 2023 10:09:21 -0400 Subject: [PATCH 38/44] Update src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Piotr Kiełkowicz --- src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index de1a7e6b309..320bc11ba47 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased * Added support for publishing `http.client.duration` metric on .NET Framework + ([#4870](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4870)) ## 1.5.1-beta.1 From 0ce3ced5e39bf0bcc022c5fe55aa88649c255acf Mon Sep 17 00:00:00 2001 From: Matthew Hensley Date: Tue, 26 Sep 2023 13:41:20 -0400 Subject: [PATCH 39/44] modify test to ensure duration was written --- .../HttpClientTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 621314a009d..feb7275cf37 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -274,6 +274,10 @@ private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.Ht var activity = Assert.Single(activities); Assert.Equal(activity.Duration.TotalMilliseconds, sum); } + else + { + Assert.True(sum > 0); + } var attributes = new KeyValuePair[metricPoint.Tags.Count]; int i = 0; From 5218b659730c76454564a8399ba844686f4d557e Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 27 Sep 2023 16:02:35 -0700 Subject: [PATCH 40/44] Fixes, tweaks, and cleanup. --- .../HttpClientMetrics.cs | 14 +- .../HttpHandlerMetricsDiagnosticListener.cs | 14 +- .../HttpWebRequestActivitySource.netfx.cs | 153 +++++--- .../MeterProviderBuilderExtensions.cs | 25 +- .../TracerProviderBuilderExtensions.cs | 2 +- .../HttpClientTests.Basic.cs | 24 +- .../HttpClientTests.cs | 350 ++++++++++++------ ...HttpWebRequestActivitySourceTests.netfx.cs | 6 +- ...WebRequestActivitySourceTestsDupe.netfx.cs | 311 ---------------- ...pWebRequestActivitySourceTestsNew.netfx.cs | 295 --------------- .../Shared/TestHttpServer.cs | 7 + 11 files changed, 402 insertions(+), 799 deletions(-) delete mode 100644 test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsDupe.netfx.cs delete mode 100644 test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsNew.netfx.cs diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientMetrics.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientMetrics.cs index 2ff85729892..2f4db9ec9b2 100644 --- a/src/OpenTelemetry.Instrumentation.Http/HttpClientMetrics.cs +++ b/src/OpenTelemetry.Instrumentation.Http/HttpClientMetrics.cs @@ -14,8 +14,6 @@ // limitations under the License. // -using System.Diagnostics.Metrics; -using System.Reflection; using OpenTelemetry.Instrumentation.Http.Implementation; namespace OpenTelemetry.Instrumentation.Http; @@ -25,10 +23,6 @@ namespace OpenTelemetry.Instrumentation.Http; /// internal sealed class HttpClientMetrics : IDisposable { - internal static readonly AssemblyName AssemblyName = typeof(HttpClientMetrics).Assembly.GetName(); - internal static readonly string InstrumentationName = AssemblyName.Name; - internal static readonly string InstrumentationVersion = AssemblyName.Version.ToString(); - private static readonly HashSet ExcludedDiagnosticSourceEvents = new() { "System.Net.Http.Request", @@ -36,7 +30,6 @@ internal sealed class HttpClientMetrics : IDisposable }; private readonly DiagnosticSourceSubscriber diagnosticSourceSubscriber; - private readonly Meter meter; private readonly Func isEnabled = (activityName, obj1, obj2) => !ExcludedDiagnosticSourceEvents.Contains(activityName); @@ -47,8 +40,10 @@ internal sealed class HttpClientMetrics : IDisposable /// HttpClient metric instrumentation options. public HttpClientMetrics(HttpClientMetricInstrumentationOptions options) { - this.meter = new Meter(InstrumentationName, InstrumentationVersion); - this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpHandlerMetricsDiagnosticListener("HttpHandlerDiagnosticListener", this.meter, options), this.isEnabled, HttpInstrumentationEventSource.Log.UnknownErrorProcessingEvent); + this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber( + new HttpHandlerMetricsDiagnosticListener("HttpHandlerDiagnosticListener", options), + this.isEnabled, + HttpInstrumentationEventSource.Log.UnknownErrorProcessingEvent); this.diagnosticSourceSubscriber.Subscribe(); } @@ -56,6 +51,5 @@ public HttpClientMetrics(HttpClientMetricInstrumentationOptions options) public void Dispose() { this.diagnosticSourceSubscriber?.Dispose(); - this.meter?.Dispose(); } } diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs index 3aac7b32160..26305a674f8 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs @@ -22,6 +22,7 @@ #if NETFRAMEWORK using System.Net.Http; #endif +using System.Reflection; using OpenTelemetry.Trace; using static OpenTelemetry.Internal.HttpSemanticConventionHelper; @@ -31,21 +32,24 @@ internal sealed class HttpHandlerMetricsDiagnosticListener : ListenerHandler { internal const string OnStopEvent = "System.Net.Http.HttpRequestOut.Stop"; + internal static readonly AssemblyName AssemblyName = typeof(HttpClientMetrics).Assembly.GetName(); + internal static readonly string MeterName = AssemblyName.Name; + internal static readonly string MeterVersion = AssemblyName.Version.ToString(); + internal static readonly Meter Meter = new(MeterName, MeterVersion); + private static readonly Histogram HttpClientDuration = Meter.CreateHistogram("http.client.duration", "ms", "Measures the duration of outbound HTTP requests."); + private static readonly PropertyFetcher StopRequestFetcher = new("Request"); private static readonly PropertyFetcher StopResponseFetcher = new("Response"); - private readonly Histogram httpClientDuration; private readonly HttpClientMetricInstrumentationOptions options; private readonly bool emitOldAttributes; private readonly bool emitNewAttributes; - public HttpHandlerMetricsDiagnosticListener(string name, Meter meter, HttpClientMetricInstrumentationOptions options) + public HttpHandlerMetricsDiagnosticListener(string name, HttpClientMetricInstrumentationOptions options) : base(name) { - this.httpClientDuration = meter.CreateHistogram("http.client.duration", "ms", "Measures the duration of outbound HTTP requests."); this.options = options; this.emitOldAttributes = this.options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old); - this.emitNewAttributes = this.options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.New); } @@ -103,7 +107,7 @@ public override void OnEventWritten(string name, object payload) // We are relying here on HttpClient library to set duration before writing the stop event. // https://github.com/dotnet/runtime/blob/90603686d314147017c8bbe1fa8965776ce607d0/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L178 // TODO: Follow up with .NET team if we can continue to rely on this behavior. - this.httpClientDuration.Record(activity.Duration.TotalMilliseconds, tags); + HttpClientDuration.Record(activity.Duration.TotalMilliseconds, tags); } } diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 628a7a9a60d..cd47b265137 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -40,20 +40,23 @@ internal static class HttpWebRequestActivitySource internal static readonly AssemblyName AssemblyName = typeof(HttpWebRequestActivitySource).Assembly.GetName(); internal static readonly string ActivitySourceName = AssemblyName.Name + ".HttpWebRequest"; internal static readonly string ActivityName = ActivitySourceName + ".HttpRequestOut"; - internal static readonly string MeterInstrumentationName = AssemblyName.Name; + internal static readonly string MeterName = AssemblyName.Name; internal static readonly Func> HttpWebRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name); internal static readonly Action HttpWebRequestHeaderValuesSetter = (request, name, value) => request.Headers.Add(name, value); - private static readonly Version Version = AssemblyName.Version; - private static readonly ActivitySource WebRequestActivitySource = new ActivitySource(ActivitySourceName, Version.ToString()); - private static readonly Meter WebRequestMeter = new Meter(MeterInstrumentationName, Version.ToString()); - private static readonly Histogram HttpClientDuration; + private static readonly string Version = AssemblyName.Version.ToString(); + private static readonly ActivitySource WebRequestActivitySource = new(ActivitySourceName, Version); + private static readonly Meter WebRequestMeter = new(MeterName, Version); + private static readonly Histogram HttpClientDuration = WebRequestMeter.CreateHistogram("http.client.duration", "ms", "Measures the duration of outbound HTTP requests."); - private static HttpClientInstrumentationOptions options; + private static HttpClientInstrumentationOptions tracingOptions; + private static HttpClientMetricInstrumentationOptions metricsOptions; - private static bool emitOldAttributes; - private static bool emitNewAttributes; + private static bool tracingEmitOldAttributes; + private static bool tracingEmitNewAttributes; + private static bool metricsEmitOldAttributes; + private static bool metricsEmitNewAttributes; // Fields for reflection private static FieldInfo connectionGroupListField; @@ -90,8 +93,8 @@ static HttpWebRequestActivitySource() PrepareReflectionObjects(); PerformInjection(); - Options = new HttpClientInstrumentationOptions(); - HttpClientDuration = WebRequestMeter.CreateHistogram("http.client.duration", "ms", "Measures the duration of outbound HTTP requests."); + TracingOptions = new HttpClientInstrumentationOptions(); + MetricsOptions = new HttpClientMetricInstrumentationOptions(); } catch (Exception ex) { @@ -100,15 +103,27 @@ static HttpWebRequestActivitySource() } } - internal static HttpClientInstrumentationOptions Options + 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); } } @@ -120,7 +135,7 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A if (activity.IsAllDataRequested) { // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md - if (emitOldAttributes) + if (tracingEmitOldAttributes) { activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method); activity.SetTag(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host); @@ -135,7 +150,7 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A } // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md - if (emitNewAttributes) + if (tracingEmitNewAttributes) { activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, request.Method); activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); @@ -150,7 +165,7 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A try { - Options.EnrichWithHttpWebRequest?.Invoke(activity, request); + TracingOptions.EnrichWithHttpWebRequest?.Invoke(activity, request); } catch (Exception ex) { @@ -166,12 +181,12 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity) if (activity.IsAllDataRequested) { - if (emitOldAttributes) + if (tracingEmitOldAttributes) { activity.SetTag(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); } - if (emitNewAttributes) + if (tracingEmitNewAttributes) { activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); } @@ -180,7 +195,7 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity) try { - Options.EnrichWithHttpWebResponse?.Invoke(activity, response); + TracingOptions.EnrichWithHttpWebResponse?.Invoke(activity, response); } catch (Exception ex) { @@ -190,10 +205,12 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static void AddExceptionTags(Exception exception, Activity activity) + private static void AddExceptionTags(Exception exception, Activity activity, out HttpStatusCode? statusCode) { Debug.Assert(activity != null, "Activity must not be null"); + statusCode = null; + if (!activity.IsAllDataRequested) { return; @@ -206,17 +223,19 @@ private static void AddExceptionTags(Exception exception, Activity activity) { if (wexc.Response is HttpWebResponse response) { - if (emitOldAttributes) + statusCode = response.StatusCode; + + if (tracingEmitOldAttributes) { - activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode); + activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)statusCode); } - if (emitNewAttributes) + if (tracingEmitNewAttributes) { - activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, (int)response.StatusCode); + activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, (int)statusCode); } - status = SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)response.StatusCode); + status = SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)statusCode); } else { @@ -249,14 +268,14 @@ private static void AddExceptionTags(Exception exception, Activity activity) } activity.SetStatus(status, exceptionMessage); - if (Options.RecordException) + if (TracingOptions.RecordException) { activity.RecordException(exception); } try { - Options.EnrichWithException?.Invoke(activity, exception); + TracingOptions.EnrichWithException?.Invoke(activity, exception); } catch (Exception ex) { @@ -276,7 +295,8 @@ private static void ProcessRequest(HttpWebRequest request) { // There are subscribers to the ActivitySource and no user-provided filter is // filtering this request. - var enableTracing = WebRequestActivitySource.HasListeners() && Options.EventFilterHttpWebRequest(request); + var enableTracing = WebRequestActivitySource.HasListeners() + && TracingOptions.EventFilterHttpWebRequest(request); if (!enableTracing && !HttpClientDuration.Enabled) { @@ -295,12 +315,9 @@ private static void ProcessRequest(HttpWebRequest request) return; } - Activity activity = null; - - if (enableTracing) - { - activity = WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client); - } + Activity activity = enableTracing + ? WebRequestActivitySource.StartActivity(ActivityName, ActivityKind.Client) + : null; var activityContext = Activity.Current?.Context ?? default; @@ -335,7 +352,7 @@ private static void ProcessRequest(HttpWebRequest request) private static void HookOrProcessResult(HttpWebRequest request) { IAsyncResult writeAsyncContext = writeAResultAccessor(request); - if (writeAsyncContext == null || !(asyncCallbackAccessor(writeAsyncContext)?.Target is AsyncCallbackWrapper writeAsyncContextCallback)) + if (writeAsyncContext == null || asyncCallbackAccessor(writeAsyncContext)?.Target is not AsyncCallbackWrapper writeAsyncContextCallback) { // If we already hooked into the read result during ProcessRequest or we hooked up after the fact already we don't need to do anything here. return; @@ -381,9 +398,16 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC try { - if (activity != null && result is Exception ex) + if (result is Exception ex) { - AddExceptionTags(ex, activity); + if (activity != null) + { + AddExceptionTags(ex, activity, out httpStatusCode); + } + else if (ex is WebException wexc && wexc.Response is HttpWebResponse response) + { + httpStatusCode = response.StatusCode; + } } else { @@ -431,8 +455,7 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC if (HttpClientDuration.Enabled) { - double durationMs = 0; - + double durationMs; if (activity != null) { durationMs = activity.Duration.TotalMilliseconds; @@ -440,20 +463,46 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC else { var endTimestamp = Stopwatch.GetTimestamp(); - var durationS = (endTimestamp - startTimestamp) / Stopwatch.Frequency; + var durationS = (endTimestamp - startTimestamp) / (double)Stopwatch.Frequency; durationMs = durationS * 1000; } TagList tags = default; - tags.Add(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)); - tags.Add(SemanticConventions.AttributeHttpMethod, request.Method); - tags.Add(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme); - tags.Add(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host); - tags.Add(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port); + + if (metricsEmitOldAttributes) + { + tags.Add(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)); + tags.Add(SemanticConventions.AttributeHttpMethod, request.Method); + tags.Add(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme); + tags.Add(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host); + if (!request.RequestUri.IsDefaultPort) + { + tags.Add(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port); + } + } + + if (metricsEmitNewAttributes) + { + tags.Add(SemanticConventions.AttributeHttpRequestMethod, request.Method); + tags.Add(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); + tags.Add(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)); + if (!request.RequestUri.IsDefaultPort) + { + tags.Add(SemanticConventions.AttributeServerPort, request.RequestUri.Port); + } + } if (httpStatusCode.HasValue) { - tags.Add(SemanticConventions.AttributeHttpStatusCode, (int)httpStatusCode.Value); + if (metricsEmitOldAttributes) + { + tags.Add(SemanticConventions.AttributeHttpStatusCode, (int)httpStatusCode.Value); + } + + if (metricsEmitNewAttributes) + { + tags.Add(SemanticConventions.AttributeHttpResponseStatusCode, (int)httpStatusCode.Value); + } } HttpClientDuration.Record(durationMs, tags); @@ -565,12 +614,8 @@ private static bool PrepareHttpWebResponseReflectionObjects(Assembly systemNetHt private static void PerformInjection() { - FieldInfo servicePointTableField = typeof(ServicePointManager).GetField("s_ServicePointTable", BindingFlags.Static | BindingFlags.NonPublic); - if (servicePointTableField == null) - { - // If anything went wrong here, just return false. There is nothing we can do. - throw new InvalidOperationException("Unable to access the ServicePointTable field"); - } + FieldInfo servicePointTableField = typeof(ServicePointManager).GetField("s_ServicePointTable", BindingFlags.Static | BindingFlags.NonPublic) + ?? throw new InvalidOperationException("Unable to access the ServicePointTable field"); Hashtable originalTable = servicePointTableField.GetValue(null) as Hashtable; ServicePointHashtable newTable = new ServicePointHashtable(originalTable ?? new Hashtable()); diff --git a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs index a2b8fec0dc0..c82138dbbc1 100644 --- a/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs @@ -14,11 +14,8 @@ // limitations under the License. // -#if NETFRAMEWORK -#else using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; -#endif using OpenTelemetry.Instrumentation.Http; using OpenTelemetry.Instrumentation.Http.Implementation; using OpenTelemetry.Internal; @@ -48,19 +45,29 @@ public static MeterProviderBuilder AddHttpClientInstrumentation( services.RegisterOptionsFactory(configuration => new HttpClientMetricInstrumentationOptions(configuration)); }); - // TODO: Implement an IDeferredMeterProviderBuilder - - // TODO: Handle HttpClientInstrumentationOptions + // TODO: Handle HttpClientMetricInstrumentationOptions // SetHttpFlavor - seems like this would be handled by views // Filter - makes sense for metric instrumentation // Enrich - do we want a similar kind of functionality for metrics? // RecordException - probably doesn't make sense for metric instrumentation + #if NETFRAMEWORK - builder.AddMeter(HttpWebRequestActivitySource.MeterInstrumentationName); + builder.AddMeter(HttpWebRequestActivitySource.MeterName); + + if (builder is IDeferredMeterProviderBuilder deferredMeterProviderBuilder) + { + deferredMeterProviderBuilder.Configure((sp, builder) => + { + var options = sp.GetRequiredService>().Get(Options.DefaultName); + + HttpWebRequestActivitySource.MetricsOptions = options; + }); + } #else - builder.AddMeter(HttpClientMetrics.InstrumentationName); + builder.AddMeter(HttpHandlerMetricsDiagnosticListener.MeterName); + builder.AddInstrumentation(sp => new HttpClientMetrics( - sp.GetRequiredService>().CurrentValue)); + sp.GetRequiredService>().Get(Options.DefaultName))); #endif return builder; } diff --git a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs index 6763fc3e204..9429819b8a2 100644 --- a/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry.Instrumentation.Http/TracerProviderBuilderExtensions.cs @@ -84,7 +84,7 @@ public static TracerProviderBuilder AddHttpClientInstrumentation( { var options = sp.GetRequiredService>().Get(name); - HttpWebRequestActivitySource.Options = options; + HttpWebRequestActivitySource.TracingOptions = options; }); } #else diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index d7b41b75359..e039f25ac12 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -26,23 +26,32 @@ using OpenTelemetry.Tests; using OpenTelemetry.Trace; using Xunit; +using Xunit.Abstractions; namespace OpenTelemetry.Instrumentation.Http.Tests; public partial class HttpClientTests : IDisposable { + private readonly ITestOutputHelper output; private readonly IDisposable serverLifeTime; + private readonly string host; + private readonly int port; private readonly string url; - public HttpClientTests() + public HttpClientTests(ITestOutputHelper output) { + this.output = output; + this.serverLifeTime = TestHttpServer.RunServer( (ctx) => { string traceparent = ctx.Request.Headers["traceparent"]; string custom_traceparent = ctx.Request.Headers["custom_traceparent"]; - if (string.IsNullOrWhiteSpace(traceparent) - && string.IsNullOrWhiteSpace(custom_traceparent)) + if ((ctx.Request.Headers["contextRequired"] == null + || bool.Parse(ctx.Request.Headers["contextRequired"])) + && + (string.IsNullOrWhiteSpace(traceparent) + && string.IsNullOrWhiteSpace(custom_traceparent))) { ctx.Response.StatusCode = 500; ctx.Response.StatusDescription = "Missing trace context"; @@ -56,6 +65,10 @@ public HttpClientTests() ctx.Response.RedirectLocation = "/"; ctx.Response.StatusCode = 302; } + else if (ctx.Request.Headers["responseCode"] != null) + { + ctx.Response.StatusCode = int.Parse(ctx.Request.Headers["responseCode"]); + } else { ctx.Response.StatusCode = 200; @@ -66,7 +79,11 @@ public HttpClientTests() out var host, out var port); + this.host = host; + this.port = port; this.url = $"http://{host}:{port}/"; + + this.output.WriteLine($"HttpServer started: {this.url}"); } [Fact] @@ -631,6 +648,7 @@ public async Task CustomPropagatorCalled(bool sample, bool createParentActivity) public void Dispose() { this.serverLifeTime?.Dispose(); + this.output.WriteLine($"HttpServer stopped: {this.url}"); Activity.Current = null; GC.SuppressFinalize(this); } diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index feb7275cf37..ee0093d6895 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -20,43 +20,92 @@ #endif using System.Reflection; using System.Text.Json; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; using OpenTelemetry.Metrics; -using OpenTelemetry.Tests; using OpenTelemetry.Trace; using Xunit; +using static OpenTelemetry.Internal.HttpSemanticConventionHelper; namespace OpenTelemetry.Instrumentation.Http.Tests; public partial class HttpClientTests { - public static IEnumerable TestData => HttpTestData.ReadTestCases(); + public static readonly IEnumerable TestData = HttpTestData.ReadTestCases(); [Theory] [MemberData(nameof(TestData))] - public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOutTestCase tc) + public async Task HttpOutCallsAreCollectedSuccessfullyTracesAndMetricsOldSemanticConventionsAsync(HttpTestData.HttpOutTestCase tc) { - await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, true, true).ConfigureAwait(false); + await HttpOutCallsAreCollectedSuccessfullyBodyAsync( + this.host, + this.port, + tc, + enableTracing: true, + enableMetrics: true, + semanticConvention: HttpSemanticConvention.Old).ConfigureAwait(false); + } + + [Theory] + [MemberData(nameof(TestData))] + public async Task HttpOutCallsAreCollectedSuccessfullyTracesAndMetricsNewSemanticConventionsAsync(HttpTestData.HttpOutTestCase tc) + { + await HttpOutCallsAreCollectedSuccessfullyBodyAsync( + this.host, + this.port, + tc, + enableTracing: true, + enableMetrics: true, + semanticConvention: HttpSemanticConvention.New).ConfigureAwait(false); + } + + [Theory] + [MemberData(nameof(TestData))] + public async Task HttpOutCallsAreCollectedSuccessfullyTracesAndMetricsDuplicateSemanticConventionsAsync(HttpTestData.HttpOutTestCase tc) + { + await HttpOutCallsAreCollectedSuccessfullyBodyAsync( + this.host, + this.port, + tc, + enableTracing: true, + enableMetrics: true, + semanticConvention: HttpSemanticConvention.Dupe).ConfigureAwait(false); } [Theory] [MemberData(nameof(TestData))] public async Task HttpOutCallsAreCollectedSuccessfullyTracesOnlyAsync(HttpTestData.HttpOutTestCase tc) { - await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, true, false).ConfigureAwait(false); + await HttpOutCallsAreCollectedSuccessfullyBodyAsync( + this.host, + this.port, + tc, + enableTracing: true, + enableMetrics: false).ConfigureAwait(false); } [Theory] [MemberData(nameof(TestData))] public async Task HttpOutCallsAreCollectedSuccessfullyMetricsOnlyAsync(HttpTestData.HttpOutTestCase tc) { - await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, false, true).ConfigureAwait(false); + await HttpOutCallsAreCollectedSuccessfullyBodyAsync( + this.host, + this.port, + tc, + enableTracing: false, + enableMetrics: true).ConfigureAwait(false); } [Theory] [MemberData(nameof(TestData))] public async Task HttpOutCallsAreCollectedSuccessfullyNoSignalsAsync(HttpTestData.HttpOutTestCase tc) { - await this.HttpOutCallsAreCollectedSuccessfullyBodyAsync(tc, false, false).ConfigureAwait(false); + await HttpOutCallsAreCollectedSuccessfullyBodyAsync( + this.host, + this.port, + tc, + enableTracing: false, + enableMetrics: false).ConfigureAwait(false); } [Fact] @@ -88,7 +137,7 @@ public async Task DebugIndividualTestAsync() ", new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); - var t = (Task)this.GetType().InvokeMember(nameof(this.HttpOutCallsAreCollectedSuccessfullyAsync), BindingFlags.InvokeMethod, null, this, HttpTestData.GetArgumentsFromTestCaseObject(input).First()); + var t = (Task)this.GetType().InvokeMember(nameof(this.HttpOutCallsAreCollectedSuccessfullyTracesAndMetricsOldSemanticConventionsAsync), BindingFlags.InvokeMethod, null, this, HttpTestData.GetArgumentsFromTestCaseObject(input).First()); await t.ConfigureAwait(false); } @@ -99,7 +148,13 @@ public async Task CheckEnrichmentWhenSampling() await CheckEnrichment(new AlwaysOnSampler(), true, this.url).ConfigureAwait(false); } - private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.HttpOutTestCase tc, bool enableTracing, bool enableMetrics) + private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( + string host, + int port, + HttpTestData.HttpOutTestCase tc, + bool enableTracing, + bool enableMetrics, + HttpSemanticConvention? semanticConvention = null) { bool enrichWithHttpWebRequestCalled = false; bool enrichWithHttpWebResponseCalled = false; @@ -107,30 +162,33 @@ private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.Ht bool enrichWithHttpResponseMessageCalled = false; bool enrichWithExceptionCalled = false; - using var serverLifeTime = TestHttpServer.RunServer( - (ctx) => - { - ctx.Response.StatusCode = tc.ResponseCode == 0 ? 200 : tc.ResponseCode; - ctx.Response.OutputStream.Close(); - }, - out var host, - out var port); - - tc.Url = HttpTestData.NormalizeValues(tc.Url, host, port); - - var metrics = new List(); - var activities = new List(); + var testUrl = HttpTestData.NormalizeValues(tc.Url, host, port); var meterProviderBuilder = Sdk.CreateMeterProviderBuilder(); if (enableMetrics) { meterProviderBuilder.AddHttpClientInstrumentation(); + + if (semanticConvention.HasValue) + { + meterProviderBuilder.ConfigureServices(s => + { + s.AddSingleton(new ConfigurationBuilder() + .AddInMemoryCollection( + new Dictionary + { + ["OTEL_SEMCONV_STABILITY_OPT_IN"] = semanticConvention == HttpSemanticConvention.Dupe + ? "http/dup" + : semanticConvention == HttpSemanticConvention.New + ? "http" + : "_invalid", + }) + .Build()); + }); + } } - var meterProvider = meterProviderBuilder - .AddInMemoryExporter(metrics) - .Build(); var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder(); if (enableTracing) @@ -144,50 +202,80 @@ private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.Ht opt.EnrichWithException = (activity, exception) => { enrichWithExceptionCalled = true; }; opt.RecordException = tc.RecordException ?? false; }); + + if (semanticConvention.HasValue) + { + tracerProviderBuilder.ConfigureServices(s => + { + s.AddSingleton(new ConfigurationBuilder() + .AddInMemoryCollection( + new Dictionary + { + ["OTEL_SEMCONV_STABILITY_OPT_IN"] = semanticConvention == HttpSemanticConvention.Dupe + ? "http/dup" + : semanticConvention == HttpSemanticConvention.New + ? "http" + : "_invalid", + }) + .Build()); + }); + } } + var metrics = new List(); + var activities = new List(); + + var meterProvider = meterProviderBuilder + .AddInMemoryExporter(metrics) + .Build(); + var tracerProvider = tracerProviderBuilder .AddInMemoryExporter(activities) .Build(); - using (tracerProvider) + try { - try + using var c = new HttpClient(); + using var request = new HttpRequestMessage { - using var c = new HttpClient(); - using var request = new HttpRequestMessage - { - RequestUri = new Uri(tc.Url), - Method = new HttpMethod(tc.Method), + RequestUri = new Uri(testUrl), + Method = new HttpMethod(tc.Method), #if NETFRAMEWORK - Version = new Version(1, 1), + Version = new Version(1, 1), #else - Version = new Version(2, 0), + Version = new Version(2, 0), #endif - }; + }; - if (tc.Headers != null) + if (tc.Headers != null) + { + foreach (var header in tc.Headers) { - foreach (var header in tc.Headers) - { - request.Headers.Add(header.Key, header.Value); - } + request.Headers.Add(header.Key, header.Value); } - - await c.SendAsync(request).ConfigureAwait(false); } - catch (Exception) - { - // test case can intentionally send request that will result in exception - } - } - meterProvider.Dispose(); + request.Headers.Add("contextRequired", "false"); + request.Headers.Add("responseCode", (tc.ResponseCode == 0 ? 200 : tc.ResponseCode).ToString()); + + await c.SendAsync(request).ConfigureAwait(false); + } + catch (Exception) + { + // test case can intentionally send request that will result in exception + } + finally + { + tracerProvider.Dispose(); + meterProvider.Dispose(); + } var requestMetrics = metrics .Where(metric => metric.Name == "http.client.duration") .ToArray(); + var normalizedAttributesTestCase = tc.SpanAttributes.ToDictionary(x => x.Key, x => HttpTestData.NormalizeValues(x.Value, host, port)); + if (!enableTracing) { Assert.Empty(activities); @@ -227,13 +315,48 @@ private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.Ht } var normalizedAttributes = activity.TagObjects.Where(kv => !kv.Key.StartsWith("otel.")).ToDictionary(x => x.Key, x => x.Value.ToString()); - var normalizedAttributesTestCase = tc.SpanAttributes.ToDictionary(x => x.Key, x => HttpTestData.NormalizeValues(x.Value, host, port)); - Assert.Equal(normalizedAttributesTestCase.Count, normalizedAttributes.Count); + var expectedAttributeCount = semanticConvention == HttpSemanticConvention.Dupe + ? 11 + (tc.ResponseExpected ? 2 : 0) + : semanticConvention == HttpSemanticConvention.New + ? 5 + (tc.ResponseExpected ? 1 : 0) + : 6 + (tc.ResponseExpected ? 1 : 0); + + Assert.Equal(expectedAttributeCount, normalizedAttributes.Count); + + if (semanticConvention == null || semanticConvention.Value.HasFlag(HttpSemanticConvention.Old)) + { + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpMethod && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpMethod]); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeNetPeerName && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerName]); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeNetPeerPort && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerPort]); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpScheme && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpScheme]); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpUrl && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpUrl]); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpFlavor && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); + if (tc.ResponseExpected) + { + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + } + else + { + Assert.DoesNotContain(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpStatusCode); + } + } - foreach (var kv in normalizedAttributesTestCase) + if (semanticConvention != null && semanticConvention.Value.HasFlag(HttpSemanticConvention.New)) { - Assert.Contains(activity.TagObjects, i => i.Key == kv.Key && i.Value.ToString().Equals(kv.Value, StringComparison.OrdinalIgnoreCase)); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpRequestMethod && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpMethod]); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeServerAddress && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerName]); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeServerPort && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerPort]); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeUrlFull && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpUrl]); + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); + if (tc.ResponseExpected) + { + Assert.Contains(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + } + else + { + Assert.DoesNotContain(normalizedAttributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode); + } } if (tc.RecordException.HasValue && tc.RecordException.Value) @@ -246,70 +369,85 @@ private async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(HttpTestData.Ht if (!enableMetrics) { Assert.Empty(requestMetrics); - return; } + else + { + Assert.Single(requestMetrics); - Assert.Single(requestMetrics); + var metric = requestMetrics[0]; + Assert.NotNull(metric); + Assert.True(metric.MetricType == MetricType.Histogram); - var metric = requestMetrics[0]; - Assert.NotNull(metric); - Assert.True(metric.MetricType == MetricType.Histogram); + var metricPoints = new List(); + foreach (var p in metric.GetMetricPoints()) + { + metricPoints.Add(p); + } - var metricPoints = new List(); - foreach (var p in metric.GetMetricPoints()) - { - metricPoints.Add(p); - } + Assert.Single(metricPoints); + var metricPoint = metricPoints[0]; - Assert.Single(metricPoints); - var metricPoint = metricPoints[0]; + var count = metricPoint.GetHistogramCount(); + var sum = metricPoint.GetHistogramSum(); - var count = metricPoint.GetHistogramCount(); - var sum = metricPoint.GetHistogramSum(); + Assert.Equal(1L, count); - Assert.Equal(1L, count); + if (enableTracing) + { + var activity = Assert.Single(activities); + Assert.Equal(activity.Duration.TotalMilliseconds, sum); + } + else + { + Assert.True(sum > 0); + } - if (enableTracing) - { - var activity = Assert.Single(activities); - Assert.Equal(activity.Duration.TotalMilliseconds, sum); - } - else - { - Assert.True(sum > 0); - } + var attributes = new Dictionary(); + foreach (var tag in metricPoint.Tags) + { + attributes[tag.Key] = tag.Value; + } - var attributes = new KeyValuePair[metricPoint.Tags.Count]; - int i = 0; - foreach (var tag in metricPoint.Tags) - { - attributes[i++] = tag; - } + var expectedAttributeCount = semanticConvention == HttpSemanticConvention.Dupe + ? 9 + (tc.ResponseExpected ? 2 : 0) + : semanticConvention == HttpSemanticConvention.New + ? 4 + (tc.ResponseExpected ? 1 : 0) + : 5 + (tc.ResponseExpected ? 1 : 0); - var method = new KeyValuePair(SemanticConventions.AttributeHttpMethod, tc.Method); - var scheme = new KeyValuePair(SemanticConventions.AttributeHttpScheme, "http"); - var statusCode = new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, tc.ResponseCode == 0 ? 200 : tc.ResponseCode); -#if NETFRAMEWORK - var flavor = new KeyValuePair(SemanticConventions.AttributeHttpFlavor, "1.1"); -#else - var flavor = new KeyValuePair(SemanticConventions.AttributeHttpFlavor, "2.0"); -#endif - var hostName = new KeyValuePair(SemanticConventions.AttributeNetPeerName, tc.ResponseExpected ? host : "sdlfaldfjalkdfjlkajdflkajlsdjf"); - var portNumber = new KeyValuePair(SemanticConventions.AttributeNetPeerPort, port); - Assert.Contains(hostName, attributes); - Assert.Contains(portNumber, attributes); - Assert.Contains(method, attributes); - Assert.Contains(scheme, attributes); - Assert.Contains(flavor, attributes); - if (tc.ResponseExpected) - { - Assert.Contains(statusCode, attributes); - Assert.Equal(6, attributes.Length); - } - else - { - Assert.DoesNotContain(statusCode, attributes); - Assert.Equal(5, attributes.Length); + Assert.Equal(expectedAttributeCount, attributes.Count); + + if (semanticConvention == null || semanticConvention.Value.HasFlag(HttpSemanticConvention.Old)) + { + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpMethod && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpMethod]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetPeerName && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerName]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetPeerPort && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerPort]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpScheme && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpScheme]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpFlavor && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); + if (tc.ResponseExpected) + { + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + } + else + { + Assert.DoesNotContain(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpStatusCode); + } + } + + 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]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeServerPort && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerPort]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetworkProtocolVersion && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); + if (tc.ResponseExpected) + { + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + } + else + { + Assert.DoesNotContain(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpResponseStatusCode); + } + } } } diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs index 5ad9f6f2d69..6eca1d40eed 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs @@ -32,7 +32,6 @@ public class HttpWebRequestActivitySourceTests : IDisposable private readonly IDisposable testServer; private readonly string testServerHost; private readonly int testServerPort; - private readonly string hostNameAndPort; private readonly string netPeerName; private readonly int netPeerPort; @@ -51,10 +50,8 @@ static HttpWebRequestActivitySourceTests() }, }; - HttpWebRequestActivitySource.Options = options; + HttpWebRequestActivitySource.TracingOptions = options; - // Need to touch something in HttpWebRequestActivitySource/Sdk to do the static injection. - GC.KeepAlive(HttpWebRequestActivitySource.Options); _ = Sdk.SuppressInstrumentation; } @@ -69,7 +66,6 @@ public HttpWebRequestActivitySourceTests() out this.testServerHost, out this.testServerPort); - this.hostNameAndPort = $"{this.testServerHost}:{this.testServerPort}"; this.netPeerName = this.testServerHost; this.netPeerPort = this.testServerPort; diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsDupe.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsDupe.netfx.cs deleted file mode 100644 index 268f805d298..00000000000 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsDupe.netfx.cs +++ /dev/null @@ -1,311 +0,0 @@ -// -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -#if NETFRAMEWORK -using System.Collections.Concurrent; -using System.Diagnostics; -using System.Net; -using System.Net.Http; -using Microsoft.Extensions.Configuration; -using OpenTelemetry.Instrumentation.Http.Implementation; -using OpenTelemetry.Tests; -using OpenTelemetry.Trace; -using Xunit; -using static OpenTelemetry.Internal.HttpSemanticConventionHelper; - -namespace OpenTelemetry.Instrumentation.Http.Tests; - -// Tests for v1.21.0 Semantic Conventions for Http spans -// see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md -// These tests emit both the new and older attributes. -// This test class can be deleted when this library is GA. -public class HttpWebRequestActivitySourceTestsDupe : IDisposable -{ - private readonly IDisposable testServer; - private readonly string testServerHost; - private readonly int testServerPort; - private readonly string hostNameAndPort; - private readonly string netPeerName; - private readonly int netPeerPort; - - static HttpWebRequestActivitySourceTestsDupe() - { - var configuration = new ConfigurationBuilder() - .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = "http/dup" }) - .Build(); - - HttpClientInstrumentationOptions options = new(configuration) - { - EnrichWithHttpWebRequest = (activity, httpWebRequest) => - { - VerifyHeaders(httpWebRequest); - }, - }; - - HttpWebRequestActivitySource.Options = options; - - // Need to touch something in HttpWebRequestActivitySource/Sdk to do the static injection. - GC.KeepAlive(HttpWebRequestActivitySource.Options); - _ = Sdk.SuppressInstrumentation; - } - - public HttpWebRequestActivitySourceTestsDupe() - { - Assert.Null(Activity.Current); - Activity.DefaultIdFormat = ActivityIdFormat.W3C; - Activity.ForceDefaultIdFormat = false; - - this.testServer = TestHttpServer.RunServer( - ctx => ProcessServerRequest(ctx), - out this.testServerHost, - out this.testServerPort); - - this.hostNameAndPort = $"{this.testServerHost}:{this.testServerPort}"; - this.netPeerName = this.testServerHost; - this.netPeerPort = this.testServerPort; - - void ProcessServerRequest(HttpListenerContext context) - { - string redirects = context.Request.QueryString["redirects"]; - if (!string.IsNullOrWhiteSpace(redirects) && int.TryParse(redirects, out int parsedRedirects) && parsedRedirects > 0) - { - context.Response.Redirect(this.BuildRequestUrl(queryString: $"redirects={--parsedRedirects}")); - context.Response.OutputStream.Close(); - return; - } - - string responseContent; - if (context.Request.QueryString["skipRequestContent"] == null) - { - using StreamReader readStream = new StreamReader(context.Request.InputStream); - - responseContent = readStream.ReadToEnd(); - } - else - { - responseContent = $"{{\"Id\":\"{Guid.NewGuid()}\"}}"; - } - - string responseCode = context.Request.QueryString["responseCode"]; - if (!string.IsNullOrWhiteSpace(responseCode)) - { - context.Response.StatusCode = int.Parse(responseCode); - } - else - { - context.Response.StatusCode = 200; - } - - if (context.Response.StatusCode != 204) - { - using StreamWriter writeStream = new StreamWriter(context.Response.OutputStream); - - writeStream.Write(responseContent); - } - else - { - context.Response.OutputStream.Close(); - } - } - } - - public void Dispose() - { - this.testServer.Dispose(); - } - - /// - /// Test to make sure we get both request and response events. - /// - [Theory] - [InlineData("GET")] - [InlineData("POST")] - [InlineData("POST", "skipRequestContent=1")] - public async Task TestBasicReceiveAndResponseEvents(string method, string queryString = null) - { - var url = this.BuildRequestUrl(queryString: queryString); - - using var eventRecords = new ActivitySourceRecorder(); - - // Send a random Http request to generate some events - using (var client = new HttpClient()) - { - (method == "GET" - ? await client.GetAsync(url).ConfigureAwait(false) - : await client.PostAsync(url, new StringContent("hello world")).ConfigureAwait(false)).Dispose(); - } - - // We should have exactly one Start and one Stop event - Assert.Equal(2, eventRecords.Records.Count); - Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Start")); - Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); - - // Check to make sure: The first record must be a request, the next record must be a response. - Activity activity = AssertFirstEventWasStart(eventRecords); - - VerifyActivityStartTags(this.netPeerName, this.netPeerPort, method, url, activity); - - Assert.True(eventRecords.Records.TryDequeue(out var stopEvent)); - Assert.Equal("Stop", stopEvent.Key); - - VerifyActivityStopTags(200, activity); - } - - private static Activity AssertFirstEventWasStart(ActivitySourceRecorder eventRecords) - { - Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair startEvent)); - Assert.Equal("Start", startEvent.Key); - return startEvent.Value; - } - - private static void VerifyHeaders(HttpWebRequest startRequest) - { - var tracestate = startRequest.Headers["tracestate"]; - Assert.Equal("some=state", tracestate); - - var baggage = startRequest.Headers["baggage"]; - Assert.Equal("k=v", baggage); - - var traceparent = startRequest.Headers["traceparent"]; - Assert.NotNull(traceparent); - Assert.Matches("^[0-9a-f]{2}-[0-9a-f]{32}-[0-9a-f]{16}-[0-9a-f]{2}$", traceparent); - } - - 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)); - } - - private static void ActivityEnrichment(Activity activity, string method, object obj) - { - switch (method) - { - case "OnStartActivity": - Assert.True(obj is HttpWebRequest); - VerifyHeaders(obj as HttpWebRequest); - break; - - case "OnStopActivity": - Assert.True(obj is HttpWebResponse); - break; - - case "OnException": - Assert.True(obj is Exception); - break; - - default: - break; - } - } - - private static void ValidateBaggage(HttpWebRequest request) - { - string[] baggage = request.Headers["baggage"].Split(','); - - Assert.Equal(3, baggage.Length); - Assert.Contains("key=value", baggage); - Assert.Contains("bad%2Fkey=value", baggage); - Assert.Contains("goodkey=bad%2Fvalue", baggage); - } - - private string BuildRequestUrl(bool useHttps = false, string path = "echo", string queryString = null) - { - return $"{(useHttps ? "https" : "http")}://{this.testServerHost}:{this.testServerPort}/{path}{(string.IsNullOrWhiteSpace(queryString) ? string.Empty : $"?{queryString}")}"; - } - - private void CleanUpActivity() - { - while (Activity.Current != null) - { - Activity.Current.Stop(); - } - } - - /// - /// is a helper class for recording events. - /// - private class ActivitySourceRecorder : IDisposable - { - private readonly Action> onEvent; - private readonly ActivityListener activityListener; - - public ActivitySourceRecorder(Action> onEvent = null, ActivitySamplingResult activitySamplingResult = ActivitySamplingResult.AllDataAndRecorded) - { - this.activityListener = new ActivityListener - { - ShouldListenTo = (activitySource) => activitySource.Name == HttpWebRequestActivitySource.ActivitySourceName, - ActivityStarted = this.ActivityStarted, - ActivityStopped = this.ActivityStopped, - Sample = (ref ActivityCreationOptions options) => activitySamplingResult, - }; - - ActivitySource.AddActivityListener(this.activityListener); - - this.onEvent = onEvent; - } - - public ConcurrentQueue> Records { get; } = new ConcurrentQueue>(); - - public void Dispose() - { - this.activityListener.Dispose(); - } - - public void ActivityStarted(Activity activity) => this.Record("Start", activity); - - public void ActivityStopped(Activity activity) => this.Record("Stop", activity); - - private void Record(string eventName, Activity activity) - { - var record = new KeyValuePair(eventName, activity); - - this.Records.Enqueue(record); - this.onEvent?.Invoke(record); - } - } -} -#endif diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsNew.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsNew.netfx.cs deleted file mode 100644 index 48661f69100..00000000000 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTestsNew.netfx.cs +++ /dev/null @@ -1,295 +0,0 @@ -// -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -#if NETFRAMEWORK -using System.Collections.Concurrent; -using System.Diagnostics; -using System.Net; -using System.Net.Http; -using Microsoft.Extensions.Configuration; -using OpenTelemetry.Instrumentation.Http.Implementation; -using OpenTelemetry.Tests; -using OpenTelemetry.Trace; -using Xunit; -using static OpenTelemetry.Internal.HttpSemanticConventionHelper; - -namespace OpenTelemetry.Instrumentation.Http.Tests; - -// Tests for v1.21.0 Semantic Conventions for Http spans -// see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md -// These tests emit the new attributes. -// This test class can replace the other class when this library is GA. -public class HttpWebRequestActivitySourceTestsNew : IDisposable -{ - private readonly IDisposable testServer; - private readonly string testServerHost; - private readonly int testServerPort; - private readonly string hostNameAndPort; - private readonly string netPeerName; - private readonly int netPeerPort; - - static HttpWebRequestActivitySourceTestsNew() - { - var configuration = new ConfigurationBuilder() - .AddInMemoryCollection(new Dictionary { [SemanticConventionOptInKeyName] = "http" }) - .Build(); - - HttpClientInstrumentationOptions options = new(configuration) - { - EnrichWithHttpWebRequest = (activity, httpWebRequest) => - { - VerifyHeaders(httpWebRequest); - }, - }; - - HttpWebRequestActivitySource.Options = options; - - // Need to touch something in HttpWebRequestActivitySource/Sdk to do the static injection. - GC.KeepAlive(HttpWebRequestActivitySource.Options); - _ = Sdk.SuppressInstrumentation; - } - - public HttpWebRequestActivitySourceTestsNew() - { - Assert.Null(Activity.Current); - Activity.DefaultIdFormat = ActivityIdFormat.W3C; - Activity.ForceDefaultIdFormat = false; - - this.testServer = TestHttpServer.RunServer( - ctx => ProcessServerRequest(ctx), - out this.testServerHost, - out this.testServerPort); - - this.hostNameAndPort = $"{this.testServerHost}:{this.testServerPort}"; - this.netPeerName = this.testServerHost; - this.netPeerPort = this.testServerPort; - - void ProcessServerRequest(HttpListenerContext context) - { - string redirects = context.Request.QueryString["redirects"]; - if (!string.IsNullOrWhiteSpace(redirects) && int.TryParse(redirects, out int parsedRedirects) && parsedRedirects > 0) - { - context.Response.Redirect(this.BuildRequestUrl(queryString: $"redirects={--parsedRedirects}")); - context.Response.OutputStream.Close(); - return; - } - - string responseContent; - if (context.Request.QueryString["skipRequestContent"] == null) - { - using StreamReader readStream = new StreamReader(context.Request.InputStream); - - responseContent = readStream.ReadToEnd(); - } - else - { - responseContent = $"{{\"Id\":\"{Guid.NewGuid()}\"}}"; - } - - string responseCode = context.Request.QueryString["responseCode"]; - if (!string.IsNullOrWhiteSpace(responseCode)) - { - context.Response.StatusCode = int.Parse(responseCode); - } - else - { - context.Response.StatusCode = 200; - } - - if (context.Response.StatusCode != 204) - { - using StreamWriter writeStream = new StreamWriter(context.Response.OutputStream); - - writeStream.Write(responseContent); - } - else - { - context.Response.OutputStream.Close(); - } - } - } - - public void Dispose() - { - this.testServer.Dispose(); - } - - /// - /// Test to make sure we get both request and response events. - /// - [Theory] - [InlineData("GET")] - [InlineData("POST")] - [InlineData("POST", "skipRequestContent=1")] - public async Task TestBasicReceiveAndResponseEvents(string method, string queryString = null) - { - var url = this.BuildRequestUrl(queryString: queryString); - - using var eventRecords = new ActivitySourceRecorder(); - - // Send a random Http request to generate some events - using (var client = new HttpClient()) - { - (method == "GET" - ? await client.GetAsync(url).ConfigureAwait(false) - : await client.PostAsync(url, new StringContent("hello world")).ConfigureAwait(false)).Dispose(); - } - - // We should have exactly one Start and one Stop event - Assert.Equal(2, eventRecords.Records.Count); - Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Start")); - Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); - - // Check to make sure: The first record must be a request, the next record must be a response. - Activity activity = AssertFirstEventWasStart(eventRecords); - - VerifyActivityStartTags(this.netPeerName, this.netPeerPort, method, url, activity); - - Assert.True(eventRecords.Records.TryDequeue(out var stopEvent)); - Assert.Equal("Stop", stopEvent.Key); - - VerifyActivityStopTags(200, activity); - } - - private static Activity AssertFirstEventWasStart(ActivitySourceRecorder eventRecords) - { - Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair startEvent)); - Assert.Equal("Start", startEvent.Key); - return startEvent.Value; - } - - private static void VerifyHeaders(HttpWebRequest startRequest) - { - var tracestate = startRequest.Headers["tracestate"]; - Assert.Equal("some=state", tracestate); - - var baggage = startRequest.Headers["baggage"]; - Assert.Equal("k=v", baggage); - - var traceparent = startRequest.Headers["traceparent"]; - Assert.NotNull(traceparent); - Assert.Matches("^[0-9a-f]{2}-[0-9a-f]{32}-[0-9a-f]{16}-[0-9a-f]{2}$", traceparent); - } - - 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)); - } - - private static void ActivityEnrichment(Activity activity, string method, object obj) - { - switch (method) - { - case "OnStartActivity": - Assert.True(obj is HttpWebRequest); - VerifyHeaders(obj as HttpWebRequest); - - break; - - case "OnStopActivity": - Assert.True(obj is HttpWebResponse); - break; - - case "OnException": - Assert.True(obj is Exception); - break; - - default: - break; - } - } - - private static void ValidateBaggage(HttpWebRequest request) - { - string[] baggage = request.Headers["baggage"].Split(','); - - Assert.Equal(3, baggage.Length); - Assert.Contains("key=value", baggage); - Assert.Contains("bad%2Fkey=value", baggage); - Assert.Contains("goodkey=bad%2Fvalue", baggage); - } - - private string BuildRequestUrl(bool useHttps = false, string path = "echo", string queryString = null) - { - return $"{(useHttps ? "https" : "http")}://{this.testServerHost}:{this.testServerPort}/{path}{(string.IsNullOrWhiteSpace(queryString) ? string.Empty : $"?{queryString}")}"; - } - - private void CleanUpActivity() - { - while (Activity.Current != null) - { - Activity.Current.Stop(); - } - } - - /// - /// is a helper class for recording events. - /// - private class ActivitySourceRecorder : IDisposable - { - private readonly Action> onEvent; - private readonly ActivityListener activityListener; - - public ActivitySourceRecorder(Action> onEvent = null, ActivitySamplingResult activitySamplingResult = ActivitySamplingResult.AllDataAndRecorded) - { - this.activityListener = new ActivityListener - { - ShouldListenTo = (activitySource) => activitySource.Name == HttpWebRequestActivitySource.ActivitySourceName, - ActivityStarted = this.ActivityStarted, - ActivityStopped = this.ActivityStopped, - Sample = (ref ActivityCreationOptions options) => activitySamplingResult, - }; - - ActivitySource.AddActivityListener(this.activityListener); - - this.onEvent = onEvent; - } - - public ConcurrentQueue> Records { get; } = new ConcurrentQueue>(); - - public void Dispose() - { - this.activityListener.Dispose(); - } - - public void ActivityStarted(Activity activity) => this.Record("Start", activity); - - public void ActivityStopped(Activity activity) => this.Record("Stop", activity); - - private void Record(string eventName, Activity activity) - { - var record = new KeyValuePair(eventName, activity); - - this.Records.Enqueue(record); - this.onEvent?.Invoke(record); - } - } -} -#endif diff --git a/test/OpenTelemetry.Tests/Shared/TestHttpServer.cs b/test/OpenTelemetry.Tests/Shared/TestHttpServer.cs index e54f4a0a957..b59062f1042 100644 --- a/test/OpenTelemetry.Tests/Shared/TestHttpServer.cs +++ b/test/OpenTelemetry.Tests/Shared/TestHttpServer.cs @@ -39,10 +39,17 @@ public static IDisposable RunServer(Action action, out stri } catch (HttpListenerException) { + server?.Dispose(); + server = null; retryCount--; } } + if (server == null) + { + throw new InvalidOperationException("Server could not be started."); + } + return server; } From e70629b4242c7d2ebed2412c7fb16d421909cabd Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 29 Sep 2023 13:41:28 -0700 Subject: [PATCH 41/44] Code review. --- .../HttpClientTests.cs | 79 ++++++++----------- 1 file changed, 34 insertions(+), 45 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index ee0093d6895..8fb027fa1cc 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -168,58 +168,28 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( if (enableMetrics) { - meterProviderBuilder.AddHttpClientInstrumentation(); - - if (semanticConvention.HasValue) - { - meterProviderBuilder.ConfigureServices(s => - { - s.AddSingleton(new ConfigurationBuilder() - .AddInMemoryCollection( - new Dictionary - { - ["OTEL_SEMCONV_STABILITY_OPT_IN"] = semanticConvention == HttpSemanticConvention.Dupe - ? "http/dup" - : semanticConvention == HttpSemanticConvention.New - ? "http" - : "_invalid", - }) - .Build()); - }); - } + meterProviderBuilder + .AddHttpClientInstrumentation() + .ConfigureServices( + s => s.AddSingleton(BuildConfigurationWithSemanticConventionOptIn(semanticConvention))); } var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder(); if (enableTracing) { - tracerProviderBuilder.AddHttpClientInstrumentation((opt) => - { - opt.EnrichWithHttpWebRequest = (activity, httpRequestMessage) => { enrichWithHttpWebRequestCalled = true; }; - opt.EnrichWithHttpWebResponse = (activity, httpResponseMessage) => { enrichWithHttpWebResponseCalled = true; }; - opt.EnrichWithHttpRequestMessage = (activity, httpRequestMessage) => { enrichWithHttpRequestMessageCalled = true; }; - opt.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) => { enrichWithHttpResponseMessageCalled = true; }; - opt.EnrichWithException = (activity, exception) => { enrichWithExceptionCalled = true; }; - opt.RecordException = tc.RecordException ?? false; - }); - - if (semanticConvention.HasValue) - { - tracerProviderBuilder.ConfigureServices(s => + tracerProviderBuilder + .AddHttpClientInstrumentation((opt) => { - s.AddSingleton(new ConfigurationBuilder() - .AddInMemoryCollection( - new Dictionary - { - ["OTEL_SEMCONV_STABILITY_OPT_IN"] = semanticConvention == HttpSemanticConvention.Dupe - ? "http/dup" - : semanticConvention == HttpSemanticConvention.New - ? "http" - : "_invalid", - }) - .Build()); - }); - } + opt.EnrichWithHttpWebRequest = (activity, httpRequestMessage) => { enrichWithHttpWebRequestCalled = true; }; + opt.EnrichWithHttpWebResponse = (activity, httpResponseMessage) => { enrichWithHttpWebResponseCalled = true; }; + opt.EnrichWithHttpRequestMessage = (activity, httpRequestMessage) => { enrichWithHttpRequestMessageCalled = true; }; + opt.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) => { enrichWithHttpResponseMessageCalled = true; }; + opt.EnrichWithException = (activity, exception) => { enrichWithExceptionCalled = true; }; + opt.RecordException = tc.RecordException ?? false; + }) + .ConfigureServices( + s => s.AddSingleton(BuildConfigurationWithSemanticConventionOptIn(semanticConvention))); } var metrics = new List(); @@ -451,6 +421,25 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( } } + private static IConfiguration BuildConfigurationWithSemanticConventionOptIn( + HttpSemanticConvention? semanticConvention) + { + var builder = new ConfigurationBuilder(); + + if (semanticConvention != null && semanticConvention != HttpSemanticConvention.Old) + { + builder.AddInMemoryCollection( + new Dictionary + { + ["OTEL_SEMCONV_STABILITY_OPT_IN"] = semanticConvention == HttpSemanticConvention.Dupe + ? "http/dup" + : "http", + }); + } + + return builder.Build(); + } + private static async Task CheckEnrichment(Sampler sampler, bool enrichExpected, string url) { bool enrichWithHttpWebRequestCalled = false; From 616f299d8ae3cdc9c9ec71e52ea64a48d6b87da4 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 2 Oct 2023 15:13:18 -0700 Subject: [PATCH 42/44] Add support for http.client.request.duration. --- .../CHANGELOG.md | 31 +++- .../HttpHandlerMetricsDiagnosticListener.cs | 22 ++- .../HttpWebRequestActivitySource.netfx.cs | 36 ++--- .../HttpClientTests.cs | 134 ++++++++++++------ 4 files changed, 154 insertions(+), 69 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 320bc11ba47..5a1414c2183 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -2,7 +2,36 @@ ## Unreleased -* Added support for publishing `http.client.duration` metric on .NET Framework +* Introduced a new metric, `http.client.request.duration` measured in seconds. + The OTel SDK + [applies custom histogram buckets](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4820) + for this metric to comply with the + [Semantic Convention for Http Metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md). + This new metric is only available for users who opt-in to the new + semantic convention by configuring the `OTEL_SEMCONV_STABILITY_OPT_IN` + environment variable to either `http` (to emit only the new metric) or + `http/dup` (to emit both the new and old metrics). + ([#4870](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4870)) + * New metric: `http.client.request.duration` + * Unit: `s` (seconds) + * Histogram Buckets: `0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, + 0.75, 1, 2.5, 5, 7.5, 10` + * Old metric: `http.client.duration` + * Unit: `ms` (milliseconds) + * Histogram Buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, + 5000, 7500, 10000` + + Note: the older `http.client.duration` metric and + `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable will eventually be + removed after the HTTP semantic conventions are marked stable. + At which time this instrumentation can publish a stable release. Refer to + the specification for more information regarding the new HTTP semantic + conventions: + * [http-spans](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-spans.md) + * [http-metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md) + +* Added support for publishing `http.client.duration` & + `http.client.request.duration` metrics on .NET Framework ([#4870](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4870)) ## 1.5.1-beta.1 diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs index 26305a674f8..3c570a5c26d 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs @@ -37,6 +37,7 @@ internal sealed class HttpHandlerMetricsDiagnosticListener : ListenerHandler internal static readonly string MeterVersion = AssemblyName.Version.ToString(); internal static readonly Meter Meter = new(MeterName, MeterVersion); private static readonly Histogram HttpClientDuration = Meter.CreateHistogram("http.client.duration", "ms", "Measures the duration of outbound HTTP requests."); + private static readonly Histogram HttpClientRequestDuration = Meter.CreateHistogram("http.client.request.duration", "s", "Measures the duration of outbound HTTP requests."); private static readonly PropertyFetcher StopRequestFetcher = new("Request"); private static readonly PropertyFetcher StopResponseFetcher = new("Response"); @@ -65,11 +66,11 @@ public override void OnEventWritten(string name, object payload) var activity = Activity.Current; if (TryFetchRequest(payload, out HttpRequestMessage request)) { - TagList tags = default; - // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md if (this.emitOldAttributes) { + TagList tags = default; + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method))); tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme)); tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version))); @@ -84,11 +85,18 @@ public override void OnEventWritten(string name, object payload) { tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode))); } + + // We are relying here on HttpClient library to set duration before writing the stop event. + // https://github.com/dotnet/runtime/blob/90603686d314147017c8bbe1fa8965776ce607d0/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L178 + // TODO: Follow up with .NET team if we can continue to rely on this behavior. + HttpClientDuration.Record(activity.Duration.TotalMilliseconds, tags); } // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md if (this.emitNewAttributes) { + TagList tags = default; + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, HttpTagHelper.GetNameForHttpMethod(request.Method))); tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version))); tags.Add(new KeyValuePair(SemanticConventions.AttributeServerAddress, request.RequestUri.Host)); @@ -102,12 +110,12 @@ public override void OnEventWritten(string name, object payload) { tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode))); } - } - // We are relying here on HttpClient library to set duration before writing the stop event. - // https://github.com/dotnet/runtime/blob/90603686d314147017c8bbe1fa8965776ce607d0/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L178 - // TODO: Follow up with .NET team if we can continue to rely on this behavior. - HttpClientDuration.Record(activity.Duration.TotalMilliseconds, tags); + // We are relying here on HttpClient library to set duration before writing the stop event. + // https://github.com/dotnet/runtime/blob/90603686d314147017c8bbe1fa8965776ce607d0/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L178 + // TODO: Follow up with .NET team if we can continue to rely on this behavior. + HttpClientRequestDuration.Record(activity.Duration.TotalMilliseconds, tags); + } } } diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index cd47b265137..d341ebbcf14 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -49,6 +49,7 @@ internal static class HttpWebRequestActivitySource private static readonly ActivitySource WebRequestActivitySource = new(ActivitySourceName, Version); private static readonly Meter WebRequestMeter = new(MeterName, Version); private static readonly Histogram HttpClientDuration = WebRequestMeter.CreateHistogram("http.client.duration", "ms", "Measures the duration of outbound HTTP requests."); + private static readonly Histogram HttpClientRequestDuration = WebRequestMeter.CreateHistogram("http.client.request.duration", "s", "Measures the duration of outbound HTTP requests."); private static HttpClientInstrumentationOptions tracingOptions; private static HttpClientMetricInstrumentationOptions metricsOptions; @@ -298,7 +299,7 @@ private static void ProcessRequest(HttpWebRequest request) var enableTracing = WebRequestActivitySource.HasListeners() && TracingOptions.EventFilterHttpWebRequest(request); - if (!enableTracing && !HttpClientDuration.Enabled) + if (!enableTracing && !HttpClientDuration.Enabled && !HttpClientRequestDuration.Enabled) { // Tracing and metrics are not enabled, so we can skip generating signals // Propagation must still be done in such cases, to allow @@ -453,24 +454,26 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC activity?.Stop(); - if (HttpClientDuration.Enabled) + if (HttpClientDuration.Enabled || HttpClientRequestDuration.Enabled) { + double durationS; double durationMs; if (activity != null) { + durationS = activity.Duration.TotalSeconds; durationMs = activity.Duration.TotalMilliseconds; } else { var endTimestamp = Stopwatch.GetTimestamp(); - var durationS = (endTimestamp - startTimestamp) / (double)Stopwatch.Frequency; + durationS = (endTimestamp - startTimestamp) / (double)Stopwatch.Frequency; durationMs = durationS * 1000; } - TagList tags = default; - if (metricsEmitOldAttributes) { + TagList tags = default; + tags.Add(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)); tags.Add(SemanticConventions.AttributeHttpMethod, request.Method); tags.Add(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme); @@ -479,10 +482,19 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC { tags.Add(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port); } + + if (httpStatusCode.HasValue) + { + tags.Add(SemanticConventions.AttributeHttpStatusCode, (int)httpStatusCode.Value); + } + + HttpClientDuration.Record(durationMs, tags); } if (metricsEmitNewAttributes) { + TagList tags = default; + tags.Add(SemanticConventions.AttributeHttpRequestMethod, request.Method); tags.Add(SemanticConventions.AttributeServerAddress, request.RequestUri.Host); tags.Add(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion)); @@ -490,22 +502,14 @@ private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncC { tags.Add(SemanticConventions.AttributeServerPort, request.RequestUri.Port); } - } - - if (httpStatusCode.HasValue) - { - if (metricsEmitOldAttributes) - { - tags.Add(SemanticConventions.AttributeHttpStatusCode, (int)httpStatusCode.Value); - } - if (metricsEmitNewAttributes) + if (httpStatusCode.HasValue) { tags.Add(SemanticConventions.AttributeHttpResponseStatusCode, (int)httpStatusCode.Value); } - } - HttpClientDuration.Record(durationMs, tags); + HttpClientRequestDuration.Record(durationS, tags); + } } } diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 8fb027fa1cc..93bdfa54ecb 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -241,7 +241,7 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( } var requestMetrics = metrics - .Where(metric => metric.Name == "http.client.duration") + .Where(metric => metric.Name == "http.client.duration" || metric.Name == "http.client.request.duration") .ToArray(); var normalizedAttributesTestCase = tc.SpanAttributes.ToDictionary(x => x.Key, x => HttpTestData.NormalizeValues(x.Value, host, port)); @@ -342,69 +342,113 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( } else { - Assert.Single(requestMetrics); - - var metric = requestMetrics[0]; - Assert.NotNull(metric); - Assert.True(metric.MetricType == MetricType.Histogram); - - var metricPoints = new List(); - foreach (var p in metric.GetMetricPoints()) - { - metricPoints.Add(p); - } - - Assert.Single(metricPoints); - var metricPoint = metricPoints[0]; - - var count = metricPoint.GetHistogramCount(); - var sum = metricPoint.GetHistogramSum(); - - Assert.Equal(1L, count); - - if (enableTracing) + if (semanticConvention == HttpSemanticConvention.Dupe) { - var activity = Assert.Single(activities); - Assert.Equal(activity.Duration.TotalMilliseconds, sum); + Assert.Equal(2, requestMetrics.Length); } else { - Assert.True(sum > 0); + Assert.Single(requestMetrics); } - var attributes = new Dictionary(); - foreach (var tag in metricPoint.Tags) + if (semanticConvention == null || semanticConvention.Value.HasFlag(HttpSemanticConvention.Old)) { - attributes[tag.Key] = tag.Value; - } + var metric = requestMetrics.FirstOrDefault(m => m.Name == "http.client.duration"); + Assert.NotNull(metric); + Assert.True(metric.MetricType == MetricType.Histogram); - var expectedAttributeCount = semanticConvention == HttpSemanticConvention.Dupe - ? 9 + (tc.ResponseExpected ? 2 : 0) - : semanticConvention == HttpSemanticConvention.New - ? 4 + (tc.ResponseExpected ? 1 : 0) - : 5 + (tc.ResponseExpected ? 1 : 0); + var metricPoints = new List(); + foreach (var p in metric.GetMetricPoints()) + { + metricPoints.Add(p); + } - Assert.Equal(expectedAttributeCount, attributes.Count); + Assert.Single(metricPoints); + var metricPoint = metricPoints[0]; - if (semanticConvention == null || semanticConvention.Value.HasFlag(HttpSemanticConvention.Old)) - { - Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpMethod && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpMethod]); - Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetPeerName && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerName]); - Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetPeerPort && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerPort]); - Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpScheme && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpScheme]); - Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpFlavor && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); - if (tc.ResponseExpected) + var count = metricPoint.GetHistogramCount(); + var sum = metricPoint.GetHistogramSum(); + + Assert.Equal(1L, count); + + if (enableTracing) { - Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + var activity = Assert.Single(activities); + Assert.Equal(activity.Duration.TotalMilliseconds, sum); } else { - Assert.DoesNotContain(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpStatusCode); + Assert.True(sum > 0); + } + + var attributes = new Dictionary(); + foreach (var tag in metricPoint.Tags) + { + attributes[tag.Key] = tag.Value; + } + + var expectedAttributeCount = 5 + (tc.ResponseExpected ? 1 : 0); + + Assert.Equal(expectedAttributeCount, attributes.Count); + + if (semanticConvention == null || semanticConvention.Value.HasFlag(HttpSemanticConvention.Old)) + { + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpMethod && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpMethod]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetPeerName && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerName]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeNetPeerPort && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerPort]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpScheme && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpScheme]); + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpFlavor && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpFlavor]); + if (tc.ResponseExpected) + { + Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpStatusCode && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeHttpStatusCode]); + } + else + { + Assert.DoesNotContain(attributes, kvp => kvp.Key == SemanticConventions.AttributeHttpStatusCode); + } } } if (semanticConvention != null && semanticConvention.Value.HasFlag(HttpSemanticConvention.New)) { + var metric = requestMetrics.FirstOrDefault(m => m.Name == "http.client.request.duration"); + Assert.NotNull(metric); + Assert.True(metric.MetricType == MetricType.Histogram); + + var metricPoints = new List(); + foreach (var p in metric.GetMetricPoints()) + { + metricPoints.Add(p); + } + + Assert.Single(metricPoints); + var metricPoint = metricPoints[0]; + + var count = metricPoint.GetHistogramCount(); + var sum = metricPoint.GetHistogramSum(); + + Assert.Equal(1L, count); + + if (enableTracing) + { + var activity = Assert.Single(activities); + Assert.Equal(activity.Duration.TotalSeconds, sum); + } + else + { + Assert.True(sum > 0); + } + + var attributes = new Dictionary(); + foreach (var tag in metricPoint.Tags) + { + attributes[tag.Key] = tag.Value; + } + + var expectedAttributeCount = 4 + (tc.ResponseExpected ? 1 : 0); + + Assert.Equal(expectedAttributeCount, attributes.Count); + 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]); Assert.Contains(attributes, kvp => kvp.Key == SemanticConventions.AttributeServerPort && kvp.Value.ToString() == normalizedAttributesTestCase[SemanticConventions.AttributeNetPeerPort]); From 1502d642738b5952bf5bf2ca2d704f7f443fe8ee Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 2 Oct 2023 16:05:06 -0700 Subject: [PATCH 43/44] Update src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs Co-authored-by: Vishwesh Bankwar --- .../Implementation/HttpHandlerMetricsDiagnosticListener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs index 3c570a5c26d..f43e3c586de 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs @@ -114,7 +114,7 @@ public override void OnEventWritten(string name, object payload) // We are relying here on HttpClient library to set duration before writing the stop event. // https://github.com/dotnet/runtime/blob/90603686d314147017c8bbe1fa8965776ce607d0/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L178 // TODO: Follow up with .NET team if we can continue to rely on this behavior. - HttpClientRequestDuration.Record(activity.Duration.TotalMilliseconds, tags); + HttpClientRequestDuration.Record(activity.Duration.TotalSeconds, tags); } } } From 34e39f47e457e742c8c57da15dfc98b422728ba9 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 2 Oct 2023 16:09:12 -0700 Subject: [PATCH 44/44] MD lint. --- .../CHANGELOG.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 5a1414c2183..26a3971a0f9 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -12,6 +12,7 @@ environment variable to either `http` (to emit only the new metric) or `http/dup` (to emit both the new and old metrics). ([#4870](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4870)) + * New metric: `http.client.request.duration` * Unit: `s` (seconds) * Histogram Buckets: `0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, @@ -21,14 +22,13 @@ * Histogram Buckets: `0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000` - Note: the older `http.client.duration` metric and - `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable will eventually be - removed after the HTTP semantic conventions are marked stable. - At which time this instrumentation can publish a stable release. Refer to - the specification for more information regarding the new HTTP semantic - conventions: - * [http-spans](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-spans.md) - * [http-metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md) + Note: The older `http.client.duration` metric and + `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable will eventually be + removed after the HTTP semantic conventions are marked stable. At which time + this instrumentation can publish a stable release. Refer to the specification + for more information regarding the new HTTP semantic conventions: + * [http-spans](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-spans.md) + * [http-metrics](https://github.com/open-telemetry/semantic-conventions/blob/2bad9afad58fbd6b33cc683d1ad1f006e35e4a5d/docs/http/http-metrics.md) * Added support for publishing `http.client.duration` & `http.client.request.duration` metrics on .NET Framework