Missing logger - errors are thrown away

[enumag]

Hello, I found a bug in

https://github.com/jcchavezs/zipkin-instrumentation-symfony/blob/2afea4735e38bef744809a4108b02adc28cdf647/src/ZipkinBundle/TracingFactory.php#L50-L65

I was using the Http reporter but configured it with wrong URL. This case is handled by the reporter and logged into logger. However here you're creating the reporter without passing the logger service so NullLogger is used instead and the errors are thrown away.

Also using $logger = $container->get('logger'); is a bad idea. I tried and it failed with this:

[critical] Uncaught Exception: The "logger" service or alias has been removed or inlined when the container was compiled. You should either make it public, or stop using the container directly and use dependency injection instead.

[jcchavezs]

What version of symfony are you using? Symfony used to come with a logger out of the box but I don't know if that changed over the time.

[enumag]

It still comes with a logger out of the box. Services just aren't public by default anymore. Meaning that 'logger' service still exists and you can autowire it or manually inject it somewhere in config, you just can't use $container->get() to get it anymore.

This isn't exactly new by the way. It has been the case ever since Symfony 3.4. https://symfony.com/blog/new-in-symfony-3-4-services-are-private-by-default

[jcchavezs]

I see, then maybe we can declare our own logger e.g. zipkin._logger, make it public and use it here instead? Would that work? Would you be up for do such change?

[enumag]

It would work but I won't do such change myself because I'm very strongly against the bad practice of using $container->get(). It would be much better to inject the logger into the factory instead in my opinion.

[jcchavezs]

Yeah I prefer injection too but how would that look like?

[enumag]

Well the main problem is that you don't resolve the configuration in the ZipkinExtension as you should but instead only create parameters and resolve it at runtime in TracingFactory. In my opinion the TracingFactory should take the sampler, reporter, service name, endpoint and noop as constructor parameters and not use the container at all. It's a bit too large refactoring for me to take on though but the main point is that the main work resolving the dependencies should be done at compile time in ZipkinExtension.