Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#60] Changes the API for the In process context propagation #61

Merged
merged 2 commits into from
Apr 13, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]);

...

Expand All @@ -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();
```
Expand All @@ -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
Expand All @@ -112,10 +112,11 @@ $root = $tracer->startActiveSpan('php');

$http = $tracer->startActiveSpan('http');
file_get_contents('http://php.net');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this example is promoting bad naming practice. root and http should really be called scope, and they should be in different syntax scopes (not sure if php allows that - if not, best to introduce a function in the example)

$http->finish();
$http->close();

$controller->finish();
$root->finish();
$controller->close();

$root->close();
```

#### Creating a child span assigning parent manually
Expand Down Expand Up @@ -153,11 +154,11 @@ $child = GlobalTracer::get()->startActiveSpan('my_second_span');

...

$child->finish();
$child->close();

...

$parent->finish();
$parent->close();
```

### Serializing to the wire
Expand Down Expand Up @@ -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:
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move "or not" after "finished"


```php
$span = $tracer->startActiveSpan('my_span', [
Expand Down
4 changes: 2 additions & 2 deletions src/OpenTracing/Exceptions/InvalidSpanOption.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
));
}
Expand Down
22 changes: 20 additions & 2 deletions src/OpenTracing/Mock/MockScope.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,35 @@ 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;
}

/**
* {@inheritdoc}
*/
public function close()
{
if ($this->finishSpanOnClose) {
$this->span->finish();
}

$this->scopeManager->deactivate($this);
}

Expand Down
23 changes: 4 additions & 19 deletions src/OpenTracing/Mock/MockScopeManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not familiar with php syntax - what does this mean? looks like assigning scalar to array.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means to append entry to an array

return $scope;
}

/**
Expand All @@ -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);
Expand Down
24 changes: 1 addition & 23 deletions src/OpenTracing/Mock/MockSpan.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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()
Expand Down
15 changes: 6 additions & 9 deletions src/OpenTracing/Mock/MockTracer.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use OpenTracing\Exceptions\UnsupportedFormat;
use OpenTracing\ScopeManager;
use OpenTracing\SpanOptions;
use OpenTracing\StartSpanOptions;
use OpenTracing\Tracer;
use OpenTracing\SpanContext;

Expand Down Expand Up @@ -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) {
Expand All @@ -52,18 +52,16 @@ public function startActiveSpan($operationName, $options = [])

$span = $this->startSpan($operationName, $options);

$this->scopeManager->activate($span);

return $span;
return $this->scopeManager->activate($span, $options->shouldFinishSpanOnClose());
}

/**
* {@inheritdoc}
*/
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())) {
Expand All @@ -73,7 +71,6 @@ public function startSpan($operationName, $options = [])
}

$span = new MockSpan(
$this->scopeManager,
$operationName,
$spanContext,
$options->getStartTime()
Expand Down
10 changes: 1 addition & 9 deletions src/OpenTracing/NoopScopeManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public static function create()
/**
* {@inheritdoc}
*/
public function activate(Span $span)
public function activate(Span $span, $finishSpanOnClose)
{
}

Expand All @@ -23,12 +23,4 @@ public function getActive()
{
return NoopScope::create();
}

/**
* {@inheritdoc}
*/
public function getScope(Span $span)
{
return NoopScope::create();
}
}
2 changes: 1 addition & 1 deletion src/OpenTracing/NoopTracer.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public function startSpan($operationName, $options = [])
public function startActiveSpan($operationName, $finishSpanOnClose = true, $options = [])
{

return NoopSpan::create();
return NoopScope::create();
}

/**
Expand Down
21 changes: 5 additions & 16 deletions src/OpenTracing/ScopeManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
Loading