Fixing error title so it reflects that of the exception being thrown

[cvallance]

Fixed the unhandled error title to be that of the exception that is being thrown. Somewhat related to this issue:
#346

Also

• I think I fixed a bug where if you set maxItems the exception handler would never send the message
• Removed RollbarMiddlewareException because there was no need for it potentially may have lost stacktrace by throwing a new error

[cvallance]

Sorry @akornich I didn't assign you at the start and now it appears I can't edit it to assign you, most likely pebkac... but anyway, if you could take a look at this, that would be great.

[cvallance]

@akornich Sorry, just making sure this on your radar... let me know if it's not appropriate and/or it needs tweaking.

[akornich]

@cvallance , thanks for submitting the PR. We are having an internal discussion regarding the validity of the change from the system point of view. At the moment it looks like we can not take it as is since it will be causing a loss of some valuable context for many (if not most) of our users. The discussion now is mainly around having your change as optional/configurable only when preferred by a specific user.

[akornich]

since we can not accept this PR as is - rejecting/closing it for now. we may include similar behavior offered by this PR but as a configurable option only.

[cvallance]

Sorry, just reviewing this 1 year later as we look to potentially move our dotnet stuff to join our python stuff in rollbar or vice versa.

@akornich Can you please elaborate on what you mean by "causing a loss of some valuable context fo many (if not most) of our users"...?

[akornich]

The Rollbar middleware component is intended to guard the "wrapped" part of the HTTP request processing middleware stack for uncaught (by that stack) exceptions. By wrapping any such exception within a RollbarException we clearly signify the fact that the guarded portion of the stack had a problem that was never fully handled and Rallbar middleware processed it. Also, the RollbarException wrapper provides a placeholder for us to attach any extra data about the request that caused the exception if necessary in the future. The original uncaught exception is fully preserved as the RollbarException's inner exception (that is a very common practice in .NET).

[cvallance]

@akornich Sorry, but I disagree... this approach is causing headaches with our setup and I believe it should be changed.

By wrapping any such exception within a RollbarException we clearly signify the fact that the guarded portion of the stack had a problem that was never fully handled and Rallbar middleware processed it.

Just by simply re-throwing the original you are clearly signifying the fact that that guarded portion of the stack had a problem that was never fully handled. By throwing a new exception it signifies that there was a problem inside the middleware itself.

What benefit does wrapping it actually give?

There should be no need for other middleware providers to know that another middleware had processed it or not... either they get an exception or not, simple. It's actually confusing things because the rollbar middleware is now taking overship of any exception and any other middleware needs to be aware of this RollbarMiddlewareExecption and why they are getting it. All middleware should be complete independant and not coupled at all.

And on a similar note, there should be no need to attach anything in the future... no other middleware should need to know about rollbar, the rollbar middleware should do it's thing and then let all the other middleware do their thing without knowing about rollbar at all.

...

Secondly, the handling of the exception in rollbar appears to be in an else { } where the if is:

                    if (RollbarScope.Current != null 
                        && RollbarLocator.RollbarInstance.Config.MaxItems > 0
                        )
                    {

This appears to mean that if there is a RollbarScope.Current and the config specifies MaxItems, it will never call RollbarLocator.RollbarInstance.Critical(rollbarPackage); because it's in the else ...?

That is why I removed the else block in the MR.

[akornich]

@cvallance , may I ask what sort of "headaches" does it create?

[cvallance]

@akornich Basically the reasons I mention above... all other middleware that handles exceptions is now getting a RollbarMiddlewareExecption when they expect the original exception.

[akornich]

I get that. But you can always change the order of the middleware components and have the RollbareMiddleware the last catch-all exception collector.
And in either case, you can always unwind a RollbarMiddlewareException by looking into its InnerException property.
What other middleware components do you use for exception handling (in addition to Rollbar)?

[cvallance]

But you can always change the order of the middleware components and have the RollbareMiddleware the last catch-all exception collector.

This doesn't work if I have an exception handler that I want to be the one that "handles" the exception and doesn't throw for others down the line. Like the builtin ExceptionHandlerMiddleware which is now always reporting RollbarMiddlewareException instead of the originally thrown exception.

you can always unwind a RollbarMiddlewareException by looking into its InnerException property

I guess that's my whole point... I shouldn't have to do that because I don't want Rollbar abstractions leaking into my other middleware, especially if those other pieces of middleware are 3rd party or built in libraries where I can't alter the code.

Do you understand where I'm coming from?

Ps. I don't have the code in front of me, so I can't tell you what other middleware we are using... but the issue is just what I've outlined above, shouldn't matter too much what we're using.

[akornich]

@cvallance , I see your point. We definetly will not be removing the RollbarMiddlewareException, but we can definetly add a config flag that would allow to opt-out the wrapping of the original exception. I think I should be able to add it in sometime next week.

[cvallance]

Brilliant, thanks @akornich 😄