Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 26, 2018
Merged

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

merged 1 commit into from
Feb 26, 2018

Conversation

ellisv
Copy link
Contributor

@ellisv ellisv commented Feb 25, 2018

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

* a string, a boolean value, or a numeric type.
* Adds a tag to the span.
*
* If there is a pre-existing tag set for key, is is overwritten.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a typo here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed.

@jcchavezs
Copy link
Collaborator

jcchavezs commented Feb 25, 2018

ping @tedsuo @beberlei @yurishkuro

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
@yurishkuro
Copy link
Member

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

@ellisv
Copy link
Contributor Author

ellisv commented Feb 25, 2018

@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
Copy link
Member

Fair enough

@jcchavezs
Copy link
Collaborator

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.

@jcchavezs jcchavezs merged commit 9fc349d into opentracing:master Feb 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants