Zipkin native instrumentation using zipkin-go #2

basvanbeek · 2018-01-09T20:14:29Z

First stab at new middleware for Zipkin.

Needs unit tests, improved documentation and improved code comments

codefromthecrypt

Looks good, nits only.

One overall comment could be that somewhere in the wiring, mention that remote context injection/extracting seems still an ot function, even if using a zipkin tracer.. unless I somehow missed where that's not the case

codefromthecrypt · 2018-01-09T22:52:48Z

examples/addsvc/cmd/addcli/addcli.go

-		zipkinURL      = fs.String("zipkin-url", "", "Enable Zipkin tracing via a collector URL e.g. http://localhost:9411/api/v1/spans")
-		lightstepToken = flag.String("lightstep-token", "", "Enable LightStep tracing via a LightStep access token")
-		appdashAddr    = flag.String("appdash-addr", "", "Enable Appdash tracing via an Appdash server host:port")
+		zipkinV2URL    = fs.String("zipkin-url", "", "Enable Zipkin v2 tracing (zipkin-go) via HTTP Reporter URL e.g. http://localhost:94111/api/v2/spans")


thx for making this explicit, as people have unfortunately sent v2 to the v1 endpoint in the past!

codefromthecrypt · 2018-01-09T22:53:41Z

examples/addsvc/cmd/addcli/addcli.go

+		noopTracer := (*zipkinV2URL == "")
+		reporter := zipkinhttp.NewReporter(*zipkinV2URL)
+		defer reporter.Close()
+		zEP, _ := zipkin.NewEndpoint(serviceName, hostPort)


pronounced "zee endpoint"

codefromthecrypt · 2018-01-09T22:57:31Z

tracing/zipkin/endpoint.go

