manticoresoftware / manticoresearch

Easy to use open source fast database for search | Good alternative to Elasticsearch now | Drop-in replacement for E in the ELK soon
https://manticoresearch.com
GNU General Public License v3.0
8.83k stars 489 forks source link

Daemon does not pass to Buddy all information about error. #1807

Open Nick-S-2018 opened 7 months ago

Nick-S-2018 commented 7 months ago

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.

sanikolaev commented 7 months ago

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 commented 4 months ago

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

Unblocked as it's done.

Nick-S-2018 commented 4 months ago

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.

sanikolaev commented 4 months ago

@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?

Nick-S-2018 commented 3 months ago

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

Nick-S-2018 commented 3 months ago

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
}
tomatolog commented 3 months ago

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

Nick-S-2018 commented 3 months ago

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

tomatolog commented 3 months ago

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

Nick-S-2018 commented 2 months ago

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
tomatolog commented 2 months ago

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.

sanikolaev commented 2 months ago

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.

donhardman commented 1 week ago

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?

tomatolog commented 6 days ago

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