elastic / elasticsearch-php

Official PHP client for Elasticsearch.
https://www.elastic.co/guide/en/elasticsearch/client/php-api/current/index.html
MIT License
5.26k stars 965 forks source link

Ping with exceptions #1106

Open joshua-bn opened 3 years ago

joshua-bn commented 3 years ago

Summary of problem or feature request

Ping currently returns a boolean after swallowing the exceptions. I'd like to know why it failed so I can log that and take appropriate actions.

Code snippet of problem

Instead of this

    /**
     * @param $params array Associative array of parameters
     *
     * @return bool
     */
    public function ping($params = [])
    {
        /** @var callback $endpointBuilder */
        $endpointBuilder = $this->endpoints;

        /** @var \Elasticsearch\Endpoints\Ping $endpoint */
        $endpoint = $endpointBuilder('Ping');
        $endpoint->setParams($params);

        try {
            $this->performRequest($endpoint);
        } catch (Missing404Exception $exception) {
            return false;
        } catch (TransportException $exception) {
            return false;
        } catch (NoNodesAvailableException $exception) {
            return false;
        }

        return true;
    }

I want this:

    /**
     * @param $params array Associative array of parameters
     *
     * @return void
     * @throws \Elasticsearch\Common\Exceptions\ElasticsearchException
     */
    public function pingCheck($params = [])
    {
        /** @var callback $endpointBuilder */
        $endpointBuilder = $this->endpoints;

        /** @var \Elasticsearch\Endpoints\Ping $endpoint */
        $endpoint = $endpointBuilder('Ping');
        $endpoint->setParams($params);

        $this->performRequest($endpoint);
    }

Then, to save code, you can have ping() wrap pingCheck() (or whatever name). ping() would just catch the exceptions from pingCheck():

    public function ping($params = [])
    {
        try {
            $this->pingCheck($params);
        } catch (Missing404Exception $exception) {
            return false;
        } catch (TransportException $exception) {
            return false;
        } catch (NoNodesAvailableException $exception) {
            return false;
        }

        return true;
    }

System details

joshua-bn commented 3 years ago

I think the current version requires PHP 7.2 or something, so you could just use a union on all of those types or even just catch ElasticsearchException instead.

If there's a workaround for this for the time being, I'd love to hear it.

ezimuel commented 2 years ago

@joshua-bn I'm so sorry for the delay of my answer. If you want to know more about the ping result you can read the lastConnection from the client transport, as follows:

try {
    $result = $client->ping();
} catch (NoNodesAvailableException $e) {
    // no nodes available (ES is offline?)
}

$conn = $client->transport->getLastConnection();
$response = $conn->getLastRequestInfo()['response'];

print_r($response['status']); // HTTP status code
print_r($response['headers']); // HTTP headers
print_r($response['body']); // HTTP response body
print_r($response['curl']['error']); // specific error message from cURL

We are working on the next 8.x version of the client and we will use the elastic/transport library that is PSR-7 and PSR-18 compliant. This means, you will be able to introspect the PSR-7 response on each endpoint result, using an approach like the one that we provided for elastic/enterprise-search-php here.

Let me know if you need more information and sorry again for having miss this issue.