[Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Nikita Pettik]

From: Kirill Shcherbatov <kshcherbatov@tarantool.org>

Part of #1148
---
rfc in human-readable format: https://github.com/tarantool/tarantool/blob/np/gh-1148-stacked-diag/doc/rfc/1148-stacked-diagnostics.md
Previous version of rfc: https://github.com/tarantool/tarantool/commit/c2a7e1a10732fdb231780ac04d1a4bc1618c0468

Changes in this version:
- All savepoint routines have been removed as meaningless;
- Removed SQL warnings mentions since they are unrelated to the subject of rfc;
- Removed mentions about exposing box.error.new() with filename and
  line parameters (again, it is barely related to the subject of rfc);
- Described in details difference between diag_set() in diag_add()
  functions as a part of C interface;
- Other minor clean-ups.

Now RFC looks quite brief and straightforward in its implementation;
it attempts at solving only #1148 issue.

 doc/rfc/1148-stacked-diagnostics.md | 214 ++++++++++++++++++++++++++++++++++++
 1 file changed, 214 insertions(+)
 create mode 100644 doc/rfc/1148-stacked-diagnostics.md

diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
new file mode 100644
index 000000000..68fa6d21a
--- /dev/null
+++ b/doc/rfc/1148-stacked-diagnostics.md
@@ -0,0 +1,214 @@
+# Stacked Diagnostics
+
+* **Status**: In progress
+* **Start date**: 30-07-2019
+* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org,
+               Pettik Nikita korablev@tarantool.org
+* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
+
+## Background and motivation
+
+Support stacked diagnostics for Tarantool allows to accumulate all errors
+occurred during request processing. It allows to better understand what
+happened, and handle errors appropriately.
+
+### Current error diagnostics
+
+Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
+Object representing error featuring following properties:
+ - type (string) error’s C++ class;
+ - code (number) error’s number;
+ - message (string) error’s message;
+ - file (string) Tarantool source file;
+ - line (number) line number in the Tarantool source file.
+
+The last error raised is exported with `box.error.last()` function.
+
+Type of error is represented by a few C++ classes (all are inherited from
+Exception class):
+```
+ClientError
+ | LoggedError
+ | AccessDeniedError
+ | UnsupportedIndexFeature
+
+XlogError
+ | XlogGapError
+
+SystemError
+ | SocketError
+ | OutOfMemory
+ | TimedOut
+
+ChannelIsClosed
+FiberIsCancelled
+LuajitError
+IllegalParams
+CollationError
+SwimError
+CryptoError
+```
+
+All codes and names of ClientError class are available in box.error.
+User is able to create a new error instance of predefined type using
+box.error.new() function. For example:
+```
+tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just cause")
+tarantool> t:unpack()
+---
+- type: ClientError
+  code: 9
+  message: 'Failed to create space ''myspace'': just because'
+  trace:
+  - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
+    line: 1
+```
+
+User is also capable of defining own errors with any code  by means of:
+```
+box.error.new({code = user_code, reason = user_error_msg})
+```
+For instance:
+```
+e = box.error.new({code = 500, reason = 'just cause'})
+```
+
+Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()`
+methods and `.type`, `.message` and `.trace` fields.
+
+## Proposal
+
+In some cases a diagnostic information should be more complicated than
+one last raised error. Consider following example: persistent Lua function
+referenced by functional index has a bug in it's definition, Lua handler sets
+an diag message. Then functional index extractor code setups an own, more
+specialized error. Without stacked diagnostic area, only last error is
+delivired to user. One way to deal with this problem is to introduce stack
+accumulating all errors happened during request processing.
+
+### C API
+
+Let's keep existent `diag_set()` method as is. It is supposed to replace the
+last error in diagnostic area with a new one. To add new error at the top of
+existing one, let's introduce new method `diag_add()`. It is assumed to keep
+an existent error message in diagnostic area (if any) and sets it as a reason
+error for a recently-constructed error object. Note that `diag_set()` is not
+going to preserve pointer to previous error which is held in error to be
+substituted. To illustrate last point consider example:
+
+```
+0. Errors: <NULL>
+1. diag_set(code = 1)
+Errors: <e1(code = 1) -> NULL>
+2. diag_add(code = 2)
+Errors: <e1(code = 1) -> e2(code = 2) -> NULL>
+3. diag_set(code = 3)
+Errors: <e3(code = 3) -> NULL>
+```
+
+Hence, developer takes responsibility of placing `diag_set()` where the most
+basic error should be raised. For instance, if during request processing
+`diag_add()` is called before `diag_set()` then it will result in inheritance
+of all errors from previous error raise:
+
+```
+-- Processing of request #1
+1. diag_set(code = 1)
+Errors: <e1(code = 1) -> NULL>
+2. diag_add(code = 2)
+Errors: <e1(code = 1) -> e2(code = 2) -> NULL>
+-- End of execution
+
+-- Processing of request #2
+1. diag_add(code = 1)
+Errors: <e1(code = 1) -> e2(code = 2) -> e3(code = 1) -> NULL>
+-- End of execution
+```
+
+As a result, at the end of execution fo second request, three errors in
+stack are reported instead of one.
+
+Another way to resolve this issue is to erase diagnostic area before
+request processing. However, it breaks current user-visible behaviour
+since box.error.last() will preserve last occurred error only until execution
+of the next request.
+
+The diagnostic area (now) contains (nothing but) pointer to the top error:
+```
+struct diag {
+  struct error *last;
+};
+
+```
+
+To organize errors in a list let's extend error structure with pointer to
+the previous element. Or alternatively, add member of any data structure
+providing list properties (struct rlist, struct stailq or whatever):
+```
+struct diag {
+  struct stailq *errors;
+};
+
+struct error {
+   ...
+   struct stailq_entry *in_errors;
+};
+```
+
+
+### Lua API
+
+Tarantool returns a last-set (diag::last) error as `cdata` object from central
+diagnostic area to Lua in case of error. User should be unable to modify it
+(since it is considered to be a bad practice - in fact object doesn't belong
+to user). On the other hand, user needs an ability to inspect a collected
+diagnostic information. Hence, let's extend the `box.error` API with a function
+which provides the way to get the previous error (reason): `:prev()` (and
+correspondingly `.prev` field).
+
+```
+-- Return a reason error object for given error
+-- (when exists, nil otherwise).
+box.error.prev(error) == error.prev
+```
+
+Furthermore, let's extend signature of `box.error.new()` with new (optional)
+argument - the 'reason' parent error object:
+
+```
+e1 = box.error.new({code = 111, reason = "just cause"})
+e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
+```
+
+### Binary protocol
+
+Currently errors are sent as `(IPROTO_ERROR | errcode)` response with an
+string message containing error details as a payload. There are not so many
+options to extend current protocol wihtout breaking backward compatibility
+(which is obviously one of implementation requirements). One way is to extend
+existent binary protocol with a new key IPROTO_ERROR_STACK (or
+IPROTO_ERROR_REASON or simply IPROTO_ERROR_V2):
+```
+{
+        // backward compatibility
+        IPROTO_ERROR: "the most recent error message",
+        // modern error message
+        IPROTO_ERROR_STACK: {
+                {
+                        // the most recent error object
+                        IPROTO_ERROR_CODE: error_code_number,
+                        IPROTO_ERROR_REASON: error_reason_string,
+                },
+                ...
+                {
+                        // the oldest (reason) error object
+                },
+        }
+}
+```
+
+IPROTO_ERROR is always sent (as in older versions) in case of error.
+IPROTO_ERROR_STACK is presented in response only if there's at least two
+elements in diagnostic list. Map which contains error stack can be optimized
+in terms of space, so that avoid including error which is already encoded
+in IPROTO_ERROR.
-- 
2.15.1

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Nikita Pettik]

On 14 Jan 23:16, Nikita Pettik wrote:
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>

Hi guys,

Does anybody want to look at this RFC? It's quite similar to the one which
got LGTM from Konstantin, but it is even more simplified. I have to get
formal approvement to start fixing code itself. Thanks.
 
> Part of #1148
> ---
> rfc in human-readable format: https://github.com/tarantool/tarantool/blob/np/gh-1148-stacked-diag/doc/rfc/1148-stacked-diagnostics.md
> Previous version of rfc: https://github.com/tarantool/tarantool/commit/c2a7e1a10732fdb231780ac04d1a4bc1618c0468
> 
> Changes in this version:
> - All savepoint routines have been removed as meaningless;
> - Removed SQL warnings mentions since they are unrelated to the subject of rfc;
> - Removed mentions about exposing box.error.new() with filename and
>   line parameters (again, it is barely related to the subject of rfc);
> - Described in details difference between diag_set() in diag_add()
>   functions as a part of C interface;
> - Other minor clean-ups.
> 
> Now RFC looks quite brief and straightforward in its implementation;
> it attempts at solving only #1148 issue.
> 
>  doc/rfc/1148-stacked-diagnostics.md | 214 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 214 insertions(+)
>  create mode 100644 doc/rfc/1148-stacked-diagnostics.md
> 
> diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
> new file mode 100644
> index 000000000..68fa6d21a
> --- /dev/null
> +++ b/doc/rfc/1148-stacked-diagnostics.md
> @@ -0,0 +1,214 @@
> +# Stacked Diagnostics
> +
> +* **Status**: In progress
> +* **Start date**: 30-07-2019
> +* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org,
> +               Pettik Nikita korablev@tarantool.org
> +* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
> +
> +## Background and motivation
> +
> +Support stacked diagnostics for Tarantool allows to accumulate all errors
> +occurred during request processing. It allows to better understand what
> +happened, and handle errors appropriately.
> +
> +### Current error diagnostics
> +
> +Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
> +Object representing error featuring following properties:
> + - type (string) error’s C++ class;
> + - code (number) error’s number;
> + - message (string) error’s message;
> + - file (string) Tarantool source file;
> + - line (number) line number in the Tarantool source file.
> +
> +The last error raised is exported with `box.error.last()` function.
> +
> +Type of error is represented by a few C++ classes (all are inherited from
> +Exception class):
> +```
> +ClientError
> + | LoggedError
> + | AccessDeniedError
> + | UnsupportedIndexFeature
> +
> +XlogError
> + | XlogGapError
> +
> +SystemError
> + | SocketError
> + | OutOfMemory
> + | TimedOut
> +
> +ChannelIsClosed
> +FiberIsCancelled
> +LuajitError
> +IllegalParams
> +CollationError
> +SwimError
> +CryptoError
> +```
> +
> +All codes and names of ClientError class are available in box.error.
> +User is able to create a new error instance of predefined type using
> +box.error.new() function. For example:
> +```
> +tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just cause")
> +tarantool> t:unpack()
> +---
> +- type: ClientError
> +  code: 9
> +  message: 'Failed to create space ''myspace'': just because'
> +  trace:
> +  - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
> +    line: 1
> +```
> +
> +User is also capable of defining own errors with any code  by means of:
> +```
> +box.error.new({code = user_code, reason = user_error_msg})
> +```
> +For instance:
> +```
> +e = box.error.new({code = 500, reason = 'just cause'})
> +```
> +
> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()`
> +methods and `.type`, `.message` and `.trace` fields.
> +
> +## Proposal
> +
> +In some cases a diagnostic information should be more complicated than
> +one last raised error. Consider following example: persistent Lua function
> +referenced by functional index has a bug in it's definition, Lua handler sets
> +an diag message. Then functional index extractor code setups an own, more
> +specialized error. Without stacked diagnostic area, only last error is
> +delivired to user. One way to deal with this problem is to introduce stack
> +accumulating all errors happened during request processing.
> +
> +### C API
> +
> +Let's keep existent `diag_set()` method as is. It is supposed to replace the
> +last error in diagnostic area with a new one. To add new error at the top of
> +existing one, let's introduce new method `diag_add()`. It is assumed to keep
> +an existent error message in diagnostic area (if any) and sets it as a reason
> +error for a recently-constructed error object. Note that `diag_set()` is not
> +going to preserve pointer to previous error which is held in error to be
> +substituted. To illustrate last point consider example:
> +
> +```
> +0. Errors: <NULL>
> +1. diag_set(code = 1)
> +Errors: <e1(code = 1) -> NULL>
> +2. diag_add(code = 2)
> +Errors: <e1(code = 1) -> e2(code = 2) -> NULL>
> +3. diag_set(code = 3)
> +Errors: <e3(code = 3) -> NULL>
> +```
> +
> +Hence, developer takes responsibility of placing `diag_set()` where the most
> +basic error should be raised. For instance, if during request processing
> +`diag_add()` is called before `diag_set()` then it will result in inheritance
> +of all errors from previous error raise:
> +
> +```
> +-- Processing of request #1
> +1. diag_set(code = 1)
> +Errors: <e1(code = 1) -> NULL>
> +2. diag_add(code = 2)
> +Errors: <e1(code = 1) -> e2(code = 2) -> NULL>
> +-- End of execution
> +
> +-- Processing of request #2
> +1. diag_add(code = 1)
> +Errors: <e1(code = 1) -> e2(code = 2) -> e3(code = 1) -> NULL>
> +-- End of execution
> +```
> +
> +As a result, at the end of execution fo second request, three errors in
> +stack are reported instead of one.
> +
> +Another way to resolve this issue is to erase diagnostic area before
> +request processing. However, it breaks current user-visible behaviour
> +since box.error.last() will preserve last occurred error only until execution
> +of the next request.
> +
> +The diagnostic area (now) contains (nothing but) pointer to the top error:
> +```
> +struct diag {
> +  struct error *last;
> +};
> +
> +```
> +
> +To organize errors in a list let's extend error structure with pointer to
> +the previous element. Or alternatively, add member of any data structure
> +providing list properties (struct rlist, struct stailq or whatever):
> +```
> +struct diag {
> +  struct stailq *errors;
> +};
> +
> +struct error {
> +   ...
> +   struct stailq_entry *in_errors;
> +};
> +```
> +
> +
> +### Lua API
> +
> +Tarantool returns a last-set (diag::last) error as `cdata` object from central
> +diagnostic area to Lua in case of error. User should be unable to modify it
> +(since it is considered to be a bad practice - in fact object doesn't belong
> +to user). On the other hand, user needs an ability to inspect a collected
> +diagnostic information. Hence, let's extend the `box.error` API with a function
> +which provides the way to get the previous error (reason): `:prev()` (and
> +correspondingly `.prev` field).
> +
> +```
> +-- Return a reason error object for given error
> +-- (when exists, nil otherwise).
> +box.error.prev(error) == error.prev
> +```
> +
> +Furthermore, let's extend signature of `box.error.new()` with new (optional)
> +argument - the 'reason' parent error object:
> +
> +```
> +e1 = box.error.new({code = 111, reason = "just cause"})
> +e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
> +```
> +
> +### Binary protocol
> +
> +Currently errors are sent as `(IPROTO_ERROR | errcode)` response with an
> +string message containing error details as a payload. There are not so many
> +options to extend current protocol wihtout breaking backward compatibility
> +(which is obviously one of implementation requirements). One way is to extend
> +existent binary protocol with a new key IPROTO_ERROR_STACK (or
> +IPROTO_ERROR_REASON or simply IPROTO_ERROR_V2):
> +```
> +{
> +        // backward compatibility
> +        IPROTO_ERROR: "the most recent error message",
> +        // modern error message
> +        IPROTO_ERROR_STACK: {
> +                {
> +                        // the most recent error object
> +                        IPROTO_ERROR_CODE: error_code_number,
> +                        IPROTO_ERROR_REASON: error_reason_string,
> +                },
> +                ...
> +                {
> +                        // the oldest (reason) error object
> +                },
> +        }
> +}
> +```
> +
> +IPROTO_ERROR is always sent (as in older versions) in case of error.
> +IPROTO_ERROR_STACK is presented in response only if there's at least two
> +elements in diagnostic list. Map which contains error stack can be optimized
> +in terms of space, so that avoid including error which is already encoded
> +in IPROTO_ERROR.
> -- 
> 2.15.1
> 

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Vladislav Shpilevoy]

