Skip to content

Commit

Permalink
Add options for client and server tracing to skip client errs (#9)
Browse files Browse the repository at this point in the history 
* Add options for client and server tracing to skip client errs

This change introduces two configuration options, one for the
server hooks, and one for the client, which configure whether
client-class errors (4xx) are recorded as "error: true" in the
span.

Often times, client errors are expected behavior from the
perspective of the service, and having them report as errors to
trace collectors can obfuscate real service issues.

This is implemented using a functional options paradigm with
var args, which maintains backwards compatibility. Current users
will see no change to behavior or method signatures when updating.

* Pull client options into a dedicated struct within the TraceHTTPClient

* Move server side trace opts into dedicated struct

In response to PR feedback, moves the server side configuraiton
into a dedicated struct, and introduces an adapter hook type to
configure the behavior of the hooks correctly based on the the
configuration options.

* Consolidate options structs
  • Loading branch information
@taywrobel
taywrobel authored May 6, 2020
1 parent ebe5112 commit a536f57
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 48 deletions.
18 changes: 14 additions & 4 deletions trace_http_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,28 @@ type HTTPClient interface {
type TraceHTTPClient struct {
client HTTPClient
tracer opentracing.Tracer
opts *TraceOptions
}

var _ HTTPClient = (*TraceHTTPClient)(nil)

func NewTraceHTTPClient(client HTTPClient, tracer opentracing.Tracer) *TraceHTTPClient {
func NewTraceHTTPClient(client HTTPClient, tracer opentracing.Tracer, opts ...TraceOption) *TraceHTTPClient {
if client == nil {
client = http.DefaultClient
}

clientOpts := &TraceOptions{
includeClientErrors: true,
}

for _, opt := range opts {
opt(clientOpts)
}

return &TraceHTTPClient{
client: client,
tracer: tracer,
opts: clientOpts,
}
}

Expand Down Expand Up @@ -65,9 +75,9 @@ func (c *TraceHTTPClient) Do(req *http.Request) (*http.Response, error) {
}
ext.HTTPStatusCode.Set(span, uint16(res.StatusCode))

// Check for error codes greater than 400, if we have these, then we should
// mark the span as an error.
if res.StatusCode >= 400 {
// Check for error codes greater than 400 if withUserErr is set and codes greater than 500 if not,
// and mark the span as an error if appropriate.
if res.StatusCode >= 400 && c.opts.includeClientErrors || res.StatusCode >= 500 {
span.SetTag("error", true)
}

Expand Down
21 changes: 18 additions & 3 deletions trace_http_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ func TestTraceHTTPClient(t *testing.T) {
desc string
errExpected bool
service twirptest.Haberdasher
clientOpts []TraceOption
expectedTags func(*httptest.Server) map[string]interface{}
}{
{
Expand Down Expand Up @@ -49,13 +50,27 @@ func TestTraceHTTPClient(t *testing.T) {
}
},
},
{
desc: "does not report client errors in span if correct option is set",
errExpected: true,
service: twirptest.ErroringHatmaker(twirp.NotFoundError("not found")),
clientOpts: []TraceOption{IncludeClientErrors(false)},
expectedTags: func(server *httptest.Server) map[string]interface{} {
return map[string]interface{}{
"span.kind": ext.SpanKindEnum("client"),
"http.status_code": uint16(404),
"http.url": fmt.Sprintf("%s/twirp/twirptest.Haberdasher/MakeHat", server.URL),
"http.method": "POST",
}
},
},
}

for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
tracer := setupMockTracer()
hooks := NewOpenTracingHooks(tracer)
server, client := TraceServerAndTraceClient(tt.service, hooks, tracer)
server, client := TraceServerAndTraceClient(tt.service, hooks, tracer, tt.clientOpts...)
defer server.Close()

_, err := client.MakeHat(context.Background(), &twirptest.Size{})
Expand All @@ -76,8 +91,8 @@ func TestTraceHTTPClient(t *testing.T) {
}
}

func TraceServerAndTraceClient(h twirptest.Haberdasher, hooks *twirp.ServerHooks, tracer opentracing.Tracer) (*httptest.Server, twirptest.Haberdasher) {
func TraceServerAndTraceClient(h twirptest.Haberdasher, hooks *twirp.ServerHooks, tracer opentracing.Tracer, opts ...TraceOption) (*httptest.Server, twirptest.Haberdasher) {
s := httptest.NewServer(WithTraceContext(twirptest.NewHaberdasherServer(h, hooks), tracer))
c := twirptest.NewHaberdasherProtobufClient(s.URL, NewTraceHTTPClient(http.DefaultClient, tracer))
c := twirptest.NewHaberdasherProtobufClient(s.URL, NewTraceHTTPClient(http.DefaultClient, tracer, opts...))
return s, c
}
115 changes: 75 additions & 40 deletions tracing.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,48 +20,80 @@ type tracingInfoKey struct{}
// TODO: Add functional options for things such as filtering or maybe logging
// custom fields?

type TraceServerHooks struct {
Tracer ot.Tracer
opts *TraceOptions
}

type TraceOptions struct {
includeClientErrors bool
}

type TraceOption func(opts *TraceOptions)

// IncludeClientErrors, if set, will report client errors (4xx) as errors in the server span.
// If not set, only 5xx status will be reported as erroneous.
func IncludeClientErrors(includeClientErrors bool) TraceOption {
return func(opts *TraceOptions) {
opts.includeClientErrors = includeClientErrors
}
}

// NewOpenTracingHooks provides a twirp.ServerHooks struct which records
// OpenTracing spans.
func NewOpenTracingHooks(tracer ot.Tracer) *twirp.ServerHooks {
hooks := &twirp.ServerHooks{
RequestReceived: startTraceSpan(tracer),
RequestRouted: handleRequestRouted,
ResponseSent: finishTrace,
Error: handleError,
func NewOpenTracingHooks(tracer ot.Tracer, opts ...TraceOption) *twirp.ServerHooks {
serverOpts := &TraceOptions{
includeClientErrors: true,
}

for _, opt := range opts {
opt(serverOpts)
}

return hooks
traceHooks := &TraceServerHooks{
Tracer: tracer,
opts: serverOpts,
}

return traceHooks.TwirpHooks()
}

func startTraceSpan(tracer ot.Tracer) func(context.Context) (context.Context, error) {
return func(ctx context.Context) (context.Context, error) {
spanContext, err := extractSpanCtx(ctx, tracer)
if err != nil && err != ot.ErrSpanContextNotFound { // nolint: megacheck, staticcheck
// TODO: We need to do error reporting here. The tracer implementation
// will have to do something because we don't know where this error will
// live.
}
// Create the initial span, it won't have a method name just yet.
span, ctx := ot.StartSpanFromContext(ctx, RequestReceivedEvent, ext.RPCServerOption(spanContext), ext.SpanKindRPCServer)
if span != nil {
span.SetTag("component", "twirp")

if packageName, ok := twirp.PackageName(ctx); ok {
span.SetTag("package", packageName)
}

if serviceName, ok := twirp.ServiceName(ctx); ok {
span.SetTag("service", serviceName)
}
func (t *TraceServerHooks) TwirpHooks() *twirp.ServerHooks {
return &twirp.ServerHooks{
RequestReceived: t.startTraceSpan,
RequestRouted: t.handleRequestRouted,
ResponseSent: t.finishTrace,
Error: t.handleError,
}
}

func (t *TraceServerHooks) startTraceSpan(ctx context.Context) (context.Context, error) {
spanContext, err := extractSpanCtx(ctx, t.Tracer)
if err != nil && err != ot.ErrSpanContextNotFound { // nolint: megacheck, staticcheck
// TODO: We need to do error reporting here. The tracer implementation
// will have to do something because we don't know where this error will
// live.
}
// Create the initial span, it won't have a method name just yet.
span, ctx := ot.StartSpanFromContext(ctx, RequestReceivedEvent, ext.RPCServerOption(spanContext), ext.SpanKindRPCServer)
if span != nil {
span.SetTag("component", "twirp")

if packageName, ok := twirp.PackageName(ctx); ok {
span.SetTag("package", packageName)
}

return ctx, nil
if serviceName, ok := twirp.ServiceName(ctx); ok {
span.SetTag("service", serviceName)
}
}

return ctx, nil
}

// handleRequestRouted sets the operation name because we won't know what it is
// until the RequestRouted hook.
func handleRequestRouted(ctx context.Context) (context.Context, error) {
func (t *TraceServerHooks) handleRequestRouted(ctx context.Context) (context.Context, error) {
span := ot.SpanFromContext(ctx)
if span != nil {
if method, ok := twirp.MethodName(ctx); ok {
Expand All @@ -72,7 +104,7 @@ func handleRequestRouted(ctx context.Context) (context.Context, error) {
return ctx, nil
}

func finishTrace(ctx context.Context) {
func (t *TraceServerHooks) finishTrace(ctx context.Context) {
span := ot.SpanFromContext(ctx)
if span != nil {
status, haveStatus := twirp.StatusCode(ctx)
Expand All @@ -87,6 +119,19 @@ func finishTrace(ctx context.Context) {
}
}

func (t *TraceServerHooks) handleError(ctx context.Context, err twirp.Error) context.Context {
span := ot.SpanFromContext(ctx)
statusCode := twirp.ServerHTTPStatusFromErrorCode(err.Code())
if span != nil {
if t.opts.includeClientErrors || statusCode >= 500 {
span.SetTag("error", true)
}
span.LogFields(otlog.String("event", "error"), otlog.String("message", err.Msg()))
}

return ctx
}

// WithTraceContext wraps the handler and extracts the span context from request
// headers to attach to the context for connecting client and server calls.
func WithTraceContext(base http.Handler, tracer ot.Tracer) http.Handler {
Expand All @@ -100,16 +145,6 @@ func WithTraceContext(base http.Handler, tracer ot.Tracer) http.Handler {
})
}

func handleError(ctx context.Context, err twirp.Error) context.Context {
span := ot.SpanFromContext(ctx)
if span != nil {
span.SetTag("error", true)
span.LogFields(otlog.String("event", "error"), otlog.String("message", err.Msg()))
}

return ctx
}

func extractSpanCtx(ctx context.Context, tracer ot.Tracer) (ot.SpanContext, error) {
carrier := ctx.Value(tracingInfoKey{})
return tracer.Extract(ot.HTTPHeaders, carrier)
Expand Down
32 changes: 31 additions & 1 deletion tracing_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ func TestTracingHooks(t *testing.T) {
tests := []struct {
desc string
service twirptest.Haberdasher
traceOpts []TraceOption
expectedTags map[string]interface{}
expectedLogs []mocktracer.MockLogRecord
errExpected bool
Expand Down Expand Up @@ -66,12 +67,41 @@ func TestTracingHooks(t *testing.T) {
},
errExpected: true,
},
{
desc: "user error should be not reported as an erroneous span when correct option is set",
service: twirptest.ErroringHatmaker(twirp.NotFoundError("not found")),
traceOpts: []TraceOption{IncludeClientErrors(false)},
expectedTags: map[string]interface{}{
"package": "twirptest",
"component": "twirp",
"service": "Haberdasher",
"span.kind": serverType,
"http.status_code": int64(404),
},
expectedLogs: []mocktracer.MockLogRecord{
{
Fields: []mocktracer.MockKeyValue{
{
Key: "event",
ValueKind: reflect.String,
ValueString: "error",
},
{
Key: "message",
ValueKind: reflect.String,
ValueString: "not found",
},
},
},
},
errExpected: true,
},
}

for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
tracer := setupMockTracer()
hooks := NewOpenTracingHooks(tracer)
hooks := NewOpenTracingHooks(tracer, tt.traceOpts...)

server, client := serverAndClient(tt.service, hooks)
defer server.Close()
Expand Down

0 comments on commit a536f57

Please sign in to comment.