opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
505 stars 56 forks source link

SpanBuilder interface #80

Closed piotrooo closed 5 years ago

piotrooo commented 6 years ago

Background

Missing SpanBuilder interface as like in the opentracing-java implementation. https://github.com/opentracing/opentracing-java/blob/3e78ba6126519053918ddae9fb8782b239c33bf7/opentracing-api/src/main/java/io/opentracing/Tracer.java#L109

Problem

Missing SpanBuilder interface.

Proposal

To make API more compatible with other languages implementations I suggest to introduce SpanBuilder interface.

Questions to address

In PHP there is no thing like a internal interface, show how this should be implemented, as a new separated interface?

jcchavezs commented 6 years ago

@piotrooo thanks for coming by. I'd like to ask you to explain more about the actual Problem (explaining why this is needed). Regarding the more compatibility with other language implementations I am not entirely sure on that as I checked OT-ruby, OT-python and OT-go and none of them include such an interface. That said, it is worth mentioning that OT-PHP includes a StartSpanOptions for creation pattern.

piotrooo commented 6 years ago

I think a fluent builder can be more handy in use. E.g: Now if you want to add more tags:

$span->setTag('key1', 'value1');
$span->setTag('key2', 'value2');
//etc...

With fluent builder could be:

$spanBuilder
    ->withTag('key1', 'value1')
    ->withTag('key2', 'value2');
//etc..

Consider following:

$scope = $tracer->startActiveSpan('something', ['child_of' => $spanContext]);
$scope->getSpan()->setTag('key1', 'value1');
$scope->getSpan()->setTag('key2', 'value2');

and compare it with:

$tracer->buildSpan('something')
    ->asChildOf($spanContext)
    ->withTag('key1', 'value1')
    ->withTag('key2', 'value2')
    ->startActive($finishSpanOnClose);

That is IMHO more elegant way to handle that case.

jcchavezs commented 6 years ago

Actually you can achieve this:

$scope = $tracer->startActiveSpan('something', [
    'child_of' => $spanContext,
    'tags' => [
        'key' => 'value',
    ]
]);
piotrooo commented 6 years ago

Ahh, don't know about this, thanks.

But that way IMO is not OOP approach - as always in PHP arrays...

//EDIT But, I'm not fully agree with you. If you provide an API you don't have control about structures inside a data structure (like array). But that kind of builder forces you to implement specific implementation. This is provided by the StartSpanOptions class, but I think this could be provided explicit by the API.