I will take a look. I wasn't in To/CC in the first email in
this thread, so I didn't pay attention.

On 22/01/2020 14:54, Nikita Pettik wrote:
> On 14 Jan 23:16, Nikita Pettik wrote:
>> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> 
> Hi guys,
> 
> Does anybody want to look at this RFC? It's quite similar to the one which
> got LGTM from Konstantin, but it is even more simplified. I have to get
> formal approvement to start fixing code itself. Thanks.
>  
>> Part of #1148
>> ---
>> rfc in human-readable format: https://github.com/tarantool/tarantool/blob/np/gh-1148-stacked-diag/doc/rfc/1148-stacked-diagnostics.md
>> Previous version of rfc: https://github.com/tarantool/tarantool/commit/c2a7e1a10732fdb231780ac04d1a4bc1618c0468
>>
>> Changes in this version:
>> - All savepoint routines have been removed as meaningless;
>> - Removed SQL warnings mentions since they are unrelated to the subject of rfc;
>> - Removed mentions about exposing box.error.new() with filename and
>>   line parameters (again, it is barely related to the subject of rfc);
>> - Described in details difference between diag_set() in diag_add()
>>   functions as a part of C interface;
>> - Other minor clean-ups.
>>
>> Now RFC looks quite brief and straightforward in its implementation;
>> it attempts at solving only #1148 issue.
>>
>>  doc/rfc/1148-stacked-diagnostics.md | 214 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 214 insertions(+)
>>  create mode 100644 doc/rfc/1148-stacked-diagnostics.md
>>
>> diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
>> new file mode 100644
>> index 000000000..68fa6d21a
>> --- /dev/null
>> +++ b/doc/rfc/1148-stacked-diagnostics.md
>> @@ -0,0 +1,214 @@
>> +# Stacked Diagnostics
>> +
>> +* **Status**: In progress
>> +* **Start date**: 30-07-2019
>> +* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org,
>> +               Pettik Nikita korablev@tarantool.org
>> +* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
>> +
>> +## Background and motivation
>> +
>> +Support stacked diagnostics for Tarantool allows to accumulate all errors
>> +occurred during request processing. It allows to better understand what
>> +happened, and handle errors appropriately.
>> +
>> +### Current error diagnostics
>> +
>> +Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
>> +Object representing error featuring following properties:
>> + - type (string) error’s C++ class;
>> + - code (number) error’s number;
>> + - message (string) error’s message;
>> + - file (string) Tarantool source file;
>> + - line (number) line number in the Tarantool source file.
>> +
>> +The last error raised is exported with `box.error.last()` function.
>> +
>> +Type of error is represented by a few C++ classes (all are inherited from
>> +Exception class):
>> +```
>> +ClientError
>> + | LoggedError
>> + | AccessDeniedError
>> + | UnsupportedIndexFeature
>> +
>> +XlogError
>> + | XlogGapError
>> +
>> +SystemError
>> + | SocketError
>> + | OutOfMemory
>> + | TimedOut
>> +
>> +ChannelIsClosed
>> +FiberIsCancelled
>> +LuajitError
>> +IllegalParams
>> +CollationError
>> +SwimError
>> +CryptoError
>> +```
>> +
>> +All codes and names of ClientError class are available in box.error.
>> +User is able to create a new error instance of predefined type using
>> +box.error.new() function. For example:
>> +```
>> +tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just cause")
>> +tarantool> t:unpack()
>> +---
>> +- type: ClientError
>> +  code: 9
>> +  message: 'Failed to create space ''myspace'': just because'
>> +  trace:
>> +  - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
>> +    line: 1
>> +```
>> +
>> +User is also capable of defining own errors with any code  by means of:
>> +```
>> +box.error.new({code = user_code, reason = user_error_msg})
>> +```
>> +For instance:
>> +```
>> +e = box.error.new({code = 500, reason = 'just cause'})
>> +```
>> +
>> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()`
>> +methods and `.type`, `.message` and `.trace` fields.
>> +
>> +## Proposal
>> +
>> +In some cases a diagnostic information should be more complicated than
>> +one last raised error. Consider following example: persistent Lua function
>> +referenced by functional index has a bug in it's definition, Lua handler sets
>> +an diag message. Then functional index extractor code setups an own, more
>> +specialized error. Without stacked diagnostic area, only last error is
>> +delivired to user. One way to deal with this problem is to introduce stack
>> +accumulating all errors happened during request processing.
>> +
>> +### C API
>> +
>> +Let's keep existent `diag_set()` method as is. It is supposed to replace the
>> +last error in diagnostic area with a new one. To add new error at the top of
>> +existing one, let's introduce new method `diag_add()`. It is assumed to keep
>> +an existent error message in diagnostic area (if any) and sets it as a reason
>> +error for a recently-constructed error object. Note that `diag_set()` is not
>> +going to preserve pointer to previous error which is held in error to be
>> +substituted. To illustrate last point consider example:
>> +
>> +```
>> +0. Errors: <NULL>
>> +1. diag_set(code = 1)
>> +Errors: <e1(code = 1) -> NULL>
>> +2. diag_add(code = 2)
>> +Errors: <e1(code = 1) -> e2(code = 2) -> NULL>
>> +3. diag_set(code = 3)
>> +Errors: <e3(code = 3) -> NULL>
>> +```
>> +
>> +Hence, developer takes responsibility of placing `diag_set()` where the most
>> +basic error should be raised. For instance, if during request processing
>> +`diag_add()` is called before `diag_set()` then it will result in inheritance
>> +of all errors from previous error raise:
>> +
>> +```
>> +-- Processing of request #1
>> +1. diag_set(code = 1)
>> +Errors: <e1(code = 1) -> NULL>
>> +2. diag_add(code = 2)
>> +Errors: <e1(code = 1) -> e2(code = 2) -> NULL>
>> +-- End of execution
>> +
>> +-- Processing of request #2
>> +1. diag_add(code = 1)
>> +Errors: <e1(code = 1) -> e2(code = 2) -> e3(code = 1) -> NULL>
>> +-- End of execution
>> +```
>> +
>> +As a result, at the end of execution fo second request, three errors in
>> +stack are reported instead of one.
>> +
>> +Another way to resolve this issue is to erase diagnostic area before
>> +request processing. However, it breaks current user-visible behaviour
>> +since box.error.last() will preserve last occurred error only until execution
>> +of the next request.
>> +
>> +The diagnostic area (now) contains (nothing but) pointer to the top error:
>> +```
>> +struct diag {
>> +  struct error *last;
>> +};
>> +
>> +```
>> +
>> +To organize errors in a list let's extend error structure with pointer to
>> +the previous element. Or alternatively, add member of any data structure
>> +providing list properties (struct rlist, struct stailq or whatever):
>> +```
>> +struct diag {
>> +  struct stailq *errors;
>> +};
>> +
>> +struct error {
>> +   ...
>> +   struct stailq_entry *in_errors;
>> +};
>> +```
>> +
>> +
>> +### Lua API
>> +
>> +Tarantool returns a last-set (diag::last) error as `cdata` object from central
>> +diagnostic area to Lua in case of error. User should be unable to modify it
>> +(since it is considered to be a bad practice - in fact object doesn't belong
>> +to user). On the other hand, user needs an ability to inspect a collected
>> +diagnostic information. Hence, let's extend the `box.error` API with a function
>> +which provides the way to get the previous error (reason): `:prev()` (and
>> +correspondingly `.prev` field).
>> +
>> +```
>> +-- Return a reason error object for given error
>> +-- (when exists, nil otherwise).
>> +box.error.prev(error) == error.prev
>> +```
>> +
>> +Furthermore, let's extend signature of `box.error.new()` with new (optional)
>> +argument - the 'reason' parent error object:
>> +
>> +```
>> +e1 = box.error.new({code = 111, reason = "just cause"})
>> +e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
>> +```
>> +
>> +### Binary protocol
>> +
>> +Currently errors are sent as `(IPROTO_ERROR | errcode)` response with an
>> +string message containing error details as a payload. There are not so many
>> +options to extend current protocol wihtout breaking backward compatibility
>> +(which is obviously one of implementation requirements). One way is to extend
>> +existent binary protocol with a new key IPROTO_ERROR_STACK (or
>> +IPROTO_ERROR_REASON or simply IPROTO_ERROR_V2):
>> +```
>> +{
>> +        // backward compatibility
>> +        IPROTO_ERROR: "the most recent error message",
>> +        // modern error message
>> +        IPROTO_ERROR_STACK: {
>> +                {
>> +                        // the most recent error object
>> +                        IPROTO_ERROR_CODE: error_code_number,
>> +                        IPROTO_ERROR_REASON: error_reason_string,
>> +                },
>> +                ...
>> +                {
>> +                        // the oldest (reason) error object
>> +                },
>> +        }
>> +}
>> +```
>> +
>> +IPROTO_ERROR is always sent (as in older versions) in case of error.
>> +IPROTO_ERROR_STACK is presented in response only if there's at least two
>> +elements in diagnostic list. Map which contains error stack can be optimized
>> +in terms of space, so that avoid including error which is already encoded
>> +in IPROTO_ERROR.
>> -- 
>> 2.15.1
>>

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Vladislav Shpilevoy]

Hi! Thanks for the RFC!

See 13 comments below.

On 14/01/2020 21:16, Nikita Pettik wrote:
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> 
> Part of #1148
> ---
> rfc in human-readable format: https://github.com/tarantool/tarantool/blob/np/gh-1148-stacked-diag/doc/rfc/1148-stacked-diagnostics.md
> Previous version of rfc: https://github.com/tarantool/tarantool/commit/c2a7e1a10732fdb231780ac04d1a4bc1618c0468
> 
> Changes in this version:
> - All savepoint routines have been removed as meaningless;
> - Removed SQL warnings mentions since they are unrelated to the subject of rfc;
> - Removed mentions about exposing box.error.new() with filename and
>   line parameters (again, it is barely related to the subject of rfc);
> - Described in details difference between diag_set() in diag_add()
>   functions as a part of C interface;
> - Other minor clean-ups.
> 
> Now RFC looks quite brief and straightforward in its implementation;
> it attempts at solving only #1148 issue.
> 
>  doc/rfc/1148-stacked-diagnostics.md | 214 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 214 insertions(+)
>  create mode 100644 doc/rfc/1148-stacked-diagnostics.md
> 
> diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
> new file mode 100644
> index 000000000..68fa6d21a
> --- /dev/null
> +++ b/doc/rfc/1148-stacked-diagnostics.md
> @@ -0,0 +1,214 @@
> +# Stacked Diagnostics
> +
> +* **Status**: In progress
> +* **Start date**: 30-07-2019
> +* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org,
> +               Pettik Nikita korablev@tarantool.org

1. You missed your GitHub login.

> +* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
> +

2. You missed 'Summary' section.

> +## Background and motivation
> +
> +Support stacked diagnostics for Tarantool allows to accumulate all errors
> +occurred during request processing. It allows to better understand what
> +happened, and handle errors appropriately.
> +
> +### Current error diagnostics
> +
> +Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
> +Object representing error featuring following properties:
> + - type (string) error’s C++ class;
> + - code (number) error’s number;
> + - message (string) error’s message;
> + - file (string) Tarantool source file;
> + - line (number) line number in the Tarantool source file.
> +
> +The last error raised is exported with `box.error.last()` function.
> +
> +Type of error is represented by a few C++ classes (all are inherited from
> +Exception class):
> +```
> +ClientError
> + | LoggedError
> + | AccessDeniedError
> + | UnsupportedIndexFeature
> +
> +XlogError
> + | XlogGapError
> +
> +SystemError
> + | SocketError
> + | OutOfMemory
> + | TimedOut
> +
> +ChannelIsClosed
> +FiberIsCancelled
> +LuajitError
> +IllegalParams
> +CollationError
> +SwimError
> +CryptoError
> +```

3. I don't remember why all these errors were listed but it certainly makes no
sense. For the RFC it does not matter what a concrete error types there are.

> +
> +All codes and names of ClientError class are available in box.error.
> +User is able to create a new error instance of predefined type using
> +box.error.new() function. For example:
> +```
> +tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just cause")
> +tarantool> t:unpack()
> +---
> +- type: ClientError
> +  code: 9
> +  message: 'Failed to create space ''myspace'': just because'

4. Unpack is able to even unpack individual words! cause -> because, wow!
(joke time).

> +  trace:
> +  - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
> +    line: 1
> +```
> +
> +User is also capable of defining own errors with any code  by means of:
> +```
> +box.error.new({code = user_code, reason = user_error_msg})
> +```
> +For instance:
> +```
> +e = box.error.new({code = 500, reason = 'just cause'})
> +```
> +
> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()`
> +methods and `.type`, `.message` and `.trace` fields.
> +

5. 'Background and motivation' section is finished, but no a word about
motivation.

> +## Proposal
> +
> +In some cases a diagnostic information should be more complicated than
> +one last raised error. Consider following example: persistent Lua function
> +referenced by functional index has a bug in it's definition, Lua handler sets
> +an diag message. Then functional index extractor code setups an own, more
> +specialized error. Without stacked diagnostic area, only last error is
> +delivired to user. One way to deal with this problem is to introduce stack
> +accumulating all errors happened during request processing.
> +
> +### C API
> +
> +Let's keep existent `diag_set()` method as is. It is supposed to replace the
> +last error in diagnostic area with a new one. To add new error at the top of
> +existing one, let's introduce new method `diag_add()`. It is assumed to keep
> +an existent error message in diagnostic area (if any) and sets it as a reason

