opentracing / opentracing-php

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

Refactor inject/extract APIs #37

Closed jcchavezs closed 6 years ago

jcchavezs commented 6 years ago

From @lvht in https://github.com/opentracing/opentracing-php/issues/31

Please all me to make another comment on the following API,

function inject(SpanContext $spanContext, $format, Writer $carrier) function extract($format, Reader $carrier)

Both the Writer and Reader interface is needless, because it should defined in different implementations.

I think we should change them to

function inject(SpanContext $spanContext, $format, &$carrier) function extract($format, $carrier)

Maybe we could make the inject method return a $carrier instance.

So that we can use any type to transfer the span context.

With the old API, we should

<?php
$tracer = $factory->initTracer('foo');

$data = ['trace-id' => $_SERVER['HTTP_UBER_TRACE_ID']];
$textMap = TextMap::fromArray($data);
$context = $tracer->extract('text_map', $textMap);
$span = $tracer->startSpan('bar', ['child_of' => $context]);

$textMap = TextMap::fromArray([]);
$tracer->inject($clientSapn->getContext(), 'text_map', $textMap);
$injectTarget = $textMap->getIterator()->getArrayCopy();

But with the API I proposed, we only

<?php
$tracer = $factory->initTracer('foo');

$trace_id = $_SERVER['HTTP_UBER_TRACE_ID'];
$context = $tracer->extract('text_map', $trace_id);
// even we could
$context = $tracer->extract('text_map', $_SERVER['HTTP_UBER_TRACE_ID']);
$span = $tracer->startSpan('bar', ['child_of' => $context]);

$tracer->inject($clientSapn->getContext(), 'text_map', $trace_id);
header("Trace-Id: $trace_id");

All in all, we should not restrict the type of carrier. Remember that we are using the language of PHP, a weak type, dynamic language, not Java, not Go, not Python.

jcchavezs commented 6 years ago

Hi @lvht thanks for the feedback.

The standard says:

A carrier, whose type is dictated by the format. The Tracer implementation will encode the SpanContext in this carrier object according to the format.

This is something that I already faced and I am ok with removing the interfaces as of format should dictate the type here. I actually use a pretty similar design to the one you proposed in my Zipkin library: https://github.com/jcchavezs/zipkin-php/blob/master/src/Zipkin/Propagation/Setter.php

jcchavezs commented 6 years ago

Ping @felixfbecker @beberlei @tPl0ch

taoso commented 6 years ago

Could you @jcchavezs include this issue in milestone 1.0.0?