Adds improvements based on feedback.

Author: jcchavezs

src/DDTrace/Exceptions/InvalidSpanArgument.php

@@ -0,0 +1,22 @@
+<?php
+
+namespace DDTrace\Exceptions;
+
+use InvalidArgumentException;
+
+final class InvalidSpanArgument extends InvalidArgumentException
+{
+    public static function forTagKey($key)
+    {
+        return new self(
+            sprintf('Invalid key type in given span tags. Expected string, got %s.', gettype($key))
+        );
+    }
+
+    public static function forError($error)
+    {
+        return new self(
+            sprintf('Error should be either Exception or Throwable, got %s.', gettype($error))
+        );
+    }
+}

src/DDTrace/Span.php

@@ -2,6 +2,7 @@
 
 namespace DDTrace;
 
+use DDTrace\Exceptions\InvalidSpanArgument;
 use Exception;
 use InvalidArgumentException;
 use OpenTracing\SpanContext as OpenTracingContext;
@@ -11,7 +12,7 @@
 final class Span implements OpenTracingSpan
 {
     /**
-     * Name is the name of the operation being measured. Some examples
+     * Operation Name is the name of the operation being measured. Some examples
      * might be "http.handler", "fileserver.upload" or "video.decompress".
      * Name should be set on every span.
      *
@@ -82,20 +83,20 @@ final class Span implements OpenTracingSpan
      * @param SpanContext $context
      * @param string $service
      * @param string $resource
-     * @param int|null $start
+     * @param int|null $startTime
      */
     public function __construct(
         $operationName,
         SpanContext $context,
         $service,
         $resource,
-        $start = null
+        $startTime = null
     ) {
         $this->context = $context;
         $this->operationName = (string) $operationName;
         $this->service = (string) $service;
         $this->resource = (string) $resource;
-        $this->startTime = $start ?: Time\now();
+        $this->startTime = $startTime ?: Time\now();
     }
 
     /**
@@ -181,9 +182,7 @@ public function setTags(array $tags)
 
         foreach ($tags as $key => $value) {
             if ($key !== (string) $key) {
-                throw new InvalidArgumentException(
-                    sprintf('First argument expected to be string, got %s', gettype($key))
-                );
+                throw InvalidSpanArgument::forTagKey($key);
             }
 
             if ($key === Tags\SERVICE_NAME) {
@@ -231,28 +230,26 @@ public function getAllTags()
      * updated and the error.Error() string is included with a default meta key.
      * If the Span has been finished, it will not be modified by this method.
      *
-     * @param Throwable|Exception $e
+     * @param Throwable|Exception $error
      * @throws InvalidArgumentException
      */
-    public function setError($e)
+    public function setError($error)
     {
         if ($this->isFinished()) {
             return;
         }
 
-        if (($e instanceof Exception) || ($e instanceof Throwable)) {
+        if (($error instanceof Exception) || ($error instanceof Throwable)) {
             $this->hasError = true;
             $this->setTags([
-                Tags\ERROR_MSG => $e->getMessage(),
-                Tags\ERROR_TYPE => get_class($e),
-                Tags\ERROR_STACK => $e->getTraceAsString(),
+                Tags\ERROR_MSG => $error->getMessage(),
+                Tags\ERROR_TYPE => get_class($error),
+                Tags\ERROR_STACK => $error->getTraceAsString(),
             ]);
             return;
         }
 
-        throw new InvalidArgumentException(
-            sprintf('Error should be either Exception or Throwable, got %s.', gettype($e))
-        );
+        throw InvalidSpanArgument::forError($error);
     }
 
     public function hasError()
@@ -273,12 +270,12 @@ public function finish($finishTime = null, array $logRecords = [])
     }
 
     /**
-     * @param Throwable|Exception $e
+     * @param Throwable|Exception $error
      * @return void
      */
-    public function finishWithError($e)
+    public function finishWithError($error)
     {
-        $this->setError($e);
+        $this->setError($error);
         $this->finish();
     }
 

src/DDTrace/Time.php

@@ -1,4 +1,5 @@
 <?php
+
 /**
  * Although DataDog uses nanotime to report spans PHP does not support nanotime
  * plus, nanotime is a uint64 which is not supported either. Microtime will be used

src/DDTrace/Tracer.php

@@ -164,6 +164,17 @@ public function flush()
             return;
         }
 
+        $tracesToBeSent = $this->shiftFinishedTraces();
+
+        if (empty($tracesToBeSent)) {
+            return;
+        }
+
+        $this->transport->send($tracesToBeSent);
+    }
+    
+    private function shiftFinishedTraces()
+    {
         $tracesToBeSent = [];
 
         foreach ($this->traces as $trace) {
@@ -174,18 +185,18 @@ public function flush()
                     $traceToBeSent = null;
                     break;
                 }
-                $tracesToBeSent[] = $span;
+                $traceToBeSent[] = $span;
             }
 
             if ($traceToBeSent === null) {
                 continue;
             }
 
             $tracesToBeSent[] = $traceToBeSent;
-            unset($this->traces[$span->getTraceId()]);
+            unset($this->traces[$traceToBeSent[0]->getTraceId()]);
         }
-
-        $this->transport->send($tracesToBeSent);
+        
+        return $tracesToBeSent;
     }
 
     private function record(Span $span)

tests/Unit/SpanTest.php

@@ -2,6 +2,7 @@
 
 namespace DDTrace\Tests\Unit;
 
+use DDTrace\Exceptions\InvalidSpanArgument;
 use DDTrace\SpanContext;
 use DDTrace\Tags;
 use DDTrace\Span;
@@ -10,26 +11,26 @@
 
 final class SpanTest extends PHPUnit_Framework_TestCase
 {
-    const NAME = 'test_span';
+    const OPERATION_NAME = 'test_span';
     const SERVICE = 'test_service';
     const RESOURCE = 'test_resource';
     const ANOTHER_NAME = 'test_span2';
     const ANOTHER_SERVICE = 'test_service2';
     const ANOTHER_RESOURCE = 'test_resource2';
     const ANOTHER_TYPE = 'test_type2';
-    const META_KEY = 'test_key';
-    const META_VALUE = 'test_value';
+    const TAG_KEY = 'test_key';
+    const TAG_VALUE = 'test_value';
     const EXCEPTION_MESSAGE = 'exception message';
 
     public function testCreateSpanSuccess()
     {
         $span = $this->createSpan();
-        $span->setTags([self::META_KEY => self::META_VALUE]);
+        $span->setTags([self::TAG_KEY => self::TAG_VALUE]);
 
-        $this->assertSame(self::NAME, $span->getOperationName());
+        $this->assertSame(self::OPERATION_NAME, $span->getOperationName());
         $this->assertSame(self::SERVICE, $span->getService());
         $this->assertSame(self::RESOURCE, $span->getResource());
-        $this->assertSame(self::META_VALUE, $span->getTag(self::META_KEY));
+        $this->assertSame(self::TAG_VALUE, $span->getTag(self::TAG_KEY));
     }
 
     public function testOverwriteOperationNameSuccess()
@@ -39,16 +40,16 @@ public function testOverwriteOperationNameSuccess()
         $this->assertSame(self::ANOTHER_NAME, $span->getOperationName());
     }
 
-    public function testSpanMetaRemainsImmutableAfterFinishing()
+    public function testSpanTagsRemainImmutableAfterFinishing()
     {
         $span = $this->createSpan();
         $span->finish();
 
-        $span->setTags([self::META_KEY => self::META_VALUE]);
-        $this->assertNull($span->getTag(self::META_KEY));
+        $span->setTags([self::TAG_KEY => self::TAG_VALUE]);
+        $this->assertNull($span->getTag(self::TAG_KEY));
     }
 
-    public function testSpanErrorAddsExpectedMeta()
+    public function testSpanErrorAddsExpectedTags()
     {
         $span = $this->createSpan();
         $span->setError(new Exception(self::EXCEPTION_MESSAGE));
@@ -67,6 +68,14 @@ public function testSpanErrorRemainsImmutableAfterFinishing()
         $this->assertFalse($span->hasError());
     }
 
+    public function testSpanErrorFailsForInvalidError()
+    {
+        $this->expectException(InvalidSpanArgument::class);
+        $this->expectExceptionMessage('Error should be either Exception or Throwable, got integer.');
+        $span = $this->createSpan();
+        $span->setError(1);
+    }
+
     public function testAddCustomTagsSuccess()
     {
         $span = $this->createSpan();
@@ -81,12 +90,20 @@ public function testAddCustomTagsSuccess()
         $this->assertEquals(self::ANOTHER_TYPE, $span->getType());
     }
 
+    public function testAddTagsFailsForInvalidTagKey()
+    {
+        $this->expectException(InvalidSpanArgument::class);
+        $this->expectExceptionMessage('Invalid key type in given span tags. Expected string, got integer.');
+        $span = $this->createSpan();
+        $span->setTags([self::TAG_KEY]);
+    }
+
     private function createSpan()
     {
         $context = SpanContext::createAsRoot();
 
         $span = new Span(
-            self::NAME,
+            self::OPERATION_NAME,
             $context,
             self::SERVICE,
             self::RESOURCE

tests/Unit/TracerTest.php

@@ -5,6 +5,7 @@
 use DDTrace\Propagator;
 use DDTrace\SpanContext;
 use DDTrace\Tracer;
+use DDTrace\Transport;
 use DDTrace\Transport\Noop as NoopTransport;
 use OpenTracing\Exceptions\UnsupportedFormat;
 use OpenTracing\NoopSpan;
@@ -14,6 +15,7 @@
 final class TracerTest extends PHPUnit_Framework_TestCase
 {
     const OPERATION_NAME = 'test_span';
+    const ANOTHER_OPERATION_NAME = 'test_span2';
     const TAG_KEY = 'test_key';
     const TAG_VALUE = 'test_value';
     const FORMAT = 'test_format';
@@ -91,4 +93,28 @@ public function testExtractCallsTheRightExtractor()
         $actualContext = $tracer->extract(self::FORMAT, $carrier);
         $this->assertEquals($expectedContext, $actualContext);
     }
+
+    public function testOnlyFinishedTracesAreBeingSent()
+    {
+        $transport = $this->prophesize(Transport::class);
+        $tracer = new Tracer($transport->reveal());
+        $span = $tracer->startSpan(self::OPERATION_NAME);
+        $tracer->startSpan(self::ANOTHER_OPERATION_NAME, [
+            'child_of' => $span,
+        ]);
+        $span->finish();
+
+        $span2 = $tracer->startSpan(self::OPERATION_NAME);
+        $span3 = $tracer->startSpan(self::ANOTHER_OPERATION_NAME, [
+            'child_of' => $span2,
+        ]);
+        $span2->finish();
+        $span3->finish();
+
+        $transport->send([
+            [$span2, $span3],
+        ])->shouldBeCalled();
+
+        $tracer->flush();
+    }
 }