Added stack trace to CosmosException, ResponseMessage.ErrorMessage includes full exception info. #1213
Conversation
… string. Returning only the error message makes it not possible to debug. CosmosException now stores the stack trace. This fixes the issues where the error information is stored and later thrown causing the exception to show the incorrect error location. Created new CosmosExceptionFactory. This helps produce a CosmosException from the various types.
j82w
added
bug
j82w
requested review from
bchong95,
kirankumarkolli and
kirillg
as code owners
Microsoft.Azure.Cosmos/src/Resource/CosmosExceptions/BadRequestException.cs
Show resolved
Hide resolved
…nostics/exception
There was a problem hiding this comment.
Hi Matias - let's chat about this change offline first. I have some concerns around the general approach - so would like to get consensus there before reviewing the actual changes.
My concerns:
Creating a StackTrace is an expensive operation - while I understand your desire to prevent users from shooting themselves in the foot and not exposing proper stack traces - I don't think it is reasonable to enforce a stack trace even in scenarios where exceptions are effectively used to express "normal" behavior - like 404 or 409 - even 429. In all these cases exceptions can occur rather frequently - and the stacktrace doesn't provide much value.
--> https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs and dotnet/coreclr#27004 with some additional information why the implementation of the StackTrace property might be insufficient or at least inconsistent with the base class's implementation
I also have concerns around pulling the stacktrace into the message - there are customers treating callstacks as something worth protecting (I have never fully understood why - but could see at least reasons to not "leak" callstacks across the wire for example). So changing the semantics of the CosmosException to include callstacks in the message is a problem.
Bottom-line: I would refrain from overloading semantics of Exceptions in .Net in the CosmosException. Instead we should add guidance in best practices as well as our own logging/diagnostics-integration on how to ensure proper callstacks are logged along with exceptions.
…eptionFactory.cs Co-Authored-By: Matias Quaranta <[email protected]>
…eptionFactory.cs Co-Authored-By: Matias Quaranta <[email protected]>
|
|
||
| internal static class Extensions | ||
| { | ||
| private static readonly char[] NewLineCharacters = new[] { '\r', '\n' }; | ||
|
|
||
| internal static bool IsSuccess(this HttpStatusCode httpStatusCode) |
There was a problem hiding this comment.
I just wonder if this will cause any issue when integrated internally, unless noone else created such an extension with that name
There was a problem hiding this comment.
It shouldn't since it's in the SDK namespace.
Pull Request Template
Description
The current CosmosException looses the original call stack. This causes a lot of confusion when user are inspecting the exception. This PR preserves the original call stack when it is passed in. Else it uses the default call stack logic.
Type of change
Please delete options that are not relevant.