Support gRPC #403

Jared-Prime · 2018-04-13T19:16:01Z

This PR provides an implementation for distributed tracing for Ruby gRPC services and clients.

Screenshot from Integration Using Demo Application: https://github.com/Jared-Prime/grpc-demo

In gRPC nomenclature, an "interceptor" is similar to middleware. When running a service or a client, the #patch method prepends the datadog interceptor onto the default middleware call chain, ensuring traces span across the scope of the protocol implementation.

The following types of gRPC calls have been implemented and tested:

request/reply
streaming requests
streaming replies
bidirectional streaming

A demo application is available at https://github.com/Jared-Prime/grpc-demo . A version of this patch is undergoing internal testing at my workplace ( Kenna Security ) as well.

This Pull Request closes #379 if accepted and merged; it closes #378 ( a previous attempt at this feature ) as well.

Review, suggestions, and refinements are welcome :)

adomokos · 2018-04-17T16:46:22Z

Hello,

we would need this feature as well. Can someone from the maintainers review it?

Much appreciated!

delner · 2018-04-17T20:35:56Z

Hey @adomokos, we're as excited about this pull request as you are, and are actively working with @Jared-Prime on this. Rest assured its coming!

adomokos · 2018-04-17T20:45:30Z

💥 Thanks @delner!

delner

Looking great so far, some misc feedback.

delner · 2018-04-20T14:54:53Z

lib/ddtrace/contrib/grpc/datadog_interceptor/client.rb

+        # inject trace context information into gRPC metadata prior to
+        # sending the request to the server.
+        class Client < Base
+          def trace(keywords)


I think we might want to catch and handle errors here. In the (unlikely) event the tracing code raises an error, the actual gRPC call would not happen. Generally speaking, the goal of trace code like this is to be minimally impactful, so it might be prudent to do some kind of begin rescue ensure else setup here.

Same applies to the Server code.

I'm uncertain how to proceed with this error handling. Maybe you had something specific in mind?

I think doing:

begin keywords[:metadata].each do |header, value| span.set_tag(header, value) end Datadog::GRPCPropagator .inject!(span.context, keywords[:metadata]) rescue StandardError => e Datadog::Tracer.log.debug("GRPC client trace failed: #{e}") end yield

Or something to that effect. It isn't bulletproof exactly without really chopping up the code, but might help. You could also consider extracting that code into another method named something like annotate! then add the error handling in there, if that seems cleaner.

oh, I get it now. I like the annotate! approach and will try that

delner · 2018-04-20T15:03:16Z

lib/ddtrace/contrib/grpc/interception_context.rb

+    # The `#intercept!` method is implemented in gRPC; this module
+    # will be prepended to the original class, effectively injecting
+    # our tracing middleware into the head of the call chain.
+    module InterceptWithDatadog


Does this constant need to be defined in GRPC::InterceptionContext? (As opposed to the Datadog::Contrib::GRPC one?) We generally try to avoid opening/modifying classes or injecting new constants into integration namespaces, if possible.

It does and is defined here. If you'd like, we can opt for the direct insertion of interceptors as in this example.

I think I understand: for the default use :grpc scenario, we're not sure what RPC service to inject into, so we just prepend our module into the InterceptionContext yeah? If so, I think I should clarify what I meant; I think we can move GRPC::InterceptionContext::InterceptWithDatadog to Datadog::Contrib::GRPC::InterceptWithDatadog or something similar. The point of which is mostly to just avoid defining constants in the GRPC namespace, even if we do have to inject modules into constants defined there.

Different question here: what happens if a user defines something like:

# Activate default GRPC tracing Datadog.configure do |c| c.use :grpc end # Override configuration for one particular client alternate_client = Demo::Echo::Service.rpc_stub_class.new( 'localhost:50051', :this_channel_is_insecure, :interceptors => [configured_interceptor] )

Would this cause this alternate_client to have two Datadog interceptors in it?

That's a good question. The login in already_prepended? should prevent that from happening. The intercept! call happens during the RPC interaction, so it should only add the default datadog interceptor to the call chain if the trace has not begun and if no other datadog interceptor has been supplied.

delner · 2018-04-20T15:06:46Z

lib/ddtrace/contrib/grpc/interception_context.rb

+      end
+
+      def choose_datadog_interceptor(args)
+        if args.key?(:metadata)


Can you explain the reasoning here for how an interceptor is chosen?

