Do we really need the Carriers?

[taoso]

In my opinion, opentracing-php is a set of interfaces, because the opentracing specification does has any implementation detail at all.

As we have already defined the interface of Reader and Write, there is no need to define the TextMap and HttpHeaders carriers.

And opentracing-php should not require psr/http-message at all.

And again, the GlobalTracer makes no sense. GlobalTracer is like a container. However, almost every modern php project has its own container to implements the singleton pattern. There is no need to shift another one.

[jonahgeorge]

I think the benefit of the GlobalTracer will really become apparent with the opentracing-contrib packages. By having a container defined, we'll be able to add tracing to various classes with effectively zero configuration. Given that these classes should allow for the tracer to be injected for testing, I don't believe there would be any issues with using your own container.

As for carriers, this issue might do a better job explaining the problem than I can.

[jcchavezs]

Hi @lvht, you are right, OT is a set of interfaces, however the standard defines specific propagation formats and for a matter of convenience we include this handy classes that otherwise will be replicated on every single applications. In addition to that, they are in other OT libraries like the golang and java and +1 to https://github.com/opentracing/opentracing-java/issues/31.

Regarding the global tracer there is a convenience for it when you want to instrument common libraries or frameworks (whether they use DI or not). In addition, this global tracer still work perfectly with any container you may use.

Thanks for your feedback!

[jcchavezs]

@lvht are you OK with this answers? Do you want to add something else?

[taoso]

In my opinion, the global tracer is needless. It is a helper class. It do be a convenience for some people, but it should not be part about opentracing-php package. Any thing not defined by the opentracing specification should not included in this package.

[taoso]

BTW, I am trying to introduce opentracing in our company. As the jukylin/jaeger-php has many issues (it does not work at all when I cloned it from github), I made my own fork at lvht/jaeger-php.

The lvht/jaeger-php has implemented your latest opentracing API. Hope the API will makes its 1.0.0 release as soon as possible.

[jcchavezs]

@lvht we want to release 1.0.0 ASAP. Have a look at this PR and give your feedback: https://github.com/opentracing/opentracing-php/pull/20.

PS: Come by to our chat to discuss further things https://gitter.im/opentracing/opentracing-php

[jcchavezs]

@lvht Is for discussing in-process context propagation. Let's continue discussing it here.

[jcchavezs]

From @lvht

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.

[taoso]

I have said what I want to see in #20 . Thank you @jcchavezs .

[jcchavezs]

From @lvht

Despite wether we should offer Carriers, the HttpHeaders has offer a method of fromRequest which depends on Psr\Http\Message\RequestInterface.

The opentracing specification do specify that all implementation should support inject into and extract from the http header carrier. But it does not means we should depends another Psr interface. Because how to get and set http header is no of our API's business.

If we really need to shift a HttpHeaders carrier, we should only offer the fromHeaders method.

[jcchavezs]

From @lvht

And finally, we offer some consts defined in src/OpenTracing/Propagation/Formats.php

Do we really need these consts? The opentracing specification do defined that every implementation should support carrier of type text-map, http-headers, and binary. But it does not means we should define three consts of value "text_map", "http_headers", "binary". The value of the these types should be defined by implementation.

Our opentracing-php should only be the opentracing specification written by PHP language. We should not offer any thing not defined in the specification.

Thank you.

[jcchavezs]

@lvht I included this issue in milestone 1.0.0. So please keep this discussion here, we won't release 1.0.0 before this issue becomes solved so don't worry.

[taoso]

@jcchavezs Thank you for reading my comments or complaint. Thank you.

[jcchavezs]

Hi @lvht thanks for the feedback.

1. Regarding the inject/extract

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

2. Regarding the Psr\RequestInterface

Http Headers comes as a string[][] which in our use case becomes inconvenient as of you have to do the array_map operation every time you need to pass it to the HttpHeaders. In that sense I would get rid of this fromRequest method if we change the fromHeaders method to explicitly receive the headers in the format Psr\RequestInterface returns it (string[][]). That makes sense to me and makes a explicit difference between the HttpHeaders and the TextMap carriers (BTW, should they be in a folder called Carrier or in the Propagators folder?).

