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

Adds activeSpanSource #22

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
55 changes: 32 additions & 23 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ In order to understand the library, one must first be familiar with the

OpenTracing-PHP can be installed via Composer:

```php
```bash
composer require jcchavezs/opentracing-php
```

Expand All @@ -38,7 +38,7 @@ The simplest starting point is to set the global tracer. As early as possible, d

### Creating a Span given an existing Request

To start a new `Span`, you can use the `StartSpanFromContext` method.
To start a new `Span`, you can use the `startActiveSpan` method.

```php
use Psr\Http\Message\RequestInterface;
Expand All @@ -53,7 +53,7 @@ To start a new `Span`, you can use the `StartSpanFromContext` method.
function doSomething(SpanContext $spanContext, ...) {
...

$span = GlobalTracer::getGlobalTracer()->startSpan('my_span', ChildOf::withContext($spanContext));
$span = GlobalTracer::getGlobalTracer()->startManualSpan('my_span', ['child_of' => $spanContext]);

...

Expand All @@ -69,30 +69,41 @@ To start a new `Span`, you can use the `StartSpanFromContext` method.

### Starting an empty trace by creating a "root span"

It's always possible to create a "root" `Span` with no parent or other causal
reference.
It's always possible to create a "root" `Span` with no parent or other causal reference.

```php
$span = $tracer->startSpan('my_span');
$span = $tracer->startActiveSpan('my_first_span');
...
$span->finish();
```

### Creating a (child) Span given an existing (parent) Span
#### Creating a child span assigning parent manually

```php
use OpenTracing\SpanReference\ChildOf;
use OpenTracing\SpanReference\ChildOf;

$parent = GlobalTracer::getGlobalTracer()->startManualSpan('parent'); $child = GlobalTracer::getGlobalTracer()->startManualSpan('child', [
'child_of' => $parent
]);
...
$child->finish();
...
$parent->finish();

Choose a reason for hiding this comment

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

shouldn't finish() always be called in a finally?

```

function xyz(Span $parentSpan, ...) {
...
$span = GlobalTracer::getGlobalTracer()->startSpan(
'my_span',
ChildOf::withContext($span->context())
);

$span->finish();
...
}
#### Creating a child span using automatic active span management
Every new span will take the active span as parent and it will take its spot.

```php
$parent = GlobalTracer::getGlobalTracer()->startManualSpan('parent'); ...

// Since the parent span has been created by using startActiveSpan we don't need
// to pass a reference for this child span
$child = GlobalTracer::getGlobalTracer()->startActiveSpan('my_second_span');
...
$child->finish();
...
$parent->finish();
```

### Serializing to the wire
Expand All @@ -112,7 +123,7 @@ reference.
);

