[Instrumentation.Http][Instrumentation.AspNetCore] Fix url.full and url.query attribute values (#5532)

vishweshbankwar · web-flow · commit e222ecb5942d · 2024-04-12T11:47:42.000-07:00
diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln
@@ -270,6 +270,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shared", "Shared", "{A49299
 		src\Shared\PeerServiceResolver.cs = src\Shared\PeerServiceResolver.cs
 		src\Shared\PeriodicExportingMetricReaderHelper.cs = src\Shared\PeriodicExportingMetricReaderHelper.cs
 		src\Shared\PooledList.cs = src\Shared\PooledList.cs
+		src\Shared\RedactionHelper.cs = src\Shared\RedactionHelper.cs
 		src\Shared\RequestMethodHelper.cs = src\Shared\RequestMethodHelper.cs
 		src\Shared\ResourceSemanticConventions.cs = src\Shared\ResourceSemanticConventions.cs
 		src\Shared\SemanticConventions.cs = src\Shared\SemanticConventions.cs
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/AspNetCoreTraceInstrumentationOptions.cs
@@ -32,6 +32,14 @@ internal AspNetCoreTraceInstrumentationOptions(IConfiguration configuration)
         {
             this.EnableGrpcAspNetCoreSupport = enableGrpcInstrumentation;
         }
+
+        if (configuration.TryGetBoolValue(
+            AspNetCoreInstrumentationEventSource.Log,
+            "OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_DISABLE_URL_QUERY_REDACTION",
+            out var disableUrlQueryRedaction))
+        {
+            this.DisableUrlQueryRedaction = disableUrlQueryRedaction;
+        }
     }
 
     /// <summary>
@@ -94,4 +102,14 @@ internal AspNetCoreTraceInstrumentationOptions(IConfiguration configuration)
     /// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-spans.md.
     /// </remarks>
     internal bool EnableGrpcAspNetCoreSupport { get; set; }
+
+    /// <summary>
+    /// Gets or sets a value indicating whether the url query value should be redacted or not.
+    /// </summary>
+    /// <remarks>
+    /// The query parameter values are redacted with value set as Redacted.
+    /// e.g. `?key1=value1` is set as `?key1=Redacted`.
+    /// The redaction can be disabled by setting this property to <see langword="true" />.
+    /// </remarks>
+    internal bool DisableUrlQueryRedaction { get; set; }
 }
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
@@ -1,6 +1,16 @@
 # Changelog
 
