From ef89cae12554545976a0552fd76875cc40d2bfd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Thu, 12 Apr 2018 17:44:20 +0200 Subject: [PATCH] [#60] Changes the API to return a scope when starting an active span and makes the span not aware of the scope. --- README.md | 27 +++++++++------- .../Exceptions/InvalidSpanOption.php | 4 +-- src/OpenTracing/Mock/MockScope.php | 22 +++++++++++-- src/OpenTracing/Mock/MockScopeManager.php | 23 +++---------- src/OpenTracing/Mock/MockSpan.php | 24 +------------- src/OpenTracing/Mock/MockTracer.php | 15 ++++----- src/OpenTracing/NoopScopeManager.php | 10 +----- src/OpenTracing/NoopTracer.php | 2 +- src/OpenTracing/ScopeManager.php | 21 +++--------- .../{SpanOptions.php => StartSpanOptions.php} | 22 ++++++------- src/OpenTracing/Tracer.php | 11 ++++--- .../OpenTracing/Mock/MockScopeManagerTest.php | 8 ++--- tests/OpenTracing/Mock/MockSpanTest.php | 3 -- tests/OpenTracing/Mock/MockTracerTest.php | 4 +-- ...tionsTest.php => StartSpanOptionsTest.php} | 32 +++++++++---------- 15 files changed, 94 insertions(+), 134 deletions(-) rename src/OpenTracing/{SpanOptions.php => StartSpanOptions.php} (89%) rename tests/OpenTracing/{SpanOptionsTest.php => StartSpanOptionsTest.php} (74%) diff --git a/README.md b/README.md index 8baab9b..446052a 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,7 @@ composer require opentracing/opentracing When consuming this library one really only need to worry about a couple of key abstractions: the `Tracer::startActiveSpan` and `Tracer::startSpan` method, -the `Span` interface, and binding a `Tracer` at bootstrap time. Here are code snippets +the `Span` interface, the `Scope` interface and binding a `Tracer` at bootstrap time. Here are code snippets demonstrating some important use cases: ### Singleton initialization @@ -58,7 +58,7 @@ $spanContext = GlobalTracer::get()->extract( function doSomething() { ... - $span = GlobalTracer::get()->startActiveSpan('my_span', ['child_of' => $spanContext]); + $span = GlobalTracer::get()->startSpan('my_span', ['child_of' => $spanContext]); ... @@ -77,7 +77,7 @@ function doSomething() { It's always possible to create a "root" `Span` with no parent or other causal reference. ```php -$span = $tracer->startActiveSpan('my_first_span'); +$span = $tracer->startSpan('my_first_span'); ... $span->finish(); ``` @@ -95,11 +95,11 @@ Unless you are using asynchronous code that tracks multiple spans at the same time, such as when using cURL Multi Exec or MySQLi Polling you are better of just using `Tracer::startActiveSpan` everywhere in your application. -The currently active span gets automatically closed and deactivated from the scope -when you call `$span->finish()` as you can see in the previous example. +The currently active span gets automatically finished when you call `$scope->close()` +as you can see in the previous example. If you don't want a span to automatically close when `Span::finish()` is called -then you must pass the option `'close_span_on_finish'=> false,` to the `$options` +then you must pass the option `'finish_span_on_close'=> false,` to the `$options` argument of `startActiveSpan`. An example of a linear, two level deep span tree using active spans looks like @@ -112,10 +112,11 @@ $root = $tracer->startActiveSpan('php'); $http = $tracer->startActiveSpan('http'); file_get_contents('http://php.net'); - $http->finish(); + $http->close(); - $controller->finish(); -$root->finish(); + $controller->close(); + +$root->close(); ``` #### Creating a child span assigning parent manually @@ -153,11 +154,11 @@ $child = GlobalTracer::get()->startActiveSpan('my_second_span'); ... -$child->finish(); +$child->close(); ... -$parent->finish(); +$parent->close(); ``` ### Serializing to the wire @@ -237,7 +238,7 @@ register_shutdown_function(function() { This is optional, tracers can decide to immediately send finished spans to a backend. The flush call can be implemented as a NO-OP for these tracers. -### Using Span Options +### Using `StartSpanOptions` Passing options to the pass can be done using either an array or the SpanOptions wrapper object. The following keys are valid: @@ -246,6 +247,8 @@ SpanOptions wrapper object. The following keys are valid: - `child_of` is an object of type `OpenTracing\SpanContext` or `OpenTracing\Span`. - `references` is an array of `OpenTracing\Reference`. - `tags` is an array with string keys and scalar values that represent OpenTracing tags. +- `finish_span_on_close` is a boolean that determines whether a span should be finished when the +scope is closed or not. ```php $span = $tracer->startActiveSpan('my_span', [ diff --git a/src/OpenTracing/Exceptions/InvalidSpanOption.php b/src/OpenTracing/Exceptions/InvalidSpanOption.php index effad2c..3cfdc36 100644 --- a/src/OpenTracing/Exceptions/InvalidSpanOption.php +++ b/src/OpenTracing/Exceptions/InvalidSpanOption.php @@ -103,10 +103,10 @@ public static function forInvalidReferenceSet($value) * @param mixed $value * @return InvalidSpanOption */ - public static function forCloseSpanOnFinish($value) + public static function forFinishSpanOnClose($value) { return new self(sprintf( - 'Invalid type for close_span_on_finish. Expected bool, got %s', + 'Invalid type for finish_span_on_close. Expected bool, got %s', is_object($value) ? get_class($value) : gettype($value) )); } diff --git a/src/OpenTracing/Mock/MockScope.php b/src/OpenTracing/Mock/MockScope.php index 7b75551..481165c 100644 --- a/src/OpenTracing/Mock/MockScope.php +++ b/src/OpenTracing/Mock/MockScope.php @@ -17,10 +17,24 @@ final class MockScope implements Scope */ private $scopeManager; - public function __construct(MockScopeManager $scopeManager, Span $span) - { + /** + * @var bool + */ + private $finishSpanOnClose; + + /** + * @param MockScopeManager $scopeManager + * @param Span $span + * @param bool $finishSpanOnClose + */ + public function __construct( + MockScopeManager $scopeManager, + Span $span, + $finishSpanOnClose + ) { $this->scopeManager = $scopeManager; $this->span = $span; + $this->finishSpanOnClose = $finishSpanOnClose; } /** @@ -28,6 +42,10 @@ public function __construct(MockScopeManager $scopeManager, Span $span) */ public function close() { + if ($this->finishSpanOnClose) { + $this->span->finish(); + } + $this->scopeManager->deactivate($this); } diff --git a/src/OpenTracing/Mock/MockScopeManager.php b/src/OpenTracing/Mock/MockScopeManager.php index a9cf560..48f0280 100644 --- a/src/OpenTracing/Mock/MockScopeManager.php +++ b/src/OpenTracing/Mock/MockScopeManager.php @@ -16,9 +16,11 @@ final class MockScopeManager implements ScopeManager /** * {@inheritdoc} */ - public function activate(Span $span) + public function activate(Span $span, $finishSpanOnClose = true) { - $this->scopes[] = new MockScope($this, $span); + $scope = new MockScope($this, $span, $finishSpanOnClose); + $this->scopes[] = $scope; + return $scope; } /** @@ -33,23 +35,6 @@ public function getActive() return $this->scopes[count($this->scopes) - 1]; } - /** - * {@inheritdoc} - * @param Span|MockSpan $span - */ - public function getScope(Span $span) - { - $scopeLength = count($this->scopes); - - for ($i = 0; $i < $scopeLength; $i++) { - if ($span === $this->scopes[$i]->getSpan()) { - return $this->scopes[$i]; - } - } - - return null; - } - public function deactivate(MockScope $scope) { $scopeLength = count($this->scopes); diff --git a/src/OpenTracing/Mock/MockSpan.php b/src/OpenTracing/Mock/MockSpan.php index 80dca05..86d8897 100644 --- a/src/OpenTracing/Mock/MockSpan.php +++ b/src/OpenTracing/Mock/MockSpan.php @@ -38,28 +38,14 @@ final class MockSpan implements Span */ private $duration; - /** - * @var bool - */ - private $closeOnFinish; - - /** - * @var ScopeManager - */ - private $scopeManager; - public function __construct( - ScopeManager $scopeManager, $operationName, MockSpanContext $context, - $startTime = null, - $closeOnFinish = false + $startTime = null ) { - $this->scopeManager = $scopeManager; $this->operationName = $operationName; $this->context = $context; $this->startTime = $startTime ?: time(); - $this->closeOnFinish = $closeOnFinish; } /** @@ -91,14 +77,6 @@ public function finish($finishTime = null) { $finishTime = ($finishTime ?: time()); $this->duration = $finishTime - $this->startTime; - - if (!$this->closeOnFinish) { - return; - } - - if (($scope = $this->scopeManager->getScope($this)) !== null) { - $scope->close(); - } } public function isFinished() diff --git a/src/OpenTracing/Mock/MockTracer.php b/src/OpenTracing/Mock/MockTracer.php index 1505078..7a84f54 100644 --- a/src/OpenTracing/Mock/MockTracer.php +++ b/src/OpenTracing/Mock/MockTracer.php @@ -4,7 +4,7 @@ use OpenTracing\Exceptions\UnsupportedFormat; use OpenTracing\ScopeManager; -use OpenTracing\SpanOptions; +use OpenTracing\StartSpanOptions; use OpenTracing\Tracer; use OpenTracing\SpanContext; @@ -42,8 +42,8 @@ public function __construct(array $injectors = [], array $extractors = []) */ public function startActiveSpan($operationName, $options = []) { - if (!($options instanceof SpanOptions)) { - $options = SpanOptions::create($options); + if (!($options instanceof StartSpanOptions)) { + $options = StartSpanOptions::create($options); } if (($activeSpan = $this->getActiveSpan()) !== null) { @@ -52,9 +52,7 @@ public function startActiveSpan($operationName, $options = []) $span = $this->startSpan($operationName, $options); - $this->scopeManager->activate($span); - - return $span; + return $this->scopeManager->activate($span, $options->shouldFinishSpanOnClose()); } /** @@ -62,8 +60,8 @@ public function startActiveSpan($operationName, $options = []) */ public function startSpan($operationName, $options = []) { - if (!($options instanceof SpanOptions)) { - $options = SpanOptions::create($options); + if (!($options instanceof StartSpanOptions)) { + $options = StartSpanOptions::create($options); } if (empty($options->getReferences())) { @@ -73,7 +71,6 @@ public function startSpan($operationName, $options = []) } $span = new MockSpan( - $this->scopeManager, $operationName, $spanContext, $options->getStartTime() diff --git a/src/OpenTracing/NoopScopeManager.php b/src/OpenTracing/NoopScopeManager.php index b7e22f7..4374f09 100644 --- a/src/OpenTracing/NoopScopeManager.php +++ b/src/OpenTracing/NoopScopeManager.php @@ -12,7 +12,7 @@ public static function create() /** * {@inheritdoc} */ - public function activate(Span $span) + public function activate(Span $span, $finishSpanOnClose) { } @@ -23,12 +23,4 @@ public function getActive() { return NoopScope::create(); } - - /** - * {@inheritdoc} - */ - public function getScope(Span $span) - { - return NoopScope::create(); - } } diff --git a/src/OpenTracing/NoopTracer.php b/src/OpenTracing/NoopTracer.php index d009fcf..9187c52 100644 --- a/src/OpenTracing/NoopTracer.php +++ b/src/OpenTracing/NoopTracer.php @@ -38,7 +38,7 @@ public function startSpan($operationName, $options = []) public function startActiveSpan($operationName, $finishSpanOnClose = true, $options = []) { - return NoopSpan::create(); + return NoopScope::create(); } /** diff --git a/src/OpenTracing/ScopeManager.php b/src/OpenTracing/ScopeManager.php index de7e8e1..9ccd245 100644 --- a/src/OpenTracing/ScopeManager.php +++ b/src/OpenTracing/ScopeManager.php @@ -13,14 +13,12 @@ interface ScopeManager * that previous spans can be resumed after a deactivation. * * @param Span $span the {@link Span} that should become the {@link #active()} + * @param bool $finishSpanOnClose whether span should automatically be finished when {@link Scope#close()} is called * - * Weather the span automatically closes when finish is called depends on - * the span options set during the creation of the span. See - * {@link SpanOptions#closeSpanOnFinish} - * - * @return Scope + * @return Scope instance to control the end of the active period for the {@link Span}. It is a + * programming error to neglect to call {@link Scope#close()} on the returned instance. */ - public function activate(Span $span); + public function activate(Span $span, $finishSpanOnClose); /** * Return the currently active {@link Scope} which can be used to access the @@ -31,16 +29,7 @@ public function activate(Span $span); * newly-created {@link Span} at {@link Tracer.SpanBuilder#startActive(boolean)} or {@link SpanBuilder#start()} * time rather than at {@link Tracer#buildSpan(String)} time. * - * @return \OpenTracing\Scope|null + * @return Scope|null */ public function getActive(); - - /** - * Access the scope of a given Span if available. - * - * @param Span $span - * - * @return \OpenTracing\Scope|null - */ - public function getScope(Span $span); } diff --git a/src/OpenTracing/SpanOptions.php b/src/OpenTracing/StartSpanOptions.php similarity index 89% rename from src/OpenTracing/SpanOptions.php rename to src/OpenTracing/StartSpanOptions.php index b9bbead..f8d101a 100644 --- a/src/OpenTracing/SpanOptions.php +++ b/src/OpenTracing/StartSpanOptions.php @@ -5,7 +5,7 @@ use OpenTracing\Exceptions\InvalidReferencesSet; use OpenTracing\Exceptions\InvalidSpanOption; -final class SpanOptions +final class StartSpanOptions { /** * @var Reference[] @@ -27,13 +27,13 @@ final class SpanOptions * * @var bool */ - private $closeSpanOnFinish = true; + private $finishSpanOnClose = true; /** * @param array $options * @throws InvalidSpanOption when one of the options is invalid * @throws InvalidReferencesSet when there are inconsistencies about the references - * @return SpanOptions + * @return StartSpanOptions */ public static function create(array $options) { @@ -86,12 +86,12 @@ public static function create(array $options) $spanOptions->startTime = $value; break; - case 'close_span_on_finish': + case 'finish_span_on_close': if (!is_bool($value)) { - throw InvalidSpanOption::forCloseSpanOnFinish($value); + throw InvalidSpanOption::forFinishSpanOnClose($value); } - $spanOptions->closeSpanOnFinish = $value; + $spanOptions->finishSpanOnClose = $value; break; default: @@ -105,15 +105,15 @@ public static function create(array $options) /** * @param Span|SpanContext $parent - * @return SpanOptions + * @return StartSpanOptions */ public function withParent($parent) { - $newSpanOptions = new SpanOptions(); + $newSpanOptions = new StartSpanOptions(); $newSpanOptions->references[] = self::buildChildOf($parent); $newSpanOptions->tags = $this->tags; $newSpanOptions->startTime = $this->startTime; - $newSpanOptions->closeSpanOnFinish = $this->closeSpanOnFinish; + $newSpanOptions->finishSpanOnClose = $this->finishSpanOnClose; return $newSpanOptions; } @@ -146,9 +146,9 @@ public function getStartTime() /** * @return bool */ - public function getCloseSpanOnFinish() + public function shouldFinishSpanOnClose() { - return $this->closeSpanOnFinish; + return $this->finishSpanOnClose; } private static function buildChildOf($value) diff --git a/src/OpenTracing/Tracer.php b/src/OpenTracing/Tracer.php index 05653a5..ef63194 100644 --- a/src/OpenTracing/Tracer.php +++ b/src/OpenTracing/Tracer.php @@ -20,7 +20,7 @@ public function getScopeManager(); * Tracer::getScopeManager()->getActive()->getSpan(), * and null will be returned if {@link Scope#active()} is null. * - * @return Span|null + * @return ScopedSpan|null */ public function getActiveSpan(); @@ -48,14 +48,15 @@ public function getActiveSpan(); * } * * @param string $operationName - * @param array|SpanOptions $options A set of optional parameters: + * @param array|StartSpanOptions $options A set of optional parameters: * - Zero or more references to related SpanContexts, including a shorthand for ChildOf and * FollowsFrom reference types if possible. * - An optional explicit start timestamp; if omitted, the current walltime is used by default * The default value should be set by the vendor. * - Zero or more tags - * - CloseSpanOnFinish option which defaults to true. - * @return Span + * - FinishSpanOnClose option which defaults to true. + * + * @return Scope */ public function startActiveSpan($operationName, $options = []); @@ -63,7 +64,7 @@ public function startActiveSpan($operationName, $options = []); * Starts and returns a new `Span` representing a unit of work. * * @param string $operationName - * @param array|SpanOptions $options + * @param array|StartSpanOptions $options * @return Span * @throws InvalidSpanOption for invalid option * @throws InvalidReferencesSet for invalid references set diff --git a/tests/OpenTracing/Mock/MockScopeManagerTest.php b/tests/OpenTracing/Mock/MockScopeManagerTest.php index 4e626f7..a119243 100644 --- a/tests/OpenTracing/Mock/MockScopeManagerTest.php +++ b/tests/OpenTracing/Mock/MockScopeManagerTest.php @@ -28,9 +28,9 @@ public function testActivateSuccess() public function testGetScopeReturnsNull() { $tracer = new MockTracer(); - $span = $tracer->startSpan(self::OPERATION_NAME); + $tracer->startSpan(self::OPERATION_NAME); $scopeManager = new MockScopeManager(); - $this->assertNull($scopeManager->getScope($span)); + $this->assertNull($scopeManager->getActive()); } public function testGetScopeSuccess() @@ -39,7 +39,7 @@ public function testGetScopeSuccess() $span = $tracer->startSpan(self::OPERATION_NAME); $scopeManager = new MockScopeManager(); $scopeManager->activate($span); - $scope = $scopeManager->getScope($span); + $scope = $scopeManager->getActive(); $this->assertSame($span, $scope->getSpan()); } @@ -49,7 +49,7 @@ public function testDeactivateSuccess() $span = $tracer->startSpan(self::OPERATION_NAME); $scopeManager = new MockScopeManager(); $scopeManager->activate($span); - $scope = $scopeManager->getScope($span); + $scope = $scopeManager->getActive(); $scopeManager->deactivate($scope); $this->assertNull($scopeManager->getActive()); } diff --git a/tests/OpenTracing/Mock/MockSpanTest.php b/tests/OpenTracing/Mock/MockSpanTest.php index e4483b3..f50367f 100644 --- a/tests/OpenTracing/Mock/MockSpanTest.php +++ b/tests/OpenTracing/Mock/MockSpanTest.php @@ -22,7 +22,6 @@ public function testCreateSpanSuccess() { $startTime = time(); $span = new MockSpan( - NoopScopeManager::create(), self::OPERATION_NAME, MockSpanContext::createAsRoot(), $startTime @@ -35,7 +34,6 @@ public function testCreateSpanSuccess() public function testAddTagsAndLogsToSpanSuccess() { $span = new MockSpan( - NoopScopeManager::create(), self::OPERATION_NAME, MockSpanContext::createAsRoot() ); @@ -51,7 +49,6 @@ public function testSpanIsFinished() { $startTime = time(); $span = new MockSpan( - NoopScopeManager::create(), self::OPERATION_NAME, MockSpanContext::createAsRoot(), $startTime diff --git a/tests/OpenTracing/Mock/MockTracerTest.php b/tests/OpenTracing/Mock/MockTracerTest.php index 5b92aa0..c401b76 100644 --- a/tests/OpenTracing/Mock/MockTracerTest.php +++ b/tests/OpenTracing/Mock/MockTracerTest.php @@ -18,9 +18,9 @@ final class MockTracerTest extends PHPUnit_Framework_TestCase public function testStartActiveSpanSuccess() { $tracer = new MockTracer(); - $span = $tracer->startActiveSpan(self::OPERATION_NAME); + $scope = $tracer->startActiveSpan(self::OPERATION_NAME); $activeSpan = $tracer->getActiveSpan(); - $this->assertEquals($span, $activeSpan); + $this->assertEquals($scope->getSpan(), $activeSpan); } public function testStartSpanSuccess() diff --git a/tests/OpenTracing/SpanOptionsTest.php b/tests/OpenTracing/StartSpanOptionsTest.php similarity index 74% rename from tests/OpenTracing/SpanOptionsTest.php rename to tests/OpenTracing/StartSpanOptionsTest.php index 5b62f4a..aaab24c 100644 --- a/tests/OpenTracing/SpanOptionsTest.php +++ b/tests/OpenTracing/StartSpanOptionsTest.php @@ -4,14 +4,14 @@ use OpenTracing\Exceptions\InvalidSpanOption; use OpenTracing\NoopSpanContext; -use OpenTracing\SpanOptions; +use OpenTracing\StartSpanOptions; use OpenTracing\Reference; use PHPUnit_Framework_TestCase; /** - * @covers SpanOptions + * @covers StartSpanOptions */ -final class SpanOptionsTest extends PHPUnit_Framework_TestCase +final class StartSpanOptionsTest extends PHPUnit_Framework_TestCase { const REFERENCE_TYPE = 'a_reference_type'; @@ -19,7 +19,7 @@ public function testSpanOptionsCanNotBeCreatedDueToInvalidOption() { $this->expectException(InvalidSpanOption::class); - SpanOptions::create([ + StartSpanOptions::create([ 'unknown_option' => 'value' ]); } @@ -28,8 +28,8 @@ public function testSpanOptionsWithInvalidCloseOnFinishOption() { $this->expectException(InvalidSpanOption::class); - SpanOptions::create([ - 'close_span_on_finish' => 'value' + StartSpanOptions::create([ + 'finish_span_on_close' => 'value' ]); } @@ -37,7 +37,7 @@ public function testSpanOptionsCanNotBeCreatedBecauseInvalidStartTime() { $this->expectException(InvalidSpanOption::class); - SpanOptions::create([ + StartSpanOptions::create([ 'start_time' => 'abc' ]); } @@ -45,7 +45,7 @@ public function testSpanOptionsCanNotBeCreatedBecauseInvalidStartTime() /** @dataProvider validStartTime */ public function testSpanOptionsCanBeCreatedBecauseWithValidStartTime($startTime) { - $spanOptions = SpanOptions::create([ + $spanOptions = StartSpanOptions::create([ 'start_time' => $startTime ]); @@ -70,7 +70,7 @@ public function testSpanOptionsCanBeCreatedWithValidReference() 'references' => Reference::create(self::REFERENCE_TYPE, $context), ]; - $spanOptions = SpanOptions::create($options); + $spanOptions = StartSpanOptions::create($options); $references = $spanOptions->getReferences()[0]; $this->assertTrue($references->isType(self::REFERENCE_TYPE)); @@ -79,24 +79,24 @@ public function testSpanOptionsCanBeCreatedWithValidReference() public function testSpanOptionsDefaultCloseOnFinishValue() { - $options = SpanOptions::create([]); + $options = StartSpanOptions::create([]); - $this->assertTrue($options->getCloseSpanOnFinish()); + $this->assertTrue($options->shouldFinishSpanOnClose()); } - public function testSpanOptionsWithValidCloseOnFinishValue() + public function testSpanOptionsWithValidFinishSpanOnClose() { - $options = SpanOptions::create([ - 'close_span_on_finish' => false, + $options = StartSpanOptions::create([ + 'finish_span_on_close' => false, ]); - $this->assertFalse($options->getCloseSpanOnFinish()); + $this->assertFalse($options->shouldFinishSpanOnClose()); } public function testSpanOptionsAddsANewReference() { $context1 = NoopSpanContext::create(); - $spanOptions = SpanOptions::create([ + $spanOptions = StartSpanOptions::create([ 'child_of' => $context1, ]); $this->assertCount(1, $spanOptions->getReferences());