try {
$span = $tracer->startSpanWithOptions('my_span', ['child_of' => $spanContext]);
$span = $tracer->startManualSpan('my_span', ['child_of' => $spanContext]);

$client = new GuzzleHttp\Client;

Expand All @@ -134,7 +145,7 @@ reference.

### Deserializing from the wire

When using http header for context propagation you can use either the `Request` or the `$_SERVER` variable:
When using http header for context propagation you can use the `Request` for example:

```php
use OpenTracing\Carriers\HttpHeaders;
Expand All @@ -160,19 +171,17 @@ cause problems for Tracer implementations. This is why the PHP API contains a

use OpenTracing\GlobalTracer;

// Do application work, buffer spans in memory
$application->run();

fastcgi_finish_request();

$tracer = GlobalTracer::getGlobalTracer();
$tracer->flush(); // release buffer to backend
$tracer->flush(); // sends span's data to the backend
```

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

Passing options to the pass can be done using either an array or the
Expand Down
40 changes: 40 additions & 0 deletions src/OpenTracing/ActiveSpanSource.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

namespace OpenTracing;

/**
* Keeps track of the current active `Span`.
*/
interface ActiveSpanSource
{
/**
* Activates an `Span`, so that it is used as a parent when creating new spans.
* The implementation must keep track of the active spans sequence, so
* that previous spans can be resumed after a deactivation.
*
* @param Span $span
*/
public function activate(Span $span);

/**
* Returns current active `Span`.
*
* @return Span
*/
public function getActiveSpan();

/**
* Deactivate the given `Span`, restoring the previous active one.
*
* This method must take in consideration that a `Span` may be deactivated
* when it's not really active. In that case, the current active stack
* must not be changed (idempotency).
*
* Do not confuse it with the finish concept:
* - $span->deactivate() -> the span is not active but still "running"
* - $span->finish() -> the span is finished and deactivated
*
* @param Span $span
*/
public function deactivate(Span $span);
}
2 changes: 1 addition & 1 deletion src/OpenTracing/GlobalTracer.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ final class GlobalTracer
* Those who use GlobalTracer (rather than directly manage a Tracer instance)
* should call setGlobalTracer as early as possible in bootstrap, prior to
* start a new span. Prior to calling `setGlobalTracer`, any Spans started
* via the `StartSpan` (etc) globals are noops.
* via the `startActiveSpan` (etc) globals are noops.
*
* @param Tracer $tracer
*/
Expand Down
24 changes: 24 additions & 0 deletions src/OpenTracing/NoopActiveSpanSource.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

namespace OpenTracing;

final class NoopActiveSpanSource implements ActiveSpanSource
{
public static function create()
{
return new self();
}

public function activate(Span $span)
{
}

public function getActiveSpan()
{
return NoopSpan::create();
}

public function deactivate(Span $span)
{
}
}
3 changes: 3 additions & 0 deletions src/OpenTracing/NoopSpan.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ public function finish($finishTime = null, array $logRecords = [])
{
}

/**
* {@inheritdoc}
*/
public function overwriteOperationName($newOperationName)
{
}
Expand Down
25 changes: 17 additions & 8 deletions src/OpenTracing/NoopTracer.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

use OpenTracing\Propagators\Reader;
use OpenTracing\Propagators\Writer;
use TracingContext\TracingContext;

final class NoopTracer implements Tracer
{
Expand All @@ -13,16 +12,16 @@ public static function create()
return new self();
}

public function startSpan(
$operationName,
SpanReference $parentReference = null,
$startTimestamp = null,
array $tags = []
) {
/**
* {@inheritdoc}
*/
public function startActiveSpan($operationName, $options = [])
{

return NoopSpan::create();
}

public function startSpanWithOptions($operationName, $options)
public function startManualSpan($operationName, $options = [])
{
return NoopSpan::create();
}
Expand All @@ -39,4 +38,14 @@ public function extract($format, Reader $carrier)
public function flush()
{
}

public function activeSpanSource()
{
return NoopActiveSpanSource::create();
}

public function activeSpan()
{
return NoopSpan::create();
}
}
4 changes: 3 additions & 1 deletion src/OpenTracing/Span.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ public function getOperationName();
public function getContext();

/**
* Finishes the span. As an implementor, make sure you call {@see Tracer::deactivate()}
* otherwise new spans might try to be child of this one.
*
* @param float|int|\DateTimeInterface|null $finishTime if passing float or int
* it should represent the timestamp (including as many decimal places as you need)
* @param array $logRecords
* @return mixed
*/
public function finish($finishTime = null, array $logRecords = []);

Expand Down
2 changes: 1 addition & 1 deletion src/OpenTracing/SpanContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function getBaggageItem($key);
* Creates a new SpanContext out of the existing one and the new key:value pair.
*
* @param string $key
* @param wstring $value
* @param string $value
* @return SpanContext
*/
public function withBaggageItem($key, $value);
Expand Down
6 changes: 4 additions & 2 deletions src/OpenTracing/SpanOptions.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ final class SpanOptions
private $tags = [];

/**
* @var int|float|\DateTime
* @var int|float|\DateTimeInterface
*/
private $startTime;

Expand Down Expand Up @@ -78,6 +78,8 @@ private static function buildChildOf($value)
return ChildOf::fromContext($value->getContext());
} elseif ($value instanceof SpanContext) {
return ChildOf::fromContext($value);
} elseif ($value instanceof ChildOf) {
return $value;
}

throw InvalidSpanOption::invalidChildOf($value);
Expand Down Expand Up @@ -123,7 +125,7 @@ public function getTags()
}

