Implement gRPC server zipkin handler. #96

anuraaga · 2018-12-01T08:52:30Z

Also removes leftover build tags from previously reverting 1.8 support and implements client's remote service name option.

I ended up making a backwards incompatible change vs the previous commit (WithRPCHandler -> WithClientRPCHandler). Let me know if this needs to be preserved across commits and can revert it.

anuraaga · 2018-12-01T09:03:38Z

Going to look at the CI failures (not happening locally for some reason)

anuraaga · 2018-12-01T09:12:44Z

Ok should be ready for a look

coveralls · 2018-12-01T09:17:50Z

Coverage remained the same at 66.714% when pulling 619dc76 on anuraaga:grpc_server into 006f66d on openzipkin:master.

coveralls · 2018-12-01T09:17:50Z

Coverage remained the same at 66.714% when pulling 619dc76 on anuraaga:grpc_server into 006f66d on openzipkin:master.

coveralls · 2018-12-01T09:17:50Z

Coverage increased (+6.3%) to 73.044% when pulling edfc0b4 on anuraaga:grpc_server into 006f66d on openzipkin:master.

codefromthecrypt

one small question! otherwise LG from non-go person pov

codefromthecrypt · 2018-12-03T08:31:44Z

middleware/grpc/client.go

 	return func(c *clientHandler) {
 		c.rpcHandlers = append(c.rpcHandlers, handler)
 	}
 }

+// WithClientRemoteServiceName will set the value for the remote endpoint's service name on
+// all spans.
+func WithClientRemoteServiceName(name string) ClientOption {


if this is in client.go do we need client prefix? not sure of go idioms. In the text I would say something that this is the name of the server :P (or withServerName might work)

It's in the same package as the server so can't have the same function appear twice.

That said I'm not happy with it from an API usage / naming point either. Will have to inspect some more..

Since we don't have a WithServerRemoteServiceName (which would make much less sense) I think there is nothing wrong with naming it WithRemoteServiceName as it's return value is a ClientOption making it clear what the scope is...

Yeah I think server remote name is inconceivable, just erred on the side of consistency. But will remove it.

basvanbeek

some points to address.

@jcchavezs how do you feel about ginkgo and gomega dependencies?

middleware/grpc/client.go

basvanbeek · 2018-12-03T12:47:57Z

middleware/grpc/client.go

 	return func(c *clientHandler) {
 		c.rpcHandlers = append(c.rpcHandlers, handler)
 	}
 }

+// WithClientRemoteServiceName will set the value for the remote endpoint's service name on
+// all spans.
+func WithClientRemoteServiceName(name string) ClientOption {


Since we don't have a WithServerRemoteServiceName (which would make much less sense) I think there is nothing wrong with naming it WithRemoteServiceName as it's return value is a ClientOption making it clear what the scope is...

basvanbeek · 2018-12-03T13:01:49Z

middleware/grpc/client_test.go

-			gomega.Expect(spans[0].Tags).To(gomega.BeEmpty())
+
+			span := spans[0]
+			gomega.Expect(span.Kind).To(gomega.Equal(model.Client))


Didn't mention this earlier but after some thought I'm still not happy about all of a sudden having introduced a new testing framework while there is a perfectly reasonable standard library testing framework available.
Having a new testing framework in our repo raises the barrier to the community to contribute.

I would have appreciated this first being discussed.

I had meant the previous PR to allow for that discussion and am sorry if it wasn't clear enough from the PR description. It's a bit disheartening hearing about it now though.

"perfectly reasonable standard library testing framework" is quite subjective and more objective is that the http middleware has very poor test coverage - IMO that is due to the lack of a reasonable testing framework.

Let me know what you'd like - I personally would not be able to contribute a PR at all without adding a test framework so none of this would have happened but it might just be me.

To set one thing straight, the state of testing in the http middleware has nothing to do with the quality of the standard library testing framework and everything with the person who wrote the http middleware in the first place. For those interested who I blame I say git blame it ;)

Given that standard library itself is heavily tested and scrutinized by using no external dependencies as well as a lot of very large projects including grpc-go you just added this middleware for, it just shows that the choice for a testing framework is very subjective and usability of frameworks is largely a matter of opinion.

I know you are eager to get grpc middleware added to this lib and I'm grateful for your work on this, don't get me wrong, but ginkgo is your personal or company choice for testing. That doesn't automatically mean it is this projects' favorite. We have a larger scope than individual contributors preferences. Unfortunately due to me not having a lot of time I let this detail slip from sinking in with me and now as initial author (and still) major contributor I'm not happy with having allowed this dependency to be added after it did.