6. Reason is a different term. It is an error message of the current error.
And that name 'reason' is already occupied in box.error.new() function. Besides,
below you call it 'prev', previous error. I propose to keep the terminology the
same everywhere. 'Reason' for error message, 'previous' for an error happened
before a new one, and linked with it. No mixing of these terms.

> +error for a recently-constructed error object. Note that `diag_set()` is not
> +going to preserve pointer to previous error which is held in error to be
> +substituted. To illustrate last point consider example:> +
> +```
> +0. Errors: <NULL>
> +1. diag_set(code = 1)
> +Errors: <e1(code = 1) -> NULL>
> +2. diag_add(code = 2)
> +Errors: <e1(code = 1) -> e2(code = 2) -> NULL>
> +3. diag_set(code = 3)
> +Errors: <e3(code = 3) -> NULL>
> +```
> +
> +Hence, developer takes responsibility of placing `diag_set()` where the most
> +basic error should be raised. For instance, if during request processing
> +`diag_add()` is called before `diag_set()` then it will result in inheritance
> +of all errors from previous error raise:> +
> +```
> +-- Processing of request #1
> +1. diag_set(code = 1)
> +Errors: <e1(code = 1) -> NULL>
> +2. diag_add(code = 2)
> +Errors: <e1(code = 1) -> e2(code = 2) -> NULL>
> +-- End of execution
> +
> +-- Processing of request #2
> +1. diag_add(code = 1)
> +Errors: <e1(code = 1) -> e2(code = 2) -> e3(code = 1) -> NULL>
> +-- End of execution
> +```
> +
> +As a result, at the end of execution fo second request, three errors in

7. fo -> of.

> +stack are reported instead of one.
> +
> +Another way to resolve this issue is to erase diagnostic area before
> +request processing. However, it breaks current user-visible behaviour
> +since box.error.last() will preserve last occurred error only until execution
> +of the next request.
> +
8. I think users would rather be surprised if a fiber would have not empty diag
when a request is just started. Just like it was recently with fiber.storage.

In case I misunderstood what do you mean as a request - in my terminology
'request' is either a manually created fiber, which dies and clears error.last
anyway after its function is finished, or it is a pooled fiber, which doesn't
die, but still should not leak anything between requests. So now we have a
not-consistent behaviour. It not only *can* be fixed, but *must* be fixed.

> +The diagnostic area (now) contains (nothing but) pointer to the top error:
> +```
> +struct diag {
> +  struct error *last;
> +};
> +
> +```
> +
> +To organize errors in a list let's extend error structure with pointer to
> +the previous element. Or alternatively, add member of any data structure
> +providing list properties (struct rlist, struct stailq or whatever):
> +```
> +struct diag {
> +  struct stailq *errors;
> +};
> +
> +struct error {
> +   ...
> +   struct stailq_entry *in_errors;
> +};
> +```
> +
> +
> +### Lua API
> +
> +Tarantool returns a last-set (diag::last) error as `cdata` object from central
> +diagnostic area to Lua in case of error. User should be unable to modify it
> +(since it is considered to be a bad practice - in fact object doesn't belong
> +to user). On the other hand, user needs an ability to inspect a collected
> +diagnostic information. Hence, let's extend the `box.error` API with a function
> +which provides the way to get the previous error (reason): `:prev()` (and
> +correspondingly `.prev` field).
> +
> +```
> +-- Return a reason error object for given error
> +-- (when exists, nil otherwise).
> +box.error.prev(error) == error.prev
> +```
> +
> +Furthermore, let's extend signature of `box.error.new()` with new (optional)
> +argument - the 'reason' parent error object:

9. 'Reason' is already occupied by a error message. And below you use 'prev'
name for a parent error.

> +
> +```
> +e1 = box.error.new({code = 111, reason = "just cause"})
> +e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
> +```

10. Are we not going to allow to link two existing errors? I imagine
it could be simpler and more flexible for a user, than filling
one big map in error.new().

