Fix ASP.NET Core ExceptionFilter prevents recording exception

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* Fix issue where when an application has an ExceptionFilter, the exception data
+  wouldn't be collected.
+  ([#3475](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3475))
+
 ## 1.0.0-rc9.6
 
 Released 2022-Aug-18

src/OpenTelemetry/DiagnosticSourceInstrumentation/PropertyFetcher.cs

@@ -76,6 +76,12 @@ public bool TryFetch(object obj, out T value, bool skipObjNullCheck = false)
                 this.innerFetcher = PropertyFetch.Create(obj.GetType().GetTypeInfo(), this.propertyName);
             }
 
+            if (this.innerFetcher == null)
+            {
+                value = default;
+                return false;
+            }
+
             return this.innerFetcher.TryFetch(obj, out value);
         }
 
@@ -96,8 +102,8 @@ static PropertyFetch CreateFetcherForProperty(PropertyInfo propertyInfo)
                 {
                     if (propertyInfo == null || !typeof(T).IsAssignableFrom(propertyInfo.PropertyType))
                     {
-                        // returns null on any fetch.
-                        return new PropertyFetch();
+                        // returns null and wait for a valid payload to arrive.
+                        return null;
                     }
 
                     var typedPropertyFetcher = typeof(TypedPropertyFetch<,>);
@@ -137,6 +143,12 @@ public override bool TryFetch(object obj, out T value)
 
                     this.innerFetcher ??= Create(obj.GetType().GetTypeInfo(), this.propertyName);
 
+                    if (this.innerFetcher == null)
+                    {
+                        value = default;
+                        return false;
+                    }
+
                     return this.innerFetcher.TryFetch(obj, out value);
                 }
             }

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs

@@ -32,6 +32,7 @@
 using OpenTelemetry.Tests;
 using OpenTelemetry.Trace;
 using TestApp.AspNetCore;
+using TestApp.AspNetCore.Filters;
 using Xunit;
 
 namespace OpenTelemetry.Instrumentation.AspNetCore.Tests
@@ -670,6 +671,29 @@ void ConfigureTestServices(IServiceCollection services)
         }
 #endif
 
