manticoresearch icon indicating copy to clipboard operation
manticoresearch copied to clipboard

Daemon does not pass to Buddy all information about error.

Open Nick-S-2018 opened this issue 1 year ago • 24 comments

In some cases, when error data is a complex object that contains multiple properties, daemon does not pass them all to Buddy.

create table test(f text);

curl -sX POST http://localhost:9308/insert  -d '{
  "index":"test",
  "id":1,
  "doc":
  {
    "f" : "test",
  }
}'

{"_index":"test","_id":1,"created":true,"result":"created","status":201}

curl -sX POST http://localhost:9308/insert  -d '{
  "index":"test",
  "id":1,
  "doc":
  {
    "f" : "test",
  }
}'

In this case, the response without Buddy is:

{"error":{"type":"duplicate id '1'","index":"test"},"status":409}

but with Buddy is:

[{"total":0,"warning":"","error":"duplicate id '1'"}]

We need to pass all data related to the error to Buddy, otherwise it cannot be returned to the client.

Nick-S-2018 avatar Feb 08 '24 07:02 Nick-S-2018

Blocked by https://github.com/manticoresoftware/manticoresearch/issues/1806

Once https://github.com/manticoresoftware/manticoresearch/issues/1806 is done, @Nick-S-2018 pls check if this issue is still actual

sanikolaev avatar Feb 08 '24 09:02 sanikolaev

Blocked by https://github.com/manticoresoftware/manticoresearch/issues/1806

Unblocked as it's done.

sanikolaev avatar Apr 26 '24 08:04 sanikolaev

It's fixed in the in the buddy_http_code branch. We'll merge that into master once we finish the other issues related to the future update of our OpenAPI YAML schema.

Nick-S-2018 avatar May 06 '24 06:05 Nick-S-2018

@Nick-S-2018 is this issue closed by mistake?

We'll merge that into master once we finish the other issues related to the future update of our OpenAPI YAML schema.

Should it be blocked by another one?

sanikolaev avatar May 08 '24 11:05 sanikolaev

Yes, it's been blocked by https://github.com/manticoresoftware/manticoresearch/issues/1808 as well. I've reopened it now.

Nick-S-2018 avatar May 21 '24 11:05 Nick-S-2018

We need daemon to pass extra info about error to Buddy, similarly to how daemon passes it to user without Buddy.
I.e., in the following example, info from type, index (if exists) , and status fields needs to be passed to Buddy as parts of the request as well.

curl -sX POST http://localhost:9408/insert  -d '{
  "index":"test",
  "id":-1,
  "doc":
  {
    "f" : "a"
  }
}'
{
  "error": {
    "type": "parse_exception",
    "reason": "Negative document ids are not allowed",
    "index": "test"
  },
  "status": 400
}

Nick-S-2018 avatar May 21 '24 12:05 Nick-S-2018

I checked daemons from the master version and the head of the buddy_http_code branch with buddy running and without and all the time for the requests with the body

{
  "index":"test",
  "id":-1,
  "doc":
  {
    "f" : "a"
  }
}

I alway see the simple error without any objects and so one

C:\dev\sphinx\build\m_dbg22\src\Debug\txt>curl -sX POST "localhost:9312/insert" -H "content-type: application/json" --data-binary @ins-1807.js    | jq
{
  "error": "Negative document ids are not allowed"
}

that is why it is not clear what should I pass to buddy. I do not get any of the type, index (if exists) , and status fields

tomatolog avatar May 30 '24 19:05 tomatolog

I forgot to mention the tests should be made with the https://github.com/manticoresoftware/manticoresearch/tree/doc_post_error branch

Nick-S-2018 avatar May 31 '24 08:05 Nick-S-2018

I use daemon from the https://github.com/manticoresoftware/manticoresearch/tree/doc_post_error but with master version of buddy daemon failed to start with the error message

