First attempt to add GRPC instrumentation

[tkvangorder]

No description provided.

[pivotal-issuemaster]

@tkvangorder Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@tkvangorder Thank you for signing the Contributor License Agreement!

[tkvangorder]

@marcingrzejszczak Here is the first stab at GRPC support. Things that make this strange:

• Guava 20.0 is used in the latest GRPC code and I had to force sleuth to use that version. I am sure you have run into this before, I am not a fan of having to do this, any ideas?
• The auto-configuration relies on functionality provided by grpc-spring-boot-starter. This is an external, third-party library.
• There is a sample included, it needs to generate stubs/skeletons as part of the maven build.

[codefromthecrypt]

Most of this code is sample related and I'd suggest making a branch similar to dubbo example here instead. that leaves the code in this project concise to the point of sleuth, not the full operations of more demos.

https://github.com/openzipkin/sleuth-webmvc-example

[codefromthecrypt]

I don't think this can be a mandatory dependency.

[tkvangorder]

yeah, after thinking about this, I can make this a test scope dependency, as it is during the tests that the failures manifest. Still not a great solution, but I am not sure what else would make more sense?

[codefromthecrypt]

Typo

Suggested change

	1. For conveinence, there is a docker-compose file in the project that can be used to launch zipkin locally. Otherwise, you will need to run zipkin separately.
	1. For convenience, there is a docker-compose file in the project that can be used to launch zipkin locally. Otherwise, you will need to run zipkin separately.

[codefromthecrypt]

mysql is unnecessary for this.. just use default in-mem.. probably docker-compose itself is overkill

See also https://github.com/openzipkin/sleuth-webmvc-example

[tkvangorder]

I will clean that up as well, thanks.

[tkvangorder]

@adriancole Thanks for the quick turn-around, I will move the sample out of the core project. Calling it a night, I will get this done tomorrow.

[codefromthecrypt]

yeah, after thinking about this, I can make this a test scope dependency, as it is during the tests that the failures manifest. Still not a great solution, but I am not sure what else would make more sense?

I agree it is a problem with grpc as it strictly depends on guava. You can try dependencyManagement section to fix the version, but in practice sleuth or boot bom will set the dependency versions. optional or provided scope I think can also help make the build work but not interfere with versions used by apps.

[codecov-io]

Codecov Report

Merging #1139 into master will increase coverage by 0.34%.
The diff coverage is 89.28%.

@@             Coverage Diff              @@
##             master    #1139      +/-   ##
============================================
+ Coverage     69.34%   69.69%   +0.34%     
- Complexity      709      723      +14     
============================================
  Files           129      132       +3     
  Lines          3321     3349      +28     
  Branches        360      363       +3     
============================================
+ Hits           2303     2334      +31     
+ Misses          816      809       -7     
- Partials        202      206       +4

Impacted Files	Coverage Δ	Complexity Δ
...th/instrument/grpc/TraceGrpcAutoConfiguration.java	100% <100%> (ø)	5 <5> (?)
...t/grpc/TracingManagedChannelBuilderCustomizer.java	100% <100%> (ø)	2 <2> (?)
...trument/grpc/SpringAwareManagedChannelBuilder.java	83.33% <83.33%> (ø)	7 <7> (?)
...ing/TracingConnectionFactoryBeanPostProcessor.java	73.21% <0%> (+5.35%)	6% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a618e6...fb91f8f. Read the comment docs.

[tkvangorder]

• Sample Project has been pulled out of this repo and is now here: https://github.com/tkvangorder/sleuth-grpc-example. @adriancole Once/if this PR is merged, I can work with you on migrating it to openzipkin.
• I moved the "forcing of the guava version" to test scope.
• I cleaned up the two failing tests.
• I still need to add some testing.

[tkvangorder]

@adriancole @marcingrzejszczak I did a little refactoring and added some documentation around the implementation details. How do you both feel about the dependency on grpc-spring-boot-starter? This can work for now but it feels like it would be better if there were a spring-sanctioned gRPC project: spring-grpc or spring-cloud-grpc?

[devinsba]

A gRPC sanctioned Spring-Boot starter would also be awesome: https://github.com/grpc-ecosystem could be a good home. I know there are other starters hanging around so it would be good to have a community maintained one that gRPC is willing to put their name on

[spencergibb]

I might think https://github.com/spring-cloud/spring-cloud-gcp

[tkvangorder]

I think after Google open sourced gRPC they handed things over to the community, so I am not sure if it makes sense to put it under the GCP umbrella? I am a relative newbie to gRPC, so take my opinion with a grain of salt.

Regardless of home, having an agreed upon home for Spring + gRPC would be great. In addition to https://github.com/LogNet/grpc-spring-boot-starter, there is also an interesting reactive effort https://github.com/salesforce/reactive-grpc

[spencergibb]

From what I recall, the boot team prefers the starter to be external until there is some Framework integration (which there isn't, but there is a jira issue).

[marcingrzejszczak]

I'm merging what we have here ATM and we can think of changing the starter to sth else if necessary.

[yishibai02]

Another repo has done this.
https://github.com/yidongnan/grpc-spring-boot-starter
This repo seems to provide a lot of features.

[tkvangorder]

@yishibai02 This PR relies on grpc-spring-boot-starter being on the classpath and will add tracing interceptors to the servers (by introducing a brave's server tracing interceptor) and to clients (by introducing a spring-aware ManagedChannelBuilder and brave's client tracing interceptor)

[yishibai02]

I see this repo also supports Spring Sleuth to trace application.
GrpcServerAutoConfiguration.java#L147-L159
GrpcClientAutoConfiguration.java#L158-L170

I think these commit be better placed in the third-party starter repo.

[tkvangorder]

@yishibai02. Thanks for the clarification!

The repo's have the same name but are very different in the implementation details.

For now, this PR introduces auto-configuration only when the LogNet/grpc-spring-boot-starter is on the classpath.

There is still work to be done to introduce an "official" project for using grpc with spring/spring boot. Perhaps yidongnan/grpc-spring-boot-starter might be a good place to start as a contribution towards that goal?