Fixing null reference in propertyfetcher (#1350)

replacing fetch for tryfetch

fixing netcoreapp3.1 issue in htp instrumentation

adding back Fetch method

adding null validation; adding null test

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

@@ -32,7 +32,7 @@ internal class HttpInListener : ListenerHandler
         private const string ActivityOperationName = "Microsoft.AspNet.HttpReqIn.Start";
         private static readonly Func<HttpRequest, string, IEnumerable<string>> HttpRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name);
         private readonly PropertyFetcher<object> routeFetcher = new PropertyFetcher<object>("Route");
-        private readonly PropertyFetcher<object> routeTemplateFetcher = new PropertyFetcher<object>("RouteTemplate");
+        private readonly PropertyFetcher<string> routeTemplateFetcher = new PropertyFetcher<string>("RouteTemplate");
         private readonly AspNetInstrumentationOptions options;
         private readonly ActivitySourceAdapter activitySource;
 
@@ -195,8 +195,8 @@ public override void OnStopActivity(Activity activity, object payload)
                     {
                         var subRouteData = attributeRouting.GetValue(0);
 
-                        var route = this.routeFetcher.Fetch(subRouteData);
-                        template = this.routeTemplateFetcher.Fetch(route) as string;
+                        _ = this.routeFetcher.TryFetch(subRouteData, out var route);
+                        _ = this.routeTemplateFetcher.TryFetch(route, out template);
                     }
                 }
                 else if (routeData.Route is Route route)

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

@@ -54,7 +54,7 @@ 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)
         {
-            HttpContext context = this.startContextFetcher.Fetch(payload);
+            _ = this.startContextFetcher.TryFetch(payload, out HttpContext context);
             if (context == null)
             {
                 AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStartActivity));
