OpenTracer.StartTrace could use map[string]interface{} to be more type safe

[slimsag]

I've use log packages like log15 which "do bad things" when you provide an uneven number of arguments.

Right here we open ourselves up to this. Where "do bad things" is really up to the implementation.

I think it would be nicer to change keyValueTags from ...interface{} to map[string]interface{}.

[bhs]

I struggle with this one... it's such a mouthful to write map[string]interface{}{...}.

A compromise would be to have that keyValueTags be of type opentracing.Tags, per https://github.com/opentracing/api-golang/blob/master/opentracing/raw.go#L9 ... then callers could just do something like this: http://play.golang.org/p/jFSw-sFp76

package main

import "fmt"

type Tags map[string]interface{}

func takesTags(t Tags) {
    for k, v := range t {
        fmt.Printf("%v -> %v\n", k, v)
    }
}

func main() {
    takesTags(Tags{
        "forty-two": 42,
        "a string":  "abcd",
    })
}

Best of both worlds?

[bhs]

(I guess it would be more like someFunction(..., opentracing.Tags{ ... }), but still a little more palatable than map[string]interface{})

[slimsag]

@bensigelman I think that opentracing.Tags{...} would be better too. I agree writing out map[string]interface{}{} is a mouthful. My motivation for this is that using the map type is more sane (even in the form type Tags map[string]interface{}) just due to the added type safety, as it is the native Go representation for such an object.

[bhs]

For me it's less about type safety and more about making it easy for people to generate and augment these tags things as they see fit... That's of course doable but annoying if it involves rolling/unrolling the flattened version.

[yurishkuro]

+1 for type. A negative of using a map is an extra object allocation / gc pressure (I assume vararg slice is read off the stack).

[slimsag]

For ideas sake, another solution could be:

package main

import "fmt"

type Tags map[string]interface{}

// Tags allows you to write Tags("key", "value", "key2", "value2") and returns a map.
func Tags(pairs ...interface{}) map[string]interface{} {
}

func takesTags(t map[string]interface{}) {
    for k, v := range t {
        fmt.Printf("%v -> %v\n", k, v)
    }
}

func main() {
    takesTags(Tags(
        "forty-two", 42,
        "a string",  "abcd",
    ))
}

This allows for the simpler usage you are aiming for while still allowing others to use map[string]interface{} if they desire.

[yurishkuro]

Another alternative is to provide a batch update mode for a span, which will obtain the lock once and execute a function. Then people can simply call addTag multiple times within that function, along with other mutating methods like addLog etc.

func (* span) batch(f func (*span)) {
   span.lock.Lock()
   defer span.lock.Unlock()
   f(this)
}

func main() {
  span = beginTrace("get_user")
  span.batch(func (s *span) {
     span.addTag(x, y)
     span.addLog("you killing me")
  })
}

[slimsag]

Interesting idea .. I do think that adds a lot of unneeded complexity for new users to understand, though.

[bhs]

Oh -- I was looking into just making one of these changes and remembered why I did it the way I did it: I think Tags are great and everything, but they're not the common case. So from an API-decluttering standpoint it's nice to only have a StartTrace(opName string, keyValueTags ...interface{}) method rather than separate StartTrace(opName string) and StartTraceWithTags(opName string, tags Tags) methods.

[bhs]

If we want to do something like the latter (separate methods, I mean), then using a type is a no-brainer.

[slimsag]

Span already has a method AddTag:

    // Adds a tag to the span. The `value` is immediately coerced into a string
    // using fmt.Sprint().
    //
    // If there is a pre-existing tag set for `key`, it is overwritten.
    SetTag(key string, value interface{}) Span

so what if we additionally had a SetTags method and just StartTrace(opName string)?

// SetTags sets the tags of this span to the given ones. It overwrites all of the old
// tags, and the tags map should be considered owned by the span (not accessed) after
// calling this method.
SetTags(tags Tags) Span

This would make usage look like:

span := tracer.StartTrace("MadeAnHTTPRequest").SetTags(opentracing.Tags{
    "Key": "Value",
    "Key2": "Value",
    "Key3": "Value",
})

[bhs]

Sorry, had a long weekend with minimal internet access!

Anyway, that seems fine to me... I'd probably do MergeTags for the new method.

I'd still like to support the existing variadic syntax since it's less typing, though I don't feel strongly.

[yurishkuro]

@slimsag @bensigelman ok to close this?

[bhs]

SGTM!

On Fri, Dec 25, 2015 at 4:25 PM, Yuri Shkuro notifications@github.com wrote:

@slimsag https://github.com/slimsag @bensigelman https://github.com/bensigelman ok to close this?

— Reply to this email directly or view it on GitHub https://github.com/opentracing/api-golang/issues/9#issuecomment-167268338 .