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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add support for configuring Gelf encoders in Monolog configuration #492

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fba862a
Add support for custom Gelf encoders in Monolog configuration
Nov 3, 2024
e0a7656
Fix compatibility issue, update class_alias to use DummyClassForClass…
Nov 3, 2024
9ec2b46
Fix typo in configuration comment
Nov 4, 2024
ff5cf01
Remove tests that fake Gelf classes
Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,12 @@
* - [bubble]: bool, defaults to true
*
* - gelf:
* - publisher: {id: ...} or {hostname: ..., port: ..., chunk_size: ...}
* - publiser:
Tib-z marked this conversation as resolved.
Show resolved Hide resolved
* - 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'
Comment on lines +54 to +58
Copy link
Member

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.

Copy link
Author

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

 * - publisher: (one of the following configurations)
 *      # Option 1: Service-based configuration
 *      - id: string, service id of a publisher implementation
 *      
 *      # Option 2: Direct connection configuration
 *      - hostname: string, server hostname
 *      - [port]: int, server port (default: 12201)
 *      - [chunk_size]: int, UDP packet size (default: 1420)
 *      - [encoder]: string, encoding format ('json' or 'compressed_json')

WDYT about this approach?

* - [level]: level name or int value, defaults to DEBUG
* - [bubble]: bool, defaults to true
*
Expand Down Expand Up @@ -816,6 +821,7 @@ private function addGelfSection(ArrayNodeDefinition $handerNode)
->scalarNode('hostname')->end()
->scalarNode('port')->defaultValue(12201)->end()
->scalarNode('chunk_size')->defaultValue(1420)->end()
->scalarNode('encoder')->end()
->end()
->validate()
->ifTrue(function ($v) {
Expand Down
19 changes: 19 additions & 0 deletions DependencyInjection/MonologExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use Symfony\Bridge\Monolog\Processor\SwitchUserTokenProcessor;
use Symfony\Bridge\Monolog\Processor\TokenProcessor;
use Symfony\Bridge\Monolog\Processor\WebProcessor;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
use Symfony\Component\Config\FileLocator;
use Symfony\Component\DependencyInjection\Argument\BoundArgument;
use Symfony\Component\DependencyInjection\ChildDefinition;
Expand Down Expand Up @@ -227,10 +228,28 @@ private function buildHandler(ContainerBuilder $container, $name, array $handler
]);
$transport->setPublic(false);

if (isset($handler['publisher']['encoder'])) {
if ('compressed_json' === $handler['publisher']['encoder']) {
$encoderClass = 'Gelf\Encoder\CompressedJsonEncoder';
} elseif ('json' === $handler['publisher']['encoder']) {
$encoderClass = 'Gelf\Encoder\JsonEncoder';
} else {
throw new InvalidConfigurationException('The gelf message encoder must be either "compressed_json" or "json".');
}

$encoder = new Definition($encoderClass);
$encoder->setPublic(false);

$transport->addMethodCall('setMessageEncoder', [$encoder]);
}

$publisher = new Definition('Gelf\Publisher', []);
$publisher->addMethodCall('addTransport', [$transport]);
$publisher->setPublic(false);
} elseif (class_exists('Gelf\MessagePublisher')) {
if (isset($handler['publisher']['encoder']) && 'compressed_json' !== $handler['publisher']['encoder']) {
throw new InvalidConfigurationException('The Gelf\MessagePublisher publisher supports only the compressed json encoding. Omit the option to use the default encoding or use "compressed_json" as the encoder option.');
}
$publisher = new Definition('Gelf\MessagePublisher', [
$handler['publisher']['hostname'],
$handler['publisher']['port'],
Expand Down
22 changes: 22 additions & 0 deletions Tests/DependencyInjection/ConfigurationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,28 @@ public function testGelfPublisherService($publisher)
$this->assertEquals('gelf.publisher', $config['handlers']['gelf']['publisher']['id']);
}

public function testGelfPublisherWithEncoder(): void
{
$configs = [
[
'handlers' => [
'gelf' => [
'type' => 'gelf',
'publisher' => [
'hostname' => 'localhost',
'encoder' => 'compressed_json',
],
],
],
],
];

$config = $this->process($configs);

$this->assertEquals('localhost', $config['handlers']['gelf']['publisher']['hostname']);
$this->assertEquals('compressed_json', $config['handlers']['gelf']['publisher']['encoder']);
}

public function testArrays()
{
$configs = [
Expand Down
16 changes: 16 additions & 0 deletions Tests/DependencyInjection/Fixtures/DummyClassForClassExistsCheck.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\MonologBundle\Tests\DependencyInjection\Fixtures;

class DummyClassForClassExistsCheck
{
}
92 changes: 92 additions & 0 deletions Tests/DependencyInjection/MonologExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Symfony\Bundle\MonologBundle\Tests\DependencyInjection\Fixtures\AsMonologProcessor\FooProcessor;
use Symfony\Bundle\MonologBundle\Tests\DependencyInjection\Fixtures\AsMonologProcessor\FooProcessorWithPriority;
use Symfony\Bundle\MonologBundle\Tests\DependencyInjection\Fixtures\AsMonologProcessor\RedeclareMethodProcessor;
use Symfony\Bundle\MonologBundle\Tests\DependencyInjection\Fixtures\DummyClassForClassExistsCheck;
use Symfony\Bundle\MonologBundle\Tests\DependencyInjection\Fixtures\ServiceWithChannel;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
use Symfony\Component\DependencyInjection\ContainerBuilder;
Expand Down Expand Up @@ -186,6 +187,97 @@ public function testExceptionWhenUsingGelfWithoutPublisherHostname()
$loader->load([['handlers' => ['gelf' => ['type' => 'gelf', 'publisher' => []]]]], $container);
}

public function testExceptionWhenUsingLegacyGelfImplementationWithUnsupportedEncoder(): void
{
if (!class_exists('Gelf\MessagePublisher')) {
class_alias(DummyClassForClassExistsCheck::class, 'Gelf\MessagePublisher');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing such class aliases is a hack, and won't work properly. If the testExceptionWhenUsingGelfWithInvalidEncoder test is run first, this test will never reach the code path guarded by class_exists('Gelf\MessagePublisher') (because the actual guard to reach that code path is !class_exists('Gelf\Transport\UdpTransport') && class_exists('Gelf\MessagePublisher') and you cannot remove the other alias).

Tests should be written against the installed dependencies, without such hacks, and skipped in case they are not relevant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback; I completely agree. While this approach is hackish, it did feel wrong that the changes couldn’t be properly tested. I initially used these as part of my testing process and I left them in the PR...

As I mentioned in my previous comment:

Generally, workarounds involving class existence checks are hacky, but I wanted to ensure the tests provided meaningful coverage.

I'll remove them.

}

$container = new ContainerBuilder();
$loader = new MonologExtension();

$this->expectException(InvalidConfigurationException::class);

$loader->load([['handlers' => ['gelf' => ['type' => 'gelf', 'publisher' => ['hostname' => 'localhost', 'encoder' => 'json']]]]], $container);
}

/**
* @dataProvider encoderOptionsProvider
*/
public function testLegacyGelfImplementationEncoderOption(array $config): void
{
if (!class_exists('Gelf\MessagePublisher')) {
class_alias(DummyClassForClassExistsCheck::class, 'Gelf\MessagePublisher');
}

$container = $this->getContainer($config);
$this->assertTrue($container->hasDefinition('monolog.handler.gelf'));

$handler = $container->getDefinition('monolog.handler.gelf');
/** @var Definition $publisher */
$publisher = $handler->getArguments()[0];

$this->assertDICConstructorArguments($publisher, ['localhost', 12201, 1420]);
}

public function encoderOptionsProvider(): array
{
return [
[
[['handlers' => ['gelf' => ['type' => 'gelf', 'publisher' => ['hostname' => 'localhost', 'encoder' => 'compressed_json']]]]],
],
[
[['handlers' => ['gelf' => ['type' => 'gelf', 'publisher' => ['hostname' => 'localhost']]]]],
],
];
}

public function testExceptionWhenUsingGelfWithInvalidEncoder(): void
{
if (!class_exists('Gelf\Transport\UdpTransport')) {
class_alias(DummyClassForClassExistsCheck::class, 'Gelf\Transport\UdpTransport');
}

$container = new ContainerBuilder();
$loader = new MonologExtension();

$this->expectException(InvalidConfigurationException::class);

$loader->load([['handlers' => ['gelf' => ['type' => 'gelf', 'publisher' => ['hostname' => 'localhost', 'encoder' => 'invalid_encoder']]]]], $container);
}

/**
* @dataProvider gelfEncoderProvider
*/
public function testGelfWithEncoder($encoderValue, $expectedClass): void
{
if (!class_exists('Gelf\Transport\UdpTransport')) {
class_alias(DummyClassForClassExistsCheck::class, 'Gelf\Transport\UdpTransport');
}

$container = $this->getContainer([['handlers' => ['gelf' => ['type' => 'gelf', 'publisher' => ['hostname' => 'localhost', 'encoder' => $encoderValue]]]]]);
$this->assertTrue($container->hasDefinition('monolog.handler.gelf'));

$handler = $container->getDefinition('monolog.handler.gelf');
/** @var Definition $publisher */
$publisher = $handler->getArguments()[0];
/** @var Definition $transport */
$transport = $publisher->getMethodCalls()[0][1][0];
$encoder = $transport->getMethodCalls()[0][1][0];

$this->assertDICConstructorArguments($transport, ['localhost', 12201, 1420]);
$this->assertDICDefinitionClass($encoder, $expectedClass);
$this->assertDICConstructorArguments($handler, [$publisher, 'DEBUG', true]);
}

public function gelfEncoderProvider(): array
{
return [
['json', 'Gelf\Encoder\JsonEncoder'],
['compressed_json', 'Gelf\Encoder\CompressedJsonEncoder'],
];
}

public function testExceptionWhenUsingServiceWithoutId()
{
$container = new ContainerBuilder();
Expand Down
Loading