Fix System.Reflection.TargetException: Non-static method requires a target

[jorive]

Description

Attempting to access the HasValue property of a nullable type through reflection tosses when the value is null. This was found to be happening in System.Diagnostics.Tracing by the Application Insights team via telemetry data and they raised the issue in corefx (issue 32606). This fixes the issue by avoiding reflection and directly checking if the nullable object is null.

This is a port of #20350

Customer Impact

The customer impact (to fill out the template) as follows:

• ~2500 of Application Insights Resources receiving data from 3 most recent stable versions of Application Insights SDK are affected by this issue. TargetException is Top-2 and 5 out of Top-10 issues AI customers are experiencing. The impact on the customer end is exclusively monitoring-related and does not cause app crash as per our knowledge. However, customers would miss certain exceptions AI would’ve reported otherwise, thus suffers from the data trustworthiness issue, equally important in the telemetry collection space.

Regression?

No

Risk

The root cause of the Application Insights bug is but in the EventSource logging code in .NET Core. This code has the ability to serialize various datatypes including the special System.Nullable type. The EventSource code that handles this case was broken for the case that the value is null. To our knowledge this is the first time a Nullable with a null value has been passed as a data value to EventSource.

The fix is small (~7 lines of code changed), and does not affect any other cases but the serialization of a Nullable by an EventSource. Thus the risk is very low (because it was broken before, it is unlikely that other scenarios were event executing the code that was changed. In addition this code is only active when EventSource logging is turned on, and EventSource is already set up to swallow exceptions that happen during logging (in general it is better to lose logging than crash the app). This further mitigates the risk (at worst, a mistake in this fix will cause loss of logging, not application crash).

Issue

https://github.com/dotnet/corefx/issues/32606

[noahfalk]

This code looks good, but we should figure out if #33013 is related before porting (assuming shiproom prefers one combined fix vs. two separate fixes)

[noahfalk]

Above #33013 = https://github.com/dotnet/corefx/issues/33013

[jorive]

@noahfalk Please take a look at this response: https://github.com/dotnet/corefx/issues/33013#issuecomment-445087043

[noahfalk]

Just getting back from vacation. From Vance's response on the other thread it sounds like that second issue is already addressed in TraceEvent and we are good to take this fix as-is. Thanks for looking into it.

cc @vancem

[jorive]

@dotnet-bot test this please

[RussKeldorph]

@jorive Do you want this considered for servicing? If so, please fill out the template.

[RussKeldorph]

Please add Servicing-consider when the template is present.

[danmoseley]

Please wait for branch to open approx 2-3 weeks. Ping me if I don't merge then.

[jorive]

@danmosemsft It's been 19 days since this PR was approved, can this be merged now?

[danmoseley]

@mmitche are branches open yet

[mmitche]

@danmosemsft Not yet, later today.

[danmoseley]

@wtgodbe would you mind making sure all our approved PR'S in the core repos get merged, if I'm not around? Going on vac soon.

[wtgodbe]

@danmosemsft sure thing