From 377c235dc504d65a5046d06193dafff46044a197 Mon Sep 17 00:00:00 2001 From: Alexander Babai Date: Mon, 22 Mar 2021 02:46:43 +0300 Subject: [PATCH 1/9] refactor(tracing.opentracing): add endpoint tracing options - optionally trace business errors - add option to set static/dynamic tags - add option to set dynamic name --- go.mod | 2 +- go.sum | 5 +- tracing/opentracing/endpoint.go | 82 ++++++++++++++++++++++--- tracing/opentracing/endpoint_options.go | 69 +++++++++++++++++++++ 4 files changed, 147 insertions(+), 11 deletions(-) create mode 100644 tracing/opentracing/endpoint_options.go diff --git a/go.mod b/go.mod index 1d66e8255..e1669cafc 100644 --- a/go.mod +++ b/go.mod @@ -35,7 +35,7 @@ require ( github.com/oklog/run v1.0.0 // indirect github.com/op/go-logging v0.0.0-20160315200505-970db520ece7 // indirect github.com/opentracing/basictracer-go v1.0.0 // indirect - github.com/opentracing/opentracing-go v1.1.0 + github.com/opentracing/opentracing-go v1.2.0 github.com/openzipkin-contrib/zipkin-go-opentracing v0.4.5 github.com/openzipkin/zipkin-go v0.2.2 github.com/pact-foundation/pact-go v1.0.4 diff --git a/go.sum b/go.sum index 9a80e3e43..b41a5f30d 100644 --- a/go.sum +++ b/go.sum @@ -106,7 +106,6 @@ github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs= github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q= -github.com/google/btree v0.0.0-20180813153112-4030bb1f1f0c h1:964Od4U6p2jUkFxvCydnIczKteheJEzHRToSGK3Bnlw= github.com/google/btree v0.0.0-20180813153112-4030bb1f1f0c/go.mod h1:lNA+9X1NB3Zf8V7Ke586lFgjr2dZNuvo3lPJSGZ5JPQ= github.com/google/btree v1.0.0 h1:0udJVsspx3VBr5FwtLhQQtuAsVc79tTq0ocGIPAU6qo= github.com/google/btree v1.0.0/go.mod h1:lNA+9X1NB3Zf8V7Ke586lFgjr2dZNuvo3lPJSGZ5JPQ= @@ -254,8 +253,9 @@ github.com/opentracing-contrib/go-observer v0.0.0-20170622124052-a52f23424492/go github.com/opentracing/basictracer-go v1.0.0 h1:YyUAhaEfjoWXclZVJ9sGoNct7j4TVk7lZWlQw5UXuoo= github.com/opentracing/basictracer-go v1.0.0/go.mod h1:QfBfYuafItcjQuMwinw9GhYKwFXS9KnPs5lxoYwgW74= github.com/opentracing/opentracing-go v1.0.2/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o= -github.com/opentracing/opentracing-go v1.1.0 h1:pWlfV3Bxv7k65HYwkikxat0+s3pV4bsqf19k25Ur8rU= github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o= +github.com/opentracing/opentracing-go v1.2.0 h1:uEJPy/1a5RIPAJ0Ov+OIO8OxWu77jEv+1B0VhjKrZUs= +github.com/opentracing/opentracing-go v1.2.0/go.mod h1:GxEUsuufX4nBwe+T+Wl9TAgYrxe9dPLANfrWvHYVTgc= github.com/openzipkin-contrib/zipkin-go-opentracing v0.4.5 h1:ZCnq+JUrvXcDVhX/xRolRBZifmabN1HcS1wrPSvxhrU= github.com/openzipkin-contrib/zipkin-go-opentracing v0.4.5/go.mod h1:/wsWhb9smxSfWAKL3wpBW7V8scJMt8N8gnaMCS9E/cA= github.com/openzipkin/zipkin-go v0.2.1/go.mod h1:NaW6tEwdmWMaCDZzg8sh+IBNOxHMPnhQw8ySjnjRyN4= @@ -380,7 +380,6 @@ golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190603091049-60506f45cf65/go.mod h1:HSz+uSET+XFnRR8LxR5pz3Of3rY3CfYBVs4xY44aLks= golang.org/x/net v0.0.0-20190613194153-d28f0bde5980/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= -golang.org/x/net v0.0.0-20190620200207-3b0461eec859 h1:R/3boaszxrf1GEUWTVDzSKVwLmSJpwZ1yqXm8j0v2QI= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7 h1:fHDIZ2oxGnUZRN6WgWFCbYBjH9uqVPRCUVUDhs0wnbA= golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= diff --git a/tracing/opentracing/endpoint.go b/tracing/opentracing/endpoint.go index 0482e9c0d..6195ceb19 100644 --- a/tracing/opentracing/endpoint.go +++ b/tracing/opentracing/endpoint.go @@ -4,7 +4,7 @@ import ( "context" "github.com/opentracing/opentracing-go" - otext "github.com/opentracing/opentracing-go/ext" + "github.com/opentracing/opentracing-go/ext" "github.com/go-kit/kit/endpoint" ) @@ -14,9 +14,21 @@ import ( // // If `ctx` already has a Span, it is re-used and the operation name is // overwritten. If `ctx` does not yet have a Span, one is created here. -func TraceServer(tracer opentracing.Tracer, operationName string) endpoint.Middleware { +func TraceServer(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { + cfg := &EndpointOptions{} + + for _, opt := range opts { + opt(cfg) + } + return func(next endpoint.Endpoint) endpoint.Endpoint { return func(ctx context.Context, request interface{}) (interface{}, error) { + if cfg.GetOperationName != nil { + if newOperationName := cfg.GetOperationName(ctx, operationName); newOperationName != "" { + operationName = newOperationName + } + } + serverSpan := opentracing.SpanFromContext(ctx) if serverSpan == nil { // All we can do is create a new root span. @@ -25,18 +37,42 @@ func TraceServer(tracer opentracing.Tracer, operationName string) endpoint.Middl serverSpan.SetOperationName(operationName) } defer serverSpan.Finish() - otext.SpanKindRPCServer.Set(serverSpan) + ext.SpanKindRPCServer.Set(serverSpan) ctx = opentracing.ContextWithSpan(ctx, serverSpan) - return next(ctx, request) + + applyTags(serverSpan, cfg.Tags) + if cfg.GetTags != nil { + extraTags := cfg.GetTags(ctx) + applyTags(serverSpan, extraTags) + } + + response, err := next(ctx, request) + if err := identifyError(response, err, cfg.IgnoreBusinessError); err != nil { + ext.LogError(serverSpan, err) + } + + return response, err } } } // TraceClient returns a Middleware that wraps the `next` Endpoint in an // OpenTracing Span called `operationName`. -func TraceClient(tracer opentracing.Tracer, operationName string) endpoint.Middleware { +func TraceClient(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { + cfg := &EndpointOptions{} + + for _, opt := range opts { + opt(cfg) + } + return func(next endpoint.Endpoint) endpoint.Endpoint { return func(ctx context.Context, request interface{}) (interface{}, error) { + if cfg.GetOperationName != nil { + if newOperationName := cfg.GetOperationName(ctx, operationName); newOperationName != "" { + operationName = newOperationName + } + } + var clientSpan opentracing.Span if parentSpan := opentracing.SpanFromContext(ctx); parentSpan != nil { clientSpan = tracer.StartSpan( @@ -47,9 +83,41 @@ func TraceClient(tracer opentracing.Tracer, operationName string) endpoint.Middl clientSpan = tracer.StartSpan(operationName) } defer clientSpan.Finish() - otext.SpanKindRPCClient.Set(clientSpan) + ext.SpanKindRPCClient.Set(clientSpan) ctx = opentracing.ContextWithSpan(ctx, clientSpan) - return next(ctx, request) + + applyTags(clientSpan, cfg.Tags) + if cfg.GetTags != nil { + extraTags := cfg.GetTags(ctx) + applyTags(clientSpan, extraTags) + } + + response, err := next(ctx, request) + if err := identifyError(response, err, cfg.IgnoreBusinessError); err != nil { + ext.LogError(clientSpan, err) + } + + return response, err } } } + +func applyTags(span opentracing.Span, tags opentracing.Tags) { + for key, value := range tags { + span.SetTag(key, value) + } +} + +func identifyError(response interface{}, err error, ignoreBusinessError bool) error { + if err != nil { + return err + } + + if !ignoreBusinessError { + if res, ok := response.(endpoint.Failer); ok { + return res.Failed() + } + } + + return nil +} diff --git a/tracing/opentracing/endpoint_options.go b/tracing/opentracing/endpoint_options.go new file mode 100644 index 000000000..b0cd2ded1 --- /dev/null +++ b/tracing/opentracing/endpoint_options.go @@ -0,0 +1,69 @@ +package opentracing + +import ( + "context" + "github.com/opentracing/opentracing-go" +) + +// EndpointOptions holds the options for tracing an endpoint +type EndpointOptions struct { + // IgnoreBusinessError if set to true will not treat a business error + // identified through the endpoint.Failer interface as a span error. + IgnoreBusinessError bool + + // GetOperationName is an optional function that can set the span operation name based on the existing one + // for the endpoint and information in the context. + // + // If the function is nil, or the returned name is empty, the existing name for the endpoint is used. + GetOperationName func(ctx context.Context, name string) string + + // Tags holds the default tags which will be set on span + // creation by our Endpoint middleware. + Tags opentracing.Tags + + // GetTags is an optional function that can extract trace tags + // from the context and add them to the span. + GetTags func(ctx context.Context) opentracing.Tags +} + +// EndpointOption allows for functional options to Opentracing endpoint +// tracing middleware. +type EndpointOption func(*EndpointOptions) + +// WithOptions sets all configuration options at once by use of the +// EndpointOptions struct. +func WithOptions(options EndpointOptions) EndpointOption { + return func(o *EndpointOptions) { + *o = options + } +} + +// WithIgnoreBusinessError if set to true will not treat a business error +// identified through the endpoint.Failer interface as a span error. +func WithIgnoreBusinessError(ignoreBusinessError bool) EndpointOption { + return func(o *EndpointOptions) { + o.IgnoreBusinessError = ignoreBusinessError + } +} + +// WithOperationName allows to set function that can set the span operation name based on the existing one +// for the endpoint and information in the context. +func WithOperationName(getOperationName func(ctx context.Context, name string) string) EndpointOption { + return func(o *EndpointOptions) { + o.GetOperationName = getOperationName + } +} + +// WithTags sets the default tags for the spans created by the Endpoint tracer. +func WithTags(tags opentracing.Tags) EndpointOption { + return func(o *EndpointOptions) { + o.Tags = tags + } +} + +// WithExtraTags extracts additional attributes from the context. +func WithExtraTags(getTags func(ctx context.Context) opentracing.Tags) EndpointOption { + return func(o *EndpointOptions) { + o.GetTags = getTags + } +} From 038371bb06e02bad23e45099287b85db2fc96851 Mon Sep 17 00:00:00 2001 From: Alexander Babai Date: Mon, 22 Mar 2021 03:55:24 +0300 Subject: [PATCH 2/9] test(tracing.opentracing): cover endpoint options --- tracing/opentracing/endpoint_test.go | 352 +++++++++++++++++++++++++++ 1 file changed, 352 insertions(+) diff --git a/tracing/opentracing/endpoint_test.go b/tracing/opentracing/endpoint_test.go index a26f7bab6..26d352c80 100644 --- a/tracing/opentracing/endpoint_test.go +++ b/tracing/opentracing/endpoint_test.go @@ -2,6 +2,9 @@ package opentracing_test import ( "context" + "errors" + "fmt" + "github.com/opentracing/opentracing-go/ext" "testing" "github.com/opentracing/opentracing-go" @@ -11,6 +14,33 @@ import ( kitot "github.com/go-kit/kit/tracing/opentracing" ) +const ( + span1 = "SPAN-1" + span2 = "SPAN-2" + span3 = "SPAN-3" + span4 = "SPAN-4" + span5 = "SPAN-5" + span6 = "SPAN-6" + span7 = "SPAN-7" +) + +var ( + err1 = errors.New("some error") + err2 = errors.New("some business error") + err3 = errors.New("other business error") +) + +// compile time assertion +var _ endpoint.Failer = failedResponse{} + +type failedResponse struct { + err error +} + +func (r failedResponse) Failed() error { + return r.err +} + func TestTraceServer(t *testing.T) { tracer := mocktracer.New() @@ -62,6 +92,167 @@ func TestTraceServerNoContextSpan(t *testing.T) { } } +func TestTraceServerWithOptions(t *testing.T) { + tracer := mocktracer.New() + + // span 1 without options + mw := kitot.TraceServer(tracer, span1) + tracedEndpoint := mw(endpoint.Nop) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 2 with options + mw = kitot.TraceServer( + tracer, + span2, + kitot.WithOptions(kitot.EndpointOptions{}), + ) + tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { + return nil, err1 + }) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 3 with disabled IgnoreBusinessError option + mw = kitot.TraceServer( + tracer, + span3, + kitot.WithIgnoreBusinessError(false), + ) + tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { + return failedResponse{ + err: err2, + }, nil + }) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 4 with enabled IgnoreBusinessError option + mw = kitot.TraceServer(tracer, span4, kitot.WithIgnoreBusinessError(true)) + tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { + return failedResponse{ + err: err3, + }, nil + }) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 5 with OperationName option + mw = kitot.TraceServer( + tracer, + span5, + kitot.WithOperationName(func(ctx context.Context, name string) string { + return fmt.Sprintf("%s-%s", "new", name) + }), + ) + tracedEndpoint = mw(endpoint.Nop) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 6 with Tags options + mw = kitot.TraceServer( + tracer, + span6, + kitot.WithTags(map[string]interface{}{ + "tag1": "tag1", + }), + ) + tracedEndpoint = mw(endpoint.Nop) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 7 with ExtraTags options + mw = kitot.TraceServer( + tracer, + span7, + kitot.WithTags(map[string]interface{}{ + "tag1": "tag1", + }), + kitot.WithExtraTags(func(ctx context.Context) opentracing.Tags { + return map[string]interface{}{ + "tag2": "tag2", + } + }), + ) + tracedEndpoint = mw(endpoint.Nop) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + finishedSpans := tracer.FinishedSpans() + if want, have := 7, len(finishedSpans); want != have { + t.Fatalf("Want %v span(s), found %v", want, have) + } + + // test span 1 + span := finishedSpans[0] + + if want, have := span1, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + // test span 2 + span = finishedSpans[1] + + if want, have := span2, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := true, span.Tag("error"); want != have { + t.Fatalf("Want %v, have %v", want, have) + } + + // test span 3 + span = finishedSpans[2] + + if want, have := span3, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := true, span.Tag("error"); want != have { + t.Fatalf("Want %v, have %v", want, have) + } + + // test span 4 + span = finishedSpans[3] + + if want, have := span4, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := (interface{})(nil), span.Tag("error"); want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + // test span 5 + span = finishedSpans[4] + + if want, have := fmt.Sprintf("%s-%s", "new", span5), span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + // test span 6 + span = finishedSpans[5] + + if want, have := span6, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := map[string]interface{}{ + "span.kind": ext.SpanKindRPCServerEnum, + "tag1": "tag1", + }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { + t.Fatalf("Want %q, have %q", want, have) + } + + // test span 7 + span = finishedSpans[6] + + if want, have := span7, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := map[string]interface{}{ + "span.kind": ext.SpanKindRPCServerEnum, + "tag1": "tag1", + "tag2": "tag2", + }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { + t.Fatalf("Want %q, have %q", want, have) + } +} + func TestTraceClient(t *testing.T) { tracer := mocktracer.New() @@ -115,3 +306,164 @@ func TestTraceClientNoContextSpan(t *testing.T) { t.Fatalf("Want %q, have %q", want, have) } } + +func TestTraceClientWithOptions(t *testing.T) { + tracer := mocktracer.New() + + // span 1 without options + mw := kitot.TraceClient(tracer, span1) + tracedEndpoint := mw(endpoint.Nop) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 2 with options + mw = kitot.TraceClient( + tracer, + span2, + kitot.WithOptions(kitot.EndpointOptions{}), + ) + tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { + return nil, err1 + }) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 3 with disabled IgnoreBusinessError option + mw = kitot.TraceClient( + tracer, + span3, + kitot.WithIgnoreBusinessError(false), + ) + tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { + return failedResponse{ + err: err2, + }, nil + }) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 4 with enabled IgnoreBusinessError option + mw = kitot.TraceClient(tracer, span4, kitot.WithIgnoreBusinessError(true)) + tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { + return failedResponse{ + err: err3, + }, nil + }) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 5 with OperationName option + mw = kitot.TraceClient( + tracer, + span5, + kitot.WithOperationName(func(ctx context.Context, name string) string { + return fmt.Sprintf("%s-%s", "new", name) + }), + ) + tracedEndpoint = mw(endpoint.Nop) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 6 with Tags options + mw = kitot.TraceClient( + tracer, + span6, + kitot.WithTags(map[string]interface{}{ + "tag1": "tag1", + }), + ) + tracedEndpoint = mw(endpoint.Nop) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 7 with ExtraTags options + mw = kitot.TraceClient( + tracer, + span7, + kitot.WithTags(map[string]interface{}{ + "tag1": "tag1", + }), + kitot.WithExtraTags(func(ctx context.Context) opentracing.Tags { + return map[string]interface{}{ + "tag2": "tag2", + } + }), + ) + tracedEndpoint = mw(endpoint.Nop) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + finishedSpans := tracer.FinishedSpans() + if want, have := 7, len(finishedSpans); want != have { + t.Fatalf("Want %v span(s), found %v", want, have) + } + + // test span 1 + span := finishedSpans[0] + + if want, have := span1, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + // test span 2 + span = finishedSpans[1] + + if want, have := span2, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := true, span.Tag("error"); want != have { + t.Fatalf("Want %v, have %v", want, have) + } + + // test span 3 + span = finishedSpans[2] + + if want, have := span3, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := true, span.Tag("error"); want != have { + t.Fatalf("Want %v, have %v", want, have) + } + + // test span 4 + span = finishedSpans[3] + + if want, have := span4, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := (interface{})(nil), span.Tag("error"); want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + // test span 5 + span = finishedSpans[4] + + if want, have := fmt.Sprintf("%s-%s", "new", span5), span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + // test span 6 + span = finishedSpans[5] + + if want, have := span6, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := map[string]interface{}{ + "span.kind": ext.SpanKindRPCClientEnum, + "tag1": "tag1", + }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { + t.Fatalf("Want %q, have %q", want, have) + } + + // test span 7 + span = finishedSpans[6] + + if want, have := span7, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := map[string]interface{}{ + "span.kind": ext.SpanKindRPCClientEnum, + "tag1": "tag1", + "tag2": "tag2", + }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { + t.Fatalf("Want %q, have %q", want, have) + } +} From 185f2f8df469b2aeca6b1113c9f17043e64be568 Mon Sep 17 00:00:00 2001 From: Alexander Babai Date: Mon, 22 Mar 2021 03:59:42 +0300 Subject: [PATCH 3/9] update comments --- tracing/opentracing/endpoint_options.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tracing/opentracing/endpoint_options.go b/tracing/opentracing/endpoint_options.go index b0cd2ded1..f4d5d1bf6 100644 --- a/tracing/opentracing/endpoint_options.go +++ b/tracing/opentracing/endpoint_options.go @@ -21,17 +21,15 @@ type EndpointOptions struct { // creation by our Endpoint middleware. Tags opentracing.Tags - // GetTags is an optional function that can extract trace tags + // GetTags is an optional function that can extract tags // from the context and add them to the span. GetTags func(ctx context.Context) opentracing.Tags } -// EndpointOption allows for functional options to Opentracing endpoint -// tracing middleware. +// EndpointOption allows for functional options to endpoint tracing middleware. type EndpointOption func(*EndpointOptions) -// WithOptions sets all configuration options at once by use of the -// EndpointOptions struct. +// WithOptions sets all configuration options at once by use of the EndpointOptions struct. func WithOptions(options EndpointOptions) EndpointOption { return func(o *EndpointOptions) { *o = options @@ -61,7 +59,7 @@ func WithTags(tags opentracing.Tags) EndpointOption { } } -// WithExtraTags extracts additional attributes from the context. +// WithExtraTags extracts additional tags from the context. func WithExtraTags(getTags func(ctx context.Context) opentracing.Tags) EndpointOption { return func(o *EndpointOptions) { o.GetTags = getTags From af4337097fb5dc50d0913cfaac3b5d7159a81165 Mon Sep 17 00:00:00 2001 From: Alexander Babai Date: Mon, 22 Mar 2021 11:07:19 +0300 Subject: [PATCH 4/9] refactor(tracing.opentracing): options improvements - some renaming - more flexible tags option: support adding tags with multiple options --- tracing/opentracing/endpoint_options.go | 18 ++++++---- tracing/opentracing/endpoint_test.go | 44 +++++++++++++++++++------ 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/tracing/opentracing/endpoint_options.go b/tracing/opentracing/endpoint_options.go index f4d5d1bf6..6dea72bbb 100644 --- a/tracing/opentracing/endpoint_options.go +++ b/tracing/opentracing/endpoint_options.go @@ -44,23 +44,29 @@ func WithIgnoreBusinessError(ignoreBusinessError bool) EndpointOption { } } -// WithOperationName allows to set function that can set the span operation name based on the existing one +// WithOperationNameFunc allows to set function that can set the span operation name based on the existing one // for the endpoint and information in the context. -func WithOperationName(getOperationName func(ctx context.Context, name string) string) EndpointOption { +func WithOperationNameFunc(getOperationName func(ctx context.Context, name string) string) EndpointOption { return func(o *EndpointOptions) { o.GetOperationName = getOperationName } } -// WithTags sets the default tags for the spans created by the Endpoint tracer. +// WithTags adds default tags for the spans created by the Endpoint tracer. func WithTags(tags opentracing.Tags) EndpointOption { return func(o *EndpointOptions) { - o.Tags = tags + if o.Tags == nil { + o.Tags = make(opentracing.Tags) + } + + for key, value := range tags { + o.Tags[key] = value + } } } -// WithExtraTags extracts additional tags from the context. -func WithExtraTags(getTags func(ctx context.Context) opentracing.Tags) EndpointOption { +// WithTagsFunc set the func to extracts additional tags from the context. +func WithTagsFunc(getTags func(ctx context.Context) opentracing.Tags) EndpointOption { return func(o *EndpointOptions) { o.GetTags = getTags } diff --git a/tracing/opentracing/endpoint_test.go b/tracing/opentracing/endpoint_test.go index 26d352c80..b9c4a0f01 100644 --- a/tracing/opentracing/endpoint_test.go +++ b/tracing/opentracing/endpoint_test.go @@ -133,11 +133,11 @@ func TestTraceServerWithOptions(t *testing.T) { }) _, _ = tracedEndpoint(context.Background(), struct{}{}) - // span 5 with OperationName option + // span 5 with OperationNameFunc option mw = kitot.TraceServer( tracer, span5, - kitot.WithOperationName(func(ctx context.Context, name string) string { + kitot.WithOperationNameFunc(func(ctx context.Context, name string) string { return fmt.Sprintf("%s-%s", "new", name) }), ) @@ -150,21 +150,29 @@ func TestTraceServerWithOptions(t *testing.T) { span6, kitot.WithTags(map[string]interface{}{ "tag1": "tag1", + "tag2": "tag2", + }), + kitot.WithTags(map[string]interface{}{ + "tag3": "tag3", }), ) tracedEndpoint = mw(endpoint.Nop) _, _ = tracedEndpoint(context.Background(), struct{}{}) - // span 7 with ExtraTags options + // span 7 with TagsFunc options mw = kitot.TraceServer( tracer, span7, kitot.WithTags(map[string]interface{}{ "tag1": "tag1", + "tag2": "tag2", }), - kitot.WithExtraTags(func(ctx context.Context) opentracing.Tags { + kitot.WithTags(map[string]interface{}{ + "tag3": "tag3", + }), + kitot.WithTagsFunc(func(ctx context.Context) opentracing.Tags { return map[string]interface{}{ - "tag2": "tag2", + "tag4": "tag4", } }), ) @@ -233,6 +241,8 @@ func TestTraceServerWithOptions(t *testing.T) { if want, have := map[string]interface{}{ "span.kind": ext.SpanKindRPCServerEnum, "tag1": "tag1", + "tag2": "tag2", + "tag3": "tag3", }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { t.Fatalf("Want %q, have %q", want, have) } @@ -248,6 +258,8 @@ func TestTraceServerWithOptions(t *testing.T) { "span.kind": ext.SpanKindRPCServerEnum, "tag1": "tag1", "tag2": "tag2", + "tag3": "tag3", + "tag4": "tag4", }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { t.Fatalf("Want %q, have %q", want, have) } @@ -348,11 +360,11 @@ func TestTraceClientWithOptions(t *testing.T) { }) _, _ = tracedEndpoint(context.Background(), struct{}{}) - // span 5 with OperationName option + // span 5 with OperationNameFunc option mw = kitot.TraceClient( tracer, span5, - kitot.WithOperationName(func(ctx context.Context, name string) string { + kitot.WithOperationNameFunc(func(ctx context.Context, name string) string { return fmt.Sprintf("%s-%s", "new", name) }), ) @@ -365,21 +377,29 @@ func TestTraceClientWithOptions(t *testing.T) { span6, kitot.WithTags(map[string]interface{}{ "tag1": "tag1", + "tag2": "tag2", + }), + kitot.WithTags(map[string]interface{}{ + "tag3": "tag3", }), ) tracedEndpoint = mw(endpoint.Nop) _, _ = tracedEndpoint(context.Background(), struct{}{}) - // span 7 with ExtraTags options + // span 7 with TagsFunc options mw = kitot.TraceClient( tracer, span7, kitot.WithTags(map[string]interface{}{ "tag1": "tag1", + "tag2": "tag2", }), - kitot.WithExtraTags(func(ctx context.Context) opentracing.Tags { + kitot.WithTags(map[string]interface{}{ + "tag3": "tag3", + }), + kitot.WithTagsFunc(func(ctx context.Context) opentracing.Tags { return map[string]interface{}{ - "tag2": "tag2", + "tag4": "tag4", } }), ) @@ -448,6 +468,8 @@ func TestTraceClientWithOptions(t *testing.T) { if want, have := map[string]interface{}{ "span.kind": ext.SpanKindRPCClientEnum, "tag1": "tag1", + "tag2": "tag2", + "tag3": "tag3", }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { t.Fatalf("Want %q, have %q", want, have) } @@ -463,6 +485,8 @@ func TestTraceClientWithOptions(t *testing.T) { "span.kind": ext.SpanKindRPCClientEnum, "tag1": "tag1", "tag2": "tag2", + "tag3": "tag3", + "tag4": "tag4", }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { t.Fatalf("Want %q, have %q", want, have) } From a4dd947e506e0fa7af2f7b823ce084c4a8297dc0 Mon Sep 17 00:00:00 2001 From: Alexander Babai Date: Mon, 22 Mar 2021 11:28:04 +0300 Subject: [PATCH 5/9] refactor(tracing.opentracing): add TraceEndpoint function - TraceServer and TraceClient should differ only in span kind tag value - remain TraceServer and TraceClient for backward compatibility --- tracing/opentracing/endpoint.go | 91 ++++------ tracing/opentracing/endpoint_test.go | 261 +++++---------------------- 2 files changed, 80 insertions(+), 272 deletions(-) diff --git a/tracing/opentracing/endpoint.go b/tracing/opentracing/endpoint.go index 6195ceb19..1e8cb813f 100644 --- a/tracing/opentracing/endpoint.go +++ b/tracing/opentracing/endpoint.go @@ -9,58 +9,16 @@ import ( "github.com/go-kit/kit/endpoint" ) -// TraceServer returns a Middleware that wraps the `next` Endpoint in an +// TraceEndpoint returns a Middleware that wraps the `next` Endpoint in an // OpenTracing Span called `operationName`. // -// If `ctx` already has a Span, it is re-used and the operation name is -// overwritten. If `ctx` does not yet have a Span, one is created here. -func TraceServer(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { - cfg := &EndpointOptions{} - - for _, opt := range opts { - opt(cfg) +// If `ctx` already has a Span, child span is created from it. +// If `ctx` doesn't yet have a Span, the new one is created. +func TraceEndpoint(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { + cfg := &EndpointOptions{ + Tags: make(opentracing.Tags), } - return func(next endpoint.Endpoint) endpoint.Endpoint { - return func(ctx context.Context, request interface{}) (interface{}, error) { - if cfg.GetOperationName != nil { - if newOperationName := cfg.GetOperationName(ctx, operationName); newOperationName != "" { - operationName = newOperationName - } - } - - serverSpan := opentracing.SpanFromContext(ctx) - if serverSpan == nil { - // All we can do is create a new root span. - serverSpan = tracer.StartSpan(operationName) - } else { - serverSpan.SetOperationName(operationName) - } - defer serverSpan.Finish() - ext.SpanKindRPCServer.Set(serverSpan) - ctx = opentracing.ContextWithSpan(ctx, serverSpan) - - applyTags(serverSpan, cfg.Tags) - if cfg.GetTags != nil { - extraTags := cfg.GetTags(ctx) - applyTags(serverSpan, extraTags) - } - - response, err := next(ctx, request) - if err := identifyError(response, err, cfg.IgnoreBusinessError); err != nil { - ext.LogError(serverSpan, err) - } - - return response, err - } - } -} - -// TraceClient returns a Middleware that wraps the `next` Endpoint in an -// OpenTracing Span called `operationName`. -func TraceClient(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { - cfg := &EndpointOptions{} - for _, opt := range opts { opt(cfg) } @@ -73,28 +31,27 @@ func TraceClient(tracer opentracing.Tracer, operationName string, opts ...Endpoi } } - var clientSpan opentracing.Span + span := opentracing.SpanFromContext(ctx) if parentSpan := opentracing.SpanFromContext(ctx); parentSpan != nil { - clientSpan = tracer.StartSpan( + span = tracer.StartSpan( operationName, opentracing.ChildOf(parentSpan.Context()), ) } else { - clientSpan = tracer.StartSpan(operationName) + span = tracer.StartSpan(operationName) } - defer clientSpan.Finish() - ext.SpanKindRPCClient.Set(clientSpan) - ctx = opentracing.ContextWithSpan(ctx, clientSpan) + defer span.Finish() + ctx = opentracing.ContextWithSpan(ctx, span) - applyTags(clientSpan, cfg.Tags) + applyTags(span, cfg.Tags) if cfg.GetTags != nil { extraTags := cfg.GetTags(ctx) - applyTags(clientSpan, extraTags) + applyTags(span, extraTags) } response, err := next(ctx, request) if err := identifyError(response, err, cfg.IgnoreBusinessError); err != nil { - ext.LogError(clientSpan, err) + ext.LogError(span, err) } return response, err @@ -102,6 +59,26 @@ func TraceClient(tracer opentracing.Tracer, operationName string, opts ...Endpoi } } +// TraceServer returns a Middleware that wraps the `next` Endpoint in an +// OpenTracing Span called `operationName` with server span.kind tag.. +func TraceServer(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { + opts = append(opts, WithTags(map[string]interface{}{ + ext.SpanKindRPCServer.Key: ext.SpanKindRPCServer.Value, + })) + + return TraceEndpoint(tracer, operationName, opts...) +} + +// TraceClient returns a Middleware that wraps the `next` Endpoint in an +// OpenTracing Span called `operationName` with client span.kind tag. +func TraceClient(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { + opts = append(opts, WithTags(map[string]interface{}{ + ext.SpanKindRPCServer.Key: ext.SpanKindRPCClient.Value, + })) + + return TraceEndpoint(tracer, operationName, opts...) +} + func applyTags(span opentracing.Span, tags opentracing.Tags) { for key, value := range tags { span.SetTag(key, value) diff --git a/tracing/opentracing/endpoint_test.go b/tracing/opentracing/endpoint_test.go index b9c4a0f01..72c42cb5a 100644 --- a/tracing/opentracing/endpoint_test.go +++ b/tracing/opentracing/endpoint_test.go @@ -41,41 +41,44 @@ func (r failedResponse) Failed() error { return r.err } -func TestTraceServer(t *testing.T) { +func TestTraceEndpoint(t *testing.T) { tracer := mocktracer.New() - // Initialize the ctx with a nameless Span. - contextSpan := tracer.StartSpan("").(*mocktracer.MockSpan) - ctx := opentracing.ContextWithSpan(context.Background(), contextSpan) + // Initialize the ctx with a parent Span. + parentSpan := tracer.StartSpan("parent").(*mocktracer.MockSpan) + defer parentSpan.Finish() + ctx := opentracing.ContextWithSpan(context.Background(), parentSpan) - tracedEndpoint := kitot.TraceServer(tracer, "testOp")(endpoint.Nop) + tracedEndpoint := kitot.TraceEndpoint(tracer, "testOp")(endpoint.Nop) if _, err := tracedEndpoint(ctx, struct{}{}); err != nil { t.Fatal(err) } + // tracedEndpoint created a new Span. finishedSpans := tracer.FinishedSpans() if want, have := 1, len(finishedSpans); want != have { t.Fatalf("Want %v span(s), found %v", want, have) } - // Test that the op name is updated endpointSpan := finishedSpans[0] if want, have := "testOp", endpointSpan.OperationName; want != have { t.Fatalf("Want %q, have %q", want, have) } - contextContext := contextSpan.Context().(mocktracer.MockSpanContext) - endpointContext := endpointSpan.Context().(mocktracer.MockSpanContext) - // ...and that the ID is unmodified. - if want, have := contextContext.SpanID, endpointContext.SpanID; want != have { - t.Errorf("Want SpanID %q, have %q", want, have) + + parentContext := parentSpan.Context().(mocktracer.MockSpanContext) + endpointContext := parentSpan.Context().(mocktracer.MockSpanContext) + + // ... and that the parent ID is set appropriately. + if want, have := parentContext.SpanID, endpointContext.SpanID; want != have { + t.Errorf("Want ParentID %q, have %q", want, have) } } -func TestTraceServerNoContextSpan(t *testing.T) { +func TestTraceEndpointNoContextSpan(t *testing.T) { tracer := mocktracer.New() // Empty/background context. - tracedEndpoint := kitot.TraceServer(tracer, "testOp")(endpoint.Nop) + tracedEndpoint := kitot.TraceEndpoint(tracer, "testOp")(endpoint.Nop) if _, err := tracedEndpoint(context.Background(), struct{}{}); err != nil { t.Fatal(err) } @@ -87,21 +90,22 @@ func TestTraceServerNoContextSpan(t *testing.T) { } endpointSpan := finishedSpans[0] + if want, have := "testOp", endpointSpan.OperationName; want != have { t.Fatalf("Want %q, have %q", want, have) } } -func TestTraceServerWithOptions(t *testing.T) { +func TestTraceEndpointWithOptions(t *testing.T) { tracer := mocktracer.New() // span 1 without options - mw := kitot.TraceServer(tracer, span1) + mw := kitot.TraceEndpoint(tracer, span1) tracedEndpoint := mw(endpoint.Nop) _, _ = tracedEndpoint(context.Background(), struct{}{}) // span 2 with options - mw = kitot.TraceServer( + mw = kitot.TraceEndpoint( tracer, span2, kitot.WithOptions(kitot.EndpointOptions{}), @@ -112,7 +116,7 @@ func TestTraceServerWithOptions(t *testing.T) { _, _ = tracedEndpoint(context.Background(), struct{}{}) // span 3 with disabled IgnoreBusinessError option - mw = kitot.TraceServer( + mw = kitot.TraceEndpoint( tracer, span3, kitot.WithIgnoreBusinessError(false), @@ -125,7 +129,7 @@ func TestTraceServerWithOptions(t *testing.T) { _, _ = tracedEndpoint(context.Background(), struct{}{}) // span 4 with enabled IgnoreBusinessError option - mw = kitot.TraceServer(tracer, span4, kitot.WithIgnoreBusinessError(true)) + mw = kitot.TraceEndpoint(tracer, span4, kitot.WithIgnoreBusinessError(true)) tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { return failedResponse{ err: err3, @@ -134,7 +138,7 @@ func TestTraceServerWithOptions(t *testing.T) { _, _ = tracedEndpoint(context.Background(), struct{}{}) // span 5 with OperationNameFunc option - mw = kitot.TraceServer( + mw = kitot.TraceEndpoint( tracer, span5, kitot.WithOperationNameFunc(func(ctx context.Context, name string) string { @@ -145,7 +149,7 @@ func TestTraceServerWithOptions(t *testing.T) { _, _ = tracedEndpoint(context.Background(), struct{}{}) // span 6 with Tags options - mw = kitot.TraceServer( + mw = kitot.TraceEndpoint( tracer, span6, kitot.WithTags(map[string]interface{}{ @@ -160,7 +164,7 @@ func TestTraceServerWithOptions(t *testing.T) { _, _ = tracedEndpoint(context.Background(), struct{}{}) // span 7 with TagsFunc options - mw = kitot.TraceServer( + mw = kitot.TraceEndpoint( tracer, span7, kitot.WithTags(map[string]interface{}{ @@ -239,10 +243,9 @@ func TestTraceServerWithOptions(t *testing.T) { } if want, have := map[string]interface{}{ - "span.kind": ext.SpanKindRPCServerEnum, - "tag1": "tag1", - "tag2": "tag2", - "tag3": "tag3", + "tag1": "tag1", + "tag2": "tag2", + "tag3": "tag3", }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { t.Fatalf("Want %q, have %q", want, have) } @@ -255,26 +258,21 @@ func TestTraceServerWithOptions(t *testing.T) { } if want, have := map[string]interface{}{ - "span.kind": ext.SpanKindRPCServerEnum, - "tag1": "tag1", - "tag2": "tag2", - "tag3": "tag3", - "tag4": "tag4", + "tag1": "tag1", + "tag2": "tag2", + "tag3": "tag3", + "tag4": "tag4", }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { t.Fatalf("Want %q, have %q", want, have) } } -func TestTraceClient(t *testing.T) { +func TestTraceServer(t *testing.T) { tracer := mocktracer.New() - // Initialize the ctx with a parent Span. - parentSpan := tracer.StartSpan("parent").(*mocktracer.MockSpan) - defer parentSpan.Finish() - ctx := opentracing.ContextWithSpan(context.Background(), parentSpan) - - tracedEndpoint := kitot.TraceClient(tracer, "testOp")(endpoint.Nop) - if _, err := tracedEndpoint(ctx, struct{}{}); err != nil { + // Empty/background context. + tracedEndpoint := kitot.TraceServer(tracer, "testOp")(endpoint.Nop) + if _, err := tracedEndpoint(context.Background(), struct{}{}); err != nil { t.Fatal(err) } @@ -284,21 +282,20 @@ func TestTraceClient(t *testing.T) { t.Fatalf("Want %v span(s), found %v", want, have) } - endpointSpan := finishedSpans[0] - if want, have := "testOp", endpointSpan.OperationName; want != have { + span := finishedSpans[0] + + if want, have := "testOp", span.OperationName; want != have { t.Fatalf("Want %q, have %q", want, have) } - parentContext := parentSpan.Context().(mocktracer.MockSpanContext) - endpointContext := parentSpan.Context().(mocktracer.MockSpanContext) - - // ... and that the parent ID is set appropriately. - if want, have := parentContext.SpanID, endpointContext.SpanID; want != have { - t.Errorf("Want ParentID %q, have %q", want, have) + if want, have := map[string]interface{}{ + ext.SpanKindRPCServer.Key: ext.SpanKindRPCServer.Value, + }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { + t.Fatalf("Want %q, have %q", want, have) } } -func TestTraceClientNoContextSpan(t *testing.T) { +func TestTraceClient(t *testing.T) { tracer := mocktracer.New() // Empty/background context. @@ -313,180 +310,14 @@ func TestTraceClientNoContextSpan(t *testing.T) { t.Fatalf("Want %v span(s), found %v", want, have) } - endpointSpan := finishedSpans[0] - if want, have := "testOp", endpointSpan.OperationName; want != have { - t.Fatalf("Want %q, have %q", want, have) - } -} - -func TestTraceClientWithOptions(t *testing.T) { - tracer := mocktracer.New() - - // span 1 without options - mw := kitot.TraceClient(tracer, span1) - tracedEndpoint := mw(endpoint.Nop) - _, _ = tracedEndpoint(context.Background(), struct{}{}) - - // span 2 with options - mw = kitot.TraceClient( - tracer, - span2, - kitot.WithOptions(kitot.EndpointOptions{}), - ) - tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { - return nil, err1 - }) - _, _ = tracedEndpoint(context.Background(), struct{}{}) - - // span 3 with disabled IgnoreBusinessError option - mw = kitot.TraceClient( - tracer, - span3, - kitot.WithIgnoreBusinessError(false), - ) - tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { - return failedResponse{ - err: err2, - }, nil - }) - _, _ = tracedEndpoint(context.Background(), struct{}{}) - - // span 4 with enabled IgnoreBusinessError option - mw = kitot.TraceClient(tracer, span4, kitot.WithIgnoreBusinessError(true)) - tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { - return failedResponse{ - err: err3, - }, nil - }) - _, _ = tracedEndpoint(context.Background(), struct{}{}) - - // span 5 with OperationNameFunc option - mw = kitot.TraceClient( - tracer, - span5, - kitot.WithOperationNameFunc(func(ctx context.Context, name string) string { - return fmt.Sprintf("%s-%s", "new", name) - }), - ) - tracedEndpoint = mw(endpoint.Nop) - _, _ = tracedEndpoint(context.Background(), struct{}{}) - - // span 6 with Tags options - mw = kitot.TraceClient( - tracer, - span6, - kitot.WithTags(map[string]interface{}{ - "tag1": "tag1", - "tag2": "tag2", - }), - kitot.WithTags(map[string]interface{}{ - "tag3": "tag3", - }), - ) - tracedEndpoint = mw(endpoint.Nop) - _, _ = tracedEndpoint(context.Background(), struct{}{}) - - // span 7 with TagsFunc options - mw = kitot.TraceClient( - tracer, - span7, - kitot.WithTags(map[string]interface{}{ - "tag1": "tag1", - "tag2": "tag2", - }), - kitot.WithTags(map[string]interface{}{ - "tag3": "tag3", - }), - kitot.WithTagsFunc(func(ctx context.Context) opentracing.Tags { - return map[string]interface{}{ - "tag4": "tag4", - } - }), - ) - tracedEndpoint = mw(endpoint.Nop) - _, _ = tracedEndpoint(context.Background(), struct{}{}) - - finishedSpans := tracer.FinishedSpans() - if want, have := 7, len(finishedSpans); want != have { - t.Fatalf("Want %v span(s), found %v", want, have) - } - - // test span 1 span := finishedSpans[0] - if want, have := span1, span.OperationName; want != have { - t.Fatalf("Want %q, have %q", want, have) - } - - // test span 2 - span = finishedSpans[1] - - if want, have := span2, span.OperationName; want != have { - t.Fatalf("Want %q, have %q", want, have) - } - - if want, have := true, span.Tag("error"); want != have { - t.Fatalf("Want %v, have %v", want, have) - } - - // test span 3 - span = finishedSpans[2] - - if want, have := span3, span.OperationName; want != have { - t.Fatalf("Want %q, have %q", want, have) - } - - if want, have := true, span.Tag("error"); want != have { - t.Fatalf("Want %v, have %v", want, have) - } - - // test span 4 - span = finishedSpans[3] - - if want, have := span4, span.OperationName; want != have { - t.Fatalf("Want %q, have %q", want, have) - } - - if want, have := (interface{})(nil), span.Tag("error"); want != have { - t.Fatalf("Want %q, have %q", want, have) - } - - // test span 5 - span = finishedSpans[4] - - if want, have := fmt.Sprintf("%s-%s", "new", span5), span.OperationName; want != have { - t.Fatalf("Want %q, have %q", want, have) - } - - // test span 6 - span = finishedSpans[5] - - if want, have := span6, span.OperationName; want != have { - t.Fatalf("Want %q, have %q", want, have) - } - - if want, have := map[string]interface{}{ - "span.kind": ext.SpanKindRPCClientEnum, - "tag1": "tag1", - "tag2": "tag2", - "tag3": "tag3", - }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { - t.Fatalf("Want %q, have %q", want, have) - } - - // test span 7 - span = finishedSpans[6] - - if want, have := span7, span.OperationName; want != have { + if want, have := "testOp", span.OperationName; want != have { t.Fatalf("Want %q, have %q", want, have) } if want, have := map[string]interface{}{ - "span.kind": ext.SpanKindRPCClientEnum, - "tag1": "tag1", - "tag2": "tag2", - "tag3": "tag3", - "tag4": "tag4", + ext.SpanKindRPCClient.Key: ext.SpanKindRPCClient.Value, }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { t.Fatalf("Want %q, have %q", want, have) } From 4af2b54f9ea0a629786fcc2bc8e4c082ff99cc49 Mon Sep 17 00:00:00 2001 From: Alexander Babai Date: Mon, 22 Mar 2021 11:35:29 +0300 Subject: [PATCH 6/9] fix imports --- tracing/opentracing/endpoint_options.go | 1 + tracing/opentracing/endpoint_test.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/tracing/opentracing/endpoint_options.go b/tracing/opentracing/endpoint_options.go index 6dea72bbb..6854271af 100644 --- a/tracing/opentracing/endpoint_options.go +++ b/tracing/opentracing/endpoint_options.go @@ -2,6 +2,7 @@ package opentracing import ( "context" + "github.com/opentracing/opentracing-go" ) diff --git a/tracing/opentracing/endpoint_test.go b/tracing/opentracing/endpoint_test.go index 72c42cb5a..4393714b4 100644 --- a/tracing/opentracing/endpoint_test.go +++ b/tracing/opentracing/endpoint_test.go @@ -4,10 +4,10 @@ import ( "context" "errors" "fmt" - "github.com/opentracing/opentracing-go/ext" "testing" "github.com/opentracing/opentracing-go" + "github.com/opentracing/opentracing-go/ext" "github.com/opentracing/opentracing-go/mocktracer" "github.com/go-kit/kit/endpoint" From b8df650710cbb09272c4d15cf12c29d7003283ed Mon Sep 17 00:00:00 2001 From: Alexander Babai Date: Mon, 22 Mar 2021 18:51:35 +0300 Subject: [PATCH 7/9] fix lint --- tracing/opentracing/endpoint.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tracing/opentracing/endpoint.go b/tracing/opentracing/endpoint.go index 1e8cb813f..96fc9d180 100644 --- a/tracing/opentracing/endpoint.go +++ b/tracing/opentracing/endpoint.go @@ -31,7 +31,7 @@ func TraceEndpoint(tracer opentracing.Tracer, operationName string, opts ...Endp } } - span := opentracing.SpanFromContext(ctx) + var span opentracing.Span if parentSpan := opentracing.SpanFromContext(ctx); parentSpan != nil { span = tracer.StartSpan( operationName, From 93f53b2ffe2edfbb2e61280efb00afd9da0dc6b8 Mon Sep 17 00:00:00 2001 From: Alexander Babai Date: Mon, 5 Apr 2021 17:47:57 +0300 Subject: [PATCH 8/9] refactor(tracing.opentracing): use ContextWithSpan after applying tags just cosmetics --- tracing/opentracing/endpoint.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tracing/opentracing/endpoint.go b/tracing/opentracing/endpoint.go index 96fc9d180..1da4b7a4f 100644 --- a/tracing/opentracing/endpoint.go +++ b/tracing/opentracing/endpoint.go @@ -41,7 +41,6 @@ func TraceEndpoint(tracer opentracing.Tracer, operationName string, opts ...Endp span = tracer.StartSpan(operationName) } defer span.Finish() - ctx = opentracing.ContextWithSpan(ctx, span) applyTags(span, cfg.Tags) if cfg.GetTags != nil { @@ -49,6 +48,8 @@ func TraceEndpoint(tracer opentracing.Tracer, operationName string, opts ...Endp applyTags(span, extraTags) } + ctx = opentracing.ContextWithSpan(ctx, span) + response, err := next(ctx, request) if err := identifyError(response, err, cfg.IgnoreBusinessError); err != nil { ext.LogError(span, err) From e5c83b1ef90e59d20b464394ce94cd2afb97a725 Mon Sep 17 00:00:00 2001 From: Alexander Babai Date: Mon, 19 Apr 2021 14:40:37 +0300 Subject: [PATCH 9/9] refactor(tracing.opentracing): standardize - support lb.Retry errors - add more logging fields as it done in opencensus - log business error anyway - improve codebase - fix typo - add more test cases --- tracing/opentracing/endpoint.go | 69 ++++++++++------ tracing/opentracing/endpoint_test.go | 119 ++++++++++++++++++++++----- 2 files changed, 146 insertions(+), 42 deletions(-) diff --git a/tracing/opentracing/endpoint.go b/tracing/opentracing/endpoint.go index 1da4b7a4f..4df1ef264 100644 --- a/tracing/opentracing/endpoint.go +++ b/tracing/opentracing/endpoint.go @@ -2,11 +2,14 @@ package opentracing import ( "context" + "strconv" "github.com/opentracing/opentracing-go" - "github.com/opentracing/opentracing-go/ext" + otext "github.com/opentracing/opentracing-go/ext" + otlog "github.com/opentracing/opentracing-go/log" "github.com/go-kit/kit/endpoint" + "github.com/go-kit/kit/sd/lb" ) // TraceEndpoint returns a Middleware that wraps the `next` Endpoint in an @@ -24,7 +27,7 @@ func TraceEndpoint(tracer opentracing.Tracer, operationName string, opts ...Endp } return func(next endpoint.Endpoint) endpoint.Endpoint { - return func(ctx context.Context, request interface{}) (interface{}, error) { + return func(ctx context.Context, request interface{}) (response interface{}, err error) { if cfg.GetOperationName != nil { if newOperationName := cfg.GetOperationName(ctx, operationName); newOperationName != "" { operationName = newOperationName @@ -50,12 +53,46 @@ func TraceEndpoint(tracer opentracing.Tracer, operationName string, opts ...Endp ctx = opentracing.ContextWithSpan(ctx, span) - response, err := next(ctx, request) - if err := identifyError(response, err, cfg.IgnoreBusinessError); err != nil { - ext.LogError(span, err) - } + defer func() { + if err != nil { + if lbErr, ok := err.(lb.RetryError); ok { + // handle errors originating from lb.Retry + fields := make([]otlog.Field, 0, len(lbErr.RawErrors)) + for idx, rawErr := range lbErr.RawErrors { + fields = append(fields, otlog.String( + "gokit.retry.error."+strconv.Itoa(idx+1), rawErr.Error(), + )) + } + + otext.LogError(span, lbErr, fields...) + + return + } + + // generic error + otext.LogError(span, err) + + return + } + + // test for business error + if res, ok := response.(endpoint.Failer); ok && res.Failed() != nil { + span.LogFields( + otlog.String("gokit.business.error", res.Failed().Error()), + ) + + if cfg.IgnoreBusinessError { + return + } + + // treating business error as real error in span. + otext.LogError(span, res.Failed()) - return response, err + return + } + }() + + return next(ctx, request) } } } @@ -64,7 +101,7 @@ func TraceEndpoint(tracer opentracing.Tracer, operationName string, opts ...Endp // OpenTracing Span called `operationName` with server span.kind tag.. func TraceServer(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { opts = append(opts, WithTags(map[string]interface{}{ - ext.SpanKindRPCServer.Key: ext.SpanKindRPCServer.Value, + otext.SpanKindRPCServer.Key: otext.SpanKindRPCServer.Value, })) return TraceEndpoint(tracer, operationName, opts...) @@ -74,7 +111,7 @@ func TraceServer(tracer opentracing.Tracer, operationName string, opts ...Endpoi // OpenTracing Span called `operationName` with client span.kind tag. func TraceClient(tracer opentracing.Tracer, operationName string, opts ...EndpointOption) endpoint.Middleware { opts = append(opts, WithTags(map[string]interface{}{ - ext.SpanKindRPCServer.Key: ext.SpanKindRPCClient.Value, + otext.SpanKindRPCClient.Key: otext.SpanKindRPCClient.Value, })) return TraceEndpoint(tracer, operationName, opts...) @@ -85,17 +122,3 @@ func applyTags(span opentracing.Span, tags opentracing.Tags) { span.SetTag(key, value) } } - -func identifyError(response interface{}, err error, ignoreBusinessError bool) error { - if err != nil { - return err - } - - if !ignoreBusinessError { - if res, ok := response.(endpoint.Failer); ok { - return res.Failed() - } - } - - return nil -} diff --git a/tracing/opentracing/endpoint_test.go b/tracing/opentracing/endpoint_test.go index 4393714b4..267415b1f 100644 --- a/tracing/opentracing/endpoint_test.go +++ b/tracing/opentracing/endpoint_test.go @@ -4,13 +4,18 @@ import ( "context" "errors" "fmt" + "reflect" "testing" + "time" "github.com/opentracing/opentracing-go" - "github.com/opentracing/opentracing-go/ext" + otext "github.com/opentracing/opentracing-go/ext" + otlog "github.com/opentracing/opentracing-go/log" "github.com/opentracing/opentracing-go/mocktracer" "github.com/go-kit/kit/endpoint" + "github.com/go-kit/kit/sd" + "github.com/go-kit/kit/sd/lb" kitot "github.com/go-kit/kit/tracing/opentracing" ) @@ -22,6 +27,7 @@ const ( span5 = "SPAN-5" span6 = "SPAN-6" span7 = "SPAN-7" + span8 = "SPAN-8" ) var ( @@ -115,10 +121,31 @@ func TestTraceEndpointWithOptions(t *testing.T) { }) _, _ = tracedEndpoint(context.Background(), struct{}{}) - // span 3 with disabled IgnoreBusinessError option + // span 3 with lb error mw = kitot.TraceEndpoint( tracer, span3, + kitot.WithOptions(kitot.EndpointOptions{}), + ) + tracedEndpoint = mw( + lb.Retry( + 5, + 1*time.Second, + lb.NewRoundRobin( + sd.FixedEndpointer{ + func(context.Context, interface{}) (interface{}, error) { + return nil, err1 + }, + }, + ), + ), + ) + _, _ = tracedEndpoint(context.Background(), struct{}{}) + + // span 4 with disabled IgnoreBusinessError option + mw = kitot.TraceEndpoint( + tracer, + span4, kitot.WithIgnoreBusinessError(false), ) tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { @@ -128,8 +155,8 @@ func TestTraceEndpointWithOptions(t *testing.T) { }) _, _ = tracedEndpoint(context.Background(), struct{}{}) - // span 4 with enabled IgnoreBusinessError option - mw = kitot.TraceEndpoint(tracer, span4, kitot.WithIgnoreBusinessError(true)) + // span 5 with enabled IgnoreBusinessError option + mw = kitot.TraceEndpoint(tracer, span5, kitot.WithIgnoreBusinessError(true)) tracedEndpoint = mw(func(context.Context, interface{}) (interface{}, error) { return failedResponse{ err: err3, @@ -137,10 +164,10 @@ func TestTraceEndpointWithOptions(t *testing.T) { }) _, _ = tracedEndpoint(context.Background(), struct{}{}) - // span 5 with OperationNameFunc option + // span 6 with OperationNameFunc option mw = kitot.TraceEndpoint( tracer, - span5, + span6, kitot.WithOperationNameFunc(func(ctx context.Context, name string) string { return fmt.Sprintf("%s-%s", "new", name) }), @@ -148,10 +175,10 @@ func TestTraceEndpointWithOptions(t *testing.T) { tracedEndpoint = mw(endpoint.Nop) _, _ = tracedEndpoint(context.Background(), struct{}{}) - // span 6 with Tags options + // span 7 with Tags options mw = kitot.TraceEndpoint( tracer, - span6, + span7, kitot.WithTags(map[string]interface{}{ "tag1": "tag1", "tag2": "tag2", @@ -163,10 +190,10 @@ func TestTraceEndpointWithOptions(t *testing.T) { tracedEndpoint = mw(endpoint.Nop) _, _ = tracedEndpoint(context.Background(), struct{}{}) - // span 7 with TagsFunc options + // span 8 with TagsFunc options mw = kitot.TraceEndpoint( tracer, - span7, + span8, kitot.WithTags(map[string]interface{}{ "tag1": "tag1", "tag2": "tag2", @@ -184,7 +211,7 @@ func TestTraceEndpointWithOptions(t *testing.T) { _, _ = tracedEndpoint(context.Background(), struct{}{}) finishedSpans := tracer.FinishedSpans() - if want, have := 7, len(finishedSpans); want != have { + if want, have := 8, len(finishedSpans); want != have { t.Fatalf("Want %v span(s), found %v", want, have) } @@ -217,6 +244,22 @@ func TestTraceEndpointWithOptions(t *testing.T) { t.Fatalf("Want %v, have %v", want, have) } + if want, have := 1, len(span.Logs()); want != have { + t.Fatalf("incorrect logs count, wanted %d, got %d", want, have) + } + + if want, have := []otlog.Field{ + otlog.String("event", "error"), + otlog.String("error.object", "some error (previously: some error; some error; some error; some error)"), + otlog.String("gokit.retry.error.1", "some error"), + otlog.String("gokit.retry.error.2", "some error"), + otlog.String("gokit.retry.error.3", "some error"), + otlog.String("gokit.retry.error.4", "some error"), + otlog.String("gokit.retry.error.5", "some error"), + }, span.Logs()[0].Fields; reflect.DeepEqual(want, have) { + t.Fatalf("Want %q, have %q", want, have) + } + // test span 4 span = finishedSpans[3] @@ -224,21 +267,59 @@ func TestTraceEndpointWithOptions(t *testing.T) { t.Fatalf("Want %q, have %q", want, have) } - if want, have := (interface{})(nil), span.Tag("error"); want != have { + if want, have := true, span.Tag("error"); want != have { + t.Fatalf("Want %v, have %v", want, have) + } + + if want, have := 2, len(span.Logs()); want != have { + t.Fatalf("incorrect logs count, wanted %d, got %d", want, have) + } + + if want, have := []otlog.Field{ + otlog.String("gokit.business.error", "some business error"), + }, span.Logs()[0].Fields; reflect.DeepEqual(want, have) { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := []otlog.Field{ + otlog.String("event", "error"), + otlog.String("error.object", "some business error"), + }, span.Logs()[1].Fields; reflect.DeepEqual(want, have) { t.Fatalf("Want %q, have %q", want, have) } // test span 5 span = finishedSpans[4] - if want, have := fmt.Sprintf("%s-%s", "new", span5), span.OperationName; want != have { + if want, have := span5, span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := (interface{})(nil), span.Tag("error"); want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + if want, have := 1, len(span.Logs()); want != have { + t.Fatalf("incorrect logs count, wanted %d, got %d", want, have) + } + + if want, have := []otlog.Field{ + otlog.String("gokit.business.error", "some business error"), + }, span.Logs()[0].Fields; reflect.DeepEqual(want, have) { t.Fatalf("Want %q, have %q", want, have) } // test span 6 span = finishedSpans[5] - if want, have := span6, span.OperationName; want != have { + if want, have := fmt.Sprintf("%s-%s", "new", span6), span.OperationName; want != have { + t.Fatalf("Want %q, have %q", want, have) + } + + // test span 7 + span = finishedSpans[6] + + if want, have := span7, span.OperationName; want != have { t.Fatalf("Want %q, have %q", want, have) } @@ -250,10 +331,10 @@ func TestTraceEndpointWithOptions(t *testing.T) { t.Fatalf("Want %q, have %q", want, have) } - // test span 7 - span = finishedSpans[6] + // test span 8 + span = finishedSpans[7] - if want, have := span7, span.OperationName; want != have { + if want, have := span8, span.OperationName; want != have { t.Fatalf("Want %q, have %q", want, have) } @@ -289,7 +370,7 @@ func TestTraceServer(t *testing.T) { } if want, have := map[string]interface{}{ - ext.SpanKindRPCServer.Key: ext.SpanKindRPCServer.Value, + otext.SpanKindRPCServer.Key: otext.SpanKindRPCServer.Value, }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { t.Fatalf("Want %q, have %q", want, have) } @@ -317,7 +398,7 @@ func TestTraceClient(t *testing.T) { } if want, have := map[string]interface{}{ - ext.SpanKindRPCClient.Key: ext.SpanKindRPCClient.Value, + otext.SpanKindRPCClient.Key: otext.SpanKindRPCClient.Value, }, span.Tags(); fmt.Sprint(want) != fmt.Sprint(have) { t.Fatalf("Want %q, have %q", want, have) }