In the gRPC Ruby core library, Client and Server interceptors have the same method names, but different keyword arguments. Clients always have a metadata keyword whereas Servers do not; servers always have a call keyword. A metadata is very similar to a header object in many HTTP libraries; it may be just a Hash in some cases. A call is an internal object ( a GRPC::ActiveCall ) that provides access to the ongoing communication between client and server; it provides access to the metadata sent by the client.

I see, is there any more concrete/reliable way of differentiating between client and server calls? It'd be worthwhile if possible, otherwise we can just go with what will work.

I'm not finding anything more concrete. I'll keep looking, but the internals of the grpc gem aren't revealing anything more direct.

delner · 2018-04-20T15:09:03Z

lib/ddtrace/contrib/grpc/patcher.rb

+
+        def prepend_interceptor
+          ::GRPC::InterceptionContext
+            .prepend(::GRPC::InterceptionContext::InterceptWithDatadog)


What versions of Ruby are we supporting with GRPC? A limited set is okay, but I can see this won't work with 1.9.3, and might not work with 2.0 (I think you might have to do send(:prepend)?)

Due to it's use of keyword arguments, only 2.x is supported.

I think if you're uncomfortable with modifying the existing object then we could simply require the more involved configuration. Otherwise, I'll look into and test some earlier 2.x rubies.

Requiring the more involved configuration seems "cleaner" and simpler, however, I think for now, prepending into the GRPC module is okay, for the sake of having some out of the box, easy GRPC tracing.

Let's just verify its working with 2.0+ for now.

I looks like the grpc gem itself has a dependency on jwt, which requires a Ruby >= 2.1. Since 2.1 is past EOL, how about we just require >= 2.2? A quick test on my local machine works out OK.

That seems fine with me; seems like a feature that should require sufficiently new Ruby anyways. We'll just have to be sure to add that to the documentation.

delner · 2018-04-20T15:11:42Z

lib/ddtrace/propagation/grpc_propagator.rb

+      metadata[GRPC_METADATA_TRACE_ID] = context.trace_id.to_s
+      metadata[GRPC_METADATA_PARENT_ID] = context.span_id.to_s
+      metadata[GRPC_METADATA_SAMPLING_PRIORITY] = context.sampling_priority.to_s
+      metadata.delete(GRPC_METADATA_SAMPLING_PRIORITY) unless context.sampling_priority


Minor but can we just do:

metadata[GRPC_METADATA_SAMPLING_PRIORITY] = context.sampling_priority.to_s if context.sampling_priority

delner · 2018-04-20T15:16:33Z

lib/ddtrace/propagation/grpc_propagator.rb

+      end
+
+      def sampling_priority
+        value = @metadata.fetch(GRPC_METADATA_SAMPLING_PRIORITY, -1).to_i


I think Datadog::Ext::Priority::USER_REJECT has a value of -1 which is legitimate. Don't we want to propagate that?

Something like?

value = @metadata.fetch(GRPC_METADATA_SAMPLING_PRIORITY, nil) value.nil? ? nil : value.to_i # Or you could do: value && value.to_i

delner · 2018-04-20T15:19:19Z

spec/ddtrace/contrib/grpc/datadog_interceptor/client_spec.rb

+
+    let(:configured_client_interceptor) do
+      Datadog::Contrib::GRPC::DatadogInterceptor::Client.new do |c|
+        c.service_name = 'cepsr'


Haha, like the name!

delner · 2018-04-20T15:20:40Z

spec/ddtrace/contrib/grpc/datadog_interceptor/client_spec.rb

+require 'ddtrace'
+
+RSpec.describe 'tracing on the client connection' do
+  subject { Datadog::Contrib::GRPC::DatadogInterceptor::Client.new }


I suggest giving subject blocks names. Something like subject(:client) would be good here.

delner · 2018-04-20T15:39:31Z

spec/ddtrace/contrib/grpc/support/grpc_helper.rb

@@ -0,0 +1,88 @@
+require 'grpc'
+
+module GRPCHelper


Not a huge deal, but food for thought: consider turning this into an RSpec shared context that you can compose into the other example files.

Then you can stub the constants to make them more composable. A reason for doing so is that it allows the RSpec example to optionally override and tweak behavior, without having to define constants per example.

e.g. a brief example of this could look like:

