opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Do not use AbstractTag<T> for Span.setTag(), but the actual impls. #317

Closed carlosalberto closed 5 years ago

carlosalberto commented 5 years ago

This is done to not use an abstract class for our public API, so we take overloads of BooleanTag, IntTag and StringTag.

Addresses a small detail mentioned in #314

@yurishkuro @tylerbenson

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-0.2%) to 76.483% when pulling 7e8cbe8e797e44fdf0876b875540f43a7609b7dd on carlosalberto:span_settag_with_types into b6f63246af9688042097f65781357d66e29141d9 on opentracing:v0.32.0.

carlosalberto commented 5 years ago

Hey, any opinion on this @yurishkuro @tylerbenson ? If it looks right, I will merge and this will be included in the RC1 else, we might probably roll it in the next RC ;)

yurishkuro commented 5 years ago

Why not use a typed interface? I'm not a fan of overloaded methods (even though it is consistent with the plain set methods).

carlosalberto commented 5 years ago

@yurishkuro

You refer to something like this?

// IntTag and the others would be inheriting this class.
public class TypedTag<T> extends AbstractTag<T> {
    protected void set(Span span, T tagValue)
    {   
        span.setTag(super.key, tagValue.toString());
    }
}
carlosalberto commented 5 years ago

Hey @yurishkuro sorry, re-read your message today and figured you simply wanted the same, but through an interface ;)

carlosalberto commented 5 years ago

Hey @yurishkuro

I added Tag.set() and updated AbstractTag as well - but left the call to MockTracer.setObjectTag() as this extra checks that no tags are added after a Span was finished, and would love to keep it around:

    // An error will be reported if this Span is already finished.
    private synchronized MockSpan setObjectTag(String key, Object value) {
        finishedCheck("Adding tag {%s:%s} to already finished span", key, value);

Let me know ;)

yurishkuro commented 5 years ago

I think it should do tag.set(span, value) instead. This way the correct typed set() method will be called by the tag itself, and those methods would call setObjectTag

carlosalberto commented 5 years ago

@yurishkuro Updated it to looks just like that ;) If you think that's fine, I will merge soon

yurishkuro commented 5 years ago

🚢

carlosalberto commented 5 years ago

👍