Fix up docs and other othttp cleanups (#218)

plugin/othttp/doc.go

@@ -0,0 +1,18 @@
+// Copyright 2019, OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package othttp provides a http.Handler and functions that are
+// intended to be used to add tracing by wrapping
+// existing handlers (with Handler) and routes WithRouteTag.
+package othttp

plugin/othttp/handler.go

@@ -0,0 +1,228 @@
+// Copyright 2019, OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package othttp
+
+import (
+	"io"
+	"net/http"
+
+	"go.opentelemetry.io/api/core"
+	"go.opentelemetry.io/api/propagation"
+	"go.opentelemetry.io/api/trace"
+	prop "go.opentelemetry.io/propagation"
+)
+
+var _ http.Handler = &Handler{}
+
+// Attribute keys that the Handler can add to a span.
+const (
+	HostKey       = core.Key("http.host")        // the http host (http.Request.Host)
+	MethodKey     = core.Key("http.method")      // the http method (http.Request.Method)
+	PathKey       = core.Key("http.path")        // the http path (http.Request.URL.Path)
+	URLKey        = core.Key("http.url")         // the http url (http.Request.URL.String())
+	UserAgentKey  = core.Key("http.user_agent")  // the http user agent (http.Request.UserAgent())
+	RouteKey      = core.Key("http.route")       // the http route (ex: /users/:id)
+	StatusCodeKey = core.Key("http.status_code") // if set, the http status
+	ReadBytesKey  = core.Key("http.read_bytes")  // if anything was read from the request body, the total number of bytes read
+	ReadErrorKey  = core.Key("http.read_error")  // If an error occurred while reading a request, the string of the error (io.EOF is not recorded)
+	WroteBytesKey = core.Key("http.wrote_bytes") // if anything was written to the response writer, the total number of bytes written
+	WriteErrorKey = core.Key("http.write_error") // if an error occurred while writing a reply, the string of the error (io.EOF is not recorded)
+)
+
+// Handler is http middleware that corresponds to the http.Handler interface and
+// is designed to wrap a http.Mux (or equivalent), while individual routes on
+// the mux are wrapped with WithRouteTag. A Handler will add various attributes
+// to the span using the core.Keys defined in this package.
+type Handler struct {
+	operation string
+	handler   http.Handler
+
+	tracer      trace.Tracer
+	prop        propagation.TextFormatPropagator
+	spanOptions []trace.SpanOption
+	public      bool
+	readEvent   bool
+	writeEvent  bool
+}
+
+// Option function used for setting *optional* Handler properties
+type Option func(*Handler)
+
+// WithTracer configures the Handler with a specific tracer. If this option
+// isn't specified then the global tracer is used.
+func WithTracer(tracer trace.Tracer) Option {
+	return func(h *Handler) {
+		h.tracer = tracer
+	}
+}
+
+// WithPublicEndpoint configures the Handler to link the span with an incoming
+// span context. If this option is not provided, then the association is a child
+// association instead of a link.
+func WithPublicEndpoint() Option {
+	return func(h *Handler) {
+		h.public = true
+	}
+}
+
+// WithPropagator configures the Handler with a specific propagator. If this
+// option isn't specificed then
+// go.opentelemetry.io/propagation.HTTPTraceContextPropagator is used.
+func WithPropagator(p propagation.TextFormatPropagator) Option {
+	return func(h *Handler) {
+		h.prop = p
+	}
+}
+
+// WithSpanOptions configures the Handler with an additional set of
+// trace.SpanOptions, which are applied to each new span.
+func WithSpanOptions(opts ...trace.SpanOption) Option {
+	return func(h *Handler) {
+		h.spanOptions = opts
+	}
+}
+
+type event int
+
+// Different types of events that can be recorded, see WithMessageEvents
+const (
+	ReadEvents event = iota
+	WriteEvents
+)
+
+// WithMessageEvents configures the Handler to record the specified events
+// (span.AddEvent) on spans. By default only summary attributes are added at the
+// end of the request.
+//
+// Valid events are:
+//     * ReadEvents: Record the number of bytes read after every http.Request.Body.Read
+//       using the ReadBytesKey
+//     * WriteEvents: Record the number of bytes written after every http.ResponeWriter.Write
+//       using the WriteBytesKey
+func WithMessageEvents(events ...event) Option {
+	return func(h *Handler) {
+		for _, e := range events {
+			switch e {
+			case ReadEvents:
+				h.readEvent = true
+			case WriteEvents:
+				h.writeEvent = true
+			}
+		}
+	}
+}
+
+// NewHandler wraps the passed handler, functioning like middleware, in a span
+// named after the operation and with any provided HandlerOptions.
+func NewHandler(handler http.Handler, operation string, opts ...Option) http.Handler {
+	h := Handler{handler: handler, operation: operation}
+	defaultOpts := []Option{
+		WithTracer(trace.GlobalTracer()),
+		WithPropagator(prop.HTTPTraceContextPropagator{}),
+	}
+
+	for _, opt := range append(defaultOpts, opts...) {
+		opt(&h)
+	}
+	return &h
+}
+
+// ServeHTTP serves HTTP requests (http.Handler)
+func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	opts := append([]trace.SpanOption{}, h.spanOptions...) // start with the configured options
+
+	sc := h.prop.Extract(r.Context(), r.Header)
+	if sc.IsValid() { // not a valid span context, so no link / parent relationship to establish
+		var opt trace.SpanOption
+		if h.public {
+			// TODO: If the endpoint is a public endpoint, it should start a new trace
+			// and incoming remote sctx should be added as a link
+			// (WithLinks(links...), this option doesn't exist yet). Replace ChildOf
+			// below with something like: opt = trace.WithLinks(sc)
+			opt = trace.ChildOf(sc)
+		} else { // not a private endpoint, so assume child relationship
+			opt = trace.ChildOf(sc)
+		}
+		opts = append(opts, opt)
+	}
+
+	ctx, span := h.tracer.Start(r.Context(), h.operation, opts...)
+	defer span.End()
+
+	readRecordFunc := func(int64) {}
+	if h.readEvent {
+		readRecordFunc = func(n int64) {
+			span.AddEvent(ctx, "read", ReadBytesKey.Int64(n))
+		}
+	}
+	bw := bodyWrapper{ReadCloser: r.Body, record: readRecordFunc}
+	r.Body = &bw
+
+	writeRecordFunc := func(int64) {}
+	if h.writeEvent {
+		writeRecordFunc = func(n int64) {
+			span.AddEvent(ctx, "write", WroteBytesKey.Int64(n))
+		}
+	}
+
+	rww := &respWriterWrapper{ResponseWriter: w, record: writeRecordFunc, ctx: ctx, injector: h.prop}
+
+	// Setup basic span attributes before calling handler.ServeHTTP so that they
+	// are available to be mutated by the handler if needed.
+	span.SetAttributes(
+		HostKey.String(r.Host),
+		MethodKey.String(r.Method),
+		PathKey.String(r.URL.Path),
+		URLKey.String(r.URL.String()),
+		UserAgentKey.String(r.UserAgent()),
+	)
+
+	h.handler.ServeHTTP(rww, r.WithContext(ctx))
+
+	setAfterServeAttributes(span, bw.read, rww.written, int64(rww.statusCode), bw.err, rww.err)
+}
+
+func setAfterServeAttributes(span trace.Span, read, wrote, statusCode int64, rerr, werr error) {
+	kv := make([]core.KeyValue, 0, 5)
+	// TODO: Consider adding an event after each read and write, possibly as an
+	// option (defaulting to off), so as to not create needlessly verbose spans.
+	if read > 0 {
+		kv = append(kv, ReadBytesKey.Int64(read))
+	}
+	if rerr != nil && rerr != io.EOF {
+		kv = append(kv, ReadErrorKey.String(rerr.Error()))
+	}
+	if wrote > 0 {
+		kv = append(kv, WroteBytesKey.Int64(wrote))
+	}
+	if statusCode > 0 {
+		kv = append(kv, StatusCodeKey.Int64(statusCode))
+	}
+	if werr != nil && werr != io.EOF {
+		kv = append(kv, WriteErrorKey.String(werr.Error()))
+	}
+	span.SetAttributes(kv...)
+}
+
+// WithRouteTag annotates a span with the provided route name using the
+// RouteKey Tag.
+func WithRouteTag(route string, h http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		span := trace.CurrentSpan(r.Context())
+		//TODO: Why doesn't tag.Upsert work?
+		span.SetAttribute(RouteKey.String(route))
+		h.ServeHTTP(w, r.WithContext(trace.SetCurrentSpan(r.Context(), span)))
+	})
+}

