opentracing / opentracing-php

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

Introduce setTag method for a Span interface and get rid of setTags #52

Closed ellisv closed 6 years ago

ellisv commented 6 years ago

Having setTags instead of setTag means that you would have to be aware of tags that are set in order to add aditional tag to a span. But that is not the case if we expose an interface to set just a single tag.

All other opentracing libraries also have setTag exposed instead of setTags so this helps to have more consistency between libraries.

Closes #51

jcchavezs commented 6 years ago

ping @tedsuo @beberlei @yurishkuro

yurishkuro commented 6 years ago

I am fine with the change, but not sure I follow its reasoning. Isn't setTags(tags) == tags.forEach(setTag)?

ellisv commented 6 years ago

@yurishkuro For a sake of argument lets assume that tags is just a simple class property. Based on a name of setTags I assume implementation would look like

public function setTags(array $tags)
{
    $this->tags = $tags;
}

Meaning it will not preserve original tags that were set before and will overwrite with new ones.

On the other hand, setTag implementation would look like

public function setTag($key, $value)
{
    $this->tags[$key] = $value;
}

So as a user I am able to add a new tag to existing tag collection without knowing all previously set tags.

yurishkuro commented 6 years ago

Fair enough

jcchavezs commented 6 years ago

The intention behind setTags was never to replace the set of tags but to iterate over the tags and replace one by one (see this) but I agree this is not what you get from the name.