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

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

Open
croensch opened this issue Apr 11, 2023 · 4 comments
Open

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

croensch opened this issue Apr 11, 2023 · 4 comments
Assignees

Comments

@croensch
Copy link

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

  • Linux
  • PHP 8.1
  • ES-PHP 8.x
  • Elasticsearch 8.x
@ezimuel
Copy link
Contributor

ezimuel commented Apr 11, 2023

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

@ezimuel ezimuel self-assigned this Apr 11, 2023
@croensch croensch changed the title Library should make an effor to report low-level network errors Library should make an effort to report low-level network errors Apr 14, 2023
@croensch
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

ezimuel commented Apr 17, 2023

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

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

No branches or pull requests

2 participants