In metric, Unable to cast object of type Int32 to type Double about diagnostics-eventflow HOT 12 CLOSED

Hi @ksmithRenweb,

The exception should be benign because ultimately it gets to line 165 where it calls ChangeType to convert it to a double which should work.
I understand the annoyance, do you have a suggested fix?

from diagnostics-eventflow.

@brahmnes is right. This is the way EventFlow has been doing property value conversion for years and we have not heard many complaints. That said, there might be a way to do it more efficiently and/or avoid these first-chance exceptions that you see--we are open for suggestions. For example, it might make sense to special-case int-to-double conversions as these are pretty common.

What I would suggest for debugging is to ignore first-chance InvalidCastException. Alternatively, you can put a condition on the InvalidCastException to break if it is thrown from some place else than EventFlow (in the Exception Settings window in VS right-click on the exception and choose "Edit Conditions")

Thank you for feedback @ksmithRenweb and feel free to reopen as necessary.

from diagnostics-eventflow.

@karolz-ms @brahmnes
I was on vacation for a week, only today I was able to get back to this.

It's benign in the average scenario. Where your code is working and you are not trying to track down an issue.
When you are trying to track down an issue and you see this generating error logs in your console. It stops being benign until you know what it is and start mentally ignoring it.
Or When for a fairly obscure error, you set Visual Studio to break on ALL exceptions and then you get this exception showing up 50 bajillion times while looking for the real error. Yes you can ignore the InvalidCastException but what if that is what you are looking for.

Ignoring if it is only once, works till its hit a second time during runtime. I created this issue only after dealing with it in the second reason i stated above for the 20th time in a few days. Where it wasn't just once in a while. I had to ignore it umpteen times each test cycle.

So. Just to avoid the cluttered log files, I'd think looking for a better way is important. Especially since this seems to be a very common Metric object issue. Also the fact that this is failing because the code is trying to do an explicit conversion from int to double where an implicit conversion exists!

Simplest fix: (defensive programming)
if(typeof(T) == typeof(double) && p.GetType() == typeof(int))
{
value = double.Convert(p);
converted = true;
}
// Check of converted before that generic try catch.

Potential performance hog fix: (instead of try catch. though reflection has gotten a lot faster in net core.)
if(typeof(T).GetMethod("op_explicit") != null)
{
value = (T)p;
converted = true;
}

from diagnostics-eventflow.

@ksmithRenweb fair enough, thanks for the perspective. Let's see what we can do

from diagnostics-eventflow.

The problem is, payload values are boxed, that is why the cast does not work. Normally, a cast is redundant, but perfectly fine when an implicit conversion exists:

int i = 7;
object io = i;
io.GetType() == typeof(int) // true
double d = (double) i;  // 7.0
d = (double) io;  // InvalidCastException

Solution idea: special-case well-known types, leveraging the knowledge of implicit numeric conversions and other type-specific information. A good starting set of commonly used types for special-casing might be byte, int, uint, long, ulong, float, double, string, DateTime, DateTimeOffset and Guid.

More info

• Type conversion in .NET
• Convert class documentation

@brahmnes, @yantang-msft feel free to chime in if you have any feedback

from diagnostics-eventflow.

Expanding on @ksmithRenweb 's example, maybe something like this?

Type type = p.GetType();
if (type.IsPrimitive && type == typeof(T)) {
value = (T)p;
converted = true;
}
// The rest is the same

Basically, if the type inside the box is primitive but not the same as T, the simple cast will never work.
I didn't test this, but someone can validate.

from diagnostics-eventflow.

@karolz-ms @brahmnes By changing Payload from Dictionary<string,object>() to Dictionary<string,dynamic>(). would fix it, and potentially simplify the code?
In general I stay away from dynamic if at all possible, because it has performance overhead.

The other thought is to create PayloadWrapper object with a GetHashCode based off the payloadName. Then use a HashSet(). Within the PayloadWrapper it'd handle the payload type better... It might again require a GetValue that returns a dynamic though.

A final thought is to have a set of Payload Dictionaries based on the primitive types + object. Then add logic methods to correctly insert and get values for said dictionaries. Which would remove casting/boxing issues entirely with a very small increase in memory. I'd think extra processing would be offset entirely by the reduced complexity around casting/boxing.

from diagnostics-eventflow.

@ksmithRenweb thank you for taking time to think through options.

Given that this is not a production issue, and that we did not get a lot of customer feedback about it, the highest priority for me is to maintain the performance of the current implementation and avoid any regressions. That is I am much more inclined to make a targeted fix than change how event data is stored in some fundamental way.

I am not sure if an implementation using dynamic types would be simpler, and it would almost certainly degrade performance, so I don't think it is a good option.

The per-property-type dictionary is an interesting idea, but unfortunately it won't completely solve the problem IMO. For better or worse, EventData public interface--methods like TryGetPropertyValue and AddPayloadProperty implies the values are objects. This was a deliberate decision--we wanted EventData users to be able to store any objects in them without the burden of serializing them as strings or something. The drawback is, boxing is unavoidable, and it won't be eliminated by changing how property values are stored alone--we would have to make some breaking changes to EventData public API.. On the other hand, having a dictionary per property type would make us create more dictionaries, increasing GC pressure. In high-throughput services, allocations and GC are often the leading performance limiter. So I do not think the tradeoff is good--although this is just a gut call and we would have to measure--but like I said above, I am pretty sure a targeted fix will solve the problem without decreasing the perf, so I would like to try that first.

Thanks again for sharing your thoughts, much appreciated!

from diagnostics-eventflow.

@karolz-ms Ok. I've been working on .net since 1.1. Very familiar with boxing problems. At the same time, the issue has been virtually irrelevant to me for years. I can't remember the last time I ran head on into a boxing problem like this. I think I've just become very good at writing code that doesn't run into it.

Though boxing/unboxing has a performance cost. It's much lower than it used to be. So unless there is a significant improvement to the TryGetPropertyValue code. You are right, any change is not worth pursuing.

If @brahmnes comment here can be done. It should improve the performance of the code a bit. Since it would bypass the failed casting attempt when dealing with primitives.
"Expanding on @ksmithRenweb 's example, maybe something like this?

Type type = p.GetType();
if (type.IsPrimitive && type == typeof(T)) {
value = (T)p;
converted = true;
}
// The rest is the same

Basically, if the type inside the box is primitive but not the same as T, the simple cast will never work.
I didn't test this, but someone can validate."

from diagnostics-eventflow.

Unfortunately the suggestion above does not work--one cannot cast a boxed primitive type to another primitive type even if an implicit conversion exists. For example, one cannot do (double) boxedIntVar.

I tried un-boxing before casting, but the compiler does not want to generate code for casting a primitive type into a generic parameter type, even if generic parameter type is restricted to unmanaged types. So I am going to try to provide optimized implementation(s) for a few often-used cases instead

from diagnostics-eventflow.

@karolz-ms Thank you for your continued effort.

from diagnostics-eventflow.

The Core package version 1.10.0, that contains the fix, is now available from nuget.org

from diagnostics-eventflow.