Comments (12)
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
@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.
Related Issues (20)
- Exception thrown when trying to use Elasticsearch output HOT 8
- New input: ActivitySource HOT 12
- Request ability to use ConnectionString in ApplicationInsightsOutputConfiguration HOT 2
- EventFlow not working with .NET 5 HOT 3
- Upgrade Microsoft.ApplicationInsights dependencies HOT 1
- Duplicate dependencies in elastic search output HOT 3
- Update Nuget authoring when PR 387 is merged HOT 3
- .NET 4.6.1 implementation of Core assembly does not use precise clock
- Need to set proxy for ElasticSearch output HOT 6
- Shall we remove unused `Microsoft.Diagnostics.EventFlow.TableStorage`? HOT 1
- Update test platform matrix HOT 1
- Remove Nuget binary from sources HOT 1
- This repo is missing important files
- Does EventFlow supports for app in Azure Kubernetes Service? HOT 1
- Guidance when to use EventFlow? HOT 3
- Service Fabric Support - Load App Insights connection string from environment variable HOT 5
- Add ability to configure index format string HOT 2
- CsvHealthReporter does not output any logs HOT 1
- EventSourceInput captures System.Runtime logs as well (NET 7) HOT 12
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from diagnostics-eventflow.