Skip to content

[Monolog] Use a standalone http client for ElasticsearchLogstashHandler #34423

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

Closed
wants to merge 1 commit into from

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Nov 17, 2019

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #32360 (comment) symfony/symfony-docs#12642
License MIT
Doc PR

This mean the logger will not be injected in this Handler, and it will
prevent an infinite loop :

a log > ElasticsearchLogstashHandler > fire an HTTP request > the http
request emits another log > ...

I did not submit this patch on monolog-bundle because:

  • The handler is defined in Symfony, not monolog
  • We need advanced DIC features, the bundle still work with SF 3.4
  • The only way to fix this issue for sure, it's in symfony

This mean the logger will not be injected in this Handler, and it will
prevent an infinite loop :

a log > ElasticsearchLogstashHandler > fire an HTTP request > the http
request emits another log > ...
@lyrixx lyrixx force-pushed the monolog-handler-es-log branch from 2e400b5 to 6fc6263 Compare November 17, 2019 16:39
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Nov 17, 2019
@nicolas-grekas
Copy link
Member

I really think this should be on monolog-bundle. I don't think any of the objections are real blockers.

@@ -446,6 +448,11 @@ public function load(array $configs, ContainerBuilder $container)
$container->registerForAutoconfiguration(LoggerAwareInterface::class)
->addMethodCall('setLogger', [new Reference('logger')]);

$handlerAutoconfiguration = $container->registerForAutoconfiguration(ElasticsearchLogstashHandler::class);
Copy link
Member

Choose a reason for hiding this comment

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

I would bind on HandlerInterface instead - all handlers that need an http_client cannot use the default one since it logs.

@lyrixx
Copy link
Member Author

lyrixx commented Nov 17, 2019

I really think this should be on monolog-bundle.

May I ask why? What's the issue with putting the code here?

@nicolas-grekas
Copy link
Member

What's the issue with putting the code here?

4.4 is closed for new feats, and what is configured here is the responsibility of the monolog-bundle :)

@lyrixx
Copy link
Member Author

lyrixx commented Nov 17, 2019

This is not a feature, it's a bug fix :)

@nicolas-grekas
Copy link
Member

Closing in favor of symfony/monolog-bundle#331

lyrixx added a commit to symfony/monolog-bundle that referenced this pull request Jan 6, 2020
This PR was merged into the 3.x-dev branch.

Discussion
----------

Use an HttpClient without logger in all handlers

fixes:

* symfony/symfony#34423
* symfony/symfony#32360 (comment)
* symfony/symfony-docs#12642

Commits
-------

faf6943 Use an HttpClient without logger in all handlers
@lyrixx lyrixx deleted the monolog-handler-es-log branch January 6, 2020 10:34
dani-danigm pushed a commit to dani-danigm/monolog-bundle that referenced this pull request Jun 15, 2022
This PR was merged into the 3.x-dev branch.

Discussion
----------

Use an HttpClient without logger in all handlers

fixes:

* symfony/symfony#34423
* symfony/symfony#32360 (comment)
* symfony/symfony-docs#12642

Commits
-------

2abc6f6 Use an HttpClient without logger in all handlers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants