From aa1f3f0459b24f1fdf843cc4ae96f9df89e36e99 Mon Sep 17 00:00:00 2001 From: Tornhoof Date: Fri, 25 Feb 2022 11:02:59 +0100 Subject: [PATCH 1/3] Improve GetUri --- .../Implementation/HttpInListener.cs | 69 +++++++++++-------- ...stsCollectionsIsAccordingToTheSpecTests.cs | 19 +++-- 2 files changed, 52 insertions(+), 36 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 07606645a8..dcad0716c0 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -300,37 +300,46 @@ public override void OnException(Activity activity, object payload) private static string GetUri(HttpRequest request) { - var builder = new StringBuilder(); - - builder.Append(request.Scheme).Append("://"); - - if (request.Host.HasValue) - { - builder.Append(request.Host.Value); - } - else + // this follows the suggestions from https://github.com/dotnet/aspnetcore/issues/28906 + var scheme = request.Scheme ?? string.Empty; + + // HTTP 1.0 request with NO host header would result in empty Host. + // Use placeholder to avoid incorrect URL like "http:///" + var host = request.Host.Value ?? UnknownHostName; + var pathBase = request.PathBase.Value ?? string.Empty; + var path = request.Path.Value ?? string.Empty; + var queryString = request.QueryString.Value ?? string.Empty; + var length = scheme.Length + Uri.SchemeDelimiter.Length + host.Length + pathBase.Length + + path.Length + queryString.Length; +#if NETSTANDARD2_1 + return string.Create(length, (scheme, host, pathBase, path, queryString), (span, parts) => { - // HTTP 1.0 request with NO host header would result in empty Host. - // Use placeholder to avoid incorrect URL like "http:///" - builder.Append(UnknownHostName); - } - - if (request.PathBase.HasValue) - { - builder.Append(request.PathBase.Value); - } - - if (request.Path.HasValue) - { - builder.Append(request.Path.Value); - } - - if (request.QueryString.HasValue) - { - builder.Append(request.QueryString); - } - - return builder.ToString(); + CopyTo(ref span, parts.scheme); + CopyTo(ref span, Uri.SchemeDelimiter); + CopyTo(ref span, parts.host); + CopyTo(ref span, parts.pathBase); + CopyTo(ref span, parts.path); + CopyTo(ref span, parts.queryString); + + static void CopyTo(ref Span buffer, ReadOnlySpan text) + { + if (!text.IsEmpty) + { + text.CopyTo(buffer); + buffer = buffer.Slice(text.Length); + } + } + }); +#else + return new StringBuilder(length) + .Append(scheme) + .Append(Uri.SchemeDelimiter) + .Append(host) + .Append(pathBase) + .Append(path) + .Append(queryString) + .ToString(); +#endif } #if !NETSTANDARD2_0 diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs index e13174041f..da4be409d0 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs @@ -49,12 +49,13 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests(WebApplicationFactor } [Theory] - [InlineData("/api/values", "user-agent", 503, "503")] - [InlineData("/api/values", null, 503, null)] - [InlineData("/api/exception", null, 503, null)] - [InlineData("/api/exception", null, 503, null, true)] + [InlineData("/api/values", null, "user-agent", 503, "503")] + [InlineData("/api/values", "?query=1", null, 503, null)] + [InlineData("/api/exception", null, null, 503, null)] + [InlineData("/api/exception", null, null, 503, null, true)] public async Task SuccessfulTemplateControllerCallGeneratesASpan( string urlPath, + string query, string userAgent, int statusCode, string reasonPhrase, @@ -81,7 +82,13 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests(WebApplicationFactor } // Act - var response = await client.GetAsync(urlPath); + var path = urlPath; + if (query != null) + { + path += query; + } + + var response = await client.GetAsync(path); } catch (Exception) { @@ -109,7 +116,7 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests(WebApplicationFactor Assert.Equal("localhost", activity.GetTagValue(SemanticConventions.AttributeHttpHost)); Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpMethod)); Assert.Equal(urlPath, activity.GetTagValue(SemanticConventions.AttributeHttpTarget)); - Assert.Equal($"http://localhost{urlPath}", activity.GetTagValue(SemanticConventions.AttributeHttpUrl)); + Assert.Equal($"http://localhost{urlPath}{query}", activity.GetTagValue(SemanticConventions.AttributeHttpUrl)); Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpStatusCode)); if (statusCode == 503) From d0456020724582e30aa56e55a7dc5a43ec2b78d5 Mon Sep 17 00:00:00 2001 From: Tornhoof Date: Fri, 25 Feb 2022 19:09:45 +0100 Subject: [PATCH 2/3] Push namespace usage down to get around IDE0005 --- .../Implementation/HttpInListener.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index dcad0716c0..7478fb5582 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -22,7 +22,6 @@ #if !NETSTANDARD2_0 using System.Runtime.CompilerServices; #endif -using System.Text; using Microsoft.AspNetCore.Http; using OpenTelemetry.Context.Propagation; using OpenTelemetry.Internal; @@ -331,7 +330,7 @@ static void CopyTo(ref Span buffer, ReadOnlySpan text) } }); #else - return new StringBuilder(length) + return new System.Text.StringBuilder(length) .Append(scheme) .Append(Uri.SchemeDelimiter) .Append(host) From d54a24b05581114074224fba445252f582f1d767 Mon Sep 17 00:00:00 2001 From: Tornhoof Date: Wed, 9 Mar 2022 16:35:07 +0100 Subject: [PATCH 3/3] Add line to changelog --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 879f7811b3..ee795ffde5 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -5,6 +5,8 @@ * Fix: drop direct reference of the `Microsoft.AspNetCore.Http.Features` from net5 & net6 targets (already part of the FrameworkReference since the net5). ([#2860](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2860)) +* Reduce allocations calculating the http.url tag. + ([#2947](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2947)) ## Unreleased