These are chat archives for exceptionless/Discuss

28th
Sep 2015
Blake Niemyjski
@niemyjski
Sep 28 2015 14:45 UTC
@frankebersoll thanks, I’ll take a look at it today
Sander Rijken
@srijken
Sep 28 2015 17:58 UTC
@niemyjski I'm happy to look into this some more exceptionless/Exceptionless.Net#15, but I think we need some discussion before that
Blake Niemyjski
@niemyjski
Sep 28 2015 18:34 UTC
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
Sander Rijken
@srijken
Sep 28 2015 19:19 UTC
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
Sander Rijken
@srijken
Sep 28 2015 19:25 UTC
otherwise you have to handle it in the UI, and have a way to navigate all the separate inner exceptions or something
Blake Niemyjski
@niemyjski
Sep 28 2015 19:54 UTC
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
Sander Rijken
@srijken
Sep 28 2015 20:13 UTC
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
Blake Niemyjski
@niemyjski
Sep 28 2015 20:16 UTC
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.
Sander Rijken
@srijken
Sep 28 2015 20:18 UTC
yeah that's a difficult to handle situation
Blake Niemyjski
@niemyjski
Sep 28 2015 20:18 UTC
well you could make a copy of the contexts but yeah
Sander Rijken
@srijken
Sep 28 2015 20:19 UTC
I think if you Parallel.ForEach something and all cases fail they're all related, for instance
Blake Niemyjski
@niemyjski
Sep 28 2015 20:19 UTC
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
Sander Rijken
@srijken
Sep 28 2015 20:22 UTC
yeah :/
well the other option is to handle it at the server
so smarter UI on the existing data or something like that
Blake Niemyjski
@niemyjski
Sep 28 2015 20:23 UTC
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
Sander Rijken
@srijken
Sep 28 2015 20:24 UTC
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
Eric J. Smith
@ejsmith
Sep 28 2015 20:29 UTC
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?
Sander Rijken
@srijken
Sep 28 2015 20:31 UTC
it's about exceptionless/Exceptionless.Net#15
Eric J. Smith
@ejsmith
Sep 28 2015 20:57 UTC
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.
Sander Rijken
@srijken
Sep 28 2015 21:08 UTC
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
Blake Niemyjski
@niemyjski
Sep 28 2015 21:26 UTC
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
Eric J. Smith
@ejsmith
Sep 28 2015 21:26 UTC
yeah, we are losing data for sure.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:27 UTC
So what should happen here, introducing multiple inner exceptions on errro object will beak everything
break*
Eric J. Smith
@ejsmith
Sep 28 2015 21:27 UTC
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.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:27 UTC
yeah that’s we were kind of saying
so what should happen on those
Eric J. Smith
@ejsmith
Sep 28 2015 21:28 UTC
so I guess any time where we have multiple inner exceptions at the top level maybe we would automatically split them up.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:28 UTC
should we have a plugin that does it?
but can’t really do that
Eric J. Smith
@ejsmith
Sep 28 2015 21:28 UTC
yeah, it will be interesting.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:28 UTC
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..
Eric J. Smith
@ejsmith
Sep 28 2015 21:30 UTC
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.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:30 UTC
esp with all our async work
yeah
kinda high on the priorty list
Eric J. Smith
@ejsmith
Sep 28 2015 21:30 UTC
what is?
Blake Niemyjski
@niemyjski
Sep 28 2015 21:31 UTC
aggregate exceptions are much more likely now.
no?
Eric J. Smith
@ejsmith
Sep 28 2015 21:31 UTC
yeah, probably.
Sander Rijken
@srijken
Sep 28 2015 21:32 UTC
not really Task based stuff without async/await leads to AggregateExceptions
async/await fixes this.. afaik
Blake Niemyjski
@niemyjski
Sep 28 2015 21:32 UTC
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
Sander Rijken
@srijken
Sep 28 2015 21:33 UTC
right
Blake Niemyjski
@niemyjski
Sep 28 2015 21:34 UTC
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*
Sander Rijken
@srijken
Sep 28 2015 21:39 UTC
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
Eric J. Smith
@ejsmith
Sep 28 2015 21:41 UTC
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.
Sander Rijken
@srijken
Sep 28 2015 21:44 UTC
if you wrap it things get really interesting
Eric J. Smith
@ejsmith
Sep 28 2015 21:44 UTC
yeah, which is why I think we shouldn’t mess with that.
Sander Rijken
@srijken
Sep 28 2015 21:44 UTC
most likely if you wrap it, there was a good reason to do that
but also in that case no information should leak
Eric J. Smith
@ejsmith
Sep 28 2015 21:44 UTC
yep
so 1st step… stop losing data.
:-)
Sander Rijken
@srijken
Sep 28 2015 21:45 UTC
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 :)
Eric J. Smith
@ejsmith
Sep 28 2015 21:46 UTC
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. :-(
Blake Niemyjski
@niemyjski
Sep 28 2015 21:50 UTC
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
Eric J. Smith
@ejsmith
Sep 28 2015 21:51 UTC
we would need something that moved single inner properties to be added to the inner array.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:51 UTC
what do you mean?
Eric J. Smith
@ejsmith
Sep 28 2015 21:52 UTC
errors will be posted to the service with a singular inner property.
and the model will be an array
Blake Niemyjski
@niemyjski
Sep 28 2015 21:52 UTC
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
Eric J. Smith
@ejsmith
Sep 28 2015 21:53 UTC
pretty sure java has aggregate exceptions as well.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:53 UTC
ok so heres a con
Blake Niemyjski
@niemyjski
Sep 28 2015 21:54 UTC
the more clients we get the more pipeline logic we would introduce to do this split server side.. vs doing it client side
Eric J. Smith
@ejsmith
Sep 28 2015 21:54 UTC
yeah, I just don’t like the idea of having logic on the client side.
we went down that path before and it sucked.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:55 UTC
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
Eric J. Smith
@ejsmith
Sep 28 2015 21:55 UTC
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?
Blake Niemyjski
@niemyjski
Sep 28 2015 21:56 UTC
but we’d lose on inner wrapped aggregate exceptions but who the hell does that
yeah
and cancels the current aggreate exception..
Eric J. Smith
@ejsmith
Sep 28 2015 21:56 UTC
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.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:56 UTC
  1. copy the context data and set the exception to one of the aggregates exceptions and let it queue..
yeah
yeah I agree
Eric J. Smith
@ejsmith
Sep 28 2015 21:57 UTC
would certainly be a lot easier all around.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:57 UTC
yeah
Eric J. Smith
@ejsmith
Sep 28 2015 21:57 UTC
especially if .net is the only platform that does this.
I thought java did, but it doesn’t look like it does.
Blake Niemyjski
@niemyjski
Sep 28 2015 21:58 UTC
I need to work late every night this week
we gotta get 4.0 out
Eric J. Smith
@ejsmith
Sep 28 2015 21:58 UTC
I am actually suprised that the multiple inner exceptions don’t go into a custom data property on the error model?
Blake Niemyjski
@niemyjski
Sep 28 2015 21:58 UTC
yeah you think it should
Eric J. Smith
@ejsmith
Sep 28 2015 22:00 UTC
The property is called innerexceptions and not innerexception
I would think it would get added to data
Are we sure it doesn't?
Blake Niemyjski
@niemyjski
Sep 28 2015 22:02 UTC
yeah, we have tests and I’ve never seen it added to be honest
he created that test that fails
Sander Rijken
@srijken
Sep 28 2015 22:07 UTC
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
Blake Niemyjski
@niemyjski
Sep 28 2015 22:10 UTC
yeah
if you want I can pair with you on this
Sander Rijken
@srijken
Sep 28 2015 22:10 UTC
to fix that problem you just need a plugin that calls Flatten()
Blake Niemyjski
@niemyjski
Sep 28 2015 22:10 UTC
or you can take a stab and we can review it
Sander Rijken
@srijken
Sep 28 2015 22:11 UTC
I can work on the testcases and check if InnerExceptions (plural) gets to the data property
and then decide what to do from there
Blake Niemyjski
@niemyjski
Sep 28 2015 22:11 UTC
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
Sander Rijken
@srijken
Sep 28 2015 22:12 UTC
hm?
Blake Niemyjski
@niemyjski
Sep 28 2015 22:13 UTC
if you call flatten and it’s one exception on the inner does the aggreate get dropped and you just have the flatten?
Sander Rijken
@srijken
Sep 28 2015 22:13 UTC
right
Blake Niemyjski
@niemyjski
Sep 28 2015 22:13 UTC
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
Sander Rijken
@srijken
Sep 28 2015 22:15 UTC
the normal .Flatten() still keeps it as AggregateException, but collapse nested AggregateExceptions
Blake Niemyjski
@niemyjski
Sep 28 2015 22:16 UTC
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
Sander Rijken
@srijken
Sep 28 2015 22:20 UTC
sure it doesn't exist?
Blake Niemyjski
@niemyjski
Sep 28 2015 22:20 UTC
not 100% sure but I think it doesn't
Sander Rijken
@srijken
Sep 28 2015 22:20 UTC
yeah it's there
Blake Niemyjski
@niemyjski
Sep 28 2015 22:21 UTC
remember trying to mess with the type when you first reported it
hmm sweet :)
I was wrong
like usual :D
Sander Rijken
@srijken
Sep 28 2015 22:21 UTC
well it kinda has to be there, because in PCL a lot more is async only?
Blake Niemyjski
@niemyjski
Sep 28 2015 22:23 UTC
ah yeah, idk for some reason I thought it wasn’t there