WARNING: [BUDDY] invalid output, should be 'Buddy ver, started address:port', got 'Error while initialization: Call to protected method Manticoresearch\Buddy\Core\Plugin\Pluggable::registerHooks() from scope Manticoresearch\Buddy\Base\Lib\QueryProcessor

still not clear what buddy version should I use for this ticket

tomatolog avatar Jun 04 '24 07:06 tomatolog

I've tested the branch version with these versions from branch's deps.txt - try to test using them:

buddy 2.3.1 24032906 541c941
executor 1.1.1 24030109 3a4d6c4

Nick-S-2018 avatar Jun 12 '24 10:06 Nick-S-2018

pushed into https://github.com/manticoresoftware/manticoresearch/tree/doc_post_error branch the fix https://github.com/manticoresoftware/manticoresearch/commit/371e324d714082c3785a9b0076b2731f5b8c0a2d that we discussed with @Nick-S-2018 .

Now daemon adds new error_body property to the request to buddy there the whole HTTP body of the reply that daemon sends to client. The buddy can parse and extract all needed fields from the error_body. As collect separate type \ index \ status types for every HTTP handlers need the large code refactoring.

tomatolog avatar Jun 17 '24 14:06 tomatolog

I've tried creating a PR to build the packages, so it's easier to test, but there are conflicts with the master branch - https://github.com/manticoresoftware/manticoresearch/pull/2317 They perhaps need to be resolved.

sanikolaev avatar Jun 18 '24 03:06 sanikolaev

I suggest implementing a change in the protocol. Instead of creating another field that may or may not be used, we should consider utilizing the error field.

I propose using the following structure to make it simpler:

"error": {
  "message": "string value with error",
  "body": "JSON-encoded extra value" or null
}

I'm not sure if "body" is the best name for what we're writing there. We may also consider using "info" or "details" instead. Suggestions are welcome.

Once we agree and proceed with this, we should also update our protocol here: https://github.com/manticoresoftware/manticoresearch-buddy?tab=readme-ov-file#communication-protocol-v2

On the Buddy side, we should improve the implementation and try to encapsulate the Error into a struct to avoid if-else statements. This way, we'll simply be able to get the message or bodyStruct and do what we need. Currently, in the pull request, we use if-else statements in the error handling section, which may be hard to read and maintain in the future.

@Nick-S-2018 @tomatolog @sanikolaev What your thoughts?

donhardman avatar Aug 29 '24 06:08 donhardman

at the dev call all agree this change is the best we could try

tomatolog avatar Sep 02 '24 07:09 tomatolog

pushed into https://github.com/manticoresoftware/manticoresearch/tree/doc_post_error branch the fix https://github.com/manticoresoftware/manticoresearch/commit/7c73ff6162d2322d945ecf23098bcadc4edd6deb with the change that request to buddy has error object with the message error string and the body object or null that is the whole reply of the daemon, ie now the request to buddy looks like

{
  "type":"unknown json request",
  "error":{
    "message":"\"index\" property value should be a string",
    "body":{
      "error":"\"index\" property value should be a string"
    }
  },
  "version":2,
  "message":{
    "path_query":"/search",
    "body":"{ \"index\" : {\"_index\":\"test\"} }{\"@timestamp\": \"1981-11-16\"}",
    "http_method":"POST"
  }
}

as discussed here https://github.com/manticoresoftware/manticoresearch/issues/1807#issuecomment-2316814597

tomatolog avatar Sep 09 '24 12:09 tomatolog

@tomatolog

"version":2,

Shouldn't it be version 3?

sanikolaev avatar Sep 10 '24 07:09 sanikolaev

yes That also should be fixed to ver 3 along with documenting the change

tomatolog avatar Sep 10 '24 07:09 tomatolog

Implemented in Buddy in https://github.com/manticoresoftware/manticoresearch-buddy/pull/341 @donhardman please, review it

Nick-S-2018 avatar Sep 19 '24 08:09 Nick-S-2018

I left my comments

donhardman avatar Sep 24 '24 10:09 donhardman