monolog-bundle icon indicating copy to clipboard operation
monolog-bundle copied to clipboard

Add support for configuring Gelf encoders in Monolog configuration

Open Tib-z opened this issue 1 year ago • 3 comments

This commit introduces the ability to specify custom encoders (json and compressed_json) for Gelf publishers in Monolog's configuration.

Overview This PR introduces an enhancement to the Symfony Monolog Bundle by adding a configuration option for setting the message encoder for the GELF handler's publisher. This change addresses a backward compatibility issue introduced in the 2.0 update of the graylog2/gelf-php package, which shifted the default message encoder from CompressedJsonEncoder to JsonEncoder. Thus an upgrade silently breakes the logging.

https://github.com/bzikarsky/gelf-php/commit/f2625cf008289e9a5004bf55eaf6e95dcbcbc6c0#diff-9520be6ea98f5ded11cbe62c500f1a8fef819810d8836d2b15189cdbc073900cR32 https://github.com/bzikarsky/gelf-php/commit/f2625cf008289e9a5004bf55eaf6e95dcbcbc6c0#diff-72a9d13c23b7d615833219f0dfc92be317d9a4981de7b4747e287db62aaa1079L69

In a Symfony project, to override this default behavior, users had to manually construct and define the transport, publisher, and encoder as a service and then use it in the Monolog configuration. This approach, while functional, is cumbersome. It would be more user-friendly to enable setting the encoder via a simple configuration option. The proposed change would be:

            gelf:
                type: gelf
                level: debug
                publisher:
                    hostname: '%env(resolve:LOGSTASH_HOSTNAME)%'
                    port: '%env(resolve:LOGSTASH_PORT)%'
                    encoder: compressed_json  # Optional: 'json' or 'compressed_json'

This new encoder configuration is optional and does not have an explicit default value, thereby preserving the existing behavior (albeit the recent default may not align with prior expectations).

While it might be tempting to expose more variability (like using class names or service IDs directly), overly customizable options may not add practical value and could complicate the configuration unnecessarily, 'coz YAGNI :)

Tib-z avatar Nov 03 '24 11:11 Tib-z

Well class_alias(\stdClass::class, 'Gelf\Transport\UdpTransport'); in the tests obviously does not work in <8.3 😓 Generally, workarounds involving class existence checks are hacky, but I wanted to ensure the tests provided meaningful coverage.

To address the compatibility issue, I've added a simple dummy class as a workaround. Another option could be creating the class dynamically with eval, though that also smells... A potential refactor might involve wrapping class_exists checks and mocking them, but I'm unsure if the benefit would justify the effort.

Let me know your thoughts

Tib-z avatar Nov 03 '24 13:11 Tib-z

In our infra, in the logstash pipeline comporessed json is a contraint, does not work without comporession, so the change in the gelf package broke the logging. Likely it's a specific setup, not sure exactly how is configured, I'll try to figure out (it's not maintained by me/my team) and see if it could affect others as well.

As the encoder is a dependency and straightforward to change it, it was logical move to expose it as a monolog config option and make the implementation easier. In a microservice architecture with 30-40 services, this approach significantly reduces the need for repetitive boilerplate service definitions in favor of a single configuration option. That said, I understand your point about using the service ID workaround as an alternative.

Tib-z avatar Nov 04 '24 19:11 Tib-z

That makes perfect sense. Thank you for the explanation.

derrabus avatar Nov 04 '24 20:11 derrabus

Thanks, I've tested the changes and everything seems to work as expected. Just have a few minor comments.

Thanks for the review.

Tib-z avatar Sep 17 '25 05:09 Tib-z

Thank you @Tib-z.

GromNaN avatar Oct 02 '25 10:10 GromNaN