Add HTTP method and path to trace span operation name

[hobochili]

Edit: addresses #732

The current tracing implementation sets the operationName of every fabio span to the configured tracing service name. This makes it difficult to distinguish between traces in the Jaeger UI, as seen here (this is with FABIO_TRACING_SERVICENAME set to fabio):

This commit sets the operationName to HTTP <REQUST_METHOD> <REQUEST_PATH> so that traces are easier to identify:

It also sets the http.method and http.url tags on the span. To set the value of http.url it uses req.URL.String(), which resolves to the request path rather than the full URL (presumably because Scheme and Host are unset in the URL struct: https://golang.org/pkg/net/url/#URL.String). This is sufficient for our purposes but can be addressed if needed.

[CLAassistant]

All committers have signed the CLA.

[pschultz]

This is a BC break, isn't it? Since the operation name in the span is part of the configuration, I assume there are reasons to change it, and with the PR any such decision is ignored. I imagine people may have monitoring/alerting in place that rely on the existing spans, for instance.

I'm not particularly familiar with ZipKin et al, so my understanding may be off, but ignoring an existing configuration value is certainly a red flag.

Would adding a child span with your proposed operation name instead of replacing the existing one fix the issue in the Jaeger UI as well? If we could do that, and create the child span as late as possible in ServeHTTP we would also get timing information on fabio's overhead for free (i.e. making the routing and access control decisions).

[hobochili]

Good point, this isn't backward compatible. I didn't consider the possibility that people may be hinging on the opname for monitoring purposes.

To your suggestion, if fabio created a child span with my proposed change my issue wouldn't be resolved because the operation descriptor would still be hidden beneath the parent in the Jaeger UI.

I could add another config option for FABIO_TRACING_OPNAME and have it default to FABIO_TRACING_SERVICENAME if unset. If we go that route, though, it would be ideal to be able to configure the operation name as a template in case HTTP <METHOD> <URL_PATH> isn't sufficient or desirable for others. Thoughts?

[aaronhurt]

I think basic text/template style options work very well for that sort of thing. It's also an already established pattern in other options.

[hobochili]

Most recent commit addresses BC concerns by adding the Tracing.SpanName property.

To preserve existing functionality, Tracing.SpanName adopts the value of Tracing.ServiceName unless explicitly set.

Potential concerns:

• The template string is parsed upon every span creation.
• The un-rendered template string will be used if parsing of Tracing.SpanName fails due to an invalid template string or unsupported key(s).

[pschultz]

I like the idea to use templates.

The un-rendered template string will be used if parsing of Tracing.SpanName fails due to an invalid template string or unsupported key(s).

I like this too. Can we perhaps log the error once every N seconds or so?

I don't think we need a new config property, though. Any "normal" value for the name (like the default "fabio") is also a valid template, so it should be okay to parse the existing field. This will only cause problems for users that use double curly braces in the value, and I think it's good enough to point this out in the changelog.

I think we can parse the template once when loading the config, where we also have a good opportunity to report any syntax errors.

What do you think?

diff --git a/config/config.go b/config/config.go
index cf97eb7..08e3eed 100644
--- a/config/config.go
+++ b/config/config.go
@@ -172,6 +173,7 @@ type Tracing struct {
        CollectorType  string
        ConnectString  string
        ServiceName    string
+       SpanNameTpl    *template.Template // parsed ServiceName
        Topic          string
        SamplerRate    float64
        SpanHost       string
diff --git a/config/load.go b/config/load.go
index 5b2978e..7e99fac 100644
--- a/config/load.go
+++ b/config/load.go
@@ -200,7 +201,7 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c
        f.BoolVar(&cfg.Tracing.TracingEnabled, "tracing.TracingEnabled", defaultConfig.Tracing.TracingEnabled, "Enable/Disable OpenTrace, one of [true, false]")
        f.StringVar(&cfg.Tracing.CollectorType, "tracing.CollectorType", defaultConfig.Tracing.CollectorType, "OpenTrace Collector Type, one of [http, kafka]")
        f.StringVar(&cfg.Tracing.ConnectString, "tracing.ConnectString", defaultConfig.Tracing.ConnectString, "OpenTrace Collector host:port")
-       f.StringVar(&cfg.Tracing.ServiceName, "tracing.ServiceName", defaultConfig.Tracing.ServiceName, "Service name to embed in OpenTrace span")
+       f.StringVar(&cfg.Tracing.ServiceName, "tracing.ServiceName", defaultConfig.Tracing.ServiceName, "Service name template used to embed in OpenTrace span")
        f.StringVar(&cfg.Tracing.Topic, "tracing.Topic", defaultConfig.Tracing.Topic, "OpenTrace Collector Kafka Topic")
        f.Float64Var(&cfg.Tracing.SamplerRate, "tracing.SamplerRate", defaultConfig.Tracing.SamplerRate, "OpenTrace sample rate percentage in decimal form")
        f.StringVar(&cfg.Tracing.SpanHost, "tracing.SpanHost", defaultConfig.Tracing.SpanHost, "Host:Port info to add to spans")
@@ -317,6 +318,12 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c
                return nil, fmt.Errorf("proxy.noroutestatus must be between 100 and 999")
        }
 
+       cfg.Tracing.SpanNameTpl, err = template.New("tracing-span-name").Parse(cfg.Tracing.ServiceName)
+       if err != nil {
+               return nil, fmt.Errorf("tracing.ServiceName: %v", err)
+       }
+
        // handle deprecations
        deprecate := func(name, msg string) {
                if f.IsSet(name) {

[aaronhurt]

@hobochili any updates?

[hobochili]

Apologies for the delay, I had to think about @pschultz's proposal a bit.

I can certainly add regular logging for invalid template strings, but I'm not so sure about co-opting the service name config because:

• On a semantic level, a template resulting in something like fabio: HTTP /some/great/path for a service name would seem a bit of a misnomer (fabio is a service name, HTTP /some/great/path is a span description).

• On a more specific level, traceConfig.ServiceName is currently being used to both initialize the zipkin recorder and name the spans, so I believe such a change would break existing functionality:

// NewRecorder creates a new Zipkin Recorder backed by the provided Collector.
//
// hostPort and serviceName allow you to set the default Zipkin endpoint
// information which will be added to the application's standard core
// annotations. hostPort will be resolved into an IPv4 and/or IPv6 address and
// Port number, serviceName will be used as the application's service
// identifier.
...
func NewRecorder(c Collector, debug bool, hostPort, serviceName string, options ...RecorderOption) SpanRecorder {...}

(https://github.com/openzipkin-contrib/zipkin-go-opentracing/blob/v0.3.4/zipkin-recorder.go)

[pschultz]

On a more specific level, traceConfig.ServiceName is currently being used to both initialize the zipkin recorder and name the spans, so I believe such a change would break existing functionality.

I missed that fact and agree that a new option is the better course of action.

[aaronhurt]

Triggering Travis ...

[aaronhurt]

@hobochili @pschultz Thank you!