[tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 9 comments below.

On 01/08/2019 13:13, Kirill Shcherbatov wrote:
> Part of #1148
> ---
>  doc/rfc/1148-stacked-diagnostics.md | 136 ++++++++++++++++++++++++++++
>  1 file changed, 136 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..a007491aa
> --- /dev/null
> +++ b/doc/rfc/1148-stacked-diagnostics.md
> @@ -0,0 +1,136 @@
> +# Stacked Diagnostics in Tarantool
> +
> +* **Status**: In progress
> +* **Start date**: 30-08-2019

1. Today is 5 August. I guess it is a typo.

> +* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org, Konstantin Osipov @kostja kostja@tarantool.org, Georgy Kirichenko @GeorgyKirichenko georgy@tarantool.org, @Totktonada Alexander Turenko alexander.turenko@tarantool.org
> +* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
> +
> +## Summary
> +Support stacked diagnostics for Tarantool allows to accumulate all occurred errors during processing a request. This allows to better understand what has happened and handle errors
> +correspondingly.
> +
> +## Background and motivation
> +
> +Tarantool statements must produce diagnostic information that populates the diagnostics area. This is a Standard SQL requirement and other vendors and languages also have such feature.
> +Diagnostics area stack must contain a diagnostics area for each nested execution context.
> +
> +### Current Tarantool's error diagnostics
> +Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
> +The last error is exported with `box.error.last()` endpoint.
> +
> +In total there are 5 error classes in Tarantool.
> Each error class is associated with set of error codes (@see ClientError::get_errcode(const struct error *e)).
> +```
> +ClientError      ->     ClientError::errcode() -> ...;
> +OutOfMemory      ->     MEMORY_ISSUE
> +SystemError      ->     SYSTEM
> +CollationError   ->     CANT_CREATE_COLLATION
> +Lua error        ->     PROC_LUA
> +```
> +All system-defined errors are exported with their unique error codes in box.error folder by NAME.
> +
> +You may use box.error.new endpoint to create a new error instance of predefined type:
> +```
> +tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just I can't")
> +tarantool> t:unpack()
> +---
> +- type: ClientError
> +  code: 9
> +  message: 'Failed to create space ''myspace'': just I can''t'
> +  trace:
> +  - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
> +    line: 1
> +```
> +
> +User is allowed to define own errors with any code (including system errors) with
> +```
> +box.error.new{code = user_code, reason = user_error_msg}
> +```
> +where `user_code` must be greater that the biggest registered system error, say `10000`.

2. 'greater than'.

> +
> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()` methods and uniform `.type`, `.message` and `.trace` fields.
> +
> +
> +## Proposal
> +In some cases a diagnostic information must be more complicated.
> +For example, when 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 sets an own,  more specialized error.
> +
> +In this and similar situations, we need stack-based error diagnostics:
> +
> +- The serialize method should return the most recent error in stack:
> +```
> +tarantool> box.error.last()
> +Failed to build a key for functional index ''idx'' of space ''withdata': function error
> +```
> +
> +- The `:unpack()` method must return the complete diagnostics stack - a table of errors sorted in their historical order:
> +```
> +tarantool> box.error.last():unpack()
> +---
> +  - type: LuajitError
> +    message: '[string "return function(tuple)..."]:2: attempt to call global
> +     ''require'' (a nil value)'
> +    trace:
> +    - file: /home/kir/WORK/tarantool/src/lua/utils.c
> +      line: 1000
> +  - type: ClientError
> +    code: 198
> +    message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
> +      function error
> +    trace:
> +    - file: /home/kir/WORK/tarantool/src/box/key_list.c
> +      line: 68
> +```
> +

3. VShard uses unpack: https://github.com/tarantool/vshard/blob/a6826b26f9ab38e338a34cb7dd7f46dabbc67af5/vshard/error.lua#L149
Is it compatible with what you are doing? Here I need to turn an error into a table to
set a different serialization method.

4. Have you considered an idea to unpack errors not as an array, but
as a list? When each error object has a field 'reason' pointing to
another object, and so on. It would allow not to change unpack() output
format - it would still return the newest error, but with a new field.

> +- We need to introduce `next_error:wrap(error)` and  `error:unwrap()` methods.
> +The `wrap` function can take any previous and last error objects to append error to the end of next_error trace. The next_error backtrace is expected to be empty.
> +The `unwrap` function removes the element's parent and returns the unwrapped content. This is effectively the inverse of the `wrap()` method.
> +```
> +Workaround:

5. Workaround for what?

> +
> +function create_new_user()
> +        name, err = pcall(box.internal.func_call,
> +                          box.func.generate_user_name.name)
> +        if not name then
> +                local e = box.error.new({
> +                              code = APP_ERROR_CODE,
> +                              reason = "create_new_user"}):
> +                e:wrap(err):raise()
> +        end
> +        box.schema.user.create(name)
> +end
> +```
> +
> +- We also need `error:has` method to test whether given error object contain given error code:

6. 'object contains'.

> +```
> +box.error.last():has(box.error.LuajitError)

7. Something is wrong.

tarantool> box.error.LuajitError
---
- null
...

> +```
> +
> +## Detailed design
> +
> +Diagnostic area object has a good encapsulated API.
> +
> +Lets extend error object with a pointer to it's reason error and introduce a few new methods (we need to rename diag_add_error to diag_set_error because it replaces an existent diag error in fact):
> +```
> +struct error {
> +...
> +/* A pointer to the reason error. */

8. Broken alignment.

> +	struct error *reason;
> +};
> +
> +/**
> + * Add a new error to the diagnostics area: the previous error
> + * becomes a reason of a current.
> + * \param diag diagnostics area
> + * \param e error to add
> + */
> +static inline void
> +diag_add_error(struct diag *diag, struct error *e);
> +
> +/** Same as diag_set, but softly extend diagnostic information. */
> +#define diag_add(class, ...)
> +```
> +
> +### IPROTO Communication
> +Currently errors are sent as (IPROTO_TYPE_ERROR | errcode) messages with error message string payload.
> +Lets introduce a new IPROTO_TYPE_ERROR_TRACE error code with
> +[[errcode1, "errstring1"], [errcode2, "errstring2"]..] payload, where errors are serialized in historical order.
> 

9. Please, describe the new binary protocol in the similar way as it
is done here: https://github.com/tarantool/tarantool/blob/master/doc/rfc/3328-wire_protocol.md#body
And add it to the docbot request with code values included.