@@ -151,7 +151,7 @@ public override void OnStopActivity(Activity activity, object payload)
         {
             if (activity.IsAllDataRequested)
             {
-                HttpContext context = this.stopContextFetcher.Fetch(payload);
+                _ = this.stopContextFetcher.TryFetch(payload, out HttpContext context);
                 if (context == null)
                 {
                     AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInListener), nameof(this.OnStopActivity));
@@ -214,9 +214,9 @@ public override void OnCustom(string name, Activity activity, object payload)
                     // The reason to use reflection is to avoid a reference on MVC package.
                     // This package can be used with non-MVC apps and this logic simply wouldn't run.
                     // 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);
+                    _ = this.beforeActionActionDescriptorFetcher.TryFetch(payload, out var actionDescriptor);
+                    _ = this.beforeActionAttributeRouteInfoFetcher.TryFetch(actionDescriptor, out var attributeRouteInfo);
+                    _ = this.beforeActionTemplateFetcher.TryFetch(attributeRouteInfo, out var template);
 
                     if (!string.IsNullOrEmpty(template))
                     {

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

@@ -42,7 +42,7 @@ public GrpcClientDiagnosticListener(ActivitySourceAdapter activitySource, GrpcCl
 
         public override void OnStartActivity(Activity activity, object payload)
         {
-            if (!(this.startRequestFetcher.Fetch(payload) is HttpRequestMessage request))
+            if (!this.startRequestFetcher.TryFetch(payload, out HttpRequestMessage request) || request == null)
             {
                 GrpcInstrumentationEventSource.Log.NullPayload(nameof(GrpcClientDiagnosticListener), nameof(this.OnStartActivity));
                 return;

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

@@ -81,7 +81,7 @@ public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options, A
 
         public override void OnStartActivity(Activity activity, object payload)
         {
-            if (!(this.startRequestFetcher.Fetch(payload) is HttpRequestMessage request))
+            if (!this.startRequestFetcher.TryFetch(payload, out HttpRequestMessage request) || request == null)
             {
                 HttpInstrumentationEventSource.Log.NullPayload(nameof(HttpHandlerDiagnosticListener), nameof(this.OnStartActivity));
                 return;
@@ -131,7 +131,7 @@ public override void OnStopActivity(Activity activity, object payload)
             {
                 // 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);
+                _ = this.stopRequestStatusFetcher.TryFetch(payload, out var requestTaskStatus);
 
                 if (requestTaskStatus != TaskStatus.RanToCompletion)
                 {
@@ -146,7 +146,7 @@ public override void OnStopActivity(Activity activity, object payload)
                     }
                 }
 
-                if (this.stopResponseFetcher.Fetch(payload) is HttpResponseMessage response)
+                if (this.stopResponseFetcher.TryFetch(payload, out HttpResponseMessage response) && response != null)
                 {
                     try
                     {
@@ -173,7 +173,7 @@ public override void OnException(Activity activity, object payload)
         {
             if (activity.IsAllDataRequested)
             {
-                if (!(this.stopExceptionFetcher.Fetch(payload) is Exception exc))
+                if (!this.stopExceptionFetcher.TryFetch(payload, out Exception exc) || exc == null)
                 {
                     HttpInstrumentationEventSource.Log.NullPayload(nameof(HttpHandlerDiagnosticListener), nameof(this.OnException));
                     return;

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

@@ -75,7 +75,7 @@ public override void OnCustom(string name, Activity activity, object payload)
                             return;
                         }
 
-                        var command = this.commandFetcher.Fetch(payload);
+                        _ = this.commandFetcher.TryFetch(payload, out var command);
                         if (command == null)
                         {
                             SqlClientInstrumentationEventSource.Log.NullPayload(nameof(SqlClientDiagnosticListener), name);
@@ -85,8 +85,8 @@ public override void OnCustom(string name, Activity activity, object payload)
 
                         if (activity.IsAllDataRequested)
                         {
-                            var connection = this.connectionFetcher.Fetch(command);
-                            var database = this.databaseFetcher.Fetch(connection);
+                            _ = this.connectionFetcher.TryFetch(command, out var connection);
+                            _ = this.databaseFetcher.TryFetch(connection, out var database);
 
                             activity.DisplayName = (string)database;
                             try
@@ -98,15 +98,15 @@ public override void OnCustom(string name, Activity activity, object payload)
                                 SqlClientInstrumentationEventSource.Log.EnrichmentException(ex);
                             }
 
-                            var dataSource = this.dataSourceFetcher.Fetch(connection);
-                            var commandText = this.commandTextFetcher.Fetch(command);
+                            _ = this.dataSourceFetcher.TryFetch(connection, out var dataSource);
+                            _ = this.commandTextFetcher.TryFetch(command, out var commandText);
 
                             activity.SetTag(SemanticConventions.AttributeDbSystem, MicrosoftSqlServerDatabaseSystemName);
                             activity.SetTag(SemanticConventions.AttributeDbName, (string)database);
 
                             this.options.AddConnectionLevelDetailsToActivity((string)dataSource, activity);
 
-                            if (this.commandTypeFetcher.Fetch(command) is CommandType commandType)
+                            if (this.commandTypeFetcher.TryFetch(command, out CommandType commandType))
                             {
                                 switch (commandType)
                                 {
@@ -183,7 +183,7 @@ public override void OnCustom(string name, Activity activity, object payload)
                         {
                             if (activity.IsAllDataRequested)
                             {
-                                if (this.exceptionFetcher.Fetch(payload) is Exception exception)
+                                if (this.exceptionFetcher.TryFetch(payload, out Exception exception) && exception != null)
                                 {
                                     activity.SetStatus(Status.Error.WithDescription(exception.Message));
                                 }

src/OpenTelemetry/Instrumentation/PropertyFetcher.cs

@@ -45,6 +45,28 @@ public PropertyFetcher(string propertyName)
         /// <returns>Property fetched.</returns>
         public T Fetch(object obj)
         {
+            if (!this.TryFetch(obj, out T value))
+            {
+                throw new ArgumentException("Supplied object was null or did not match the expected type.", nameof(obj));
+            }
+
+            return value;
+        }
+
+        /// <summary>
+        /// Try to fetch the property from the object.
+        /// </summary>
+        /// <param name="obj">Object to be fetched.</param>
+        /// <param name="value">Fetched value.</param>
+        /// <returns><see langword="true"/> if the property was fetched.</returns>
+        public bool TryFetch(object obj, out T value)
+        {
+            if (obj == null)
+            {
+                value = default;
+                return false;
+            }
+
             if (this.innerFetcher == null)
             {
                 var type = obj.GetType().GetTypeInfo();
@@ -57,7 +79,7 @@ public T Fetch(object obj)
                 this.innerFetcher = PropertyFetch.FetcherForProperty(property);
             }
 
-            return this.innerFetcher.Fetch(obj);
+            return this.innerFetcher.TryFetch(obj, out value);
         }
 
         // see https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs
@@ -81,9 +103,10 @@ public static PropertyFetch FetcherForProperty(PropertyInfo propertyInfo)
                 return (PropertyFetch)Activator.CreateInstance(instantiatedTypedPropertyFetcher, propertyInfo);
             }
 
-            public virtual T Fetch(object obj)
+            public virtual bool TryFetch(object obj, out T value)
             {
-                return default;
+                value = default;
+                return false;
             }
 
             private class TypedPropertyFetch<TDeclaredObject, TDeclaredProperty> : PropertyFetch
@@ -96,14 +119,16 @@ public TypedPropertyFetch(PropertyInfo property)
                     this.propertyFetch = (Func<TDeclaredObject, TDeclaredProperty>)property.GetMethod.CreateDelegate(typeof(Func<TDeclaredObject, TDeclaredProperty>));
                 }
 
-                public override T Fetch(object obj)
+                public override bool TryFetch(object obj, out T value)
                 {
                     if (obj is TDeclaredObject o)
                     {
-                        return this.propertyFetch(o);
+                        value = this.propertyFetch(o);
+                        return true;
                     }
 
-                    return default;
+                    value = default;
+                    return false;
                 }
             }
         }

test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs

@@ -27,8 +27,7 @@ public void FetchValidProperty()
         {
             var activity = new Activity("test");
             var fetch = new PropertyFetcher<string>("DisplayName");
-            var result = fetch.Fetch(activity);
-
+            Assert.True(fetch.TryFetch(activity, out string result));
             Assert.Equal(activity.DisplayName, result);
         }
 
@@ -37,13 +36,20 @@ public void FetchInvalidProperty()
         {
             var activity = new Activity("test");
             var fetch = new PropertyFetcher<string>("DisplayName2");
-            var result = fetch.Fetch(activity);
+            Assert.False(fetch.TryFetch(activity, out string result));
 
             var fetchInt = new PropertyFetcher<int>("DisplayName2");
-            var resultInt = fetchInt.Fetch(activity);
+            Assert.False(fetchInt.TryFetch(activity, out int resultInt));
 
             Assert.Equal(default, result);
             Assert.Equal(default, resultInt);
         }
+
+        [Fact]
+        public void FetchNullProperty()
+        {
+            var fetch = new PropertyFetcher<string>("null");
+            Assert.False(fetch.TryFetch(null, out _));
+        }
     }
 }