> +
> +### Binary protocol
> +
> +Currently errors are sent as `(IPROTO_ERROR | errcode)` response with an
> +string message containing error details as a payload. There are not so many
> +options to extend current protocol wihtout breaking backward compatibility
> +(which is obviously one of implementation requirements). One way is to extend
> +existent binary protocol with a new key IPROTO_ERROR_STACK (or
> +IPROTO_ERROR_REASON or simply IPROTO_ERROR_V2):
> +```
> +{
> +        // backward compatibility
> +        IPROTO_ERROR: "the most recent error message",
> +        // modern error message
> +        IPROTO_ERROR_STACK: {
> +                {
> +                        // the most recent error object
> +                        IPROTO_ERROR_CODE: error_code_number,
> +                        IPROTO_ERROR_REASON: error_reason_string,
> +                },
> +                ...
> +                {
> +                        // the oldest (reason) error object
> +                },
> +        }
> +}
> +```

11. The names look good. Except that maybe change IPROTO_ERROR_REASON ->
IPROTO_ERROR_MESSAGE. I don't know why it is called reason everywhere
in our public API. In struct error it is message, not reason. And it
just looks more correct, IMO.

12. Another option - send multiple IPROTO_ERROR's. In MP_MAP you can
put multiple keys with the same value, and even keep a strict order.
So we could send IPROTO_ERROR: ..., IPROTO_ERROR: ..., IPROTO_ERROR: ...
For each message in the stack. But that looks like an illegal hack,
and also does not allow to fix this thing alongside:
https://github.com/tarantool/tarantool/issues/3399

So a proper stack looks better, like you proposed.

> +
> +IPROTO_ERROR is always sent (as in older versions) in case of error.
> +IPROTO_ERROR_STACK is presented in response only if there's at least two
> +elements in diagnostic list. Map which contains error stack can be optimized
> +in terms of space, so that avoid including error which is already encoded
> +in IPROTO_ERROR.
> 

13. I don't think it is worth saving a few bytes for the cold path of error
reporting. Lets always send IPROTO_ERROR with a last error like now *and*
IPROTO_ERROR_STACK. Even if they contain the same error. It will make much
easier to remove IPROTO_ERROR later. Otherwise we will need to carry them
both forever, if IPROTO_ERROR_STACK would depend on IPROTO_ERROR.

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Nikita Pettik]

On 23 Jan 23:56, Vladislav Shpilevoy wrote:
> Hi! Thanks for the RFC!
> 
> See 13 comments below.
> 
> On 14/01/2020 21:16, Nikita Pettik wrote:
> > +* **Start date**: 30-07-2019
> > +* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org,
> > +               Pettik Nikita korablev@tarantool.org
> 
> 1. You missed your GitHub login.

Okay, added.
 
> > +* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
> > +
> 
> 2. You missed 'Summary' section.

I believe it is redundant for such brief RFC. 
 
> > +### Current error diagnostics
> > +
> > +Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
> > +Object representing error featuring following properties:
> > + - type (string) error’s C++ class;
> > + - code (number) error’s number;
> > + - message (string) error’s message;
> > + - file (string) Tarantool source file;
> > + - line (number) line number in the Tarantool source file.
> > +
> > +The last error raised is exported with `box.error.last()` function.
> > +
> > +Type of error is represented by a few C++ classes (all are inherited from
> > +Exception class):
> > +```
> > +ClientError
> > + | LoggedError
> > + | AccessDeniedError
> > + | UnsupportedIndexFeature
> > +
> > +XlogError
> > + | XlogGapError
> > +
> > +SystemError
> > + | SocketError
> > + | OutOfMemory
> > + | TimedOut
> > +
> > +ChannelIsClosed
> > +FiberIsCancelled
> > +LuajitError
> > +IllegalParams
> > +CollationError
> > +SwimError
> > +CryptoError
> > +```
> 
> 3. I don't remember why all these errors were listed but it certainly makes no
> sense. For the RFC it does not matter what a concrete error types there are.

Ok, left only ClientError hierarchy as an example.

> > +tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just cause")
> > +tarantool> t:unpack()
> > +---
> > +- type: ClientError
> > +  code: 9
> > +  message: 'Failed to create space ''myspace'': just because'
> 
> 4. Unpack is able to even unpack individual words! cause -> because, wow!
> (joke time).

Fixed.

> > +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()`
> > +methods and `.type`, `.message` and `.trace` fields.
> > +
> 
> 5. 'Background and motivation' section is finished, but no a word about
> motivation.

Ok, moved motivating example from "Proposal" to "Background and motivation"
section.

> > +## Proposal
> > +
> > +In some cases a diagnostic information should be more complicated than
> > +one last raised error. Consider following example: persistent Lua function
> > +referenced by functional index has a bug in it's definition, Lua handler sets
> > +an diag message. Then functional index extractor code setups an own, more
> > +specialized error. Without stacked diagnostic area, only last error is
> > +delivired to user. One way to deal with this problem is to introduce stack
> > +accumulating all errors happened during request processing.
> > +
> > +### C API
> > +
> > +Let's keep existent `diag_set()` method as is. It is supposed to replace the
> > +last error in diagnostic area with a new one. To add new error at the top of
> > +existing one, let's introduce new method `diag_add()`. It is assumed to keep
> > +an existent error message in diagnostic area (if any) and sets it as a reason
> 
> 6. Reason is a different term. It is an error message of the current error.
> And that name 'reason' is already occupied in box.error.new() function. Besides,
> below you call it 'prev', previous error. I propose to keep the terminology the
> same everywhere. 'Reason' for error message, 'previous' for an error happened
> before a new one, and linked with it. No mixing of these terms.

Fair, fixed.
 
> > +-- Processing of request #2
> > +1. diag_add(code = 1)
> > +Errors: <e1(code = 1) -> e2(code = 2) -> e3(code = 1) -> NULL>
> > +-- End of execution
> > +```
> > +
> > +As a result, at the end of execution fo second request, three errors in
> 
> 7. fo -> of.

Fixed.
 
> > +Another way to resolve this issue is to erase diagnostic area before
> > +request processing. However, it breaks current user-visible behaviour
> > +since box.error.last() will preserve last occurred error only until execution
> > +of the next request.
> > +
> 8. I think users would rather be surprised if a fiber would have not empty diag
> when a request is just started. Just like it was recently with fiber.storage.
> 
> In case I misunderstood what do you mean as a request - in my terminology
> 'request' is either a manually created fiber, which dies and clears error.last
> anyway after its function is finished, or it is a pooled fiber, which doesn't
> die, but still should not leak anything between requests. So now we have a
> not-consistent behaviour. It not only *can* be fixed, but *must* be fixed.

