diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index f8944085f5c..4cd25682683 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/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 diff --git a/src/OpenTelemetry/DiagnosticSourceInstrumentation/PropertyFetcher.cs b/src/OpenTelemetry/DiagnosticSourceInstrumentation/PropertyFetcher.cs index d451e9b8de9..d2ec900b051 100644 --- a/src/OpenTelemetry/DiagnosticSourceInstrumentation/PropertyFetcher.cs +++ b/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); } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index 247cc874c1a..dfa1ba09ff4 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/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(); + + // 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 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 exportedItems) + { + switch (mode) + { + case 1: + services.AddMvc(x => x.Filters.Add()); + break; + case 2: + services.AddMvc(x => x.Filters.Add()); + services.AddMvc(x => x.Filters.Add()); + break; + default: + break; + } + + this.tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddAspNetCoreInstrumentation(x => x.RecordException = true) + .AddInMemoryExporter(exportedItems) + .Build(); + } + private class ExtractOnlyPropagator : TextMapPropagator { private readonly ActivityContext activityContext; diff --git a/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs b/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs index 89714dc46e8..f49d61dfbaa 100644 --- a/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs +++ b/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("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"; diff --git a/test/TestApp.AspNetCore/Controllers/ErrorController.cs b/test/TestApp.AspNetCore/Controllers/ErrorController.cs new file mode 100644 index 00000000000..174888fec05 --- /dev/null +++ b/test/TestApp.AspNetCore/Controllers/ErrorController.cs @@ -0,0 +1,30 @@ +// +// 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. +// +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!"); + } + } +} diff --git a/test/TestApp.AspNetCore/Filters/ExceptionFilter1.cs b/test/TestApp.AspNetCore/Filters/ExceptionFilter1.cs new file mode 100644 index 00000000000..74a597a06ad --- /dev/null +++ b/test/TestApp.AspNetCore/Filters/ExceptionFilter1.cs @@ -0,0 +1,28 @@ +// +// 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. +// + +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 + } + } +} diff --git a/test/TestApp.AspNetCore/Filters/ExceptionFilter2.cs b/test/TestApp.AspNetCore/Filters/ExceptionFilter2.cs new file mode 100644 index 00000000000..96934bcb551 --- /dev/null +++ b/test/TestApp.AspNetCore/Filters/ExceptionFilter2.cs @@ -0,0 +1,28 @@ +// +// 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. +// + +using Microsoft.AspNetCore.Mvc.Filters; + +namespace TestApp.AspNetCore.Filters +{ + public class ExceptionFilter2 : IExceptionFilter + { + public void OnException(ExceptionContext context) + { + // test the behaviour when an application has two ExceptionFilters defined + } + } +}