@frankebersoll thanks, I’ll take a look at it today
@niemyjski I'm happy to look into this some more exceptionless/Exceptionless.Net#15, but I think we need some discussion before that
yeah
that would be a huge help
yeah we should have a discussion on that
I’m not sure what needs tobe done
cause if it’s multiple exceptions should they be split out into two?
currently we have no concept of that
indeed, I don't know what should be done when there's 2 inner exceptions
maybe the best situation is if you end up with 2 separate events?
they are related, but can be totally different
if you want to end up with 2 separate events, it's easy enough to fix it in the .Net client code
otherwise you have to handle it in the UI, and have a way to navigate all the separate inner exceptions or something
yeah
but to me if you did toExceptionless() on an aggregate exception you wouldn’t be expecting two events. but maybe it should be
@ejsmith what do you think
or maybe split the event after you call .Submit()?
so that all properties/tags/etc that are set are set on both/all the events
and if there's just one inner exception, there's no problem
and if there are 10 inside that are the same, it'll be handled with the dupe checker and stacking at the server side
yeah
that might work pretty good. is there ever a case where they would all be related
The problem is the ToExceptionless() returns a builder ad you work with that one exception and now it just transformed into multiple contexts.
yeah that's a difficult to handle situation
well you could make a copy of the contexts but yeah
I think if you Parallel.ForEach something and all cases fail they're all related, for instance
hmm
and how are you supposed to know that
pray our duplicate checker is workin
but even then you’re not getting a representation of how many are actually failing
yeah :/
well the other option is to handle it at the server
so smarter UI on the existing data or something like that
yeah, guess we could do that.
then the issue arises what if you made your client submit 50 exceptions under an aggregate event on purpose. would that count as one event or 50
lol
maybe even show the tree of exception or something, and select the one whose details you want to see
and there's that yes
if you separate them at the client you don't have that problem lol
We want the clients to be dumb. So if we are going to do something then it should probably be on the server side. Are you seeing an aggregate exception and you want both of them reported?
it's about exceptionless/Exceptionless.Net#15
tests that reproduce the problem: https://github.com/exceptionless/Exceptionless.Net/pull/32/files
sorry, had some stuff going on… reading now.
I guess we can chance the errormodel to allow multiple inner exceptions… then we can use whatever logic that we want on the server side to handle it.
it's probably 2 problems into one. 1) what happens if you have an aggregate exception, that contains an aggregate exceptoin, and the real thing is under that. 2) what happens with multiple inner exceptions
so at some point the exceptions need to be flattened, but even then you can end up with a list of them
yeah I think there is a biggeri ssue we are totally not even looking at all the inner exceptions, just the first one returned by inner exception @ejsmith
yeah, we are losing data for sure.
So what should happen here, introducing multiple inner exceptions on errro object will beak everything
break*
inner exceptions are ok and don’t need to be turned into multiple events, but aggregate exceptions really are multiple exceptions that should be split up.
yeah that’s we were kind of saying
so what should happen on those
so I guess any time where we have multiple inner exceptions at the top level maybe we would automatically split them up.
should we have a plugin that does it?
but can’t really do that
yeah, it will be interesting.
guess a plugin could but it leads to a weird behavior lol
it could copy the contexts and call apply on the exception, guess we would ditch the aggregate exception totally and just use root exceptiones..
yeah, it is interesting for sure.
I think the 1st step is to just stop losing data.
then we can figure out if we want to break them up.
esp with all our async work
yeah
kinda high on the priorty list
what is?
aggregate exceptions are much more likely now.
no?
yeah, probably.
not really Task based stuff without async/await leads to AggregateExceptions
async/await fixes this.. afaik
it’s almost like we should have a plugin and when it detects an aggregate excpetion we go over each inner exception and submit it with the a copy of the context and set the error on each context and then cancel the current context so we get all inners and not the aggregate exception
@srijken but we aren’t always uisng async and await.. only if we have to await something
right
But the shitty thing about doing the plugin route.. is your client is doing all this work in the ToError() extension method to calculate this crap
so you’d do the hit twice
take the hit*
but suppose the leaking is stopped. and you have an AggregateException with 2 different internal exception. I think it would make most sense to have that result in 2 stacks
yeah, agreed. I think we need to change the model to allow multiple inner exceptions so that we stop losing data and then eventually have a piece server side that will split any error up that has multiple inner exceptions at the top level.
Ideally, the error model would only allow for multiple inner exceptions at the top level.
I guess that isn’t true though, eh?
I can catch an aggregateexception and then wrap it...
but if I did that… then I think at that point it is a single error because you are rethrowing it and wrapping the error.
if you wrap it things get really interesting
yeah, which is why I think we shouldn’t mess with that.
most likely if you wrap it, there was a good reason to do that
but also in that case no information should leak
yep
so 1st step… stop losing data.
:-)
so you have a chance to figure out that the thing you did on purpose doesn't handle AggregateException like it should, and then you can change that :)
yeah, which is why we want the clients to be dumb so that we can change the logic if we need to later without getting everyone to update their clients.
in this case we messed up the dumb part and need everyone to update. :-(
we’ll we can change the model
but this would be major version number change
we have the document upgrader that runs that could convert existing ones to an enumerable
but still doesn’t stop data loss. on all existing clients
makes our pipeline more complicated too but that’s step 2
lol
we would need something that moved single inner properties to be added to the inner array.
what do you mean?
errors will be posted to the service with a singular inner property.
and the model will be an array
the only crappy thing.. is there anyt other platforms out there that would have aggregate exceptions like this.
guess it’s good to be open ended..
hmm yeah but we can conver that with our json upgrader
pretty sure java has aggregate exceptions as well.
ok so heres a con
the more clients we get the more pipeline logic we would introduce to do this split server side.. vs doing it client side
yeah, I just don’t like the idea of having logic on the client side.
we went down that path before and it sucked.
yeah me either but it almost makes sense
otherwise every client implementation becomes a bit more complicated
I think we could do this with a plugin pretty dang easy
with just a little overhead
guess it would just be add a plugin to the pipeline that looks for aggregate exception and unwraps it and then submits the individual ones?
but we’d lose on inner wrapped aggregate exceptions but who the hell does that
yeah
and cancels the current aggreate exception..
again, I think it should only happen at the top level.
we can’t be messing around in the middle of a heirarchy of errors.
- copy the context data and set the exception to one of the aggregates exceptions and let it queue..
yeah
yeah I agree
would certainly be a lot easier all around.
yeah
especially if .net is the only platform that does this.
I thought java did, but it doesn’t look like it does.
I need to work late every night this week
we gotta get 4.0 out
I am actually suprised that the multiple inner exceptions don’t go into a custom data property on the error model?
yeah you think it should
The property is called innerexceptions and not innerexception
I would think it would get added to data
Are we sure it doesn't?
yeah, we have tests and I’ve never seen it added to be honest
he created that test that fails
let me check the test
the testcase doesn't inspect data right
and it was a what-if situation iirc
I think the main reason for the testcases was AggregateException -> AggregateException -> RealException, with RealException getting lost
yeah
if you want I can pair with you on this
to fix that problem you just need a plugin that calls Flatten()
or you can take a stab and we can review it
I can work on the testcases and check if InnerExceptions (plural) gets to the data property
and then decide what to do from there
ok
would be nice to add a check to our ToErrorModel to call flatten for us
so is this the behavior that happens
aggregate -> 1 error -> .flatten == 1 error
aggreate -> 2 errors -> .flatten == aggreate -> 2 errors
hm?
if you call flatten and it’s one exception on the inner does the aggreate get dropped and you just have the flatten?
right
if you call flatten and theres two inner.. does it just return the aggregate?
if that’s the case then we should call flatten on the ToErrorModel extension method as it might cut down on a lot of use cases and then only when there is two inner ones would we have to do something with it :)
would be easy to test this too :)
ahh i feel so fat lol
the normal .Flatten() still keeps it as AggregateException, but collapse nested AggregateExceptions
hmm
just thought of something
our pcl client would lose info
don’t think the AggregateException type exists in pcl, but can be thrown..
but I’m fine with that
yeah, so we should be calling that all the time
sure it doesn't exist?
not 100% sure but I think it doesn't
yeah it's there
remember trying to mess with the type when you first reported it
hmm sweet :)
I was wrong
like usual :D
well it kinda has to be there, because in PCL a lot more is async only?
ah yeah, idk for some reason I thought it wasn’t there