PropertyFetcher API Improvements (#1238)

* Trying to improve the PropertyFetcher API.

* Cleanup.

* Cleanup.

* updating files, adding propertyFetcherTest

* CHANGELOG update.

* Attempting to fix line endings.

Co-authored-by: Eddy Nakamura <ednakamu@microsoft.com>
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs

@@ -19,7 +19,6 @@
 using System.Diagnostics;
 using System.Web;
 using System.Web.Routing;
-using OpenTelemetry.Context;
 using OpenTelemetry.Context.Propagation;
 using OpenTelemetry.Trace;
 
@@ -31,8 +30,8 @@ internal class HttpInListener : ListenerHandler
         public const string ResponseCustomPropertyName = "OTel.AspNet.Response";
         private const string ActivityNameByHttpInListener = "ActivityCreatedByHttpInListener";
         private static readonly Func<HttpRequest, string, IEnumerable<string>> HttpRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name);
-        private readonly PropertyFetcher routeFetcher = new PropertyFetcher("Route");
-        private readonly PropertyFetcher routeTemplateFetcher = new PropertyFetcher("RouteTemplate");
+        private readonly PropertyFetcher<object> routeFetcher = new PropertyFetcher<object>("Route");
+        private readonly PropertyFetcher<object> routeTemplateFetcher = new PropertyFetcher<object>("RouteTemplate");
         private readonly AspNetInstrumentationOptions options;
         private readonly ActivitySourceAdapter activitySource;
 

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -21,7 +21,6 @@
 using System.Text;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
-using OpenTelemetry.Context;
 using OpenTelemetry.Context.Propagation;
 using OpenTelemetry.Instrumentation.GrpcNetClient;
 using OpenTelemetry.Trace;
@@ -35,11 +34,11 @@ internal class HttpInListener : ListenerHandler
         private const string UnknownHostName = "UNKNOWN-HOST";
         private const string ActivityNameByHttpInListener = "ActivityCreatedByHttpInListener";
         private static readonly Func<HttpRequest, string, IEnumerable<string>> HttpRequestHeaderValuesGetter = (request, name) => request.Headers[name];
-        private readonly PropertyFetcher startContextFetcher = new PropertyFetcher("HttpContext");
-        private readonly PropertyFetcher stopContextFetcher = new PropertyFetcher("HttpContext");
-        private readonly PropertyFetcher beforeActionActionDescriptorFetcher = new PropertyFetcher("actionDescriptor");
-        private readonly PropertyFetcher beforeActionAttributeRouteInfoFetcher = new PropertyFetcher("AttributeRouteInfo");
-        private readonly PropertyFetcher beforeActionTemplateFetcher = new PropertyFetcher("Template");
+        private readonly PropertyFetcher<HttpContext> startContextFetcher = new PropertyFetcher<HttpContext>("HttpContext");
+        private readonly PropertyFetcher<HttpContext> stopContextFetcher = new PropertyFetcher<HttpContext>("HttpContext");
+        private readonly PropertyFetcher<object> beforeActionActionDescriptorFetcher = new PropertyFetcher<object>("actionDescriptor");
+        private readonly PropertyFetcher<object> beforeActionAttributeRouteInfoFetcher = new PropertyFetcher<object>("AttributeRouteInfo");
+        private readonly PropertyFetcher<string> beforeActionTemplateFetcher = new PropertyFetcher<string>("Template");
         private readonly bool hostingSupportsW3C;
         private readonly AspNetCoreInstrumentationOptions options;
         private readonly ActivitySourceAdapter activitySource;
@@ -55,7 +54,8 @@ public HttpInListener(string name, AspNetCoreInstrumentationOptions options, Act
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The objects should not be disposed.")]
         public override void OnStartActivity(Activity activity, object payload)
         {
-            if (!(this.startContextFetcher.Fetch(payload) is HttpContext context))
+            HttpContext context = this.startContextFetcher.Fetch(payload);
+            if (context == null)
             {
                 AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity));
                 return;
@@ -138,7 +138,8 @@ public override void OnStopActivity(Activity activity, object payload)
         {
             if (activity.IsAllDataRequested)
             {
-                if (!(this.stopContextFetcher.Fetch(payload) is HttpContext context))
+                HttpContext context = this.stopContextFetcher.Fetch(payload);
+                if (context == null)
                 {
                     AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStopActivity));
                     return;
@@ -193,7 +194,7 @@ public override void OnCustom(string name, Activity activity, object payload)
                     // Taking reference on MVC will increase size of deployment for non-MVC apps.
                     var actionDescriptor = this.beforeActionActionDescriptorFetcher.Fetch(payload);
                     var attributeRouteInfo = this.beforeActionAttributeRouteInfoFetcher.Fetch(actionDescriptor);
-                    var template = this.beforeActionTemplateFetcher.Fetch(attributeRouteInfo) as string;
+                    var template = this.beforeActionTemplateFetcher.Fetch(attributeRouteInfo);
 
                     if (!string.IsNullOrEmpty(template))
                     {

src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs

@@ -27,7 +27,7 @@ internal class GrpcClientDiagnosticListener : ListenerHandler
         private readonly GrpcClientInstrumentationOptions options;
 
         private readonly ActivitySourceAdapter activitySource;
-        private readonly PropertyFetcher startRequestFetcher = new PropertyFetcher("Request");
+        private readonly PropertyFetcher<HttpRequestMessage> startRequestFetcher = new PropertyFetcher<HttpRequestMessage>("Request");
 
         public GrpcClientDiagnosticListener(ActivitySourceAdapter activitySource, GrpcClientInstrumentationOptions options)
             : base("Grpc.Net.Client")

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

@@ -13,6 +13,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 // </copyright>
+
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
@@ -22,7 +23,6 @@
 using System.Runtime.Versioning;
 using System.Text.RegularExpressions;
 using System.Threading.Tasks;
-using OpenTelemetry.Context;
 using OpenTelemetry.Context.Propagation;
 using OpenTelemetry.Trace;
 
@@ -49,10 +49,10 @@ internal class HttpHandlerDiagnosticListener : ListenerHandler
         private static readonly Regex CoreAppMajorVersionCheckRegex = new Regex("^\\.NETCoreApp,Version=v(\\d+)\\.", RegexOptions.Compiled | RegexOptions.IgnoreCase);
 
         private readonly ActivitySourceAdapter activitySource;
-        private readonly PropertyFetcher startRequestFetcher = new PropertyFetcher("Request");
-        private readonly PropertyFetcher stopResponseFetcher = new PropertyFetcher("Response");
-        private readonly PropertyFetcher stopExceptionFetcher = new PropertyFetcher("Exception");
-        private readonly PropertyFetcher stopRequestStatusFetcher = new PropertyFetcher("RequestTaskStatus");
+        private readonly PropertyFetcher<HttpRequestMessage> startRequestFetcher = new PropertyFetcher<HttpRequestMessage>("Request");
+        private readonly PropertyFetcher<HttpResponseMessage> stopResponseFetcher = new PropertyFetcher<HttpResponseMessage>("Response");
+        private readonly PropertyFetcher<Exception> stopExceptionFetcher = new PropertyFetcher<Exception>("Exception");
+        private readonly PropertyFetcher<TaskStatus> stopRequestStatusFetcher = new PropertyFetcher<TaskStatus>("RequestTaskStatus");
         private readonly bool httpClientSupportsW3C;
         private readonly HttpClientInstrumentationOptions options;
 
@@ -120,21 +120,20 @@ public override void OnStopActivity(Activity activity, object payload)
         {
             if (activity.IsAllDataRequested)
             {
-                var requestTaskStatus = this.stopRequestStatusFetcher.Fetch(payload) as TaskStatus?;
+                // https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs
+                // requestTaskStatus is not null
+                var requestTaskStatus = this.stopRequestStatusFetcher.Fetch(payload);
 
-                if (requestTaskStatus.HasValue)
+                if (requestTaskStatus != TaskStatus.RanToCompletion)
                 {
-                    if (requestTaskStatus != TaskStatus.RanToCompletion)
+                    if (requestTaskStatus == TaskStatus.Canceled)
                     {
-                        if (requestTaskStatus == TaskStatus.Canceled)
-                        {
-                            activity.SetStatus(Status.Cancelled);
-                        }
-                        else if (requestTaskStatus != TaskStatus.Faulted)
-                        {
-                            // Faults are handled in OnException and should already have a span.Status of Unknown w/ Description.
-                            activity.SetStatus(Status.Unknown);
-                        }
+                        activity.SetStatus(Status.Cancelled);
+                    }
+                    else if (requestTaskStatus != TaskStatus.Faulted)
+                    {
+                        // Faults are handled in OnException and should already have a span.Status of Unknown w/ Description.
+                        activity.SetStatus(Status.Unknown);
                     }
                 }
 

src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

@@ -43,13 +43,13 @@ internal class SqlClientDiagnosticListener : ListenerHandler
         internal static readonly ActivitySource SqlClientActivitySource = new ActivitySource(ActivitySourceName, Version.ToString());
 #pragma warning restore SA1202 // Elements should be ordered by access
 
-        private readonly PropertyFetcher commandFetcher = new PropertyFetcher("Command");
-        private readonly PropertyFetcher connectionFetcher = new PropertyFetcher("Connection");
-        private readonly PropertyFetcher dataSourceFetcher = new PropertyFetcher("DataSource");
-        private readonly PropertyFetcher databaseFetcher = new PropertyFetcher("Database");
-        private readonly PropertyFetcher commandTypeFetcher = new PropertyFetcher("CommandType");
-        private readonly PropertyFetcher commandTextFetcher = new PropertyFetcher("CommandText");
-        private readonly PropertyFetcher exceptionFetcher = new PropertyFetcher("Exception");
+        private readonly PropertyFetcher<object> commandFetcher = new PropertyFetcher<object>("Command");
+        private readonly PropertyFetcher<object> connectionFetcher = new PropertyFetcher<object>("Connection");
+        private readonly PropertyFetcher<object> dataSourceFetcher = new PropertyFetcher<object>("DataSource");
+        private readonly PropertyFetcher<object> databaseFetcher = new PropertyFetcher<object>("Database");
+        private readonly PropertyFetcher<CommandType> commandTypeFetcher = new PropertyFetcher<CommandType>("CommandType");
+        private readonly PropertyFetcher<object> commandTextFetcher = new PropertyFetcher<object>("CommandText");
+        private readonly PropertyFetcher<Exception> exceptionFetcher = new PropertyFetcher<Exception>("Exception");
         private readonly SqlClientInstrumentationOptions options;
 
         public SqlClientDiagnosticListener(string sourceName, SqlClientInstrumentationOptions options)

src/OpenTelemetry/CHANGELOG.md

@@ -7,6 +7,8 @@
   ([#1203](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1203))
 * `PropertyFetcher` is now public
   ([#1232](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1232))
+* `PropertyFetcher` changed to `PropertyFetcher<T>`
+  ([#1238](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1238))
 
 ## 0.5.0-beta.2
 

src/OpenTelemetry/Instrumentation/PropertyFetcher.cs

@@ -23,13 +23,14 @@ namespace OpenTelemetry.Instrumentation
     /// <summary>
     /// PropertyFetcher fetches a property from an object.
     /// </summary>
-    public class PropertyFetcher
+    /// <typeparam name="T">The type of the property being fetched.</typeparam>
+    public class PropertyFetcher<T>
     {
         private readonly string propertyName;
         private PropertyFetch innerFetcher;
 
         /// <summary>
-        /// Initializes a new instance of the <see cref="PropertyFetcher"/> class.
+        /// Initializes a new instance of the <see cref="PropertyFetcher{T}"/> class.
         /// </summary>
         /// <param name="propertyName">Property name to fetch.</param>
         public PropertyFetcher(string propertyName)
@@ -42,7 +43,7 @@ public PropertyFetcher(string propertyName)
         /// </summary>
         /// <param name="obj">Object to be fetched.</param>
         /// <returns>Property fetched.</returns>
-        public object Fetch(object obj)
+        public T Fetch(object obj)
         {
             if (this.innerFetcher == null)
             {
@@ -56,7 +57,7 @@ public object Fetch(object obj)
                 this.innerFetcher = PropertyFetch.FetcherForProperty(property);
             }
 
-            return this.innerFetcher?.Fetch(obj);
+            return this.innerFetcher.Fetch(obj);
         }
 
         // see https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs
@@ -68,43 +69,41 @@ private class PropertyFetch
             /// </summary>
             public static PropertyFetch FetcherForProperty(PropertyInfo propertyInfo)
             {
-                if (propertyInfo == null)
+                if (propertyInfo == null || !typeof(T).IsAssignableFrom(propertyInfo.PropertyType))
                 {
                     // returns null on any fetch.
                     return new PropertyFetch();
                 }
 
-                var typedPropertyFetcher = typeof(TypedFetchProperty<,>);
-                var instantiatedTypedPropertyFetcher = typedPropertyFetcher.GetTypeInfo().MakeGenericType(
-                    propertyInfo.DeclaringType, propertyInfo.PropertyType);
+                var typedPropertyFetcher = typeof(TypedPropertyFetch<,>);
+                var instantiatedTypedPropertyFetcher = typedPropertyFetcher.MakeGenericType(
+                    typeof(T), propertyInfo.DeclaringType, propertyInfo.PropertyType);
                 return (PropertyFetch)Activator.CreateInstance(instantiatedTypedPropertyFetcher, propertyInfo);
             }
 
-            /// <summary>
-            /// Given an object, fetch the property that this propertyFetch represents.
-            /// </summary>
-            public virtual object Fetch(object obj)
+            public virtual T Fetch(object obj)
             {
-                return null;
+                return default;
             }
 
-            private class TypedFetchProperty<TObject, TProperty> : PropertyFetch
+            private class TypedPropertyFetch<TDeclaredObject, TDeclaredProperty> : PropertyFetch
+                where TDeclaredProperty : T
             {
-                private readonly Func<TObject, TProperty> propertyFetch;
+                private readonly Func<TDeclaredObject, TDeclaredProperty> propertyFetch;
 
-                public TypedFetchProperty(PropertyInfo property)
+                public TypedPropertyFetch(PropertyInfo property)
                 {
-                    this.propertyFetch = (Func<TObject, TProperty>)property.GetMethod.CreateDelegate(typeof(Func<TObject, TProperty>));
+                    this.propertyFetch = (Func<TDeclaredObject, TDeclaredProperty>)property.GetMethod.CreateDelegate(typeof(Func<TDeclaredObject, TDeclaredProperty>));
                 }
 
-                public override object Fetch(object obj)
+                public override T Fetch(object obj)
                 {
-                    if (obj is TObject o)
+                    if (obj is TDeclaredObject o)
                     {
                         return this.propertyFetch(o);
                     }
 
-                    return null;
+                    return default;
                 }
             }
         }

test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs

@@ -0,0 +1,49 @@
+// <copyright file="PropertyFetcherTest.cs" company="OpenTelemetry Authors">
+// 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.
+// </copyright>
+
+using System.Diagnostics;
+using OpenTelemetry.Instrumentation;
+using Xunit;
+
+namespace OpenTelemetry.Tests.Instrumentation
+{
+    public class PropertyFetcherTest
+    {
+        [Fact]
+        public void FetchValidProperty()
+        {
+            var activity = new Activity("test");
+            var fetch = new PropertyFetcher<string>("DisplayName");
+            var result = fetch.Fetch(activity);
+
+            Assert.Equal(activity.DisplayName, result);
+        }
+
+        [Fact]
+        public void FetchInvalidProperty()
+        {
+            var activity = new Activity("test");
+            var fetch = new PropertyFetcher<string>("DisplayName2");
+            var result = fetch.Fetch(activity);
+
+            var fetchInt = new PropertyFetcher<int>("DisplayName2");
+            var resultInt = fetchInt.Fetch(activity);
+
+            Assert.Equal(default, result);
+            Assert.Equal(default, resultInt);
+        }
+    }
+}