Unless the rest of @openzipkin/core is adamant on continuing the use of ginkgo I will probably revert this to standard testing in the future.

Although we don't have official contributing guidelines I believe it is common to keep consistency with the rest of the library, introducing a new testing library should include not only the necessity for it (which is important) but also an agreement among contributors. In this specific case I think we are not doing anything we could achieve with std library so I would go for keeping it simple and less dependencies. As @basvanbeek mentioned, using ginkgo more a particular preference and I don't think we need it at this point, tho everybody is encouraged to open an issue to discuss introducing a new library for testing if there is a real need (although using std has the advantage that is is widely adopted, idiomatic and requires no previous knowledge more than some experience with go).

I really love your work @anuraaga!

I guess it was revealed that I'm a Java guy writing Go and need my abstractions :)

I was just hoping to hear in the previous PR but no worries now. Should I leave the tests as is for now or convert to standard lib now?

Will be getting back to this PR tomorrow just want to confirm to get a fast start then.

Leave the tests as is for now... We will convert them in a next PR.

middleware/grpc/shared.go

middleware/grpc/server.go

anuraaga

Updated

anuraaga · 2018-12-04T06:44:47Z

middleware/grpc/client_test.go

-			gomega.Expect(spans[0].Tags).To(gomega.BeEmpty())
+
+			span := spans[0]
+			gomega.Expect(span.Kind).To(gomega.Equal(model.Client))


basvanbeek · 2019-01-02T13:38:47Z

Thanks for the contribution @anuraaga!

anuraaga added 6 commits December 1, 2018 17:24

Implement server zipkin handler.

51f4440

Remove accidentally left-over build tag.

7dc646e

Add remote endpoint.

9035e4d

Fix comment

4fd5abc

Newline

ba25bf7

Close connections after tests.

6e03469

anuraaga changed the title ~~Implement gRPC server zipkin handler.~~ [WIP] Implement gRPC server zipkin handler. Dec 1, 2018

anuraaga added 2 commits December 1, 2018 18:08

Don't reuse lis variable

619dc76

Lint

2eceabc

anuraaga changed the title ~~[WIP] Implement gRPC server zipkin handler.~~ Implement gRPC server zipkin handler. Dec 1, 2018

codefromthecrypt reviewed Dec 3, 2018

View reviewed changes

basvanbeek requested changes Dec 3, 2018

View reviewed changes

jcchavezs reviewed Dec 3, 2018

View reviewed changes

middleware/grpc/server.go Show resolved Hide resolved

Address comments

d7d9b02

anuraaga commented Dec 4, 2018

View reviewed changes

Update comments

edfc0b4

basvanbeek merged commit b7a66d0 into openzipkin:master Jan 2, 2019

anuraaga mentioned this pull request Jan 3, 2019

Release 0.1.5? #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement gRPC server zipkin handler. #96

Implement gRPC server zipkin handler. #96

anuraaga commented Dec 1, 2018

anuraaga commented Dec 1, 2018

anuraaga commented Dec 1, 2018

coveralls commented Dec 1, 2018

coveralls commented Dec 1, 2018

coveralls commented Dec 1, 2018 •

edited

Loading

codefromthecrypt left a comment

codefromthecrypt Dec 3, 2018

basvanbeek Dec 3, 2018

basvanbeek Dec 3, 2018

anuraaga Dec 3, 2018

basvanbeek left a comment

basvanbeek Dec 3, 2018

basvanbeek Dec 3, 2018

anuraaga Dec 3, 2018

basvanbeek Dec 3, 2018

jcchavezs Dec 3, 2018

anuraaga Dec 3, 2018

basvanbeek Dec 3, 2018

anuraaga Dec 4, 2018

anuraaga left a comment

anuraaga Dec 4, 2018

basvanbeek commented Jan 2, 2019

Implement gRPC server zipkin handler. #96

Implement gRPC server zipkin handler. #96

Conversation

anuraaga commented Dec 1, 2018

anuraaga commented Dec 1, 2018

anuraaga commented Dec 1, 2018

coveralls commented Dec 1, 2018

coveralls commented Dec 1, 2018

coveralls commented Dec 1, 2018 • edited Loading

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

basvanbeek 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

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basvanbeek commented Jan 2, 2019

coveralls commented Dec 1, 2018 •

edited

Loading