Set $logger for the Encoder class, and log errors encoding JSON

[chuck]

Follow-up to #17 - pass the $logger to the Encoder, and if there's an error encoding JSON, log it at a debug level. If others like the LoggerAwareTrait pattern we could switch the Transport to use it as well. I'm not 100% happy with this since if you call Encoder methods yourself you aren't guaranteed to have a Logger, though. But passing $logger directly to the encodeTraces() method, while expedient, is almost certainly wrong.

[jcchavezs]

I discourage to do this change. In my experience, logging for reporting error and specially in PHP (one request = one report) ends in a HUGE pollution of logs, instead I would recommend to use metrics or other sort of acknowledge. Check this https://github.com/jcchavezs/zipkin-php/blob/master/src/Zipkin/Reporters/Http.php

[chuck]

I agree that you wouldn't want this in production, but it's the kind of thing that turning on debug logging is for.

Would it potentially make sense to try serializing without the tags, so that we just lose them instead of the whole span?

[pawelchcki]

Thanks for the PR!
I've left a few questions about the logger handling. Looks good otherwise!

[pawelchcki]

Is $this->logger always initialized at this point?

[chuck]

Taken care of with constructor injection

[pawelchcki]

Do we need this trait in Noop class?

[chuck]

The way things are currently set up, yes, because we call setLogger() on the encoder object from within the transport. Though the Noop Encoder is a bit of an odd duck anyway in terms of when you'd need it...

[pawelchcki]

Could the logger be set in constructor one line above?

[localheinz]

I agree it would be better to allow injection via constructor.

How about

diff --git a/src/DDTrace/Encoders/Json.php b/src/DDTrace/Encoders/Json.php
index 01876e6..8b8c8f2 100644
--- a/src/DDTrace/Encoders/Json.php
+++ b/src/DDTrace/Encoders/Json.php
@@ -4,12 +4,19 @@ namespace DDTrace\Encoders;

 use DDTrace\Encoder;
 use DDTrace\Span;
-use Psr\Log\LoggerAwareInterface;
-use Psr\Log\LoggerAwareTrait;
+use Psr\Log;

 final class Json implements Encoder
 {
-    use LoggerAwareTrait;
+    /**
+     * @var Log\LoggerInterface
+     */
+    private $logger;
+
+    public function __construct(Log\LoggerInterface $logger = null)
+    {
+        $this->logger = $logger ?: new Log\NullLogger();
+    }

     /**
      * {@inheritdoc}
diff --git a/tests/Unit/Encoders/JsonTest.php b/tests/Unit/Encoders/JsonTest.php
index 5db16ef..1f46924 100644
--- a/tests/Unit/Encoders/JsonTest.php
+++ b/tests/Unit/Encoders/JsonTest.php
@@ -54,8 +54,7 @@ JSON;
             )
             ->shouldBeCalled();

-        $jsonEncoder = new Json();
-        $jsonEncoder->setLogger($logger->reveal());
+        $jsonEncoder = new Json($logger->reveal());
         $encodedTrace = $jsonEncoder->encodeTraces([[$span, $span]]);
         $this->assertEquals($expectedPayload, $encodedTrace);
     }

?

[jcchavezs]

@chuck I don't think logs are meant to recover traces. I honestly would take advantage of the opportunity that DataDog also supports metrics.

Ping @palazzem

[chuck]

I'm not thinking about this as recovering the trace, I'm thinking of it as giving someone developing an integration, or instrumenting their own code, a way to see why things are otherwise silently failing.

[chuck]

Updated for constructor injection, which also means that to get these log message, you have to explicitly set a logger on the encoder. For me that's good to have for troubleshooting, but makes it pretty hard to get huge numbers of log lines without meaning to.

[localheinz]

👍 Looking good!

[pawelchcki]

Looking good!