-## Unreleased
+## 1.8.1
+
+Released 2024-Apr-12
+
+* **Breaking Change**: Fixed tracing instrumentation so that by default any
+  values detected in the query string component of requests are replaced with
+  the text `Redacted` when building the `url.query` tag. For example,
+  `?key1=value1&key2=value2` becomes `?key1=Redacted&key2=Redacted`. You can
+  disable this redaction by setting the environment variable
+  `OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_DISABLE_URL_QUERY_REDACTION` to `true`.
+  ([#5532](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5532))
 
 ## 1.8.0
 
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
@@ -193,8 +193,14 @@ public void OnStartActivity(Activity activity, object payload)
 
             if (request.QueryString.HasValue)
             {
-                // QueryString should be sanitized. see: https://github.com/open-telemetry/opentelemetry-dotnet/issues/4571
-                activity.SetTag(SemanticConventions.AttributeUrlQuery, request.QueryString.Value);
+                if (this.options.DisableUrlQueryRedaction)
+                {
+                    activity.SetTag(SemanticConventions.AttributeUrlQuery, request.QueryString.Value);
+                }
+                else
+                {
+                    activity.SetTag(SemanticConventions.AttributeUrlQuery, RedactionHelper.GetRedactedQueryString(request.QueryString.Value));
+                }
             }
 
             RequestMethodHelper.SetHttpMethodTag(activity, request.Method);
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj b/src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj
@@ -17,6 +17,7 @@
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Instrumentation.GrpcNetClient\GrpcTagHelper.cs" Link="Includes\GrpcTagHelper.cs" />
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Instrumentation.GrpcNetClient\StatusCanonicalCode.cs" Link="Includes\StatusCanonicalCode.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" />
+    <Compile Include="$(RepoRoot)\src\Shared\RedactionHelper.cs" Link="Includes\RedactionHelper.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\RequestMethodHelper.cs" Link="Includes\RequestMethodHelper.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Shims\NullableAttributes.cs" Link="Includes\Shims\NullableAttributes.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Options\*.cs" Link="Includes\Options\%(Filename).cs" />
diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/README.md b/src/OpenTelemetry.Instrumentation.AspNetCore/README.md
@@ -75,7 +75,11 @@ for more details about each individual attribute:
 * `server.address`
 * `server.port`
 * `url.path`
-* `url.query`
+* `url.query` - By default, the values in the query component are replaced with
+  the text `Redacted`. For example, `?key1=value1&key2=value2` becomes
+  `?key1=Redacted&key2=Redacted`. You can disable this redaction by setting the
+  environment variable
+  `OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_DISABLE_URL_QUERY_REDACTION` to `true`.
 * `url.scheme`
 
 [Enrich Api](#enrich) can be used if any additional attributes are
diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
@@ -1,6 +1,16 @@
 # Changelog
 
-## Unreleased
+## 1.8.1
+
+Released 2024-Apr-12
+
+* **Breaking Change**: Fixed tracing instrumentation so that by default any
+  values detected in the query string component of requests are replaced with
+  the text `Redacted` when building the `url.full` tag. For example,
+  `?key1=value1&key2=value2` becomes `?key1=Redacted&key2=Redacted`. You can
+  disable this redaction by setting the environment variable
+  `OTEL_DOTNET_EXPERIMENTAL_HTTPCLIENT_DISABLE_URL_QUERY_REDACTION` to `true`.
+  ([#5532](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5532))
 
 ## 1.8.0
 
diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationTracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationTracerProviderBuilderExtensions.cs
@@ -59,6 +59,8 @@ public static TracerProviderBuilder AddHttpClientInstrumentation(
             {
                 services.Configure(name, configureHttpClientTraceInstrumentationOptions);
             }
+
+            services.RegisterOptionsFactory(configuration => new HttpClientTraceInstrumentationOptions(configuration));
         });
 
 #if NETFRAMEWORK
diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.Http/HttpClientTraceInstrumentationOptions.cs
@@ -3,10 +3,11 @@
 
 using System.Diagnostics;
 using System.Net;
+using System.Runtime.CompilerServices;
 #if NETFRAMEWORK
 using System.Net.Http;
 #endif
-using System.Runtime.CompilerServices;
+using Microsoft.Extensions.Configuration;
 using OpenTelemetry.Instrumentation.Http.Implementation;
 
 namespace OpenTelemetry.Instrumentation.Http;
@@ -16,6 +17,27 @@ namespace OpenTelemetry.Instrumentation.Http;
 /// </summary>
 public class HttpClientTraceInstrumentationOptions
 {
+    /// <summary>
+    /// Initializes a new instance of the <see cref="HttpClientTraceInstrumentationOptions"/> class.
+    /// </summary>
+    public HttpClientTraceInstrumentationOptions()
+        : this(new ConfigurationBuilder().AddEnvironmentVariables().Build())
+    {
+    }
+
+    internal HttpClientTraceInstrumentationOptions(IConfiguration configuration)
+    {
+        Debug.Assert(configuration != null, "configuration was null");
+
+        if (configuration.TryGetBoolValue(
+           HttpInstrumentationEventSource.Log,
+           "OTEL_DOTNET_EXPERIMENTAL_HTTPCLIENT_DISABLE_URL_QUERY_REDACTION",
+           out var disableUrlQueryRedaction))
+        {
+            this.DisableUrlQueryRedaction = disableUrlQueryRedaction;
+        }
+    }
+
     /// <summary>
     /// Gets or sets a filter function that determines whether or not to
     /// collect telemetry on a per request basis.
@@ -125,6 +147,16 @@ public class HttpClientTraceInstrumentationOptions
     /// </remarks>
     public bool RecordException { get; set; }
 
+    /// <summary>
+    /// Gets or sets a value indicating whether the url query value should be redacted or not.
+    /// </summary>
+    /// <remarks>
+    /// The query parameter values are redacted with value set as Redacted.
+    /// e.g. `?key1=value1` is set as `?key1=Redacted`.
+    /// The redaction can be disabled by setting this property to <see langword="true" />.
+    /// </remarks>
+    internal bool DisableUrlQueryRedaction { get; set; }
+
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     internal bool EventFilterHttpRequestMessage(string activityName, object arg1)
     {
diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
@@ -149,7 +149,7 @@ public void OnStartActivity(Activity activity, object payload)
             activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host);
             activity.SetTag(SemanticConventions.AttributeServerPort, request.RequestUri.Port);
 
-            activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));
+            activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri, this.options.DisableUrlQueryRedaction));
 
             try
             {
diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpInstrumentationEventSource.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpInstrumentationEventSource.cs
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 using System.Diagnostics.Tracing;
+using Microsoft.Extensions.Configuration;
 using OpenTelemetry.Internal;
 
 namespace OpenTelemetry.Instrumentation.Http.Implementation;
@@ -10,7 +11,7 @@ namespace OpenTelemetry.Instrumentation.Http.Implementation;
 /// EventSource events emitted from the project.
 /// </summary>
 [EventSource(Name = "OpenTelemetry-Instrumentation-Http")]
-internal sealed class HttpInstrumentationEventSource : EventSource
+internal sealed class HttpInstrumentationEventSource : EventSource, IConfigurationExtensionsLogger
 {
     public static HttpInstrumentationEventSource Log = new();
 
@@ -100,4 +101,15 @@ public void UnknownErrorProcessingEvent(string handlerName, string eventName, st
     {
         this.WriteEvent(7, handlerName, eventName, ex);
     }
+
+    [Event(8, Message = "Configuration key '{0}' has an invalid value: '{1}'", Level = EventLevel.Warning)]
+    public void InvalidConfigurationValue(string key, string value)
+    {
+        this.WriteEvent(8, key, value);
+    }
+
+    void IConfigurationExtensionsLogger.LogInvalidConfigurationValue(string key, string value)
+    {
+        this.InvalidConfigurationValue(key, value);
+    }
 }
diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpTagHelper.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpTagHelper.cs
@@ -1,6 +1,8 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
+using OpenTelemetry.Internal;
+
 namespace OpenTelemetry.Instrumentation.Http.Implementation;
 
 /// <summary>
@@ -12,15 +14,18 @@ internal static class HttpTagHelper
     /// Gets the OpenTelemetry standard uri tag value for a span based on its request <see cref="Uri"/>.
     /// </summary>
     /// <param name="uri"><see cref="Uri"/>.</param>
+    /// <param name="disableQueryRedaction">Indicates whether query parameter should be redacted or not.</param>
     /// <returns>Span uri value.</returns>
-    public static string GetUriTagValueFromRequestUri(Uri uri)
+    public static string GetUriTagValueFromRequestUri(Uri uri, bool disableQueryRedaction)
     {
-        if (string.IsNullOrEmpty(uri.UserInfo))
+        if (string.IsNullOrEmpty(uri.UserInfo) && disableQueryRedaction)
         {
             return uri.OriginalString;
         }
 
-        return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment);
+        var query = disableQueryRedaction ? uri.Query : RedactionHelper.GetRedactedQueryString(uri.Query);
+
+        return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.AbsolutePath, query, uri.Fragment);
     }
 
     public static string GetProtocolVersionString(Version httpVersion) => (httpVersion.Major, httpVersion.Minor) switch
diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs
@@ -105,7 +105,7 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A
             activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host);
             activity.SetTag(SemanticConventions.AttributeServerPort, request.RequestUri.Port);
 
