From 0bc00f3d53ed06a6ac1139845eaad1e41b2f6118 Mon Sep 17 00:00:00 2001 From: Bas van Beek Date: Wed, 6 Jul 2016 20:25:26 +0200 Subject: [PATCH 1/3] OpenTracing breaking API change: SpanContext --- tracing/opentracing/endpoint.go | 8 ++++---- tracing/opentracing/grpc.go | 14 +++++++++----- tracing/opentracing/http.go | 14 ++++++++------ 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/tracing/opentracing/endpoint.go b/tracing/opentracing/endpoint.go index 9f626a79c..ef7fa63fe 100644 --- a/tracing/opentracing/endpoint.go +++ b/tracing/opentracing/endpoint.go @@ -37,10 +37,10 @@ func TraceClient(tracer opentracing.Tracer, operationName string) endpoint.Middl return func(next endpoint.Endpoint) endpoint.Endpoint { return func(ctx context.Context, request interface{}) (interface{}, error) { parentSpan := opentracing.SpanFromContext(ctx) - clientSpan := tracer.StartSpanWithOptions(opentracing.StartSpanOptions{ - OperationName: operationName, - Parent: parentSpan, // may be nil - }) + clientSpan := tracer.StartSpan( + operationName, + opentracing.ChildOf(parentSpan.Context()), + ) defer clientSpan.Finish() otext.SpanKind.Set(clientSpan, otext.SpanKindRPCClient) ctx = opentracing.ContextWithSpan(ctx, clientSpan) diff --git a/tracing/opentracing/grpc.go b/tracing/opentracing/grpc.go index 258dff205..db3cc8ed2 100644 --- a/tracing/opentracing/grpc.go +++ b/tracing/opentracing/grpc.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/opentracing/opentracing-go" + "github.com/opentracing/opentracing-go/ext" "golang.org/x/net/context" "google.golang.org/grpc/metadata" @@ -20,7 +21,7 @@ func ToGRPCRequest(tracer opentracing.Tracer, logger log.Logger) func(ctx contex return func(ctx context.Context, md *metadata.MD) context.Context { if span := opentracing.SpanFromContext(ctx); span != nil { // There's nothing we can do with an error here. - if err := tracer.Inject(span, opentracing.TextMap, metadataReaderWriter{md}); err != nil { + if err := tracer.Inject(span.Context(), opentracing.TextMap, metadataReaderWriter{md}); err != nil { logger.Log("err", err) } } @@ -37,12 +38,15 @@ func ToGRPCRequest(tracer opentracing.Tracer, logger log.Logger) func(ctx contex // The logger is used to report errors and may be nil. func FromGRPCRequest(tracer opentracing.Tracer, operationName string, logger log.Logger) func(ctx context.Context, md *metadata.MD) context.Context { return func(ctx context.Context, md *metadata.MD) context.Context { - span, err := tracer.Join(operationName, opentracing.TextMap, metadataReaderWriter{md}) + var span opentracing.Span + wireContext, err := tracer.Extract(opentracing.TextMap, metadataReaderWriter{md}) if err != nil { + logger.Log("err", err) + } + if wireContext != nil { + span = tracer.StartSpan(operationName, ext.RPCServerOption(wireContext)) + } else { span = tracer.StartSpan(operationName) - if err != opentracing.ErrTraceNotFound { - logger.Log("err", err) - } } return opentracing.ContextWithSpan(ctx, span) } diff --git a/tracing/opentracing/http.go b/tracing/opentracing/http.go index 63c59f8c7..0050301f4 100644 --- a/tracing/opentracing/http.go +++ b/tracing/opentracing/http.go @@ -36,7 +36,7 @@ func ToHTTPRequest(tracer opentracing.Tracer, logger log.Logger) kithttp.Request // There's nothing we can do with any errors here. if err = tracer.Inject( - span, + span.Context(), opentracing.TextMap, opentracing.HTTPHeaderTextMapCarrier(req.Header), ); err != nil { @@ -57,16 +57,18 @@ func ToHTTPRequest(tracer opentracing.Tracer, logger log.Logger) kithttp.Request func FromHTTPRequest(tracer opentracing.Tracer, operationName string, logger log.Logger) kithttp.RequestFunc { return func(ctx context.Context, req *http.Request) context.Context { // Try to join to a trace propagated in `req`. - span, err := tracer.Join( - operationName, + var span opentracing.Span + wireContext, err := tracer.Extract( opentracing.TextMap, opentracing.HTTPHeaderTextMapCarrier(req.Header), ) if err != nil { + logger.Log("err", err) + } + if wireContext != nil { + span = tracer.StartSpan(operationName, ext.RPCServerOption(wireContext)) + } else { span = tracer.StartSpan(operationName) - if err != opentracing.ErrTraceNotFound { - logger.Log("err", err) - } } return opentracing.ContextWithSpan(ctx, span) } From 941a0199c96f4160f136d01ae4bcf3dfb492419e Mon Sep 17 00:00:00 2001 From: Bas van Beek Date: Wed, 6 Jul 2016 21:42:55 +0200 Subject: [PATCH 2/3] utilize newly available easier creation of RPC Server span --- tracing/opentracing/grpc.go | 6 +----- tracing/opentracing/http.go | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/tracing/opentracing/grpc.go b/tracing/opentracing/grpc.go index db3cc8ed2..e678e301c 100644 --- a/tracing/opentracing/grpc.go +++ b/tracing/opentracing/grpc.go @@ -43,11 +43,7 @@ func FromGRPCRequest(tracer opentracing.Tracer, operationName string, logger log if err != nil { logger.Log("err", err) } - if wireContext != nil { - span = tracer.StartSpan(operationName, ext.RPCServerOption(wireContext)) - } else { - span = tracer.StartSpan(operationName) - } + span = tracer.StartSpan(operationName, ext.RPCServerOption(wireContext)) return opentracing.ContextWithSpan(ctx, span) } } diff --git a/tracing/opentracing/http.go b/tracing/opentracing/http.go index 0050301f4..49fb7113f 100644 --- a/tracing/opentracing/http.go +++ b/tracing/opentracing/http.go @@ -65,11 +65,7 @@ func FromHTTPRequest(tracer opentracing.Tracer, operationName string, logger log if err != nil { logger.Log("err", err) } - if wireContext != nil { - span = tracer.StartSpan(operationName, ext.RPCServerOption(wireContext)) - } else { - span = tracer.StartSpan(operationName) - } + span = tracer.StartSpan(operationName, ext.RPCServerOption(wireContext)) return opentracing.ContextWithSpan(ctx, span) } } From 060a8cebf69feb8ddf79e2d469162ce11d018746 Mon Sep 17 00:00:00 2001 From: Bas van Beek Date: Thu, 7 Jul 2016 00:15:49 +0200 Subject: [PATCH 3/3] propagation tests fixed pending change in mocktracer --- tracing/opentracing/endpoint_test.go | 10 +++++++--- tracing/opentracing/grpc_test.go | 19 ++++++++++++------- tracing/opentracing/http_test.go | 19 ++++++++++++------- 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/tracing/opentracing/endpoint_test.go b/tracing/opentracing/endpoint_test.go index d89136c93..f7727b441 100644 --- a/tracing/opentracing/endpoint_test.go +++ b/tracing/opentracing/endpoint_test.go @@ -33,9 +33,10 @@ func TestTraceServer(t *testing.T) { 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 := contextSpan.SpanID, endpointSpan.SpanID; want != have { + if want, have := contextContext.SpanID, endpointContext.SpanID; want != have { t.Errorf("Want SpanID %q, have %q", want, have) } } @@ -85,8 +86,11 @@ func TestTraceClient(t *testing.T) { 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 := parentSpan.SpanID, endpointSpan.ParentID; want != have { + if want, have := parentContext.SpanID, endpointContext.SpanID; want != have { t.Errorf("Want ParentID %q, have %q", want, have) } } diff --git a/tracing/opentracing/grpc_test.go b/tracing/opentracing/grpc_test.go index 2db9db373..c23b63b5d 100644 --- a/tracing/opentracing/grpc_test.go +++ b/tracing/opentracing/grpc_test.go @@ -8,19 +8,21 @@ import ( "golang.org/x/net/context" "google.golang.org/grpc/metadata" + "github.com/go-kit/kit/log" kitot "github.com/go-kit/kit/tracing/opentracing" ) func TestTraceGRPCRequestRoundtrip(t *testing.T) { + logger := log.NewNopLogger() tracer := mocktracer.New() // Initialize the ctx with a Span to inject. beforeSpan := tracer.StartSpan("to_inject").(*mocktracer.MockSpan) defer beforeSpan.Finish() - beforeSpan.SetBaggageItem("baggage", "check") + beforeSpan.Context().SetBaggageItem("baggage", "check") beforeCtx := opentracing.ContextWithSpan(context.Background(), beforeSpan) - toGRPCFunc := kitot.ToGRPCRequest(tracer, nil) + toGRPCFunc := kitot.ToGRPCRequest(tracer, logger) md := metadata.Pairs() // Call the RequestFunc. afterCtx := toGRPCFunc(beforeCtx, &md) @@ -38,22 +40,25 @@ func TestTraceGRPCRequestRoundtrip(t *testing.T) { } // Use FromGRPCRequest to verify that we can join with the trace given MD. - fromGRPCFunc := kitot.FromGRPCRequest(tracer, "joined", nil) + fromGRPCFunc := kitot.FromGRPCRequest(tracer, "joined", logger) joinCtx := fromGRPCFunc(afterCtx, &md) joinedSpan := opentracing.SpanFromContext(joinCtx).(*mocktracer.MockSpan) - if joinedSpan.SpanID == beforeSpan.SpanID { - t.Error("SpanID should have changed", joinedSpan.SpanID, beforeSpan.SpanID) + joinedContext := joinedSpan.Context().(*mocktracer.MockSpanContext) + beforeContext := beforeSpan.Context().(*mocktracer.MockSpanContext) + + if joinedContext.SpanID == beforeContext.SpanID { + t.Error("SpanID should have changed", joinedContext.SpanID, beforeContext.SpanID) } // Check that the parent/child relationship is as expected for the joined span. - if want, have := beforeSpan.SpanID, joinedSpan.ParentID; want != have { + if want, have := beforeContext.SpanID, joinedSpan.ParentID; want != have { t.Errorf("Want ParentID %q, have %q", want, have) } if want, have := "joined", joinedSpan.OperationName; want != have { t.Errorf("Want %q, have %q", want, have) } - if want, have := "check", joinedSpan.BaggageItem("baggage"); want != have { + if want, have := "check", joinedSpan.Context().BaggageItem("baggage"); want != have { t.Errorf("Want %q, have %q", want, have) } } diff --git a/tracing/opentracing/http_test.go b/tracing/opentracing/http_test.go index a6f22bf78..5035db828 100644 --- a/tracing/opentracing/http_test.go +++ b/tracing/opentracing/http_test.go @@ -8,19 +8,21 @@ import ( "github.com/opentracing/opentracing-go/mocktracer" "golang.org/x/net/context" + "github.com/go-kit/kit/log" kitot "github.com/go-kit/kit/tracing/opentracing" ) func TestTraceHTTPRequestRoundtrip(t *testing.T) { + logger := log.NewNopLogger() tracer := mocktracer.New() // Initialize the ctx with a Span to inject. beforeSpan := tracer.StartSpan("to_inject").(*mocktracer.MockSpan) defer beforeSpan.Finish() - beforeSpan.SetBaggageItem("baggage", "check") + beforeSpan.Context().SetBaggageItem("baggage", "check") beforeCtx := opentracing.ContextWithSpan(context.Background(), beforeSpan) - toHTTPFunc := kitot.ToHTTPRequest(tracer, nil) + toHTTPFunc := kitot.ToHTTPRequest(tracer, logger) req, _ := http.NewRequest("GET", "http://test.biz/url", nil) // Call the RequestFunc. afterCtx := toHTTPFunc(beforeCtx, req) @@ -38,22 +40,25 @@ func TestTraceHTTPRequestRoundtrip(t *testing.T) { } // Use FromHTTPRequest to verify that we can join with the trace given a req. - fromHTTPFunc := kitot.FromHTTPRequest(tracer, "joined", nil) + fromHTTPFunc := kitot.FromHTTPRequest(tracer, "joined", logger) joinCtx := fromHTTPFunc(afterCtx, req) joinedSpan := opentracing.SpanFromContext(joinCtx).(*mocktracer.MockSpan) - if joinedSpan.SpanID == beforeSpan.SpanID { - t.Error("SpanID should have changed", joinedSpan.SpanID, beforeSpan.SpanID) + joinedContext := joinedSpan.Context().(*mocktracer.MockSpanContext) + beforeContext := beforeSpan.Context().(*mocktracer.MockSpanContext) + + if joinedContext.SpanID == beforeContext.SpanID { + t.Error("SpanID should have changed", joinedContext.SpanID, beforeContext.SpanID) } // Check that the parent/child relationship is as expected for the joined span. - if want, have := beforeSpan.SpanID, joinedSpan.ParentID; want != have { + if want, have := beforeContext.SpanID, joinedSpan.ParentID; want != have { t.Errorf("Want ParentID %q, have %q", want, have) } if want, have := "joined", joinedSpan.OperationName; want != have { t.Errorf("Want %q, have %q", want, have) } - if want, have := "check", joinedSpan.BaggageItem("baggage"); want != have { + if want, have := "check", joinedSpan.Context().BaggageItem("baggage"); want != have { t.Errorf("Want %q, have %q", want, have) } }