opentracing / opentracing-php

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

Removes carriers. #38

Closed jcchavezs closed 6 years ago

jcchavezs commented 6 years ago

This PR removes carriers (and interfaces Reader and Writer) as suggested by @lvht.

Reasons are:

  1. By providing carriers we also dictate how injectors/extractors should work and for that we add a lot of constraints and complexity around them that they might not need. In addition, there is no specific structure for baggage items so forcing them to use an strict carrier will add some overhead we want to avoid.
  2. There is a lot of noise around how PHP frameworks work with headers so the arguments of providing a standard carrier that will work in most of the cases is invalid. For example, Symfony request lowercase the header keys so a custom extractor should be written for B3 extraction.

Ping @felixfbecker @beberlei @lvht @tedsuo @yurishkuro @tedsuo

Closes #37

taoso commented 6 years ago

:+1:

yurishkuro commented 6 years ago

@jcchavezs do you mean that you're using standard types as carriers? E.g. in Python the carrier for the TextMap format is the default dictionary. If so, I think that's the right approach, but it doesn't seem to be mentioned in the documentation for the formats - see Go API that clearly states which carrier is expected with which format.

On a separate point, I would suggest implementing a Mock tracer (#40) so that changes like this can be validated in an actual implementation, rather than just the API.

tedsuo commented 6 years ago

This looks good to me. One question: for http headers, is []string more useful that [][]string? I don't think tracers will inject/extract more than one value per header. But if most http header implementations use [][]string perhaps we should match that? I believe I saw this discussed before, and []string is fine. Just double checking.

felixfbecker commented 6 years ago

Can this be merged?

taoso commented 6 years ago

I wish this PR to be merged as early as possible.

taoso commented 6 years ago

@jcchavezs Would you like make a v0.x.x tag? I need to require a release, even it is unstable.

felixfbecker commented 6 years ago

The next version is 1.0.0-beta.2

taoso commented 6 years ago

ping @jcchavezs

jcchavezs commented 6 years ago

https://github.com/opentracing/opentracing-php/tree/1.0.0-beta2

jcchavezs commented 6 years ago

@tedsuo so most implementations use string[][], problem is that there are different ways to get headers, for example getallheaders will return string[] having duplicated values glued by a comma whereas the PSR7 implementation returns a string[][]. I believe every vendor would setup something that works for them based on their propagation model.