-            activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));
+            activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri, tracingOptions.DisableUrlQueryRedaction));
 
             try
             {
diff --git a/src/OpenTelemetry.Instrumentation.Http/OpenTelemetry.Instrumentation.Http.csproj b/src/OpenTelemetry.Instrumentation.Http/OpenTelemetry.Instrumentation.Http.csproj
@@ -12,8 +12,12 @@
   </PropertyGroup>
 
   <ItemGroup>
+    <Compile Include="$(RepoRoot)\src\Shared\Configuration\*.cs" Link="Includes\Configuration\%(Filename).cs" />
+    <Compile Include="$(RepoRoot)\src\Shared\EnvironmentVariables\*.cs" Link="Includes\EnvironmentVariables\%(Filename).cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Shims\NullableAttributes.cs" Link="Includes\Shims\NullableAttributes.cs" />
+    <Compile Include="$(RepoRoot)\src\Shared\Options\*.cs" Link="Includes\Options\%(Filename).cs" />
+    <Compile Include="$(RepoRoot)\src\Shared\RedactionHelper.cs" Link="Includes\RedactionHelper.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\RequestMethodHelper.cs" Link="Includes\RequestMethodHelper.cs" />
   </ItemGroup>
 
@@ -29,6 +33,7 @@
   <ItemGroup>
     <Reference Include="System.Net.Http" Condition="'$(TargetFramework)' == '$(NetFrameworkMinimumSupportedVersion)'" />
     <PackageReference Include="Microsoft.Extensions.Options" />
+    <PackageReference Include="Microsoft.Extensions.Configuration" />
   </ItemGroup>
 
 </Project>
diff --git a/src/OpenTelemetry.Instrumentation.Http/README.md b/src/OpenTelemetry.Instrumentation.Http/README.md
@@ -68,7 +68,11 @@ for more details about each individual attribute:
 * `network.protocol.version`
 * `server.address`
 * `server.port`
-* `url.full`
+* `url.full` - By default, the values in the query component of the url are
+  replaced with the text `Redacted`. For example, `?key1=value1&key2=value2`
+  becomes `?key1=Redacted&key2=Redacted`. You can disable this redaction by
+  setting the environment variable
+  `OTEL_DOTNET_EXPERIMENTAL_HTTPCLIENT_DISABLE_URL_QUERY_REDACTION` to `true`.
 
 [Enrich Api](#enrich-httpclient-api) can be used if any additional attributes are
 required on activity.
diff --git a/src/Shared/RedactionHelper.cs b/src/Shared/RedactionHelper.cs
@@ -0,0 +1,59 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+#nullable enable
+
+using System.Text;
+
+namespace OpenTelemetry.Internal;
+
+internal class RedactionHelper
+{
+    private static readonly string RedactedText = "Redacted";
+
+    public static string? GetRedactedQueryString(string query)
+    {
+        int length = query.Length;
+        int index = 0;
+
+        // Preallocate some size to avoid re-sizing multiple times.
+        // Since the size will increase, allocating twice as much.
+        // TODO: Check to see if we can borrow from https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/System/Text/ValueStringBuilder.cs
+        // to improve perf.
+        StringBuilder queryBuilder = new(2 * length);
+        while (index < query.Length)
+        {
+            // Check if the character is = for redacting value.
+            if (query[index] == '=')
+            {
+                // Append =
+                queryBuilder.Append('=');
+                index++;
+
+                // Append redactedText in place of original value.
+                queryBuilder.Append(RedactedText);
+
+                // Move until end of this key/value pair.
+                while (index < length && query[index] != '&')
+                {
+                    index++;
+                }
+
+                // End of key/value.
+                if (index < length && query[index] == '&')
+                {
+                    queryBuilder.Append(query[index]);
+                }
+            }
+            else
+            {
+                // Keep adding to the result
+                queryBuilder.Append(query[index]);
+            }
+
+            index++;
+        }
+
+        return queryBuilder.ToString();
+    }
+}
diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs
diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs
diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs
diff --git a/test/OpenTelemetry.Tests/Internal/RedactionHelperTest.cs b/test/OpenTelemetry.Tests/Internal/RedactionHelperTest.cs
diff --git a/test/OpenTelemetry.Tests/OpenTelemetry.Tests.csproj b/test/OpenTelemetry.Tests/OpenTelemetry.Tests.csproj

Original file line number	Diff line number	Diff line change
`@@ -59,6 +59,8 @@ public static TracerProviderBuilder AddHttpClientInstrumentation(`
`59`	`59`	`{`
`60`	`60`	`services.Configure(name, configureHttpClientTraceInstrumentationOptions);`
`61`	`61`	`}`
	`62`	`+`
	`63`	`+ services.RegisterOptionsFactory(configuration => new HttpClientTraceInstrumentationOptions(configuration));`
`62`	`64`	`});`
`63`	`65`
`64`	`66`	`#if NETFRAMEWORK`
Original file line number	Diff line number	Diff line change
`@@ -149,7 +149,7 @@ public void OnStartActivity(Activity activity, object payload)`
`149`	`149`	`activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host);`
`150`	`150`	`activity.SetTag(SemanticConventions.AttributeServerPort, request.RequestUri.Port);`
`151`	`151`
`152`		`- activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));`
	`152`	`+ activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri, this.options.DisableUrlQueryRedaction));`
`153`	`153`
`154`	`154`	`try`
`155`	`155`	`{`