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 opentracing compatibility. #7

Merged
merged 4 commits into from
Feb 27, 2018
Merged

Conversation

jcchavezs
Copy link
Contributor

This PR adds compatibility with OpenTracing.

Ping @palazzem

@palazzem palazzem self-requested a review February 16, 2018 14:07
@palazzem palazzem added the 🍏 core Changes to the core tracing functionality label Feb 16, 2018
@palazzem palazzem added this to the 0.1.0 milestone Feb 16, 2018
Copy link

@vlad-mh vlad-mh left a comment

Choose a reason for hiding this comment

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

Couple of nitpicks. Overall structure is GTM

$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 👍

foreach ($tags as $key => $value) {
if ($key !== (string) $key) {
throw new InvalidArgumentException(
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

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.

$traceToBeSent = null;
break;
}
$tracesToBeSent[] = $span;
Copy link

Choose a reason for hiding this comment

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

I think it was supposed to be $traceToBeSent instead of $tracesToBeSent? We might want to rename one of the 2 vars to avoid confusion.

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. Nice catch!

Copy link
Contributor

@kevinlebrun kevinlebrun left a comment

Choose a reason for hiding this comment

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

LGTM.

The only thing that could bite you is the use of SPL exceptions. I do think you should scope those exceptions. It's totally fine to forward OpenTracing exceptions though.

Which versions of PHP are supported by the client?

* @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.

$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.

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.

);
foreach ($tags as $key => $value) {
if ($key !== (string) $key) {
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.

@@ -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

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.


$tracesToBeSent = [];

foreach ($this->traces as $trace) {
Copy link
Contributor

@kevinlebrun kevinlebrun Feb 21, 2018

Choose a reason for hiding this comment

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

It may be overkill but I see a case for a Traces object which basically is a collection that implements this filtering logic.

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 like the idea but I would really prefer to keep an array for performance issues. Having a TraceCollection object is a great idea but such a collection would introduce an immutable style that adds some performance overhead. I isolated the shifting (filtering + removing) logic to a private method tho.

@@ -13,28 +13,39 @@
const NAME = 'test_span';
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be renamed into OPERATION_NAME for consistency.

}
foreach ($tags as $key => $value) {
if ($key !== (string) $key) {
throw InvalidSpanArgument::forTagKey($key);
Copy link

@clutchski clutchski Feb 22, 2018

Choose a reason for hiding this comment

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

Will this crash a user program? I'm more in favour I think of letting it pass and logging a warning or using whatever the string representation is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. In all other languages we avoid raising exceptions from our tracing library (even if it's legit when designing a component that can fail). We should consider the library as a tool that you install and never impact your application, even if used "wrongly". In C# we did the same adding log for invalid arguments.

Bear in mind that if this is a hot-path in users' app (i.e. SetTag for each request), we may spam users' logs, so a rate-limited logger is eventually required.

Copy link
Contributor Author

@jcchavezs jcchavezs Feb 22, 2018

Choose a reason for hiding this comment

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

I have mixed feelings here. InvalidSpanArgument extends InvalidArgumentException which is a DomainException: exceptions that should not come up in production (as opposed to RuntimeException which are exceptions coming from the runtime). In the other hand, framework instrumentation libraries should have this issue well controlled as you most likely use standard OpenTracing tags. Finally, this is about the key of the tag, not the value so in 99% of the cases, this will be a harcoded or fixed value (from OT tags) so I really see this as a legit exception.

As a note, framework instrumentation libraries will probably do the try/catch in order to decide what to do with this exception, adding a logger here is not only adding a dependency on the tracer but adding a logger reference on every span. In addition, OpenTracing itself throws some exceptions when things that should be checked statically are not (like the key of a tag being an string) so try/catch in middlewares/hooks is required.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I see, we can proceed that way then since it's not a RuntimeException.

Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

Good to me!

}
foreach ($tags as $key => $value) {
if ($key !== (string) $key) {
throw InvalidSpanArgument::forTagKey($key);
Copy link
Contributor

Choose a reason for hiding this comment

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

OK I see, we can proceed that way then since it's not a RuntimeException.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍏 core Changes to the core tracing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants