Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Detailed exception data for InternalServerError #210

Open
Mark-ICS opened this issue Oct 17, 2023 · 3 comments
Open

[FEATURE] Detailed exception data for InternalServerError #210

Mark-ICS opened this issue Oct 17, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@Mark-ICS
Copy link

Is your feature request related to a problem?

I am using the client to communicate with an OpenSearch instance that uses the ML Commons plugin.

With the ML commons plugin, the model must be loaded each time the OpenSearch server restarts. The model can only be loaded via an API request.

If I make a query using a model id for a model that is not loaded, the client throws the following exception:

OpenSearch::Transport::Transport::Errors::InternalServerError

With the message:

[500] {"error":{"root_cause":[{"type":"m_l_exception","reason":"model not loaded"}],"type":"m_l_exception","reason":"model not loaded"},"status":500}

What solution would you like?

While the information in the exception message does contain the relevant information, it's clunky because I need to trim the [500] in order to parse the JSON in the message.

Would would be ideal, is if the exception had a status attribute and parsed_data which contained the response as a hash containing the parsed data.

An example of usage would be:

def my_search_method(index, body)
  client.search(index: index, body: body)
rescue OpenSearch::Transport::Transport::Errors::InternalServerError => e
  if e.status == 500 && e.parsed_data["error"]["root_cause"][0]["reason"] == 'model not loaded'
    load_model
  end
end

What alternatives have you considered?

A clear and concise description of any alternative solutions or features you've considered.

The alternative is to trim the [500] from the beginning and the parse the rest. This would work but it's prone to errors and also a bit clunky.

Do you have any additional context?

Hopefully I've provided enough context above but happy to clarify and questions.

Also, if it turns out I'm being an idiot and there is already a better way to do this that I'm not thinking of, I'd be grateful for the feedback 😅

@Mark-ICS Mark-ICS added enhancement New feature or request untriaged labels Oct 17, 2023
@dblock
Copy link
Member

dblock commented Oct 17, 2023

If all server-side exceptions have the error structure, you probably want to be able to do e.error.root_causes.first.reason.

@Mark-ICS
Copy link
Author

That makes sense @dblock. Just to clarify though, are you suggesting that in terms of design, or should that currently work?

@dblock
Copy link
Member

dblock commented Oct 17, 2023

In terms of design, someone should implement this (wink wink :)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants