From 079558052567a79da88f0c602c07c36c85454641 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Fri, 16 Feb 2018 08:32:03 -0500 Subject: [PATCH 1/4] Adds opentracing compatibility. --- composer.json | 6 +- src/DDTrace/Span.php | 218 +++++++++++++++++++-------------- src/DDTrace/Tracer.php | 173 ++++++++++++++++++++++++-- src/DDTrace/Transport/Noop.php | 2 +- tests/Unit/SpanTest.php | 50 ++++++-- tests/Unit/TracerTest.php | 50 ++++++++ 6 files changed, 380 insertions(+), 119 deletions(-) create mode 100644 tests/Unit/TracerTest.php diff --git a/composer.json b/composer.json index 2c7d9c0b4a5..792d7b9c0ec 100644 --- a/composer.json +++ b/composer.json @@ -18,7 +18,9 @@ "minimum-stability": "dev", "require": { "opentracing/opentracing": "1.0.0-beta2", - "symfony/polyfill": "~1.7.0" + "psr/log": "~1.0.2", + "symfony/polyfill": "~1.7.0", + "guzzlehttp/psr7": "^1.4@dev" }, "require-dev": { "phpunit/phpunit": "~5.7.19", @@ -36,7 +38,7 @@ }, "autoload-dev": { "psr-4": { - "DDTrace\\Tests\\": "./tests/" + "DDTrace\\Tests\\": "./tests/DdTrace/" } }, "scripts": { diff --git a/src/DDTrace/Span.php b/src/DDTrace/Span.php index c2ef5dc608f..4d96785d144 100644 --- a/src/DDTrace/Span.php +++ b/src/DDTrace/Span.php @@ -4,38 +4,12 @@ use Exception; use InvalidArgumentException; +use OpenTracing\SpanContext as OpenTracingContext; use Throwable; +use OpenTracing\Span as OpenTracingSpan; -final class Span +final class Span implements OpenTracingSpan { - /** - * @var Tracer - */ - private $tracer; - - /** - * The unique integer (64-bit unsigned) ID of the trace containing this span. - * It is stored in hexadecimal representation. - * - * @var string - */ - private $traceId; - - /** - * The span integer ID of the parent span. - * - * @var string - */ - private $parentId; - - /** - * The span integer (64-bit unsigned) ID. - * It is stored in hexadecimal representation. - * - * @var string - */ - private $spanId; - /** * Name is the name of the operation being measured. Some examples * might be "http.handler", "fileserver.upload" or "video.decompress". @@ -43,7 +17,12 @@ final class Span * * @var string */ - private $name; + private $operationName; + + /** + * @var OpenTracingContext + */ + private $context; /** * Resource is a query to a service. A web application might use @@ -72,14 +51,14 @@ final class Span /** * Protocol associated with the span * - * @var string + * @var string|null */ private $type; /** * @var int */ - private $start; + private $startTime; /** * @var int|null @@ -89,31 +68,34 @@ final class Span /** * @var array */ - private $meta = []; + private $tags = []; /** * @var bool */ private $hasError = false; + + /** + * Span constructor. + * @param string $operationName + * @param SpanContext $context + * @param string $service + * @param string $resource + * @param int|null $start + */ public function __construct( - Tracer $tracer, - $name, + $operationName, + SpanContext $context, $service, $resource, - $traceId, - $spanId, - $parentId = null, $start = null ) { - $this->tracer = $tracer; - $this->name = (string) $name; - $this->service = $service; + $this->context = $context; + $this->operationName = (string) $operationName; + $this->service = (string) $service; $this->resource = (string) $resource; - $this->start = $start ?: Time\now(); - $this->traceId = $traceId; - $this->spanId = $spanId; - $this->parentId = $parentId; + $this->startTime = $start ?: Time\now(); } /** @@ -121,7 +103,7 @@ public function __construct( */ public function getTraceId() { - return $this->traceId; + return $this->context->getTraceId(); } /** @@ -129,7 +111,7 @@ public function getTraceId() */ public function getSpanId() { - return $this->spanId; + return $this->context->getSpanId(); } /** @@ -137,24 +119,15 @@ public function getSpanId() */ public function getParentId() { - return $this->parentId; - } - - /** - * @return string - */ - public function getName() - { - return $this->name; + return $this->context->getParentId(); } /** - * @param string $name - * @return void + * {@inheritdoc} */ - public function setName($name) + public function overwriteOperationName($operationName) { - $this->name = $name; + $this->operationName = $operationName; } /** @@ -174,7 +147,7 @@ public function getService() } /** - * @return string + * @return string|null */ public function getType() { @@ -184,9 +157,9 @@ public function getType() /** * @return int */ - public function getStart() + public function getStartTime() { - return $this->start; + return $this->startTime; } /** @@ -198,41 +171,61 @@ public function getDuration() } /** - * Adds an arbitrary meta field to the current Span. - * If the Span has been finished, it will not be modified by the method. - * - * @param string $key - * @param string $value - * @throws InvalidArgumentException + * {@inheritdoc} */ - public function setMeta($key, $value) + public function setTags(array $tags) { if ($this->isFinished()) { return; } - if ($key !== (string) $key) { - throw new InvalidArgumentException( - sprintf('First argument expected to be string, got %s', gettype($key)) - ); + foreach ($tags as $key => $value) { + if ($key !== (string) $key) { + throw new InvalidArgumentException( + sprintf('First argument expected to be string, got %s', gettype($key)) + ); + } + + if ($key === Tags\SERVICE_NAME) { + $this->service = $value; + continue; + } + + if ($key === Tags\RESOURCE_NAME) { + $this->resource = $value; + continue; + } + + if ($key === Tags\SPAN_TYPE) { + $this->type = $value; + continue; + } + + $this->tags[$key] = (string) $value; } - - $this->meta[$key] = (string) $value; } /** * @param string $key * @return string|null */ - public function getMeta($key) + public function getTag($key) { - if (array_key_exists($key, $this->meta)) { - return $this->meta[$key]; + if (array_key_exists($key, $this->tags)) { + return $this->tags[$key]; } return null; } + /** + * @return array + */ + public function getAllTags() + { + return $this->tags; + } + /** * Stores a Throwable object within the span meta. The error status is * updated and the error.Error() string is included with a default meta key. @@ -249,9 +242,11 @@ public function setError($e) if (($e instanceof Exception) || ($e instanceof Throwable)) { $this->hasError = true; - $this->setMeta(Tags\ERROR_MSG, $e->getMessage()); - $this->setMeta(Tags\ERROR_TYPE, get_class($e)); - $this->setMeta(Tags\ERROR_STACK, $e->getTraceAsString()); + $this->setTags([ + Tags\ERROR_MSG => $e->getMessage(), + Tags\ERROR_TYPE => get_class($e), + Tags\ERROR_STACK => $e->getTraceAsString(), + ]); return; } @@ -266,23 +261,15 @@ public function hasError() } /** - * Finish closes this Span (but not its children) providing the duration - * of this part of the tracing session. This method is idempotent so - * calling this method multiple times is safe and doesn't update the - * current Span. Once a Span has been finished, methods that modify the Span - * will become no-ops. - * - * @param int|null $finish - * @return void + * {@inheritdoc} */ - public function finish($finish = null) + public function finish($finishTime = null, array $logRecords = []) { if ($this->isFinished()) { return; } - $this->duration = ($finish ?: Time\now()) - $this->start; - $this->tracer->record($this); + $this->duration = ($finishTime ?: Time\now()) - $this->startTime; } /** @@ -295,8 +282,51 @@ public function finishWithError($e) $this->finish(); } - private function isFinished() + /** + * @return bool + */ + public function isFinished() { return $this->duration !== null; } + + /** + * {@inheritdoc} + */ + public function getOperationName() + { + return $this->operationName; + } + + /** + * {@inheritdoc} + */ + public function getContext() + { + return $this->context; + } + + /** + * {@inheritdoc} + */ + public function log(array $fields = [], $timestamp = null) + { + throw new \BadMethodCallException('not implemented'); + } + + /** + * {@inheritdoc} + */ + public function addBaggageItem($key, $value) + { + $this->context = $this->context->withBaggageItem($key, $value); + } + + /** + * {@inheritdoc} + */ + public function getBaggageItem($key) + { + return $this->context->getBaggageItem($key); + } } diff --git a/src/DDTrace/Tracer.php b/src/DDTrace/Tracer.php index c20ccfdcc3e..852afa2f216 100644 --- a/src/DDTrace/Tracer.php +++ b/src/DDTrace/Tracer.php @@ -2,36 +2,157 @@ namespace DDTrace; -use DDTrace\Transport\Noop; +use DDTrace\Propagators\Noop as NoopPropagator; +use DDTrace\Transport\Noop as NoopTransport; +use OpenTracing\Exceptions\UnsupportedFormat; +use OpenTracing\NoopSpan; +use OpenTracing\Reference; +use OpenTracing\SpanContext as OpenTracingContext; +use OpenTracing\SpanOptions; +use OpenTracing\Tracer as OpenTracingTracer; +use OpenTracing\Formats; -final class Tracer +final class Tracer implements OpenTracingTracer { /** - * @var array + * @var Span[][] */ private $traces = []; /** + * The transport mechanism used to delivery spans to the agent + * * @var Transport */ private $transport; - public function __construct(Transport $transport) + /** + * @var Propagator[] + */ + private $propagators; + + /** + * @var array + */ + private $config = [ + /** + * Enabled, when false, returns a no-op implementation of the Tracer. + */ + 'enabled' => true, + /** + * Debug, when true, writes details to logs. + */ + 'debug' => false, + /** + * ServiceName specifies the name of this application. + */ + 'service_name' => PHP_SAPI, + /** GlobalTags holds a set of tags that will be automatically applied to + * all spans. + */ + 'global_tags' => [], + ]; + + /** + * Tracer constructor. + * @param Transport $transport + * @param Propagator[] $propagators + * @param array $config + */ + public function __construct(Transport $transport, array $propagators = [], array $config = []) { $this->transport = $transport; + $this->propagators = $propagators; + $this->config = array_merge($this->config, $config); } public static function noop() { - return new self(new Noop); + return new self( + new NoopTransport, + [ + Formats\BINARY => new NoopPropagator, + Formats\TEXT_MAP => new NoopPropagator, + Formats\HTTP_HEADERS => new NoopPropagator, + ], + ['enabled' => false] + ); } /** - * @param Span $span + * {@inheritdoc} */ - public function record(Span $span) + public function startSpan($operationName, $options = []) { - $this->traces[$span->getTraceId()][] = $span; + if (!$this->config['enabled']) { + return NoopSpan::create(); + } + + if (!($options instanceof SpanOptions)) { + $options = SpanOptions::create($options); + } + + $reference = $this->findParent($options->getReferences()); + + if ($reference === null) { + $context = SpanContext::createAsRoot(); + } else { + $context = SpanContext::createAsChild($reference->getContext()); + } + + $span = new Span( + $operationName, + $context, + $this->config['service_name'], + array_key_exists('resource', $this->config) ? $this->config['resource'] : $operationName, + $options->getStartTime() + ); + + $span->setTags($options->getTags() + $this->config['global_tags']); + + $this->record($span); + + return $span; + } + + /** + * @param array|Reference[] $references + * @return null|Reference + */ + private function findParent(array $references) + { + foreach ($references as $reference) { + if ($reference->isType(Reference::CHILD_OF)) { + return $reference; + } + } + + return null; + } + + /** + * {@inheritdoc} + */ + public function inject(OpenTracingContext $spanContext, $format, &$carrier) + { + if (array_key_exists($format, $this->propagators)) { + $this->propagators[$format]->inject($spanContext, $carrier); + return; + } + + throw UnsupportedFormat::forFormat($format); + } + + /** + * {@inheritdoc} + */ + public function extract($format, $carrier) + { + if (array_key_exists($format, $this->propagators)) { + return $this->propagators[$format]->extract($carrier); + } + + throw UnsupportedFormat::forFormat($format); } /** @@ -39,6 +160,40 @@ public function record(Span $span) */ public function flush() { - $this->transport->send($this->traces); + if (!$this->config['enabled']) { + return; + } + + $tracesToBeSent = []; + + foreach ($this->traces as $trace) { + $traceToBeSent = []; + + foreach ($trace as $span) { + if (!$span->isFinished()) { + $traceToBeSent = null; + break; + } + $tracesToBeSent[] = $span; + } + + if ($traceToBeSent === null) { + continue; + } + + $tracesToBeSent[] = $traceToBeSent; + unset($this->traces[$span->getTraceId()]); + } + + $this->transport->send($tracesToBeSent); + } + + private function record(Span $span) + { + if (!array_key_exists($span->getTraceId(), $this->traces)) { + $this->traces[$span->getTraceId()] = []; + } + + $this->traces[$span->getTraceId()][$span->getSpanId()] = $span; } } diff --git a/src/DDTrace/Transport/Noop.php b/src/DDTrace/Transport/Noop.php index 872a55710a6..b9b92644c76 100644 --- a/src/DDTrace/Transport/Noop.php +++ b/src/DDTrace/Transport/Noop.php @@ -5,7 +5,7 @@ use DDTrace\Transport; use GuzzleHttp\Psr7\Response; -class Noop implements Transport +final class Noop implements Transport { public function send(array $traces) { diff --git a/tests/Unit/SpanTest.php b/tests/Unit/SpanTest.php index b79eeb87c16..58c6b7127f8 100644 --- a/tests/Unit/SpanTest.php +++ b/tests/Unit/SpanTest.php @@ -2,9 +2,9 @@ namespace DDTrace\Tests\Unit; +use DDTrace\SpanContext; use DDTrace\Tags; use DDTrace\Span; -use DDTrace\Tracer; use Exception; use PHPUnit_Framework_TestCase; @@ -13,6 +13,10 @@ final class SpanTest extends PHPUnit_Framework_TestCase const 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 EXCEPTION_MESSAGE = 'exception message'; @@ -20,12 +24,19 @@ final class SpanTest extends PHPUnit_Framework_TestCase public function testCreateSpanSuccess() { $span = $this->createSpan(); - $span->setMeta(self::META_KEY, self::META_VALUE); + $span->setTags([self::META_KEY => self::META_VALUE]); - $this->assertSame(self::NAME, $span->getName()); + $this->assertSame(self::NAME, $span->getOperationName()); $this->assertSame(self::SERVICE, $span->getService()); $this->assertSame(self::RESOURCE, $span->getResource()); - $this->assertSame(self::META_VALUE, $span->getMeta(self::META_KEY)); + $this->assertSame(self::META_VALUE, $span->getTag(self::META_KEY)); + } + + public function testOverwriteOperationNameSuccess() + { + $span = $this->createSpan(); + $span->overwriteOperationName(self::ANOTHER_NAME); + $this->assertSame(self::ANOTHER_NAME, $span->getOperationName()); } public function testSpanMetaRemainsImmutableAfterFinishing() @@ -33,8 +44,8 @@ public function testSpanMetaRemainsImmutableAfterFinishing() $span = $this->createSpan(); $span->finish(); - $span->setMeta(self::META_KEY, self::META_VALUE); - $this->assertNull($span->getMeta(self::META_KEY)); + $span->setTags([self::META_KEY => self::META_VALUE]); + $this->assertNull($span->getTag(self::META_KEY)); } public function testSpanErrorAddsExpectedMeta() @@ -43,8 +54,8 @@ public function testSpanErrorAddsExpectedMeta() $span->setError(new Exception(self::EXCEPTION_MESSAGE)); $this->assertTrue($span->hasError()); - $this->assertEquals($span->getMeta(Tags\ERROR_MSG), self::EXCEPTION_MESSAGE); - $this->assertEquals($span->getMeta(Tags\ERROR_TYPE), Exception::class); + $this->assertEquals($span->getTag(Tags\ERROR_MSG), self::EXCEPTION_MESSAGE); + $this->assertEquals($span->getTag(Tags\ERROR_TYPE), Exception::class); } public function testSpanErrorRemainsImmutableAfterFinishing() @@ -56,16 +67,29 @@ public function testSpanErrorRemainsImmutableAfterFinishing() $this->assertFalse($span->hasError()); } + public function testAddCustomTagsSuccess() + { + $span = $this->createSpan(); + $span->setTags([ + Tags\SERVICE_NAME => self::ANOTHER_SERVICE, + Tags\RESOURCE_NAME => self::ANOTHER_RESOURCE, + Tags\SPAN_TYPE => self::ANOTHER_TYPE, + ]); + + $this->assertEquals(self::ANOTHER_SERVICE, $span->getService()); + $this->assertEquals(self::ANOTHER_RESOURCE, $span->getResource()); + $this->assertEquals(self::ANOTHER_TYPE, $span->getType()); + } + private function createSpan() { - $tracer = Tracer::noop(); + $context = SpanContext::createAsRoot(); + $span = new Span( - $tracer, self::NAME, + $context, self::SERVICE, - self::RESOURCE, - 'abc123', - 'abc123' + self::RESOURCE ); return $span; diff --git a/tests/Unit/TracerTest.php b/tests/Unit/TracerTest.php new file mode 100644 index 00000000000..9186ba70760 --- /dev/null +++ b/tests/Unit/TracerTest.php @@ -0,0 +1,50 @@ +startSpan(self::OPERATION_NAME); + $this->assertInstanceOf(NoopSpan::class, $span); + } + + public function testCreateSpanSuccessWithExpectedValues() + { + $tracer = new Tracer(new NoopTransport); + $startTime = Time\now(); + $span = $tracer->startSpan(self::OPERATION_NAME, [ + 'tags' => [ + self::TAG_KEY => self::TAG_VALUE + ], + 'start_time' => $startTime, + ]); + + $this->assertEquals(self::OPERATION_NAME, $span->getOperationName()); + $this->assertEquals(self::TAG_VALUE, $span->getTag(self::TAG_KEY)); + $this->assertEquals($startTime, $span->getStartTime()); + } + + public function testStartSpanAsChild() + { + $context = SpanContext::createAsRoot(); + $tracer = new Tracer(new NoopTransport); + $span = $tracer->startSpan(self::OPERATION_NAME, [ + 'child_of' => $context, + ]); + $this->assertEquals($context->getSpanId(), $span->getParentId()); + } +} From 6f16ae09a534481a1f99b4aa939fca531c63bd94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Fri, 16 Feb 2018 08:43:16 -0500 Subject: [PATCH 2/4] Adds tests for the propagation. --- tests/Unit/TracerTest.php | 44 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/Unit/TracerTest.php b/tests/Unit/TracerTest.php index 9186ba70760..77e219b134f 100644 --- a/tests/Unit/TracerTest.php +++ b/tests/Unit/TracerTest.php @@ -2,9 +2,11 @@ namespace DDTrace\Tests\Unit; +use DDTrace\Propagator; use DDTrace\SpanContext; use DDTrace\Tracer; use DDTrace\Transport\Noop as NoopTransport; +use OpenTracing\Exceptions\UnsupportedFormat; use OpenTracing\NoopSpan; use PHPUnit_Framework_TestCase; use DDTrace\Time; @@ -14,6 +16,7 @@ final class TracerTest extends PHPUnit_Framework_TestCase const OPERATION_NAME = 'test_span'; const TAG_KEY = 'test_key'; const TAG_VALUE = 'test_value'; + const FORMAT = 'test_format'; public function testStartSpanAsNoop() { @@ -47,4 +50,45 @@ public function testStartSpanAsChild() ]); $this->assertEquals($context->getSpanId(), $span->getParentId()); } + + public function testInjectThrowsUnsupportedFormatException() + { + $this->expectException(UnsupportedFormat::class); + $context = SpanContext::createAsRoot(); + $carrier = []; + + $tracer = new Tracer(new NoopTransport); + $tracer->inject($context, self::FORMAT, $carrier); + } + + public function testInjectCallsTheRightInjector() + { + $context = SpanContext::createAsRoot(); + $carrier = []; + + $propagator = $this->prophesize(Propagator::class); + $propagator->inject($context, $carrier)->shouldBeCalled(); + $tracer = new Tracer(new NoopTransport, [self::FORMAT => $propagator->reveal()]); + $tracer->inject($context, self::FORMAT, $carrier); + } + + public function testExtractThrowsUnsupportedFormatException() + { + $this->expectException(UnsupportedFormat::class); + $carrier = []; + $tracer = new Tracer(new NoopTransport); + $tracer->extract(self::FORMAT, $carrier); + } + + public function testExtractCallsTheRightExtractor() + { + $expectedContext = SpanContext::createAsRoot(); + $carrier = []; + + $propagator = $this->prophesize(Propagator::class); + $propagator->extract($carrier)->shouldBeCalled()->willReturn($expectedContext); + $tracer = new Tracer(new NoopTransport, [self::FORMAT => $propagator->reveal()]); + $actualContext = $tracer->extract(self::FORMAT, $carrier); + $this->assertEquals($expectedContext, $actualContext); + } } From dc2f460b8c6f1f2d178bec70f75e0211bfc1a7aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Fri, 16 Feb 2018 09:28:50 -0500 Subject: [PATCH 3/4] Removes logger dependency and fixes dev autoloading. --- composer.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 792d7b9c0ec..9e927e6abd7 100644 --- a/composer.json +++ b/composer.json @@ -18,7 +18,6 @@ "minimum-stability": "dev", "require": { "opentracing/opentracing": "1.0.0-beta2", - "psr/log": "~1.0.2", "symfony/polyfill": "~1.7.0", "guzzlehttp/psr7": "^1.4@dev" }, @@ -38,7 +37,7 @@ }, "autoload-dev": { "psr-4": { - "DDTrace\\Tests\\": "./tests/DdTrace/" + "DDTrace\\Tests\\": "./tests/" } }, "scripts": { From d405aaf71aaa8826f54d9a1857f1cc827990a801 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Thu, 22 Feb 2018 14:57:20 +0100 Subject: [PATCH 4/4] Adds improvements based on feedback. --- .../Exceptions/InvalidSpanArgument.php | 22 +++++++++++ src/DDTrace/Span.php | 35 ++++++++--------- src/DDTrace/Time.php | 1 + src/DDTrace/Tracer.php | 19 +++++++-- tests/Unit/SpanTest.php | 39 +++++++++++++------ tests/Unit/TracerTest.php | 26 +++++++++++++ 6 files changed, 108 insertions(+), 34 deletions(-) create mode 100644 src/DDTrace/Exceptions/InvalidSpanArgument.php diff --git a/src/DDTrace/Exceptions/InvalidSpanArgument.php b/src/DDTrace/Exceptions/InvalidSpanArgument.php new file mode 100644 index 00000000000..03cc94138f4 --- /dev/null +++ b/src/DDTrace/Exceptions/InvalidSpanArgument.php @@ -0,0 +1,22 @@ +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(); } diff --git a/src/DDTrace/Time.php b/src/DDTrace/Time.php index b480ce70938..27d34f9e077 100644 --- a/src/DDTrace/Time.php +++ b/src/DDTrace/Time.php @@ -1,4 +1,5 @@ shiftFinishedTraces(); + + if (empty($tracesToBeSent)) { + return; + } + + $this->transport->send($tracesToBeSent); + } + + private function shiftFinishedTraces() + { $tracesToBeSent = []; foreach ($this->traces as $trace) { @@ -174,7 +185,7 @@ public function flush() $traceToBeSent = null; break; } - $tracesToBeSent[] = $span; + $traceToBeSent[] = $span; } if ($traceToBeSent === null) { @@ -182,10 +193,10 @@ public function flush() } $tracesToBeSent[] = $traceToBeSent; - unset($this->traces[$span->getTraceId()]); + unset($this->traces[$traceToBeSent[0]->getTraceId()]); } - - $this->transport->send($tracesToBeSent); + + return $tracesToBeSent; } private function record(Span $span) diff --git a/tests/Unit/SpanTest.php b/tests/Unit/SpanTest.php index 58c6b7127f8..9b0731c0fe8 100644 --- a/tests/Unit/SpanTest.php +++ b/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 diff --git a/tests/Unit/TracerTest.php b/tests/Unit/TracerTest.php index 77e219b134f..831c94e8540 100644 --- a/tests/Unit/TracerTest.php +++ b/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(); + } }