+		return func(ctx context.Context, request interface{}) (interface{}, error) {
+			var sp zipkin.Span
+			// try to retrieve Span from Go context, create new Span if not found.
+			if sp = zipkin.SpanFromContext(ctx); sp == nil {


so this creates a span based on the context, if a server span I'd usually expect a check for some incoming remote context.. does that happen prior?

codefromthecrypt · 2018-01-09T23:12:19Z

tracing/zipkin/grpc.go

+	return func(c *kitgrpc.Client) {
+		clientBefore(c)
+		clientAfter(c)
+		clientFinalizer(c)


Easy to understand

codefromthecrypt · 2018-01-09T23:12:43Z

tracing/zipkin/grpc.go

+		tags:      make(map[string]string),
+		name:      "",
+		logger:    log.NewNopLogger(),
+		propagate: true,


is there a way to doclink this to options.AllowPropagation?

codefromthecrypt · 2018-01-09T23:13:35Z

tracing/zipkin/http.go

+				// here.
+				span.Finish()
+				// send span to the Reporter
+				span.Flush()


flush on finalize makes a lot of sense. notes here help.

jcchavezs · 2018-01-10T06:52:32Z

examples/addsvc/cmd/addcli/addcli.go

+			hostPort    = "" // if host:port is unknown we can keep this empty
+			serviceName = "addsvc-cli"
+		)
+		noopTracer := (*zipkinV2URL == "")


Just for readability purposes I'd call it useNoopTracer. It took me a couple of seconds to realize what was the intention in line 104.

agreed... will update

jcchavezs · 2018-01-10T06:53:37Z

examples/addsvc/cmd/addcli/addcli.go

+			reporter, zipkin.WithLocalEndpoint(zEP), zipkin.WithNoopTracer(noopTracer),
+		)
+		if err != nil {
+			fmt.Fprintln(os.Stderr, err.Error())


I think we could add more context to this error. Something like

fmt.Fprintf(os.Stderr, "error when creating the zipkin tracer: %s" err.Error()).

jcchavezs · 2018-01-10T07:01:11Z

transport/http/server.go

@@ -17,7 +17,7 @@ type Server struct {
 	before       []RequestFunc
 	after        []ServerResponseFunc
 	errorEncoder ErrorEncoder
-	finalizer    ServerFinalizerFunc
+	finalizer    []ServerFinalizerFunc


Is line 87 still correct or we should check the len of the slice (formerly it was a pointer)?

it is correct as it is not initialized when not appended to, but since len is more explicit and handles both nil and []{}I will change

jcchavezs · 2018-01-10T07:02:20Z

transport/http/server.go

@@ -76,8 +76,8 @@ func ServerErrorLogger(logger log.Logger) ServerOption {

 // ServerFinalizer is executed at the end of every HTTP request.
 // By default, no finalizer is registered.
-func ServerFinalizer(f ServerFinalizerFunc) ServerOption {
-	return func(s *Server) { s.finalizer = f }
+func ServerFinalizer(f ...ServerFinalizerFunc) ServerOption {


Since line 93 receives two pointers (that could be modified inside the finalizer), does it make sense to specify that the order matters?

I only see one being *http.Request and that one in the context of a HTTP Server handler typically isn't changed by Go devs.

jcchavezs · 2018-01-10T07:16:57Z

tracing/zipkin/README.md

  serviceName        = "MyService"
  serviceHostPort    = "localhost:8000"
-  zipkinHTTPEndpoint = "localhost:9411"
+  zipkinHTTPEndpoint = "http://localhost:9411/api/v2/spans"


codefromthecrypt · 2018-01-10T07:24:07Z

Isn't req.URI a better span name? nope because uri are open-ended in terms of cardinality, leading to an

never-ending span name list when you have paths like /things/UUID See openzipkin/zipkin#1874

basvanbeek · 2018-01-10T08:01:32Z

@adriancole there is nothing used/reused from zipkin-go-opentracing. maybe confusion is because OT is wired in differently then native Zipkin...

Example HTTPServerTrace does this:

ServerBefore: if AllowPropagation check for incoming b3 headers
ServerBefore: create new server span potentially with a parent
ServerAfter: check if span is found (it should) and finish it
ServerFinalizer: check if span is found (it should), try to finish it (because ServerAfter might not fire due to error) and flush

I also wired in TraceEndpoint Middleware which wraps a Go kit endpoint (free of transport details). The service could really do without as TraceServer and TraceClient are already detailed enough.

codefromthecrypt · 2018-01-10T08:05:42Z

great summary.. sorry missed the ServerBefore conditional check!

…

On Wed, Jan 10, 2018 at 4:01 PM, Bas van Beek ***@***.***> wrote: @adriancole <https://github.com/adriancole> there is nothing used/reused from zipkin-go-opentracing. maybe confusion is because OT is wired in differently then native Zipkin... Example HTTPServerTrace does this: - ServerBefore: if AllowPropagation check for incoming b3 headers - ServerBefore: create new server span potentially with a parent - ServerAfter: check if span is found (it should) and finish it - ServerFinalizer: check if span is found (it should), try to finish it (because ServerAfter might not fire due to error) and flush I also wired in TraceEndpoint Middleware which wraps a Go kit endpoint (free of transport details). The service could really do without as TraceServer and TraceClient are already detailed enough. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD6186Znei0p7ptS9KR2gaBr5NUq1hmks5tJG5dgaJpZM4RYYQR> .

basvanbeek added 2 commits December 24, 2017 01:10

Added new Zipkin tracing middleware using the native zipkin-go library

0dc1263

new approach to Zipkin tracing middleware for Go kit

367c60a

codefromthecrypt approved these changes Jan 9, 2018

View reviewed changes

jcchavezs reviewed Jan 10, 2018

View reviewed changes

improvements to zipkin tracer docs and code

c633c28

basvanbeek mentioned this pull request Jan 15, 2018

Support for tags and fields in opentracing middleware go-kit/kit#653

Closed

source comments changes

2593fe7

basvanbeek changed the title ~~WIP: Zipkin native instrumentation using zipkin-go~~ Zipkin native instrumentation using zipkin-go Jan 17, 2018

basvanbeek merged commit 335191f into master Jan 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zipkin native instrumentation using zipkin-go #2

Zipkin native instrumentation using zipkin-go #2

basvanbeek commented Jan 9, 2018

codefromthecrypt left a comment

codefromthecrypt Jan 9, 2018

codefromthecrypt Jan 9, 2018

basvanbeek Jan 10, 2018

codefromthecrypt Jan 9, 2018

codefromthecrypt Jan 9, 2018

codefromthecrypt Jan 9, 2018

codefromthecrypt Jan 9, 2018

jcchavezs Jan 10, 2018

basvanbeek Jan 10, 2018

jcchavezs Jan 10, 2018 •

edited

Loading

basvanbeek Jan 10, 2018

jcchavezs Jan 10, 2018 •

edited

Loading

basvanbeek Jan 10, 2018

jcchavezs Jan 10, 2018

basvanbeek Jan 10, 2018

jcchavezs Jan 10, 2018

codefromthecrypt commented Jan 10, 2018 via email

basvanbeek commented Jan 10, 2018

codefromthecrypt commented Jan 10, 2018 via email

Zipkin native instrumentation using zipkin-go #2

Zipkin native instrumentation using zipkin-go #2

Conversation

basvanbeek commented Jan 9, 2018

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Jan 10, 2018 via email

basvanbeek commented Jan 10, 2018

codefromthecrypt commented Jan 10, 2018 via email

jcchavezs Jan 10, 2018 •

edited

Loading

jcchavezs Jan 10, 2018 •

edited

Loading