'Request' in context I used is any box operation (e.g. space:insert(),
index:select() or cn:execute()). Also I 'member that Konstantin was against
nullifing last error before processing box.execute() and other box functions
(it was in scope of reworking SQL parser's errors).

> > +The diagnostic area (now) contains (nothing but) pointer to the top error:
> > +```
> > +struct diag {
> > +  struct error *last;
> > +};
> > +
> > +
> > +-- Return a reason error object for given error
> > +-- (when exists, nil otherwise).
> > +box.error.prev(error) == error.prev
> > +```
> > +
> > +Furthermore, let's extend signature of `box.error.new()` with new (optional)
> > +argument - the 'reason' parent error object:
> 
> 9. 'Reason' is already occupied by a error message. And below you use 'prev'
> name for a parent error.

Fixed.
 
> 10. Are we not going to allow to link two existing errors? I imagine
> it could be simpler and more flexible for a user, than filling
> one big map in error.new().

Okay, I'm not against it:

 Another way to resolve this issue is to erase diagnostic area before
@@ -173,13 +158,23 @@ box.error.prev(error) == error.prev
 ```
 
 Furthermore, let's extend signature of `box.error.new()` with new (optional)
-argument - the 'reason' parent error object:
+argument - 'prev' - previous error object:
 
 ```
 e1 = box.error.new({code = 111, reason = "just cause"})
 e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
 ```
 
+User may want to link already existing errors. To achieve this let's add
+`set_prev` method to error object or/and `link` to `box.error` so that one can
+join two errors:
+```
+e1 = box.error.new({code = 111, reason = "just cause"})
+e2 = box.error.new({code = 222, reason = "just cause x2"})
+...
+e2.set_prev(e1) -- e2.prev == e1
+box.error.link(e1, e2) -- e2.prev == e1
+```

> > +### Binary protocol
> > +
> > +Currently errors are sent as `(IPROTO_ERROR | errcode)` response with an
> > +string message containing error details as a payload. There are not so many
> > +options to extend current protocol wihtout breaking backward compatibility
> > +(which is obviously one of implementation requirements). One way is to extend
> > +existent binary protocol with a new key IPROTO_ERROR_STACK (or
> > +IPROTO_ERROR_REASON or simply IPROTO_ERROR_V2):
> > +```
> > +{
> > +        // backward compatibility
> > +        IPROTO_ERROR: "the most recent error message",
> > +        // modern error message
> > +        IPROTO_ERROR_STACK: {
> > +                {
> > +                        // the most recent error object
> > +                        IPROTO_ERROR_CODE: error_code_number,
> > +                        IPROTO_ERROR_REASON: error_reason_string,
> > +                },
> > +                ...
> > +                {
> > +                        // the oldest (reason) error object
> > +                },
> > +        }
> > +}
> > +```
> 
> 11. The names look good. Except that maybe change IPROTO_ERROR_REASON ->
> IPROTO_ERROR_MESSAGE. I don't know why it is called reason everywhere
> in our public API. In struct error it is message, not reason. And it
> just looks more correct, IMO.

On the other hand, it might look a bit inconsistent if in binary protocole
field would have different name from one in Lua API. Nevertheless, I
don't mind this renaming. Fixed in RFC.

> 12. Another option - send multiple IPROTO_ERROR's. In MP_MAP you can
> put multiple keys with the same value, and even keep a strict order.
> So we could send IPROTO_ERROR: ..., IPROTO_ERROR: ..., IPROTO_ERROR: ...
> For each message in the stack. But that looks like an illegal hack,
> and also does not allow to fix this thing alongside:
> https://github.com/tarantool/tarantool/issues/3399
> 
> So a proper stack looks better, like you proposed.
> 
> > +
> > +IPROTO_ERROR is always sent (as in older versions) in case of error.
> > +IPROTO_ERROR_STACK is presented in response only if there's at least two
> > +elements in diagnostic list. Map which contains error stack can be optimized
> > +in terms of space, so that avoid including error which is already encoded
> > +in IPROTO_ERROR.
> > 
> 
> 13. I don't think it is worth saving a few bytes for the cold path of error
> reporting. Lets always send IPROTO_ERROR with a last error like now *and*
> IPROTO_ERROR_STACK. Even if they contain the same error. It will make much
> easier to remove IPROTO_ERROR later. Otherwise we will need to carry them
> both forever, if IPROTO_ERROR_STACK would depend on IPROTO_ERROR.

Ok (it is only suggestion in scope of RFC, so I guess your comment doesn't
imply that last remark concerning optimization should be erased).

Full diff:

diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
index 68fa6d21a..0d1d07dc3 100644
--- a/doc/rfc/1148-stacked-diagnostics.md
+++ b/doc/rfc/1148-stacked-diagnostics.md
@@ -3,14 +3,17 @@
 * **Status**: In progress
 * **Start date**: 30-07-2019
 * **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org,
-               Pettik Nikita korablev@tarantool.org
+               Pettik Nikita @Korablev77 korablev@tarantool.org
 * **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
 
 ## Background and motivation
 
 Support stacked diagnostics for Tarantool allows to accumulate all errors
 occurred during request processing. It allows to better understand what
-happened, and handle errors appropriately.
+happened, and handle errors appropriately. Consider following example:
+persistent Lua function referenced by functional index has a bug in it's
+definition, Lua handler sets an diag message. Then functional index extractor
+code setups an own, more specialized error.
 
 ### Current error diagnostics
 
@@ -25,28 +28,12 @@ Object representing error featuring following properties:
 The last error raised is exported with `box.error.last()` function.
 
 Type of error is represented by a few C++ classes (all are inherited from
-Exception class):
+Exception class). For instance hierarchy for ClientError is following:
 ```
 ClientError
  | LoggedError
  | AccessDeniedError
  | UnsupportedIndexFeature
-
-XlogError
- | XlogGapError
-
-SystemError
- | SocketError
- | OutOfMemory
- | TimedOut
-
-ChannelIsClosed
-FiberIsCancelled
-LuajitError
-IllegalParams
-CollationError
-SwimError
-CryptoError
 ```
 
 All codes and names of ClientError class are available in box.error.
@@ -58,7 +45,7 @@ tarantool> t:unpack()
 ---
 - type: ClientError
   code: 9
-  message: 'Failed to create space ''myspace'': just because'
+  message: 'Failed to create space ''myspace'': just cause'
   trace:
   - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
     line: 1
@@ -78,12 +65,10 @@ methods and `.type`, `.message` and `.trace` fields.
 
 ## Proposal
 
-In some cases a diagnostic information should be more complicated than
-one last raised error. Consider following example: persistent Lua function
-referenced by functional index has a bug in it's definition, Lua handler sets
-an diag message. Then functional index extractor code setups an own, more
-specialized error. Without stacked diagnostic area, only last error is
-delivired to user. One way to deal with this problem is to introduce stack
+In some cases a diagnostic area should be more complicated than
+one last raised error to provide decent information concerning incident (see
+motivating example above). Without stacked diagnostic area, only last error is
+delivered to user. One way to deal with this problem is to introduce stack
 accumulating all errors happened during request processing.
 
 ### C API
@@ -91,7 +76,7 @@ accumulating all errors happened during request processing.
 Let's keep existent `diag_set()` method as is. It is supposed to replace the
 last error in diagnostic area with a new one. To add new error at the top of
 existing one, let's introduce new method `diag_add()`. It is assumed to keep
-an existent error message in diagnostic area (if any) and sets it as a reason
+an existent error message in diagnostic area (if any) and sets it as a previous
 error for a recently-constructed error object. Note that `diag_set()` is not
 going to preserve pointer to previous error which is held in error to be
 substituted. To illustrate last point consider example:
@@ -125,7 +110,7 @@ Errors: <e1(code = 1) -> e2(code = 2) -> e3(code = 1) -> NULL>
 -- End of execution
 ```
 
-As a result, at the end of execution fo second request, three errors in
+As a result, at the end of execution of second request, three errors in
 stack are reported instead of one.
 
 Another way to resolve this issue is to erase diagnostic area before
@@ -173,13 +158,23 @@ box.error.prev(error) == error.prev
 ```
 
 Furthermore, let's extend signature of `box.error.new()` with new (optional)
-argument - the 'reason' parent error object:
+argument - 'prev' - previous error object:
 
 ```
 e1 = box.error.new({code = 111, reason = "just cause"})
 e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
 ```
 
+User may want to link already existing errors. To achieve this let's add
+`set_prev` method to error object or/and `link` to `box.error` so that one can
+join two errors:
+```
+e1 = box.error.new({code = 111, reason = "just cause"})
+e2 = box.error.new({code = 222, reason = "just cause x2"})
+...
+e2.set_prev(e1) -- e2.prev == e1
+box.error.link(e1, e2) -- e2.prev == e1
+```
 ### Binary protocol
 
 Currently errors are sent as `(IPROTO_ERROR | errcode)` response with an
@@ -197,7 +192,7 @@ IPROTO_ERROR_REASON or simply IPROTO_ERROR_V2):
                 {
                         // the most recent error object
                         IPROTO_ERROR_CODE: error_code_number,
-                        IPROTO_ERROR_REASON: error_reason_string,
+                        IPROTO_ERROR_MESSAGE: error_message_string,
                 },
                 ...
                 {

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

It is not a second review. I just wanted to say that I am
going to spend less time on reviewes to the end of this
week due to intensive preparations to FOSDEM. I will get
back to this RFC as soon as I can, I didn't forget.

Just one comment for now, below.

>>> +* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
>>> +
>>
>> 2. You missed 'Summary' section.
> 
> I believe it is redundant for such brief RFC. 
>  

Nevertheless, please, add it. If that RFC is so simpler,
then it should not be a problem to make it match the
RFC template. Template says:

    "Short description what, why and how is implemented."

So just say something like

    The document describes a stacked diagnostics feature. The
    feature is needed for the cases, when there is a complex and
    big callstack with different subsystems in it, and sometimes
    the most lowlevel error is not descriptive enough. So it is
    wanted to be able to look at errors along the whole callstack
    from the place where a first error happened. The feature
    implementation is going to change a fiber's error object to a
    list of objects, a stack. Its first element will always be
    created by the deepest and the most basic errors.

Or anything you think would be better here.

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Nikita Pettik]

On 28 Jan 01:23, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> It is not a second review. I just wanted to say that I am
> going to spend less time on reviewes to the end of this
> week due to intensive preparations to FOSDEM. I will get
> back to this RFC as soon as I can, I didn't forget.
> 
> Just one comment for now, below.
> 
> >>> +* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
> >>> +
> >>
> >> 2. You missed 'Summary' section.
> > 
> > I believe it is redundant for such brief RFC. 
> >  
> 
> Nevertheless, please, add it. If that RFC is so simpler,
> then it should not be a problem to make it match the
> RFC template. Template says:
> 
>     "Short description what, why and how is implemented."
> 
> So just say something like
> 
>     The document describes a stacked diagnostics feature. The
>     feature is needed for the cases, when there is a complex and
>     big callstack with different subsystems in it, and sometimes
>     the most lowlevel error is not descriptive enough. So it is
>     wanted to be able to look at errors along the whole callstack
>     from the place where a first error happened. The feature
>     implementation is going to change a fiber's error object to a
>     list of objects, a stack. Its first element will always be
>     created by the deepest and the most basic errors.
> 
> Or anything you think would be better here.

Ok, as you wish. A bit reformulated your description:

diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
index 0d1d07dc3..d57e040ba 100644
--- a/doc/rfc/1148-stacked-diagnostics.md
+++ b/doc/rfc/1148-stacked-diagnostics.md
@@ -6,6 +6,18 @@
                Pettik Nikita @Korablev77 korablev@tarantool.org
 * **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
 
+
+## Summary
+
+The document describes a stacked diagnostics feature. It is needed for the cases,
+when there is a complex and huge call stack with variety of subsystems in it.
+It may turn out that the most low-level error is not descriptive enough. In turn
+user may want to be able to look at errors along the whole call stack from the
+place where a first error happened. In terms of implementation single fiber's
+error object is going to be replaced with a list of objects forming diagnostic
+stack. Its first element will always be created by the deepest and the most
+basic error. Both C and Lua APIs are extended to support adding errors in stack.
+

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Vladislav Shpilevoy]

Thanks for the RFC.

It still does not conform with the template, but ok. I see that
that ship has sailed already, some other RFCs also violate the
template.

See 2 comments below.

>> 10. Are we not going to allow to link two existing errors? I imagine
>> it could be simpler and more flexible for a user, than filling
>> one big map in error.new().
> 
> Okay, I'm not against it:
> 
>  Another way to resolve this issue is to erase diagnostic area before
> @@ -173,13 +158,23 @@ box.error.prev(error) == error.prev
>  ```
>  
>  Furthermore, let's extend signature of `box.error.new()` with new (optional)
> -argument - the 'reason' parent error object:
> +argument - 'prev' - previous error object:
>  
>  ```
>  e1 = box.error.new({code = 111, reason = "just cause"})
>  e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
>  ```
>  
> +User may want to link already existing errors. To achieve this let's add
> +`set_prev` method to error object or/and `link` to `box.error` so that one can
> +join two errors:
> +```
> +e1 = box.error.new({code = 111, reason = "just cause"})
> +e2 = box.error.new({code = 222, reason = "just cause x2"})
> +...
> +e2.set_prev(e1) -- e2.prev == e1
> +box.error.link(e1, e2) -- e2.prev == e1
> +```

1. I don't think we need to change box.error global API. It would be
enough to add new methods to error object. box.error.link() and
box.error.prev() look redundant. What is a case, when they should
be used instead of error object methods?

box.error.new() and last() exist because there is no other way to
create an error, or to get a last one.

>>> +### Binary protocol
>>> +
>>> +Currently errors are sent as `(IPROTO_ERROR | errcode)` response with an
>>> +string message containing error details as a payload. There are not so many
>>> +options to extend current protocol wihtout breaking backward compatibility
>>> +(which is obviously one of implementation requirements). One way is to extend
>>> +existent binary protocol with a new key IPROTO_ERROR_STACK (or
>>> +IPROTO_ERROR_REASON or simply IPROTO_ERROR_V2):
>>> +```
>>> +{
>>> +        // backward compatibility
>>> +        IPROTO_ERROR: "the most recent error message",
>>> +        // modern error message
>>> +        IPROTO_ERROR_STACK: {
>>> +                {
>>> +                        // the most recent error object
>>> +                        IPROTO_ERROR_CODE: error_code_number,
>>> +                        IPROTO_ERROR_REASON: error_reason_string,
>>> +                },
>>> +                ...
>>> +                {
>>> +                        // the oldest (reason) error object
>>> +                },
>>> +        }
>>> +}
>>> +```
>>
>> 11. The names look good. Except that maybe change IPROTO_ERROR_REASON ->
>> IPROTO_ERROR_MESSAGE. I don't know why it is called reason everywhere
>> in our public API. In struct error it is message, not reason. And it
>> just looks more correct, IMO.
> 
> On the other hand, it might look a bit inconsistent if in binary protocole
> field would have different name from one in Lua API. Nevertheless, I
> don't mind this renaming. Fixed in RFC.
> 
>> 12. Another option - send multiple IPROTO_ERROR's. In MP_MAP you can
>> put multiple keys with the same value, and even keep a strict order.
>> So we could send IPROTO_ERROR: ..., IPROTO_ERROR: ..., IPROTO_ERROR: ...
>> For each message in the stack. But that looks like an illegal hack,
>> and also does not allow to fix this thing alongside:
>> https://github.com/tarantool/tarantool/issues/3399
>>
>> So a proper stack looks better, like you proposed.
>>
>>> +
>>> +IPROTO_ERROR is always sent (as in older versions) in case of error.
>>> +IPROTO_ERROR_STACK is presented in response only if there's at least two
>>> +elements in diagnostic list. Map which contains error stack can be optimized
>>> +in terms of space, so that avoid including error which is already encoded
>>> +in IPROTO_ERROR.
>>>
>>
>> 13. I don't think it is worth saving a few bytes for the cold path of error
>> reporting. Lets always send IPROTO_ERROR with a last error like now *and*
>> IPROTO_ERROR_STACK. Even if they contain the same error. It will make much
>> easier to remove IPROTO_ERROR later. Otherwise we will need to carry them
>> both forever, if IPROTO_ERROR_STACK would depend on IPROTO_ERROR.
> 
> Ok (it is only suggestion in scope of RFC, so I guess your comment doesn't
> imply that last remark concerning optimization should be erased).

2. The comments and alternatives can be kept, yes.

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Nikita Pettik]

On 04 Feb 21:48, Vladislav Shpilevoy wrote:
> Thanks for the RFC.
> 
> It still does not conform with the template, but ok. I see that
> that ship has sailed already, some other RFCs also violate the
> template.
> 
> See 2 comments below.
> 
> >> 10. Are we not going to allow to link two existing errors? I imagine
> >> it could be simpler and more flexible for a user, than filling
> >> one big map in error.new().
> > 
> > Okay, I'm not against it:
> > 
> >  Another way to resolve this issue is to erase diagnostic area before
> > @@ -173,13 +158,23 @@ box.error.prev(error) == error.prev
> >  ```
> >  
> >  Furthermore, let's extend signature of `box.error.new()` with new (optional)
> > -argument - the 'reason' parent error object:
> > +argument - 'prev' - previous error object:
> >  
> >  ```
> >  e1 = box.error.new({code = 111, reason = "just cause"})
> >  e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
> >  ```
> >  
> > +User may want to link already existing errors. To achieve this let's add
> > +`set_prev` method to error object or/and `link` to `box.error` so that one can
> > +join two errors:
> > +```
> > +e1 = box.error.new({code = 111, reason = "just cause"})
> > +e2 = box.error.new({code = 222, reason = "just cause x2"})
> > +...
> > +e2.set_prev(e1) -- e2.prev == e1
> > +box.error.link(e1, e2) -- e2.prev == e1
> > +```
> 
> 1. I don't think we need to change box.error global API. It would be
> enough to add new methods to error object. box.error.link() and
> box.error.prev() look redundant. What is a case, when they should
> be used instead of error object methods?
> 
> box.error.new() and last() exist because there is no other way to
> create an error, or to get a last one.

I've added both since was not sure which one is better.
Since you'd prefer avoid changing global interface (which is
reasonable argument) let's leave only e:set_prev():

diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
index d57e040ba..ed121770d 100644
--- a/doc/rfc/1148-stacked-diagnostics.md
+++ b/doc/rfc/1148-stacked-diagnostics.md
@@ -178,14 +178,12 @@ e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
 ```
 
 User may want to link already existing errors. To achieve this let's add