3. Regarding the format constants

OT spec defines explicitly the formats that should be supported

Both injection and extraction rely on an extensible format parameter that dictates the type of the associated "carrier" as well as how a SpanContext is encoded in that carrier. All of the following formats must be supported by all Tracer implementations. Text Map: an arbitrary string-to-string map with an unrestricted character set for both keys and values HTTP Headers: a string-to-string map with keys and values that are suitable for use in HTTP headers (a la RFC 7230. In practice, since there is such "diversity" in the way that HTTP headers are treated in the wild, it is strongly recommended that Tracer implementations use a limited HTTP header key space and escape values conservatively. Binary: a (single) arbitrary binary blob representing a SpanContext

Since they are explicitly defined by the OT spec it makes sense to express them in the API by constants. In the other hand, the idea of having a OT interfaces makes it a requirement as of, imagine your implementation call them http_headers while mine calls it httpheaders, if I want to switch from your implementation to mine I should have the format parameter in every Tracer::inject and Tracer::extract. Using constants gets rids of all this ambiguity keeping the contract if you switch between implementations.

Any thoughts on this @felixfbecker @beberlei @jonahgeorge @tPl0ch?

[taoso]

@jcchavezs

Http Headers comes as a string[][]

Http headers does not comes as a string[][] naturally. In fact, we seldomly need to process multi header value with the same name.

As defined in RFC2616

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

Multi header value with the same name like

Cache-Control: no-cache
Cache-Control: no-store

can be treated as Cache-Control: no-cache, no-store safely.

There is one exception, the Set-Cookie header. You can send multi Set-Cookie header, but you cannot combine them with one. However, it is not a problem, because we should not set cookie by sending Set-Cookie header ourself. We should call setcookie function or others. And it should not influence our API design.

So I think parse http header as a string[][] is a over-designed aspect of Psr7. There is no need to depend on its mistake.

As OT spec says

In practice, since there is such "diversity" in the way that HTTP headers are treated in the wild, it is strongly recommended that Tracer implementations use a limited HTTP header key space and escape values conservatively.

The only difference between HTTP Headers and Text Map is that the former only support a limited range of char as key. In fact, you can only set http header name by pure ascii.

[taoso]

@jcchavezs The consts defined in Formats.php should be moved into Ext, just like Tags.php. They are only recommendations.

[taoso]

@jcchavezs OT spec do defines explicitly the formats, but not the represent value of formats. opentracing-php is just a PHP language representation of OT spec, it should not have any specification that OT spec does not have. The opentracing-php should never be a super-set of OT spec.

[jcchavezs]

Regarding the headers, PSR7 is the standard for requests so even if we don't agree with it I believe that is the direction we are moving to so if we should add support for headers (which I believe we should) I would go for the standard. If someone wants to implement its own HttpHeaders propagation that is OK in my opinion.

Regarding the const for format I still don't agree and you haven't commented the point I made by switching implementations under the standard. I truly believe the point of having this standard is that changes in the implementation should be transparent for clients.

[taoso]

@jcchavezs Maybe do not shift an HttpHeaders carrier is better.

[felixfbecker]

@lvht Having constants that define the keys for all implementations and the user to use is crucial to enable swapping out implementations seamlessly. All OT interface implementations in every language define them.

You can like PSR7 or not, it is the interop standard that PHP agreed upon. You can use whatever you like in your app and convert it to PSR7 with a helper function, but we should use the standard.

[tedsuo]

I agree that we need the constants. Tracers need to understand what kind of carrier they are injecting/extracting from. If the designations are not standard, then vendor lock-in starts to creep in.

When thinking about OT instrumentation, it's important to imagine you are instrumenting a library or framework, and have no knowledge of the tracer being used, or any 3rd-party helpers. If I write a plugin to instrument an http client library, there needs to be a standard way to inform the tracer to inject into an http carrier. That way, we can all share instrumentation libraries, which is an important part of the OT project.

[taoso]

@tedsuo If you agreed that we need these constants, could you gave your opinion on why the OT spec does not defined these constants?

I think you have forgotten that the opentracing-php is only a OT spec written by PHP language. It should never include any limit that the OT spec does not requires.

[felixfbecker]

@lvht they are defined in the spec. https://github.com/opentracing/specification/blob/master/specification.md#inject-a-spancontext-into-a-carrier

[taoso]

@felixfbecker Please specify which line they are defined. Thank you.

[felixfbecker]

A format descriptor (typically but not necessarily a string constant)

Note: required formats for injection and extraction Both injection and extraction rely on an extensible format parameter that dictates the type of the associated "carrier" as well as how a SpanContext is encoded in that carrier. All of the following formats must be supported by all Tracer implementations.

• Text Map
• HTTP Headers
• Binary

The OT spec defines three required formats that must be possible to pass as a format descriptor to extract or inject a span context. It states that the format descriptor is typically represented as a string constant. The obvious choice that literally every OT spec implementation in every language chose is to provide the three required supported formats as string constants so implementations and users can use them for interop while still being able to define their own. I see absolutely no reason why we would not want to do that in opentracing-php.

[taoso]

Thank you @felixfbecker for your reply. Thank you.

The OT spec does require that all implementations must support the carrier of type text-map, http-headers, and binary. However, the spec also says,

A format descriptor (typically but not necessarily a string constant)

The format descriptor could be string constant, but the value of these constants has not be specified, hasn't it? The descriptor could event be an object!

In our opentracing-php, we defined a const like

<?php
const HTTP_HEADERS = 'http_headers';

Does the OT spec requires that the value of HTTP_HEADERS must be http_headers?

In my opinion, the value of these constants does not matter at all! These value should be defined by the implementation and be vendor specific.

But If you want to switch OT implementation from on vendor to another? You should change all your code that depend on these constants' values. Why? Because they are vendor define! You should make these code as dependence as less as possible.

In fact, we do not use this const so much. I think, we only need these constants in two place.

• One is in the beginning of the request, we need extract data from carrier.
• The other one is in the beginning of the out call, like curl, rpc, db, etc, we need inject data into carrier

Both of these place is limited and easy to change.

[felixfbecker]

@lvht please explain how you would ensure interop between different tracer implementations and consumers if we did not specify these constants and the problem we would solve by removing them

[taoso]

please explain how you would ensure interop between different tracer implementations and consumers if we did not specify these constants and the problem we would solve by removing them

What you pointed out do be an small issue. The root of this issue is that the OT spec does not define the value of these constant. This issue should be resolved by update the OT spec.

Our opentracing-php is just a binding of the OT spec.

Please @felixfbecker see my former reply. I edited.

[taoso]

So, I propose to,

1. drop the definitions of Formats, or move them into the Ext namespace.
2. drop all the carriers, and drop the psr-http-message
3. drop the Reader and Writer interface
4. modify the extract and inject API like

   <?php
   // return context
   function extract($format, $carrier);
   function inject(SpanContext $spanContext, $format, &$carrier);

We can use variable of any type(string, int, object, even array) to store the format.

And we can use variable of any type(string, array, object) to store the format.

What OT spec requires us to do is that the inject method must modify the carrier, So what we only need to do is to set these parameter as reference.

Any thing else should be done by the implementation.

Thank you.

@jcchavezs

[felixfbecker]

@lvht Thank you, but if you want to add the constant values to be added to the spec please raise that as an issue at the spec repo, this is the wrong place to discuss.

But If you want to switch OT implementation from on vendor to another? You should change all your code that depend on these constants' values. Why? Because they are vendor define! You should make these code as dependence as less as possible.

As mentioned by @tedsuo, this doesn't work for a library that can't make assumptions about the implementation and is not a scalable approach. The constants make your code less dependent. Using values coupled to an implementation makes it more dependent. You are arguing for making the values vendor defined, so stating that they are already vendor-defined is not a good argument, given as a fact that they are currently not. The three required formats are definitely not vendor but spec defined, and we need to define shared constant values for interoperability. The reason why OT leaves this open in opposite to the tag constants is that these are not sent over the wire so it's fine to have language implementations define this in ways that makes sense for the language. Imo string constants make sense for PHP and everyone else seems to agree.

[taoso]

@felixfbecker

but if you want to add the constant values to be added to the spec please raise that as an issue at the spec repo, this is the wrong place to discuss.

Sorry to see that. You even not got my idea. That a pity.

The reason that I typed some many words is really that I want to make a discussion about the issue of the OT spec???

What I pointed out is just a fact, a fact about the OT spec.

[taoso]

@felixfbecker

You are arguing for making the values vendor defined, so stating that they are already vendor-defined is not a good argument, given as a fact that they are currently not.

What I argument is that every thing that not defined in the OT spec should be and must be defined by the vendor!

[taoso]

@felixfbecker @tedsuo

this doesn't work for a library that can't make assumptions about the implementation and is not a scalable approach

Does it really not work? If it does not work, why the OT spec does not defined these constants?

[taoso]

@felixfbecker

The reason why OT leaves this open in opposite to the tag constants is that these are not sent over the wire so it's fine to have language implementations define this in ways that makes sense for the language.

Agreed! Even though they are not sent over the wire, they represents type format of the wire. All of which should be define by the vendor.

Imo string constants make sense for PHP and everyone else seems to agree.

Why int does not make sense? How many people are included in the everyone else?

[jcchavezs]

@lvht I think you are avoiding the issue of the interop among implementations which is a clear issue, not a small issue. We are not likely to get rid of the format as those are the abstractions of what is described in the spec. In addition, other libraries include them as well

• https://github.com/opentracing/opentracing-python/blob/master/opentracing/propagation.py#L70
• https://github.com/opentracing/opentracing-javascript/blob/master/src/constants.ts#L14
• https://github.com/opentracing/opentracing-go/blob/master/propagation.go#L62

If you want to keep discussing the carrier thingie I will create another issue just for that but it is pretty clear that there is a common agreement in terms of PSR and Format constants.

[tedsuo]

@lvht nothing that interacts directly with the OpenTracing APIs should be defined by a vendor. Anything vendor-specific needs to be behind the API, not on top of it. Otherwise you would have to change all your instrumentation code whenever you changed tracing tools, and we would not be able to share libraries. Ensuring that tracers can be swapped out without needing to change instrumentation code is what drives most API decisions in OpenTracing.

There is going to be a documentation initiative soon, I'll try to make sure the reasoning for some of these decisions is clear. Right now, the website does not explain the logic behind some of the choices in the specification and the various language implementations, but please have faith that the concepts that @jcchavezs and @felixfbecker have been explaining in this thread are commonly agreed upon by the OTSC and the language API maintainers.

[jcchavezs]

Closing this issue, continuing the inject/extract interface in https://github.com/opentracing/opentracing-php/issues/37

[beberlei]

The GlobalTracer should stay, all the other specs have it too and it is required for contrib libraries, because PHP has no concept of "context" like go for example.

With regard to the other suggestions, they are reasonable :) especially the not depending on psr-message one.

[taoso]

@beberlei

The GlobalTracer is nothing but a global place to store user's Tracer.

For legacy PHP project, you can do this

<?php
// create $tracer instance according vendor's method
$GLOBALS['tracer'] = $tracer;

// and then you can use it anywhere
$tracer = $GLOBALS['tracer'];

For modern PHP project with their own container(something like PHP-DI, you can do this

<?php
// fetch the global $container
// create $tracer instance according vendor's method
$container->set('tracer', $tracer);

// and then you cna use it anywhere
$tracer = $container->get('tracer');
// or even we can use container auto inject tracer to our logic class

all the other specs have it too

We are using the PHP language, please Think in PHP. Any design or idea from other language should be refactored by PHP style.

If you guys still consistent that we should shift the GlobalTracer. Feel free. Even though it is useless, it makes no harm.