elastic / elasticsearch-php

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

Library should make an effort to report low-level network errors #1308

Open croensch opened 1 year ago

croensch commented 1 year ago

Summary of problem or feature request

The transport or pool should make an effort to report low-level network errors that lead to NoNodeAvailableException.

There have been a couple of issues and stackoverflow questions, that often ultimately resolved in "My nodes ARE available - it's the client that can't connect" often due to misconfiguration or circumstances (wrong host, self-signed HTTPS, Firewalls, Proxies, etc.). Since this library uses cURL by default it would be proper to wrap the cURL error into this exception.

Code snippet of problem

This does not help:


        try {
            $info = $this->esClient->info();
            $this->logger->info('ES: Info', ['info' => $info->asArray()]);
        } catch (NoNodeAvailableException $exception) {
            $lastResponse = $this->esClient->getTransport()->getLastResponse();
            $status = $lastResponse->getStatusCode();
            $reason = $lastResponse->getReasonPhrase();
            $contents = $lastResponse->getBody()->getContents();
            $this->logger->error('ES: Info not available', ['exception' => $exception, 'status' => $status, 'reason' => $reason, 'contents' => $contents]);
        } catch (ElasticsearchException $e) {
            $this->logger->warning('ES: Info not retrieved', ['exception' => $e]);
        }

Because Transport.lastResponse is not set.

System details

ezimuel commented 1 year ago

@croensch can you provide an example for the NoNodeAvailableException error? I think we can propose to improvement the error message in the elastic-transport-php library, since the NoNodeAvailableException is trown here. Thanks!

croensch commented 1 year ago

I did not realize it's a separate library. Also i did not stumble upon the logging. Let me think if this enough IMO.

croensch commented 1 year ago

Perhaps, the last NetworkExceptionInterface, if it exists, should be added as previous exception.

-        throw new NoNodeAvailableException($exceededMsg);
+        throw new NoNodeAvailableException($exceededMsg, 0, $e ?? null);

There are a couple more places, i might create a PR to use this pattern for all of them.

ezimuel commented 1 year ago

@croensch thanks, let me know if you need some help.