search

opentracing / opentracing-php

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

Adds convinient methods for the SpanReference for coherence #12

Closed jcchavezs closed 7 years ago

jcchavezs commented 7 years ago

This PR adds isType methods to keep coherence with the creation methods.

Ping @felixfbecker @beberlei

felixfbecker commented 7 years ago

This is a step backwards imo, it makes custom span types second-class citizens again. $ref->isType(SpanReference::CHILD_OF) accomplishes the same amount of chars and safety from typos.

jcchavezs commented 7 years ago

@felixfbecker I have mixed feelings here, this is convinient in terms of usage + keeps coherence with the creation methods createAsChildOf or createAsFollowsFrom. In addition, I am not sure if custom span references should keep the same relevance as they are not the main types supported by OpenTracing. If we don't add this, do you think we should remove the creation methods? Any opinions @beberlei @yurishkuro @tedsuo?

felixfbecker commented 7 years ago

Well I had the same concern with createAsChildOf(), I think passing the type is just as convenient. Feels overengineered/bloated to me. I would rather remove those for consistency. But that's just me :)

jcchavezs commented 7 years ago

I see. As I said before it always prefer to ask meaningful questions to the objects and I believe isType goes on that line but I also see the convenience of having the isTypeChildOf method and not calling isType(Reference::CHILD_OF) often.

yurishkuro commented 7 years ago

@jcchavezs keep in mind that only tracer implementation needs to ask those kinds of questions from the reference. We don't need to extend the API for the convenience of tracer implementors - they know what they are doing and they are 0.1% of the total user base of the API.

jcchavezs commented 7 years ago

Agree, I will close this PR for now and then open a new one removing the creation method for a matter of coherence.

@yurishkuro @felixfbecker