-`set_prev` method to error object or/and `link` to `box.error` so that one can
-join two errors:
+`set_prev` method to error object so that one can join two errors:
 ```
 e1 = box.error.new({code = 111, reason = "just cause"})
 e2 = box.error.new({code = 222, reason = "just cause x2"})
 ...
 e2.set_prev(e1) -- e2.prev == e1
-box.error.link(e1, e2) -- e2.prev == e1
 ```
 ### Binary protocol

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Vladislav Shpilevoy]

Thanks for the fix!

On 05/02/2020 07:16, Nikita Pettik wrote:
> On 04 Feb 21:48, Vladislav Shpilevoy wrote:
>> Thanks for the RFC.
>>
>> It still does not conform with the template, but ok. I see that
>> that ship has sailed already, some other RFCs also violate the
>> template.
>>
>> See 2 comments below.
>>
>>>> 10. Are we not going to allow to link two existing errors? I imagine
>>>> it could be simpler and more flexible for a user, than filling
>>>> one big map in error.new().
>>>
>>> Okay, I'm not against it:
>>>
>>>  Another way to resolve this issue is to erase diagnostic area before
>>> @@ -173,13 +158,23 @@ box.error.prev(error) == error.prev
>>>  ```
>>>  
>>>  Furthermore, let's extend signature of `box.error.new()` with new (optional)
>>> -argument - the 'reason' parent error object:
>>> +argument - 'prev' - previous error object:
>>>  
>>>  ```
>>>  e1 = box.error.new({code = 111, reason = "just cause"})
>>>  e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
>>>  ```
>>>  
>>> +User may want to link already existing errors. To achieve this let's add
>>> +`set_prev` method to error object or/and `link` to `box.error` so that one can
>>> +join two errors:
>>> +```
>>> +e1 = box.error.new({code = 111, reason = "just cause"})
>>> +e2 = box.error.new({code = 222, reason = "just cause x2"})
>>> +...
>>> +e2.set_prev(e1) -- e2.prev == e1
>>> +box.error.link(e1, e2) -- e2.prev == e1
>>> +```
>>
>> 1. I don't think we need to change box.error global API. It would be
>> enough to add new methods to error object. box.error.link() and
>> box.error.prev() look redundant. What is a case, when they should
>> be used instead of error object methods?
>>
>> box.error.new() and last() exist because there is no other way to
>> create an error, or to get a last one.
> 
> I've added both since was not sure which one is better.
> Since you'd prefer avoid changing global interface (which is
> reasonable argument) let's leave only e:set_prev():
> 
> diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
> index d57e040ba..ed121770d 100644
> --- a/doc/rfc/1148-stacked-diagnostics.md
> +++ b/doc/rfc/1148-stacked-diagnostics.md
> @@ -178,14 +178,12 @@ e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
>  ```
>  
>  User may want to link already existing errors. To achieve this let's add
> -`set_prev` method to error object or/and `link` to `box.error` so that one can
> -join two errors:
> +`set_prev` method to error object so that one can join two errors:
>  ```
>  e1 = box.error.new({code = 111, reason = "just cause"})
>  e2 = box.error.new({code = 222, reason = "just cause x2"})
>  ...
>  e2.set_prev(e1) -- e2.prev == e1
> -box.error.link(e1, e2) -- e2.prev == e1
>  ```
>  ### Binary protocol

What about box.error.prev()? I don't think we need this one as
well. How will it work anyway? You just call box.error.prev()
multiple times and iterate over the error list? Or it can only
return the second error in the stack. Both ways looks not really
useful. Probably, error:prev() is enough.

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Nikita Pettik]

On 05 Feb 23:07, Vladislav Shpilevoy wrote:
> Thanks for the fix!
> 
> On 05/02/2020 07:16, Nikita Pettik wrote:
> > On 04 Feb 21:48, Vladislav Shpilevoy wrote:
> >> Thanks for the RFC.
> >>
> >> It still does not conform with the template, but ok. I see that
> >> that ship has sailed already, some other RFCs also violate the
> >> template.
> >>
> >> See 2 comments below.
> >>
> >>>> 10. Are we not going to allow to link two existing errors? I imagine
> >>>> it could be simpler and more flexible for a user, than filling
> >>>> one big map in error.new().
> >>>
> >>> Okay, I'm not against it:
> >>>
> >>>  Another way to resolve this issue is to erase diagnostic area before
> >>> @@ -173,13 +158,23 @@ box.error.prev(error) == error.prev
> >>>  ```
> >>>  
> >>>  Furthermore, let's extend signature of `box.error.new()` with new (optional)
> >>> -argument - the 'reason' parent error object:
> >>> +argument - 'prev' - previous error object:
> >>>  
> >>>  ```
> >>>  e1 = box.error.new({code = 111, reason = "just cause"})
> >>>  e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
> >>>  ```
> >>>  
> >>> +User may want to link already existing errors. To achieve this let's add
> >>> +`set_prev` method to error object or/and `link` to `box.error` so that one can
> >>> +join two errors:
> >>> +```
> >>> +e1 = box.error.new({code = 111, reason = "just cause"})
> >>> +e2 = box.error.new({code = 222, reason = "just cause x2"})
> >>> +...
> >>> +e2.set_prev(e1) -- e2.prev == e1
> >>> +box.error.link(e1, e2) -- e2.prev == e1
> >>> +```
> >>
> >> 1. I don't think we need to change box.error global API. It would be
> >> enough to add new methods to error object. box.error.link() and
> >> box.error.prev() look redundant. What is a case, when they should
> >> be used instead of error object methods?
> >>
> >> box.error.new() and last() exist because there is no other way to
> >> create an error, or to get a last one.
> > 
> > I've added both since was not sure which one is better.
> > Since you'd prefer avoid changing global interface (which is
> > reasonable argument) let's leave only e:set_prev():
> > 
> > diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
> > index d57e040ba..ed121770d 100644
> > --- a/doc/rfc/1148-stacked-diagnostics.md
> > +++ b/doc/rfc/1148-stacked-diagnostics.md
> > @@ -178,14 +178,12 @@ e2 = box.error.new({code = 222, reason = "just cause x2", prev = e1})
> >  ```
> >  
> >  User may want to link already existing errors. To achieve this let's add
> > -`set_prev` method to error object or/and `link` to `box.error` so that one can
> > -join two errors:
> > +`set_prev` method to error object so that one can join two errors:
> >  ```
> >  e1 = box.error.new({code = 111, reason = "just cause"})
> >  e2 = box.error.new({code = 222, reason = "just cause x2"})
> >  ...
> >  e2.set_prev(e1) -- e2.prev == e1
> > -box.error.link(e1, e2) -- e2.prev == e1
> >  ```
> >  ### Binary protocol
> 
> What about box.error.prev()? I don't think we need this one as
> well. How will it work anyway? You just call box.error.prev()
> multiple times and iterate over the error list? Or it can only
> return the second error in the stack. Both ways looks not really
> useful. Probably, error:prev() is enough.

Okay, I don't mind this change:

diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
index ed121770d..0e365361b 100644
--- a/doc/rfc/1148-stacked-diagnostics.md
+++ b/doc/rfc/1148-stacked-diagnostics.md
@@ -159,14 +159,14 @@ Tarantool returns a last-set (diag::last) error as `cdata` object from central
 diagnostic area to Lua in case of error. User should be unable to modify it
 (since it is considered to be a bad practice - in fact object doesn't belong
 to user). On the other hand, user needs an ability to inspect a collected
-diagnostic information. Hence, let's extend the `box.error` API with a function
+diagnostic information. Hence, let's extend the error object API with a method
 which provides the way to get the previous error (reason): `:prev()` (and
 correspondingly `.prev` field).
 
 ```
--- Return a reason error object for given error
+-- Return a reason error object for given error object 'e'
 -- (when exists, nil otherwise).
-box.error.prev(error) == error.prev
+e:prev(error) == error.prev
 ```

Re: [Tarantool-patches] [PATCH] box: rfc for stacked diagnostic area

[Vladislav Shpilevoy]

Thanks for the fix!

LGTM now.