# Shared context RSpec.shared_context 'GRPC service' do def run_request_reply(address = '0.0.0.0:50052', client = nil) runner(address, client) { |c| c.basic(message_class.new) } end # Stubbed constant # This message class can be overidden in an example to change behavior let(:message_class) do stub_const('TestMessage', Class.new do class << self def marshal(_o) '' end def unmarshal(_o) new end end end) end end # Compose it into a test RSpec.describe "GRPC test" do include_context 'GRPC service' end

For a better example in practice, check out the Resque job shared context here: https://github.com/DataDog/dd-trace-rb/pull/405/files

This is only a suggestion though; only implement it if you think it will benefit these tests.

delner · 2018-04-26T14:47:04Z

I think we're really close to merging this one now. Final steps:

Need to try out the performance tests on our end. You had setup a repo for this already, yeah? Where could I find that?
Add GRPC to a sample app to generate sample traces. Do you have any examples of a minimal GRPC setup that I could use as a reference?
Add documentation to GettingStarted.md, in a new GRPC section.

Jared-Prime · 2018-04-26T17:03:08Z

https://github.com/Jared-Prime/grpc-demo/ should be ready. I've also pushed a docker image with Ruby GRPC and the latest version of this branch: https://hub.docker.com/r/jprime/grpc-demo/tags/
The demo code here implements this protocol. The setup generally follows what happens in the benchmark script. I'll be happy to answer any questions or write some basic docs if the setups not clear.
Would you like me to add it or would you like to add the documentation after testing and experimenting with the code a bit? Either way works for me.

Thanks!

delner · 2018-04-26T17:05:18Z

I don't see why we can't add a rough draft for it now. Feel free to add something to GettingStarted.md, and we can revise it later if need be.

delner · 2018-04-26T19:04:51Z

docs/GettingStarted.md

+
+| Key | Description | Default |
+| --- | --- | --- |
+| ``service_name`` | Service name used for `grpc` instrumentation | grape |


You might want to use a default other than grape 😄

oops 🤦‍♂️

delner · 2018-04-26T19:05:55Z

docs/GettingStarted.md

@@ -1102,6 +1154,30 @@ Then you must enable the request queuing feature in the integration handling the

 For Rack based applications, see the [documentation](#rack) for details for enabling this feature.

+### HTTP request queuing


How did this end up in this PR? I think this got merged to 0.13-dev already. Bad rebase? Looking at this, this PR has a lot of commits it shouldn't have in it (which have already been merged to 0.13-dev.)

interesting. Fixing....

…C::Interceptor registry.This leads to cleaner separation between client side and server sidetracing implementation and testing.

use service name from the pin

some rubocop cleanup

- change module name to `Datadog::Contrib::GRPC::InterceptWithDatadog` - allow sampling priority value less than zero ( for user reject ) - use named subjects in spec

delner · 2018-04-26T21:28:33Z

Okay, I've tried this out in a sample application. Seems to be working.

I think the big gap between the client and server span is clock drift.

delner

Okay, I'm satisfied with this. Given the size of this, I'm going to request one other review from another maintainer on our side, but otherwise I think we should be good to merge this.

palazzem

Impressive contribution! Thank you very much @Jared-Prime! It's a huge improvement to our supported libraries!

palazzem · 2018-04-30T09:35:05Z

lib/ddtrace/contrib/grpc/datadog_interceptor/client.rb

+            Datadog::GRPCPropagator
+              .inject!(span.context, metadata)
+          rescue StandardError => e
+            Datadog::Tracer.log.debug("GRPC client trace failed: #{e}")


Jared-Prime · 2018-04-30T13:38:17Z

Thank you for your help in completing this, @delner

delner · 2018-04-30T18:58:11Z

Hey @Jared-Prime , I merged this to 0.13-dev but it appears to be breaking the CI. Could you take a look at this? https://circleci.com/gh/DataDog/dd-trace-rb/2608#tests/containers/1

Seems like these two examples are flaky, tend to break randomly:

1) gRPC integration test server stream behaves like associates child spans with the parent should eq 8432511801728020899
     Failure/Error: expect(child_span.parent_id).to eq parent_span.span_id
     
       expected: 8432511801728020899
            got: 0
     
       (compared using ==)
     Shared Example Group: "associates child spans with the parent" called from ./spec/ddtrace/contrib/grpc/integration_spec.rb:65
     # ./spec/ddtrace/contrib/grpc/integration_spec.rb:49:in `block (3 levels) in <top (required)>'

  2) gRPC integration test bidirectional stream behaves like associates child spans with the parent should eq 5439390562754383015
     Failure/Error: expect(child_span.parent_id).to eq parent_span.span_id
     
       expected: 5439390562754383015
            got: 0
     
       (compared using ==)
     Shared Example Group: "associates child spans with the parent" called from ./spec/ddtrace/contrib/grpc/integration_spec.rb:70
     # ./spec/ddtrace/contrib/grpc/integration_spec.rb:49:in `block (3 levels) in <top (required)>'