+        [Theory]
+        [InlineData(1)]
+        [InlineData(2)]
+        public async Task ShouldExportActivityWithOneOrMoreExceptionFilters(int mode)
+        {
+            var exportedItems = new List<Activity>();
+
+            // Arrange
+            using (var client = this.factory
+                .WithWebHostBuilder(builder => builder.ConfigureTestServices(
+                    (s) => this.ConfigureExceptionFilters(s, mode, ref exportedItems)))
+                .CreateClient())
+            {
+                // Act
+                var response = await client.GetAsync("/api/error");
+
+                WaitForActivityExport(exportedItems, 1);
+            }
+
+            // Assert
+            AssertException(exportedItems);
+        }
+
         public void Dispose()
         {
             this.tracerProvider?.Dispose();
@@ -722,6 +746,40 @@ private static void ActivityEnrichment(Activity activity, string method, object
             activity.SetTag("enriched", "yes");
         }
 
+        private static void AssertException(List<Activity> exportedItems)
+        {
+            Assert.Single(exportedItems);
+            var activity = exportedItems[0];
+
+            var exMessage = "something's wrong!";
+            Assert.Single(activity.Events);
+            Assert.Equal("System.Exception", activity.Events.First().Tags.FirstOrDefault(t => t.Key == SemanticConventions.AttributeExceptionType).Value);
+            Assert.Equal(exMessage, activity.Events.First().Tags.FirstOrDefault(t => t.Key == SemanticConventions.AttributeExceptionMessage).Value);
+
+            ValidateAspNetCoreActivity(activity, "/api/error");
+        }
+
+        private void ConfigureExceptionFilters(IServiceCollection services, int mode, ref List<Activity> exportedItems)
+        {
+            switch (mode)
+            {
+                case 1:
+                    services.AddMvc(x => x.Filters.Add<ExceptionFilter1>());
+                    break;
+                case 2:
+                    services.AddMvc(x => x.Filters.Add<ExceptionFilter1>());
+                    services.AddMvc(x => x.Filters.Add<ExceptionFilter2>());
+                    break;
+                default:
+                    break;
+            }
+
+            this.tracerProvider = Sdk.CreateTracerProviderBuilder()
+                .AddAspNetCoreInstrumentation(x => x.RecordException = true)
+                .AddInMemoryExporter(exportedItems)
+                .Build();
+        }
+
         private class ExtractOnlyPropagator : TextMapPropagator
         {
             private readonly ActivityContext activityContext;

test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs

@@ -67,6 +67,17 @@ public void FetchPropertyMultiplePayloadTypes()
             Assert.False(fetch.TryFetch(null, out _));
         }
 
+        [Fact]
+        public void FetchPropertyMultiplePayloadTypes_IgnoreTypesWithoutExpectedPropertyName()
+        {
+            var fetch = new PropertyFetcher<string>("Property");
+
+            Assert.False(fetch.TryFetch(new PayloadTypeC(), out _));
+
+            Assert.True(fetch.TryFetch(new PayloadTypeA(), out string propertyValue));
+            Assert.Equal("A", propertyValue);
+        }
+
         private class PayloadTypeA
         {
             public string Property { get; set; } = "A";

test/TestApp.AspNetCore.3.1/Controllers/ErrorController.cs

@@ -0,0 +1,31 @@
+// <copyright file="ErrorController.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;
+using Microsoft.AspNetCore.Mvc;
+
+namespace TestApp.AspNetCore._3._1.Controllers
+{
+    [Route("api/[controller]")]
+    public class ErrorController : Controller
+    {
+        // GET api/error
+        [HttpGet]
+        public string Get()
+        {
+            throw new Exception("something's wrong!");
+        }
+    }
+}

test/TestApp.AspNetCore.3.1/Filters/ExceptionFilter1.cs

@@ -0,0 +1,29 @@
+// <copyright file="ExceptionFilter1.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 Microsoft.AspNetCore.Mvc.Filters;
+
+namespace TestApp.AspNetCore._3._1.Filters
+{
+    public class ExceptionFilter1 : IExceptionFilter
+    {
+        public void OnException(ExceptionContext context)
+        {
+            // test if the changes that were made to fix the behaviour where
+            // an application with a defined ExceptionFilter wouldn't collect those
+        }
+    }
+}

test/TestApp.AspNetCore.3.1/Filters/ExceptionFilter2.cs

@@ -0,0 +1,28 @@
+// <copyright file="ExceptionFilter2.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 Microsoft.AspNetCore.Mvc.Filters;
+
+namespace TestApp.AspNetCore._3._1.Filters
+{
+    public class ExceptionFilter2 : IExceptionFilter
+    {
+        public void OnException(ExceptionContext context)
+        {
+            // test the behaviour when an application has two ExceptionFilters defined
+        }
+    }
+}

test/TestApp.AspNetCore.6.0/Controllers/ErrorController.cs

@@ -0,0 +1,31 @@
+// <copyright file="ErrorController.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;
+using Microsoft.AspNetCore.Mvc;
+
+namespace TestApp.AspNetCore._6._0.Controllers
+{
+    [Route("api/[controller]")]
+    public class ErrorController : Controller
+    {
+        // GET api/error
+        [HttpGet]
+        public string Get()
+        {
+            throw new Exception("something's wrong!");
+        }
+    }
+}

test/TestApp.AspNetCore.6.0/Filters/ExceptionFilter1.cs

@@ -0,0 +1,29 @@
+// <copyright file="ExceptionFilter1.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 Microsoft.AspNetCore.Mvc.Filters;
+
+namespace TestApp.AspNetCore._6._0.Filters
+{
+    public class ExceptionFilter1 : IExceptionFilter
+    {
+        public void OnException(ExceptionContext context)
+        {
+            // test if the changes that were made to fix the behaviour where
+            // an application with a defined ExceptionFilter wouldn't collect those
+        }
+    }
+}

test/TestApp.AspNetCore.6.0/Filters/ExceptionFilter2.cs

@@ -0,0 +1,28 @@
+// <copyright file="ExceptionFilter2.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 Microsoft.AspNetCore.Mvc.Filters;
+
+namespace TestApp.AspNetCore._6._0.Filters
+{
+    public class ExceptionFilter2 : IExceptionFilter
+    {
+        public void OnException(ExceptionContext context)
+        {
+            // test the behaviour when an application has two ExceptionFilters defined
+        }
+    }
+}

test/TestApp.AspNetCore/Controllers/ErrorController.cs

@@ -0,0 +1,30 @@
+// <copyright file="ErrorController.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 Microsoft.AspNetCore.Mvc;
+
+namespace TestApp.AspNetCore.Controllers
+{
+    [Route("api/[controller]")]
+    public class ErrorController : Controller
+    {
+        // GET api/error
+        [HttpGet]
+        public string Get()
+        {
+            throw new Exception("something's wrong!");
+        }
+    }
+}

test/TestApp.AspNetCore/Filters/ExceptionFilter1.cs

@@ -0,0 +1,28 @@
+// <copyright file="ExceptionFilter1.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 Microsoft.AspNetCore.Mvc.Filters;
+
+namespace TestApp.AspNetCore.Filters
+{
+    public class ExceptionFilter1 : IExceptionFilter
+    {
+        public void OnException(ExceptionContext context)
+        {
+            // test the behaviour when an application has two ExceptionFilters defined
+        }
+    }
+}