diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs index a60e5bab6c6..30a95fa938a 100644 --- a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs +++ b/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> HttpRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name); - private readonly PropertyFetcher routeFetcher = new PropertyFetcher("Route"); - private readonly PropertyFetcher routeTemplateFetcher = new PropertyFetcher("RouteTemplate"); + private readonly PropertyFetcher routeFetcher = new PropertyFetcher("Route"); + private readonly PropertyFetcher routeTemplateFetcher = new PropertyFetcher("RouteTemplate"); private readonly AspNetInstrumentationOptions options; private readonly ActivitySourceAdapter activitySource; diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 0df0da839fe..e4665e1bac0 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/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> 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 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 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)) { diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs index c3408d3ddd5..c8ab24e4ee4 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs +++ b/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 startRequestFetcher = new PropertyFetcher("Request"); public GrpcClientDiagnosticListener(ActivitySourceAdapter activitySource, GrpcClientInstrumentationOptions options) : base("Grpc.Net.Client") diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index 2b1b6376675..d43c9e0e01c 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/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. // + 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 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 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); } } diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs index b95081a6ee5..10fbd1bcfa7 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs +++ b/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 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 SqlClientInstrumentationOptions options; public SqlClientDiagnosticListener(string sourceName, SqlClientInstrumentationOptions options) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 87e1c8f0268..1dea61ffe7b 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/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` + ([#1238](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1238)) ## 0.5.0-beta.2 diff --git a/src/OpenTelemetry/Instrumentation/PropertyFetcher.cs b/src/OpenTelemetry/Instrumentation/PropertyFetcher.cs index b31cb3d00d5..19cdf50b418 100644 --- a/src/OpenTelemetry/Instrumentation/PropertyFetcher.cs +++ b/src/OpenTelemetry/Instrumentation/PropertyFetcher.cs @@ -23,13 +23,14 @@ namespace OpenTelemetry.Instrumentation /// /// PropertyFetcher fetches a property from an object. /// - public class PropertyFetcher + /// The type of the property being fetched. + public class PropertyFetcher { private readonly string propertyName; private PropertyFetch innerFetcher; /// - /// Initializes a new instance of the class. + /// Initializes a new instance of the class. /// /// Property name to fetch. public PropertyFetcher(string propertyName) @@ -42,7 +43,7 @@ public PropertyFetcher(string propertyName) /// /// Object to be fetched. /// Property fetched. - 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 /// 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); } - /// - /// Given an object, fetch the property that this propertyFetch represents. - /// - public virtual object Fetch(object obj) + public virtual T Fetch(object obj) { - return null; + return default; } - private class TypedFetchProperty : PropertyFetch + private class TypedPropertyFetch : PropertyFetch + where TDeclaredProperty : T { - private readonly Func propertyFetch; + private readonly Func propertyFetch; - public TypedFetchProperty(PropertyInfo property) + public TypedPropertyFetch(PropertyInfo property) { - this.propertyFetch = (Func)property.GetMethod.CreateDelegate(typeof(Func)); + this.propertyFetch = (Func)property.GetMethod.CreateDelegate(typeof(Func)); } - 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; } } } diff --git a/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs b/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs new file mode 100644 index 00000000000..92f10fe4c34 --- /dev/null +++ b/test/OpenTelemetry.Tests/Instrumentation/PropertyFetcherTest.cs @@ -0,0 +1,49 @@ +// +// 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 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("DisplayName"); + var result = fetch.Fetch(activity); + + Assert.Equal(activity.DisplayName, result); + } + + [Fact] + public void FetchInvalidProperty() + { + var activity = new Activity("test"); + var fetch = new PropertyFetcher("DisplayName2"); + var result = fetch.Fetch(activity); + + var fetchInt = new PropertyFetcher("DisplayName2"); + var resultInt = fetchInt.Fetch(activity); + + Assert.Equal(default, result); + Assert.Equal(default, resultInt); + } + } +}