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.

Sign up for GitHub

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

Jump to bottom

Adds opentracing compatibility. #7

Merged
merged 4 commits into from
Feb 27, 2018
Merged

Adds opentracing compatibility. #7

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0795580
Adds opentracing compatibility.
jcchavezs Feb 16, 2018
6f16ae0
Adds tests for the propagation.
jcchavezs Feb 16, 2018
dc2f460
Removes logger dependency and fixes dev autoloading.
jcchavezs Feb 16, 2018
d405aaf
Adds improvements based on feedback.
jcchavezs Feb 22, 2018
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
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
"minimum-stability": "dev",
"require": {
"opentracing/opentracing": "1.0.0-beta2",
"symfony/polyfill": "~1.7.0"
"symfony/polyfill": "~1.7.0",
"guzzlehttp/psr7": "^1.4@dev"
},
"require-dev": {
"phpunit/phpunit": "~5.7.19",
Expand Down
218 changes: 124 additions & 94 deletions src/DDTrace/Span.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,46 +4,25 @@

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
Copy link
Contributor

Choose a reason for hiding this comment

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

A detail but Name should be updated to operationName.

* might be "http.handler", "fileserver.upload" or "video.decompress".
* Name should be set on every span.
*
* @var string
*/
private $name;
private $operationName;

/**
* @var OpenTracingContext
*/
private $context;

/**
* Resource is a query to a service. A web application might use
Expand Down Expand Up @@ -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
Expand All @@ -89,72 +68,66 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

$startTime is more consistent with privates and $finishTime.

) {
$this->tracer = $tracer;
$this->name = (string) $name;
$this->service = $service;
$this->context = $context;
Copy link

Choose a reason for hiding this comment

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

To my understanding, since this constructor is part of the publicly exposed API, we have no guarantee $context is not null, and we're not checking in the following function when using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context can't be null as the typehint forces it to be SpanContext.

Copy link

Choose a reason for hiding this comment

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

ah cool then 👍

$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();
}

/**
* @return string
*/
public function getTraceId()
{
return $this->traceId;
return $this->context->getTraceId();
}

/**
* @return string
*/
public function getSpanId()
{
return $this->spanId;
return $this->context->getSpanId();
}

/**
* @return null|string
*/
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;
}

/**
Expand All @@ -174,7 +147,7 @@ public function getService()
}

/**
* @return string
* @return string|null
*/
public function getType()
{
Expand All @@ -184,9 +157,9 @@ public function getType()
/**
* @return int
*/
public function getStart()
public function getStartTime()
{
return $this->start;
return $this->startTime;
}

/**
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is_string is a little be more readable but... detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know but this way is more performant and observability should have the least overhead possible.

throw new InvalidArgumentException(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should start to scope exceptions to the context of this library. A lot of client code tend to use catch all exceptions and you have this opportunity to introduce semantic exceptions too.

sprintf('First argument expected to be string, got %s', gettype($key))
Copy link

Choose a reason for hiding this comment

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

I think we can re-word this exception to better reflect the function's new signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I am not a native so do you have any advice on how the reword would be better?

Copy link

@vlad-mh vlad-mh Feb 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

uhm, something like Invalid key type in given span tags. Expected string, got %s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally. The message I wrote is silly.

);
}

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.
Expand All @@ -249,9 +242,11 @@ public function setError($e)

if (($e instanceof Exception) || ($e instanceof Throwable)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

From http://php.net/manual/en/class.throwable.php

PHP classes cannot implement the Throwable interface directly, and must instead extend Exception.

Is it not enough to typehint for Exception ?

Copy link
Contributor Author

@jcchavezs jcchavezs Feb 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Throwable comes in PHP 7 and is meant for errors and exceptions. In PHP 5.6 there is not Throwable interface so we include it with the polifyll but need to check for both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok, good to know! I didn't see the new Error object in PHP 7.

$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;
}

Expand All @@ -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;
}

/**
Expand All @@ -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);
}
}
Loading