Enable the Client to keep requests

[emilgodsk]

Currently the Client only saves the latest request made using the Client, but if you are interested in seeing all the requests made by the client in its lifetime, there is no real way of getting this data, without enabling the logger, which logs the requests and responses in the way that this library has specified.

With this Pull-Request, you can enable the client to keep an array of requests made using the Client. This will be useful for debugging and testing purposes mainly, or for profiling purposes.

Please let me know what you think about this :)

Feel free to comment, review and edit if needed.

Regards,
Emil G.

[ezimuel]

@egodsk thanks for the PR, I see your point but actually you can read all the HTTP requests and responses made by the client using the Logger feature. You can use the information stored by the logger for monitoring/debugging your application, this is the main reason for logging.
The same information that you proposed to store in Transport::keptRequests array are available in the log and you don't need to change the code for debug. Moreover, keptRequests is stored in memory only for the time that your PHP script is running and you need to store/print it in order to debug, at the end you are providing a logger here.
Do you have some special case that you cannot debug using the log information that we have already in place?

[emilgodsk]

Hi @ezimuel. Thank you for your reply. I see your thinking with the logger, however the problem I see with only using the logger is that, the information that is printing and the way that it is printed, is determined by the way that it is hardcoded in the library. Ex. when a request is made it will write the following to the logger: es.DEBUG: Request Body ["{\"query\":{\"match\":{\"user\":1}}}"] [] (with my logger named "es"), followed by a new log line with es.INFO: Request Success: {"method":"GET","uri":"http://localhost:9200/tests/test/_search","headers":{"Host":["localhost:9200"],"Content-Type":["application/json"],"Accept":["application/json"]},"HTTP code":200,"duration":0.236729} []. What if I do not care about the logger writing "Request Body" or if I would like the logger to write both of these in one line in the logger, instead of it being split across two different lines? In the code the Connection->logRequestSuccess is responsible for correctly logging request, but when looking at the code, it is completely hardcoded the way that the library handles the printing of the information.

I'm aware that the keptRequests only is stored in memory, and will be deleted at the end of the life of the Client, however the use case that I see this useful for, is for debugging, if you wish to log or check on the requests and responses made with Client and displaying it in a certain way, or if you are only interested in the time the response took, that you can retrieve that without all of the other information that is being printed to the log. A specific example would be, that if I run a script and the Client is making multiple searches with different queries, and I would like to check at the end of the script, which requests where made, with what body params and their response body, and then analyse on that code afterwards in the script, or return back to the user calling the script, that all of the requests made during the script took X milliseconds, their request body params were as follows, etc. How would I get a record of the requests that has been made using the Client?

I'm open to suggestion in ways other then introducing an array for keeping the requests, as to how, one would be able to easily retrieve and work on the requests made, but I'm unaware of other ways to do it? If you can suggest another approach, I will happily try and make a new PR with the suggestion :).

Regards,
Emil G.

[polyfractal]

Few things:

• If you're wanting low-level stats about the request, you can toggle verbose on the request to get a bunch of curl details

$params = [
  // all the normal request details here
];

// toggle verbosity
$params['client']['verbose'] = true;

$response = $client->search($params);
print_r($response);

[...,
'transfer_stats' => [
    'url'                     => '...',
    'content_type'            => 'application/json; charset=UTF-8',
    'http_code'               => 200,
    'header_size'             => 144,
    'request_size'            => 419,
    'filetime'                => -1,
    'ssl_verify_result'       => 0,
    'redirect_count'          => 0,
    'total_time'              => 60.845132,
    'namelookup_time'         => 0.0013470000000000001,
    'connect_time'            => 0.029269,
    'pretransfer_time'        => 0.16522499999999996,
    'size_upload'             => 0.0,
    'size_download'           => 0.0,
    'speed_download'          => 0.0,
    'speed_upload'            => 0.0,
    'download_content_length' => 1907.0,
    'upload_content_length'   => 0.0,
    'starttransfer_time'      => 60.845117999999999,
    'redirect_time'           => 0.0,
    'redirect_url'            => '',
    'primary_ip'              => 'XXXXXXX',
    'certinfo'                => [],
  ],
...
]

• Regarding logging request and responses for later analysis, this feels like something that is better handled in the domain of your application? E.g. the application knows what requests it is feeding to the client, and what responses are given back. Similarly, it can measure the total round-trip request time fairly easily from "outside" the client.

  The logging built into the client is for rudimentary debugging (am I talking to the right server? Does this PHP query look like the JSON I'm expecting?), not so much detailed transaction playbacks. That feels like something that should be handled outside the client imo.

• If you'd like more control over the logging itself, you can provide your own Connection and ConnectionFactory classes which override methods like logRequestSuccess to format in the manner you like

• Finally, if you want to keep a complete playback history of which nodes are talked to at what point, you could provide your own ConnectionPool class that tracks/logs which connections are returned to the transport.

Just throwing out some other options, not saying we do/don't want some kind of more robust tracking in the client. I'll leave that to @ezimuel :)

[ezimuel]

@egodsk I agree with @polyfractal we have already different ways to debug the HTTP request/response for this library. I continue to see the logging the best way to go. The question become if we can facilitate the customization of the logging format, as you pointed out.

Right now, the logging is provided using the functions:

• Connection::logRequestSuccess()
• Connection::logRequestFail().

These functions have a lot of parameters, maybe we can simplify it using just $request, $response and $exception and delegate the log format inside. @polyfractal already suggested a way to extend the Connection with a custom implementation for logRequestSuccess(). This will looks as follows:

use Elasticsearch\ClientBuilder;
use Elasticsearch\Connections\Connection;
use Elasticsearch\Connections\ConnectionFactory;

class MyConnection extends Connection
{
    public function logRequestSuccess($request, $response, $exception) {
        // custom log
    }
}
class MyConnectionFactoy extends ConnectionFactory
{
    public function create($hostDetails) {
        return new MyConnection(
            $this->handler,
            $hostDetails,
            $this->connectionParams,
            $this->serializer,
            $this->logger,
            $this->tracer
        );
    }
}

$client = ClientBuilder::fromConfig([
    'ConnectionFactory' => MyConnectionFactory::class
]);

We can definitely work to simplify the customization of the logging messages in the future, maybe using some interface or callable for logRequestSuccess or logRequestFail.
@egodsk do you want to work on this direction? We can start to simplify the parameters in logRequestSucess and logRequestFailt. Let me know, thanks!

[emilgodsk]

Hi @ezimuel and @polyfractal. Thank you for your feedback. You all make some great points.

I'm going to close this pull request and start a little on the work in the direction suggested by @ezimuel , by starting to simply the logRequest and see, if it can be easily later be customized.