plugin/othttp/handler_example_test.go

@@ -68,12 +68,7 @@ func ExampleNewHandler() {
 		case "":
 			err = fmt.Errorf("expected /hello/:name in %q", s)
 		default:
-			span := trace.CurrentSpan(ctx)
+			trace.CurrentSpan(ctx).SetAttribute(core.Key("name").String(pp[1]))
-			span.SetAttribute(
-				core.KeyValue{Key: "name",
-					Value: core.Value{Type: core.STRING, String: pp[1]},
-				},
-			)
 		}
 		return pp[1], err
 	}
@@ -113,7 +108,7 @@ func ExampleNewHandler() {
 
 	if err := http.ListenAndServe(":7777",
 		othttp.NewHandler(&mux, "server",
-			othttp.WithMessageEvents(othttp.EventRead, othttp.EventWrite),
+			othttp.WithMessageEvents(othttp.ReadEvents, othttp.WriteEvents),
 		),
 	); err != nil {
 		log.Fatal(err)

plugin/othttp/server.go

@@ -1,297 +0,0 @@
-// Copyright 2019, OpenTelemetry Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package othttp
-
-import (
-	"io"
-	"net/http"
-
-	"go.opentelemetry.io/api/core"
-	"go.opentelemetry.io/api/propagation"
-	"go.opentelemetry.io/api/trace"
-	prop "go.opentelemetry.io/propagation"
-)
-
-var _ http.Handler = &HTTPHandler{}
-
-type httpEvent int
-
-// Possible message events that can be enabled via WithMessageEvents
-const (
-	EventRead  httpEvent = iota // An event that records the number of bytes read is created for every Read
-	EventWrite                  // an event that records the number of bytes written is created for every Write
-)
-
-// Attribute keys that HTTPHandler could write out.
-const (
-	HostKeyName       core.Key = "http.host"        // the http host (http.Request.Host)
-	MethodKeyName     core.Key = "http.method"      // the http method (http.Request.Method)
-	PathKeyName       core.Key = "http.path"        // the http path (http.Request.URL.Path)
-	URLKeyName        core.Key = "http.url"         // the http url (http.Request.URL.String())
-	UserAgentKeyName  core.Key = "http.user_agent"  // the http user agent (http.Request.UserAgent())
-	RouteKeyName      core.Key = "http.route"       // the http route (ex: /users/:id)
-	StatusCodeKeyName core.Key = "http.status_code" // if set, the http status
-	ReadBytesKeyName  core.Key = "http.read_bytes"  // if anything was read from the request body, the total number of bytes read
-	ReadErrorKeyName  core.Key = "http.read_error"  // If an error occurred while reading a request, the string of the error (io.EOF is not recorded)
-	WroteBytesKeyName core.Key = "http.wrote_bytes" // if anything was written to the response writer, the total number of bytes written
-	WriteErrorKeyName core.Key = "http.write_error" // if an error occurred while writing a reply, the string of the error (io.EOF is not recorded)
-)
-
-// HTTPHandler provides http middleware that corresponds to the http.Handler interface
-type HTTPHandler struct {
-	operation string
-	handler   http.Handler
-
-	tracer      trace.Tracer
-	prop        propagation.TextFormatPropagator
-	spanOptions []trace.SpanOption
-	public      bool
-	readEvent   bool
-	writeEvent  bool
-}
-
-type HandlerOption func(*HTTPHandler)
-
-// WithTracer configures the HTTPHandler with a specific tracer. If this option
-// isn't specified then global tracer is used.
-func WithTracer(tracer trace.Tracer) HandlerOption {
-	return func(h *HTTPHandler) {
-		h.tracer = tracer
-	}
-}
-
-// WithPublicEndpoint configures the HTTPHandler to link the span with an
-// incoming span context. If this option is not provided (the default), then the
-// association is a child association (instead of a link).
-func WithPublicEndpoint() HandlerOption {
-	return func(h *HTTPHandler) {
-		h.public = true
-	}
-}
-
-// WithPropagator configures the HTTPHandler with a specific propagator. If this
-// option isn't specificed then a w3c trace context propagator.
-func WithPropagator(p propagation.TextFormatPropagator) HandlerOption {
-	return func(h *HTTPHandler) {
-		h.prop = p
-	}
-}
-
-// WithSpanOptions configures the HTTPHandler with an additional set of
-// trace.SpanOptions, which are applied to each new span.
-func WithSpanOptions(opts ...trace.SpanOption) HandlerOption {
-	return func(h *HTTPHandler) {
-		h.spanOptions = opts
-	}
-}
-
-// WithMessageEvents configures the HTTPHandler with a set of message events. By
-// default only the summary attributes are added at the end of the request.
-func WithMessageEvents(events ...httpEvent) HandlerOption {
-	return func(h *HTTPHandler) {
-		for _, e := range events {
-			switch e {
-			case EventRead:
-				h.readEvent = true
-			case EventWrite:
-				h.writeEvent = true
-			}
-		}
-	}
-}
-
-// NewHandler wraps the passed handler, functioning like middleware, in a span
-// named after the operation and with any provided HandlerOptions.
-func NewHandler(handler http.Handler, operation string, opts ...HandlerOption) http.Handler {
-	h := HTTPHandler{handler: handler}
-	defaultOpts := []HandlerOption{
-		WithTracer(trace.GlobalTracer()),
-		WithPropagator(prop.HTTPTraceContextPropagator{}),
-	}
-
-	for _, opt := range append(defaultOpts, opts...) {
-		opt(&h)
-	}
-	return &h
-}
-
-func (h *HTTPHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	opts := append([]trace.SpanOption{}, h.spanOptions...) // start with the configured options
-
-	sc := h.prop.Extract(r.Context(), r.Header)
-	if sc.IsValid() { // not a valid span context, so no link / parent relationship to establish
-		var opt trace.SpanOption
-		if h.public {
-			// TODO: If the endpoint is a public endpoint, it should start a new trace
-			// and incoming remote sctx should be added as a link
-			// (WithLinks(links...), this option doesn't exist yet). Replace ChildOf
-			// below with something like: opt = trace.WithLinks(sc)
-			opt = trace.ChildOf(sc)
-		} else { // not a private endpoint, so assume child relationship
-			opt = trace.ChildOf(sc)
-		}
-		opts = append(opts, opt)
-	}
-
-	ctx, span := h.tracer.Start(r.Context(), h.operation, opts...)
-	defer span.End()
-
-	readRecordFunc := func(int) {}
-	if h.readEvent {
-		readRecordFunc = func(n int) {
-			span.AddEvent(ctx, "read", core.KeyValue{
-				Key: ReadBytesKeyName,
-				Value: core.Value{
-					Type:  core.INT64,
-					Int64: int64(n),
-				}})
-		}
-	}
-	bw := bodyWrapper{ReadCloser: r.Body, record: readRecordFunc}
-	r.Body = &bw
-
-	writeRecordFunc := func(int) {}
-	if h.writeEvent {
-		writeRecordFunc = func(n int) {
-			span.AddEvent(ctx, "write", core.KeyValue{
-				Key: WroteBytesKeyName,
-				Value: core.Value{
-					Type:  core.INT64,
-					Int64: int64(n),
-				},
-			})
-		}
-	}
-	rww := &respWriterWrapper{ResponseWriter: w, record: writeRecordFunc}
-
-	setBeforeServeAttributes(span, r.Host, r.Method, r.URL.Path, r.URL.String(), r.UserAgent())
-	// inject the response header before calling ServeHTTP because a Write in
-	// ServeHTTP will cause all headers to be written out.
-	h.prop.Inject(ctx, rww.Header())
-
-	h.handler.ServeHTTP(rww, r.WithContext(ctx))
-	setAfterServeAttributes(span, bw.read, rww.written, int64(rww.statusCode), bw.err, rww.err)
-}
-
-func setBeforeServeAttributes(span trace.Span, host, method, path, url, uagent string) {
-	// Setup basic span attributes before calling handler.ServeHTTP so that they
-	// are available to be mutated by the handler if needed.
-	span.SetAttributes(
-		core.KeyValue{
-			Key: HostKeyName,
-			Value: core.Value{
-				Type:   core.STRING,
-				String: host,
-			}},
-		core.KeyValue{
-			Key: MethodKeyName,
-			Value: core.Value{
-				Type:   core.STRING,
-				String: method,
-			}},
-		core.KeyValue{
-			Key: PathKeyName,
-			Value: core.Value{
-				Type:   core.STRING,
-				String: path,
-			}},
-		core.KeyValue{
-			Key: URLKeyName,
-			Value: core.Value{
-				Type:   core.STRING,
-				String: url,
-			}},
-		core.KeyValue{
-			Key: UserAgentKeyName,
-			Value: core.Value{
-				Type:   core.STRING,
-				String: uagent,
-			}},
-	)
-}
-
-func setAfterServeAttributes(span trace.Span, read, wrote, statusCode int64, rerr, werr error) {
-	kv := make([]core.KeyValue, 0, 5)
-	// TODO: Consider adding an event after each read and write, possibly as an
-	// option (defaulting to off), so at to not create needlesly verbose spans.
-	if read > 0 {
-		kv = append(kv,
-			core.KeyValue{
-				Key: ReadBytesKeyName,
-				Value: core.Value{
-					Type:  core.INT64,
-					Int64: read,
-				}})
-	}
-
-	if rerr != nil && rerr != io.EOF {
-		kv = append(kv,
-			core.KeyValue{
-				Key: ReadErrorKeyName,
-				Value: core.Value{
-					Type:   core.STRING,
-					String: rerr.Error(),
-				}})
-	}
-
-	if wrote > 0 {
-		kv = append(kv,
-			core.KeyValue{
-				Key: WroteBytesKeyName,
-				Value: core.Value{
-					Type:  core.INT64,
-					Int64: wrote,
-				}})
-	}
-
-	if statusCode > 0 {
-		kv = append(kv,
-			core.KeyValue{
-				Key: StatusCodeKeyName,
-				Value: core.Value{
-					Type:  core.INT64,
-					Int64: statusCode,
-				}})
-	}
-
-	if werr != nil && werr != io.EOF {
-		kv = append(kv,
-			core.KeyValue{
-				Key: WriteErrorKeyName,
-				Value: core.Value{
-					Type:   core.STRING,
-					String: werr.Error(),
-				}})
-	}
-
-	span.SetAttributes(kv...)
-}
-
-func WithRouteTag(route string, h http.Handler) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		span := trace.CurrentSpan(r.Context())
-		//TODO: Why doesn't tag.Upsert work?
-		span.SetAttribute(
-			core.KeyValue{
-				Key: RouteKeyName,
-				Value: core.Value{
-					Type:   core.STRING,
-					String: route,
-				},
-			},
-		)
-
-		h.ServeHTTP(w, r.WithContext(trace.SetCurrentSpan(r.Context(), span)))
-	})
-}

