Add gRPC client handler. #95

anuraaga · 2018-11-23T09:26:30Z

Mostly inspired by https://github.com/census-instrumentation/opencensus-go/blob/master/plugin/ocgrpc/trace_common.go

When comparing to the httpclient tracing, the main difference is I didn't add a ClientTags function since I couldn't see where those default tags were used. And I guess it's generally better to add default tags to the tracer? Not sure - welcome any feedback :)

anuraaga · 2018-11-23T09:26:38Z

/cc @adriancole for ramen tracking

coveralls · 2018-11-23T09:39:17Z

Coverage decreased (-2.6%) to 72.26% when pulling f146583 on anuraaga:grpc-client into 2eebe2a on openzipkin:master.

codefromthecrypt · 2018-11-23T09:41:17Z

/cc @adriancole <https://github.com/adriancole> for ramen tracking

ramen++

jcchavezs · 2018-11-23T10:13:06Z

middleware/grpc/client.go

+	name := spanName(rti)
+	span, ctx := c.tracer.StartSpanFromContext(ctx, name, zipkin.Kind(model.Client))
+	md, ok := metadata.FromOutgoingContext(ctx)
+	if !ok {


nit pick, what about doing it the other way around? if ok { } else { }

jcchavezs · 2018-11-23T10:19:51Z

middleware/grpc/client.go

+// TagRPC implements per-RPC context management.
+func (c *clientHandler) TagRPC(ctx context.Context, rti *stats.RPCTagInfo) context.Context {
+	name := spanName(rti)
+	span, ctx := c.tracer.StartSpanFromContext(ctx, name, zipkin.Kind(model.Client))


Me again being nit picker but I believe we could make this more readable by declaring the type of the variable span before so we do:

span, ctx = c.tracer.StartSpanFromContext(ctx, name, zipkin.Kind(model.Client))

and then it is clear we override ctx. What do you think?

Nit picks are welcome :)

codefromthecrypt · 2018-11-23T10:57:05Z

thanks for kick-starting this!

One overview comment I'd make is it is probably more helpful to have a name and tagging policy similar to brave (very conservative) vs otherwise as each tag and annotation eat index.

https://github.com/openzipkin/brave/blob/master/instrumentation/grpc/src/test/java/brave/grpc/ITTracingClientInterceptor.java#L209
https://github.com/openzipkin/brave/blob/master/instrumentation/grpc/src/test/java/brave/grpc/ITTracingServerInterceptor.java#L240

you might notice in brave that we have this XXParser thing which allows people to add anything else they want.. crazy enough some people tag the full bodies sometimes!

anuraaga

I was so focused on the other golang tracing libraries for inspiration I forgot to check Brave itself indeed. Made it so the tags are the same between the two (only error code tags).

Also took the liberty of adding a testing framework since it made it much easier for me to write them to get better test coverage, hope it's ok.

anuraaga · 2018-11-25T07:10:07Z

middleware/grpc/client.go

+// TagRPC implements per-RPC context management.
+func (c *clientHandler) TagRPC(ctx context.Context, rti *stats.RPCTagInfo) context.Context {
+	name := spanName(rti)
+	span, ctx := c.tracer.StartSpanFromContext(ctx, name, zipkin.Kind(model.Client))


Nit picks are welcome :)

anuraaga · 2018-11-25T09:29:41Z

Hmm - was expecting adding the build tag to allow this to work on 1.8, but I think the lack of vendor directory might be causing issues (I believe go get does not care about build tags, and it'd be stabler and faster for the builds to use a vendor directory and not call go get).

Before digging more, just wondering if removing support for 1.8 is on the table, then don't have to worry :)

basvanbeek · 2018-11-25T09:34:19Z

I’m not ready to give up on 1.8.

anuraaga · 2018-11-25T10:40:35Z

Ok no worries. Took some effort but got it to build on 1.8

Checked in vendor since latest x/net will not compile on 1.8
Renamed to legacy x/context package
Forced IPv4 since IPv6 doesn't work on 1.8 for some reason

I feel sorry for anyone that still has to use 1.8 in production 😮

basvanbeek · 2018-11-29T11:38:49Z

I'll take a look... don't like the idea of having to include a vendor dir here...

basvanbeek · 2018-11-29T12:55:56Z

I've inspected the landscape... to make the support burden on us all lower we should drop the 1.8 support for zipkin-go... If people need to stick with 1.8 our latest release will have to do.

@anuraaga thanks for the hard work and sorry for making you go through the pain of supporting 1.8. Looking at this it is just not worth it anymore. Can you revert the alterations you needed to do for 1.8?

…or copied metadata.

anuraaga · 2018-11-30T03:57:12Z

Thanks @basvanbeek - I have removed 1.8 from the build too

basvanbeek

Some minor nits about dot imports, for the rest LGTM.

basvanbeek · 2018-11-30T09:21:29Z

middleware/grpc/client_test.go

+	"google.golang.org/grpc/stats"
+
+	"github.com/openzipkin/zipkin-go"
+	. "github.com/openzipkin/zipkin-go/middleware/grpc"


Can you remove the dot imports?

I'd rather not have them in the code. Rather be verbose prefixing the functions with the package they came from so outsiders understand what method is being called and from which dependency.

basvanbeek · 2018-11-30T09:22:06Z

middleware/grpc/grpc_suite_test.go

+	"net"
+	"testing"
+
+	. "github.com/onsi/ginkgo"


No dot imports, see above.

anuraaga · 2018-11-30T09:32:30Z

Thanks - removed the dot imports

basvanbeek · 2018-11-30T09:57:47Z

Thanks @anuraaga are you planing on adding a gRPC server interceptor too?

anuraaga · 2018-11-30T09:59:28Z

Yup - hoping to knock it out tomorrow

basvanbeek · 2018-11-30T10:00:16Z

Cool... if you can update the Readme gRPC section with the upcoming PR that would be great!

Add gRPC client handler.

882b748

jcchavezs reviewed Nov 23, 2018

View reviewed changes

anuraaga added 2 commits November 25, 2018 18:01

Add extension point for adding tags, more test coverage.

d8db0ec

Newlines

6bfafab

anuraaga commented Nov 25, 2018

View reviewed changes

anuraaga added 7 commits November 25, 2018 18:07

Newline

2f86054

Assert span name

27c53a5

Fix endpoint name

08b9860

Move test service into test suite file.

e506b81

Cleanup

5a3d67e

Cleanup

8760fa0

Formatting

8773577

codefromthecrypt mentioned this pull request Nov 29, 2018

adding gRPC client instrumentation openzipkin/zipkin-js#303

Merged

basvanbeek added the do not merge label Nov 29, 2018

Anuraag Agrawal added 2 commits November 30, 2018 12:15

Simplify rs.Error check since nil is handled correctly and add test f…

990fb01

…or copied metadata.

Remove 1.8 from travis

02d7a8a

anuraaga force-pushed the grpc-client branch from 137a7bc to 02d7a8a Compare November 30, 2018 03:16

Anuraag Agrawal added 2 commits November 30, 2018 12:20

Remove 1.8 check from Makefile

7c9cb05

Don't introduce unnecessary interface and other lint fixes.

98445f7

basvanbeek requested changes Nov 30, 2018

View reviewed changes

basvanbeek added enhancement and removed do not merge labels Nov 30, 2018

Remove dot imports.

095b9fc

Fix imports

f146583

basvanbeek approved these changes Nov 30, 2018

View reviewed changes

basvanbeek merged commit 006f66d into openzipkin:master Nov 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gRPC client handler. #95

Add gRPC client handler. #95

anuraaga commented Nov 23, 2018 •

edited

Loading

anuraaga commented Nov 23, 2018

coveralls commented Nov 23, 2018 •

edited

Loading

codefromthecrypt commented Nov 23, 2018 via email

jcchavezs Nov 23, 2018

jcchavezs Nov 23, 2018

anuraaga Nov 25, 2018

codefromthecrypt commented Nov 23, 2018

anuraaga left a comment

anuraaga Nov 25, 2018

anuraaga commented Nov 25, 2018 •

edited

Loading

basvanbeek commented Nov 25, 2018

anuraaga commented Nov 25, 2018 •

edited

Loading

basvanbeek commented Nov 29, 2018

basvanbeek commented Nov 29, 2018 •

edited

Loading

anuraaga commented Nov 30, 2018

basvanbeek left a comment

basvanbeek Nov 30, 2018

basvanbeek Nov 30, 2018

anuraaga commented Nov 30, 2018

basvanbeek commented Nov 30, 2018

anuraaga commented Nov 30, 2018

basvanbeek commented Nov 30, 2018

Add gRPC client handler. #95

Add gRPC client handler. #95

Conversation

anuraaga commented Nov 23, 2018 • edited Loading

anuraaga commented Nov 23, 2018

coveralls commented Nov 23, 2018 • edited Loading

codefromthecrypt commented Nov 23, 2018 via email

jcchavezs Nov 23, 2018

Choose a reason for hiding this comment

jcchavezs Nov 23, 2018

Choose a reason for hiding this comment

anuraaga Nov 25, 2018

Choose a reason for hiding this comment

codefromthecrypt commented Nov 23, 2018

anuraaga left a comment

Choose a reason for hiding this comment

anuraaga Nov 25, 2018

Choose a reason for hiding this comment

anuraaga commented Nov 25, 2018 • edited Loading

basvanbeek commented Nov 25, 2018

anuraaga commented Nov 25, 2018 • edited Loading

basvanbeek commented Nov 29, 2018

basvanbeek commented Nov 29, 2018 • edited Loading

anuraaga commented Nov 30, 2018

basvanbeek left a comment

Choose a reason for hiding this comment

basvanbeek Nov 30, 2018

Choose a reason for hiding this comment

basvanbeek Nov 30, 2018

Choose a reason for hiding this comment

anuraaga commented Nov 30, 2018

basvanbeek commented Nov 30, 2018

anuraaga commented Nov 30, 2018

basvanbeek commented Nov 30, 2018

anuraaga commented Nov 23, 2018 •

edited

Loading

coveralls commented Nov 23, 2018 •

edited

Loading

anuraaga commented Nov 25, 2018 •

edited

Loading

anuraaga commented Nov 25, 2018 •

edited

Loading

basvanbeek commented Nov 29, 2018 •

edited

Loading