Jared-Prime · 2018-04-30T21:52:00Z

@delner Do you think we need to set sampling priority on these two examples?

delner · 2018-04-30T22:53:49Z

@Jared-Prime Probably. Distributed tracing is supposed to always distribute the same sampling_priority value via headers so spans within the same trace don't get randomly sampled out.

delner · 2018-05-01T18:02:07Z

I think this has been solved in #412.

delner · 2018-05-01T18:06:06Z

@Jared-Prime Just want to say, thanks for your amazing contribution! This is a really cool feature, and you did a great job with it, as well as being on top of things throughout the PR process. It's been a pleasure working with you on this from the maintainer side!

Looking forward to seeing others enjoy this new feature as well!

🎉 🎉 🎉

Support gRPC

This was referenced Apr 13, 2018

gRPC Interceptors #378

Closed

Support gRPC #379

Closed

delner reviewed Apr 20, 2018

View reviewed changes

delner force-pushed the 0.13-dev branch from c51f3ac to 91e7e8b Compare April 23, 2018 18:53

delner reviewed Apr 26, 2018

View reviewed changes

Jared Davis added 8 commits April 26, 2018 14:17

gRPC Distributed TracingThis alternate implementation patches the GRP…

35a7f2c

…C::Interceptor registry.This leads to cleaner separation between client side and server sidetracing implementation and testing.

use service name from the pin

efb3dda

use service name from the pin

place the implementing class data into the server resource tag

ec2aac6

some rubocop cleanup

use shared context in specs

bce2fae

allow overriding global configurations

a1c6d9e

PR updates

38c2e67

- change module name to `Datadog::Contrib::GRPC::InterceptWithDatadog` - allow sampling priority value less than zero ( for user reject ) - use named subjects in spec

additional error handling around the trace

21943da

add documentation

94b1212

Jared-Prime force-pushed the feature/grpc branch from 3b01ffc to 94b1212 Compare April 26, 2018 19:18

s/grape/grpc

1f47670

delner requested a review from palazzem April 27, 2018 17:36

delner assigned Jared-Prime Apr 27, 2018

delner added integrations Involves tracing integrations community Was opened by a community member feature Involves a product feature labels Apr 27, 2018

delner added this to the 0.13.0 milestone Apr 27, 2018

delner approved these changes Apr 27, 2018

View reviewed changes

palazzem approved these changes Apr 30, 2018

View reviewed changes

delner merged commit 42c9b0a into DataDog:0.13-dev Apr 30, 2018

delner added a commit that referenced this pull request May 8, 2018

Merge pull request #403 from Jared-Prime/feature/grpc

01a4df2

Support gRPC

		@@ -1102,6 +1154,30 @@ Then you must enable the request queuing feature in the integration handling the

		For Rack based applications, see the [documentation](#rack) for details for enabling this feature.

		### HTTP request queuing

Support gRPC #403

Support gRPC #403

Conversation

Jared-Prime commented Apr 13, 2018 • edited Loading

adomokos commented Apr 17, 2018

delner commented Apr 17, 2018

adomokos commented Apr 17, 2018

delner 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

delner Apr 23, 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

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

Choose a reason for hiding this comment

delner commented Apr 26, 2018

Jared-Prime commented Apr 26, 2018

delner commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner commented Apr 26, 2018

delner left a comment

Choose a reason for hiding this comment

palazzem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jared-Prime commented Apr 30, 2018

delner commented Apr 30, 2018 • edited Loading

Jared-Prime commented Apr 30, 2018

delner commented Apr 30, 2018

delner commented May 1, 2018

delner commented May 1, 2018 • edited Loading

Jared-Prime commented Apr 13, 2018 •

edited

Loading

delner Apr 23, 2018 •

edited

Loading

delner Apr 26, 2018 •

edited

Loading

delner commented Apr 30, 2018 •

edited

Loading

delner commented May 1, 2018 •

edited

Loading