-
-
Notifications
You must be signed in to change notification settings - Fork 232
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
Add support for configuring Gelf encoders in Monolog configuration #492
base: master
Are you sure you want to change the base?
Conversation
This commit introduces the ability to specify custom encoders (`json` and `compressed_json`) for Gelf publishers in Monolog's configuration.
…ExistsCheck Replaced instances of \stdClass in class_alias calls with DummyClassForClassExistsCheck as before php 8.3 only user created class can be aliased.
Well class_alias(\stdClass::class, 'Gelf\Transport\UdpTransport'); in the tests obviously does not work in <8.3 😓 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR. Can you elaborate a bit more why I should care about which encoder is used? If the protocol allows both and thus a backend implementing the protocol should support both, I don't understand why it makes a difference for my app.
I also wonder how common your problem is. If this only affects a handful of projects, we could recommend to configure the publisher service manually and configure a service ID instead of connection details.
@@ -50,7 +50,12 @@ | |||
* - [bubble]: bool, defaults to true | |||
* | |||
* - gelf: | |||
* - publisher: {id: ...} or {hostname: ..., port: ..., chunk_size: ...} | |||
* - publiser: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* - publiser: | |
* - publisher: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo fixed 😄
* - id: string, service id of a publisher implementation, optional if hostname is given | ||
* - hostname: string, optional if id is given | ||
* - [port]: int, defaults to 12201 | ||
* - [chunk_size]: int, defaults to 1420 | ||
* - [encoder]: string, its value can be 'json' or 'compressed_json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before your change, it was clear that I should either provide a service ID or configure the connection by specifying the hostname, port etc. The host name is not "optional if id is given" as you wrote, it's ignored entirely if a service ID has been configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I like how it was before, since it is clear and sound, however I it lacks details about the options (type, enum options, defaults, etc) and I wanted to provide more verbose information especially for the new option as it is has predefined values. btw I tried to keep consistent with the other docs, this comment is reused from the mongo section. 😃
Would you add comment just like you said "ignored when service ID has been configured" to all of these options?
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. |
That makes perfect sense. Thank you for the explanation. |
This commit introduces the ability to specify custom encoders (
json
andcompressed_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.
bzikarsky/gelf-php@f2625cf#diff-9520be6ea98f5ded11cbe62c500f1a8fef819810d8836d2b15189cdbc073900cR32
bzikarsky/gelf-php@f2625cf#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:
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 :)