/**
* @return int|float|\DateTime if returning float or int it should represent
* @return int|float|\DateTimeInterface if returning float or int it should represent
* the timestamp (including as many decimal places as you need)
*/
public function getStartTime()
Expand Down
6 changes: 6 additions & 0 deletions src/OpenTracing/SpanReference/ChildOf.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,10 @@ public function referencedContext()
{
return $this->spanContext;
}

public function isEqual(SpanReference $spanReference)
{
return ($spanReference instanceof ChildOf)
&& $this->spanContext->isEqual($spanReference->referencedContext());
}
}
55 changes: 44 additions & 11 deletions src/OpenTracing/Tracer.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,28 +25,61 @@ interface Tracer
const FORMAT_HTTP_HEADERS = 3;

/**
* @deprecated use either startActiveSpan or startManualSpan instead.

Choose a reason for hiding this comment

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

Imo the initial official version shouldn't have anything deprecated.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The problem is that we'd prefer the python one to be released before merging this one so either we wait or either we release the library without the active span record.

Choose a reason for hiding this comment

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

what is the reason for that?

* As implementor consider this as a backward compatibility alias for
* startActiveSpan
*/
public function startSpan($operationName, $options = []);

/**
* Starts and returns a new `Span` representing a unit of work.
*
* This method differs from `startManualSpan` because it uses in-process
* context propagation to keep track of the current active `Span` (if
* available).
*
* Starting a root `Span` with no casual references and a child `Span`
* in a different function, is possible without passing the parent
* reference around:
*
* function handleRequest(Request $request, $userId)
* {
* $rootSpan = $this->tracer->startActiveSpan('request.handler');
* $user = $this->repository->getUser($userId);
* }
*
* function getUser($userId)
* {
* // `$childSpan` has `$rootSpan` as parent.
* $childSpan = $this->tracer->startActiveSpan('db.query');
* }
*
* @param string $operationName
* @param SpanReference|null $parentReference
* @param float|int|\DateTimeInterface|null $startTimestamp if passing float or int
* it should represent the timestamp (including as many decimal places as you need)
* @param array $tags
* @param array|SpanOptions $options
* @return Span
* @throws InvalidSpanOption for invalid option
*/
public function startSpan(
$operationName,
SpanReference $parentReference = null,
$startTimestamp = null,
array $tags = []
);
public function startActiveSpan($operationName, $options = []);

/**
* Starts and returns a new Span representing a unit of work.
*
* @param string $operationName
* @param array|SpanOptions $options
* @return Span
* @throws InvalidSpanOption for invalid option
*/
public function startSpanWithOptions($operationName, $options);
public function startManualSpan($operationName, $options = []);

/**
* @return ActiveSpanSource
*/
public function activeSpanSource();

Choose a reason for hiding this comment

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

the coding style is inconsistent, some places use get*, this place here does not. It looks like a direct translation from python, where this is a coding convention.

Copy link
Owner Author

@jcchavezs jcchavezs Jun 15, 2017

Choose a reason for hiding this comment

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

I tried to avoid all gets in the code. If there is any around I will open a PR changing them.

Choose a reason for hiding this comment

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

@jcchavezs you mentioned you consider get/set anti patterns, I'm curious why? It certainly is common in PHP libraries

Copy link
Owner Author

@jcchavezs jcchavezs Jul 6, 2017

Choose a reason for hiding this comment

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

Based on the changes on #30 I will change the methods here to use get prefixes. About setters, as I expressed before (somewhere) I prefer meaningful methods like $object->doSomethingYouUnderstand() rather than $object->setYourStateTo($whatever) (for example, ->activate() over ->setStatus('active').

Copy link

@felixfbecker felixfbecker Jul 6, 2017

Choose a reason for hiding this comment

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

yeah, totally agree with meaningful method names like activate(), as long as they are verbs. E.g. activeSpanSource is not a verb and therefor gives no real indication what it does (does it return the value? or does it set it? or both, like jQuery?)


/**
* @return Span
*/
public function activeSpan();

/**
* @param SpanContext $spanContext
Expand Down