plugin/othttp/wrap.go

@@ -11,11 +11,15 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
+
 package othttp
 
 import (
+	"context"
 	"io"
 	"net/http"
+
+	"go.opentelemetry.io/api/propagation"
 )
 
 var _ io.ReadCloser = &bodyWrapper{}
@@ -24,7 +28,7 @@ var _ io.ReadCloser = &bodyWrapper{}
 // of bytes read and the last error
 type bodyWrapper struct {
 	io.ReadCloser
-	record func(n int) // must not be nil
+	record func(n int64) // must not be nil
 
 	read int64
 	err  error
@@ -32,9 +36,10 @@ type bodyWrapper struct {
 
 func (w *bodyWrapper) Read(b []byte) (int, error) {
 	n, err := w.ReadCloser.Read(b)
-	w.read += int64(n)
+	n1 := int64(n)
+	w.read += n1
 	w.err = err
-	w.record(n)
+	w.record(n1)
 	return n, err
 }
 
@@ -44,6 +49,10 @@ func (w *bodyWrapper) Close() error {
 
 var _ http.ResponseWriter = &respWriterWrapper{}
 
+type injector interface {
+	Inject(context.Context, propagation.Supplier)
+}
+
 // respWriterWrapper wraps a http.ResponseWriter in order to track the number of
 // bytes written, the last error, and to catch the returned statusCode
 // TODO: The wrapped http.ResponseWriter doesn't implement any of the optional
@@ -51,7 +60,11 @@ var _ http.ResponseWriter = &respWriterWrapper{}
 // that may be useful when using it in real life situations.
 type respWriterWrapper struct {
 	http.ResponseWriter
-	record func(n int) // must not be nil
+	record func(n int64) // must not be nil
+
+	// used to inject the header
+	ctx context.Context
+	injector
 
 	written     int64
 	statusCode  int
@@ -69,14 +82,19 @@ func (w *respWriterWrapper) Write(p []byte) (int, error) {
 		w.wroteHeader = true
 	}
 	n, err := w.ResponseWriter.Write(p)
-	w.record(n)
+	n1 := int64(n)
-	w.written += int64(n)
+	w.record(n1)
+	w.written += n1
 	w.err = err
 	return n, err
 }
 
 func (w *respWriterWrapper) WriteHeader(statusCode int) {
+	if w.wroteHeader {
+		return
+	}
 	w.wroteHeader = true
 	w.statusCode = statusCode
+	w.injector.Inject(w.ctx, w.Header())
 	w.ResponseWriter.WriteHeader(statusCode)
 }