Wish: GelfPublisher should have an option to ignore transport errors

[jdoose]

I am using the GelfPublisher with UdpTransport in a website combination with Monolog to log to an logstash/elasticsearch server (ELK) and it is working great!

There is only one thing missing:
If a target server is not available the logging attempt terminates with a PHP error.
In this case I would prefer to have my website do its function as before, even if Gelf logging is not available.
This could be configured using an "ignore_error" option.

To avoid the need to do it in every transport it could be done in Publisher::publish if the foreach-loop is changed like this:

foreach ($this->transports as $transport) {
/* @var $transport TransportInterface */

    try {
        $transport->send($message);
    } catch (ExceptionInterface $e) {
        if (!$this->options['ignore_error']) {
            throw new \RuntimeException("Error sending messages to transport", 0, $e);
        }
    }
}

Or, if it is preferred to enable this per transport it could go in AbstractTransport

[bzikarsky]

Thank you for your input!

I agree: A logger should be able to fail silently. I put a little thought into this, and my current idea is:

I'll add "SilentPublisher"-class which wraps (or inherits from -- haven't decided on this yet) a normal Publisher. In case any transport fails it'll catch the exception and optionally logs both the error and the original message to another Psr\Logger.

My reasoning is this:

1. Using a separate class instead of a ignore_errors option: I don't want to clutter the interface/implementation with more and more options. Those usually are problematic in regards to backwards compatibility breaks some time in the future.

2. The option to provide an additional Psr\Logger allows "paranoid" people to store the logs and the information about e.g. a network failure in a better location (filesystem, syslog, ...)

What do you think about this approach?

[jdoose]

I agree that a separate class would be nice for clarity.

A wrapper of a regular publisher would have the disadvantage that it would only possible to catch an exception in the method calling Publisher::publish. In a case of only one transport it would be fine. In a case of multiple transports I would prefer a solution that would continue calling remaining transports after an error.
That would mean you would have to duplicate the current implementation of Publisher::publish. I would like to avoid that.

What would you think of a wrapper to a transport? That wrapper could ignore errors and could be used on any transport. I.e. Implementation of a class implementing AbstractTransport. It would be possible to give that new class an instance of another AbstractTransport implementation class. The wrapper would try-catch the send method?

Usage would be like:

$transport = new Gelf\Transport\UdpTransport(...)

$silentTransport = new IgnoreErrorTransportWrapper($transport);

$publisher = new GelfPublisher($silentTransport);

[bzikarsky]

Implemented in #60, thanks @wildpascal!

[jdoose]

Great work! Thanks @wildpascal!

[notFloran]

Hi @bzikarsky @wildpascal,

Thanks for the IgnoreErrorTransportWrapper but I don't find the class in the current stable release. It seems that the PR was closed instead of being merged.

[Gummibeer]

@notFloran it is in the latest release: https://github.com/bzikarsky/gelf-php/blob/1.5.0/src/Gelf/Transport/IgnoreErrorTransportWrapper.php and the PR #60 was merged not closed.

[bzikarsky]

Not completely correct, I did this in reaction to @notFloran's post. 😉 -- Thanks for the headsup!

[notFloran]

Thanks @bzikarsky !