The PropertyFetchers are not working with ASP.NET Core 3.0

[markvincze]

The property fetchers in this section of the code (https://github.com/opentracing-contrib/csharp-netcore/blob/master/src/OpenTracing.Contrib.NetCore/AspNetCore/MvcEventProcessor.cs#L23-L26) don't work with ASP.NET Core 3.0, the Fetch() calls return null.

I'm not 100% sure of the reason, but probably it's that event argument types have changed in ASP.NET Core. For example in the BeforeAction event the arg is of this type, in which the properties have upper case names (HttpContext, ActionDescriptor), but in the PropertyFetchers we're trying to use lower case names (httpContext, actionDescriptor).
If I change the strings there to start with upper case, then it starts working.

What should be the fix for this? I guess if I change the strings to uppercase, that fixes it for 3.0, but breaks it for 2.2. Would it be possible to make the PropertyFetcher ignore the case of the property name?

[markvincze]

I looked into this in a bit more detail, and I see two different possible fixes:

Approach 1: Change the PropertyFetcher to store not just one string _propertyName, but multiple string[] _propertyNames, and pass them in through the ctor with public PropertyFetcher(params string[] propertyName).
Then in Fetch try them one by one until we either get a non-null PropertyInfo, or we run out of property names.
This way we could explicitly specify all the "known" property names we expect, so for example in MvcEventProcessor we can change the line

        private static readonly PropertyFetcher _beforeAction_httpContextFetcher = new PropertyFetcher("httpContext");

to

        private static readonly PropertyFetcher _beforeAction_httpContextFetcher = new PropertyFetcher("httpContext", "HttpContext");

This solution has the advantage that we can handle if at some point we want to specify a fallback with a completely different name, where not just the casing differs.

Approach 2: The other approach I can think of is to make the retrieval of the PropertyInfo in Fetch to be case insensitive. I don't see a built-in way to do this with TypeInfo, but we can look up the property manually like this:

        public object Fetch(object obj)
        {
            Type objType = obj.GetType();
            if (objType != _expectedType)
            {
                TypeInfo typeInfo = objType.GetTypeInfo();
                var propertyInfo = typeInfo.DeclaredProperties.FirstOrDefault(p => string.Equals(p.Name, _propertyName, StringComparison.InvariantCultureIgnoreCase));
                _fetchForExpectedType = PropertyFetch.FetcherForProperty(propertyInfo);
                _expectedType = objType;
            }
            return _fetchForExpectedType.Fetch(obj);
        }

---

To me both solutions look viable, I'm not sure which one is better. Probably performance is not a problem, since the retrieval of the PropertInfo only runs once anyway.

Does one of these solutions look ok?

[cwe1ss]

Hi. Yes, ASP.NET Core 3 introduces breaking changes for diagnostics. Seems like they moved from anonymous types with camel-case properties to actual types with pascal-case properties. This also breaks the Application Insights tracer ( microsoft/ApplicationInsights-aspnetcore#957 ).

Fixing this would have to happen in a backwards-compatible way, yes.

As said in a different ticket, I'm not using this project myself anymore and I don't think this project has any future, given that OpenTelemetry will have built-in diagnostics for ASP.NET Core ( https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/src/OpenTelemetry.Collector.AspNetCore ), so let's have a talk about the general next steps in #45 first.

[pcwiese]

I like approach #2. The change is localized and should be backwards compatible.

[austinlparker]

I'll +1 the second option as well. Given that this library has current users and we're a few months out from OTel being GA, would you be interested in submitting a PR for this @markvincze ?

[markvincze]

Hey all,

Thanks for the info, @cwe1ss, I didn't know about the OpenTelemetry.Collector.AspNetCore project, I'll check it out.

@pcwiese @austinlparker I actually implemented the fix already, and published the patched version on our internal Nuget, and has been using it since with ASP.NET Core 3.0, and so far seems to be working nicely.
I'll issue a PR shortly.

[austinlparker]

A fix for this was released in v0.6.1. Thank you everyone!

[tillig]

Just to verify - I know NuGet takes some time to index things, but it doesn't appear 0.6.1 has made it out to NuGet though it's in the list of releases here. Is there a button somewhere that needs to be pushed?

[austinlparker]

Hm, looks like there's a deploy step on AppVeyor and I'm unable to access it. I'll email some folks and figure it out.

[austinlparker]

Just published it to nuget a few minutes ago, so it should show up within the hour.

[rwkarg]

Can confirm that it's available and that the fix appears to be working.