gRPC Interceptors #378

Jared-Prime · 2018-03-20T21:59:52Z

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

In gRPC nomenclature, an "interceptor" is similar to middleware. When running a service or a client, a programmer must declare intended interceptors for use during runtime; as a result, the Datadog #patch implementation merely supplies the constants GRPC::DatadogClientInterceptor and GRPC::DatadogServerInterceptor for use in interceptor registries. Future work may embed tracing in the interceptor call chain itself, making the instrumentation more transparent to the application developer.

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

synchronous request/reply
synchronous streaming requests
synchronous streaming replies

The following types of gRPC have not yet been implemented and/or tested:

synchronous bidirectional streams
asynchronous calls of any kind

I've relied upon Datadog's documentation on their platform as well as documentation of the Datadog Ruby gem, Datadog Python package, the Opentracing.io standard, and the existing Rack implementation in this project. Thanks a million to those contributors, as I couldn't have figured out how to do this without their work.

I plan to use my patch going forward and am grateful for any feedback that will help get this code ready for general usage.

Jared-Prime · 2018-03-21T13:38:53Z

Looks like there's a Rubocop complaint.

Running RuboCop...
Inspecting 190 files
..............................................................................................................................................................................W...............

Offenses:

test/propagation/grpc_propagator_test.rb:30:5: W: Lint/NestedMethodDefinition: Method definitions must not be nested. Use lambda instead.
    def test_extract ...
    ^^^^^^^^^^^^^^^^

190 files inspected, 1 offense detected
RuboCop failed!

I'll submit a commit to fix that sometime today.

A couple question for the reviewer(s):

Do you prefer squashing the commits for the PR or having the full history?
I have left the bidirectional support untested in this PR. Do you prefer I complete testing / verification of that message flow on my end, or is it OK to submit a follow-up once ready?

Thanks,

Jared

delner · 2018-03-21T15:10:12Z

Hey @Jared-Prime thanks for submitting this. Pretty excited about this one!

In answer to your questions:

We're not too particular about squashing or full history, as long as the history is meaningful.
My first thought, in the interest of thoroughness, would be "yes". Although, I'm not yet very familiar with the gRPC world, and what you mean by bidirectional support, and how important this is to test. Can you elaborate in more detail what this entails, how prominent/important this particular implementation is to gRPC, and maybe any risk factors you anticipate? Would the bidirectional testing already overlap with what's been tested so far? (If so, how much?)

delner

Some general feedback, but overall off to a great start here.

I might need to do additional reviews later given there's a lot of rich contextual detail to be absorbed/understood. Hopefully this gets things rolling, though.

delner · 2018-03-21T15:22:43Z

lib/ddtrace/contrib/grpc/datadog_client_interceptor.rb

+            service: Datadog.configuration[:grpc][:service_name],
+            span_type: 'grpc'
+          }
+          span = ddtracer.trace(name, tracer_options)


Question about name: looks like this value would be server or client. Is there a more meaningful name that can be given? That might add a little context to what it's doing?

Secondary consideration; in many of the ddtrace integrations, we often prefix name with the name of the integration. Perhaps grpc.X might be helpful?

delner · 2018-03-21T15:25:49Z

lib/ddtrace/contrib/grpc/datadog_client_interceptor.rb

+        private
+
+        def ddtracer
+          @ddtracer ||= @options.fetch(:ddtracer, Datadog.tracer)


There is a tracer option on the gRPC configuration. Should we replace Datadog.tracer with Datadog.configuration[:grpc][:tracer]?

delner · 2018-03-21T15:31:24Z

lib/ddtrace/propagation/grpc_propagator.rb

+    include Ext::DistributedTracing
+
+    def self.inject!(context, metadata)
+      metadata[HTTP_HEADER_TRACE_ID] = context.trace_id.to_s


I appreciate the idea of being consistent with the other context propagation headers, but would it make sense to create a new set of constants for gRPC? e.g. GRPC_METADATA_TRACE_ID/GRPC_TRACE_ID? Even if the values are roughly the same or equivalent?

Might be a good idea to avoid coupling with HTTP implementation in case there's some divergence in implementation later.

delner · 2018-03-21T15:39:44Z

lib/ddtrace/contrib/grpc/datadog_client_interceptor.rb

+            span_type: 'grpc'
+          }
+          span = ddtracer.trace(name, tracer_options)
+          Datadog::GRPCPropagator.inject!(span.context, metadata)


This might be a silly question, but in regards to injecting the context info into the metadata...

If I'm understanding the code correctly, GRPC is supposed to provide a metadata Hash via request_response(request: nil, call: nil, method: nil, metadata: nil) options correct? Is there a case where it doesn't actually provide this metadata Hash?

I ask here because above, metadata ||= {} will initialize the nil value to a Hash, but the inject! would end up modifying a local Hash variable that'd end up discarded anyways wouldn't it?

That's a good question, and I'm not sure about the answer. I'll look it up. My suspicion is that the Hash is always supplied; I'm matching a method signature that I've seen elsewhere. I'll verify what's really needed here. Oh, I get it. Will improve

delner · 2018-03-21T15:44:55Z

lib/ddtrace/contrib/grpc/datadog_client_interceptor.rb

+          span = ddtracer.trace(name, tracer_options)
+          Datadog::GRPCPropagator.inject!(span.context, metadata)
+
+          yield


Since an interceptor is like middleware, does this yield equivocate to the actual gRPC call? In which case, could we wrap the trace method around it like:

ddtracer.trace(name, tracer_options) do |span| yield end

The benefit you'd get here is automatic span.set_error and span.finish functions rendering the rescue and ensure blocks as moot.

delner · 2018-03-21T15:46:48Z

lib/ddtrace/contrib/grpc/datadog_server_interceptor.rb

+        private
+
+        def ddtracer
+          @ddtracer ||= @options.fetch(:ddtracer, Datadog.tracer)


Just a note that the same general feedback from DatadogClientInterceptor likely applies here, too.

delner · 2018-03-21T15:54:58Z

spec/support/grpc_helpers.rb

@@ -0,0 +1,64 @@
+require 'grpc'
+
+def run_service


Not a major thing, but would it be more appropriate to put this method into a module? The module could then be composed into the RSpec examples using include GRPCHelpers, while having the benefit of not having a global level run_service method.

delner · 2018-03-21T15:56:51Z

test/propagation/grpc_propagator_test.rb

+require 'ddtrace/tracer'
+require 'ddtrace/propagation/grpc_propagator'
+
+class GRPCPropagatorTest < Minitest::Test


We've been working on moving away from Minitest towards RSpec. Does this test benefit from something in Minitest that isn't available or conducive to RSpec?

No major benefit of Minitest over RSpec. I will rewrite this in RSpec instead

Jared-Prime · 2018-03-21T22:19:40Z

Thanks @delner. I've refactored a bit according to your recommendations. I also found that my test for bidirectional (bidi) streaming was simply broken; I've fixed it and able to confirm the code should work for bidi streams as well.

No rush on reviewing; I will be on vacation the rest of this week and next. I'll look forward to tying up any loose ends when I'm back.

delner · 2018-03-28T18:24:46Z

General thought when looking at this implementation: would it be conceivable to use GRPC to connect to different services? And if that's the case, would it be reasonable to want to trace those GRPC calls as different services?

If so, we might want to change how we tag traces with service_name. I think configuration[:grpc][:service_name] is a good start, but would inevitably pool multiple services as one; probably not something most users would want.

To this end we have the Datadog::Pin object, which would allow us to tag an individual object with configuration. Typically, we might "pin" service configuration onto each "client" object with Datadog.configure(client, service_name: 'client_1'). Then for the trace call, we'd retrieve the configuration from the client via pin = Datadog::Pin.get_from(client) and use that to configure the trace.

Does GRPC have a "client" or something similar we could pin configuration on? If not, maybe we should think about some strategy to support GRPC calls as different services.

delner · 2018-03-28T18:46:37Z

Also:

RSpec examples for GRPC currently aren't running in our CI. If you add a command to run the examples here, they should run. https://github.com/Jared-Prime/dd-trace-rb/blob/57846c44648529378f6caf1147ee9915a09712fd/Rakefile#L233
We should rebase this on top of 0.12-dev. We put new features like this in our current dev branch, so I think this would be a suitable base.

delner · 2018-03-28T18:46:50Z

Just to give you a sense of what the plan is for this one, I think we'd ultimately like to target a 0.13.0 release for this PR. This is a really awesome, but sophisticated feature that warrants some additional performance testing among other due diligence measures. Our 0.12.0 release will be rolling soon, and instead of rushing this in, it'd be good to give this one the time and attention it deserves.

We'll be opening a 0.13-dev branch soon, and I think it could be a good candidate for an early beta version of 0.13.0. Next steps between now and then would be to 1) address any outstanding feedback/issues in this PR and 2) develop a performance test plan (and perhaps a sample app) which we can use to verify that everything is in good shape, then finally 3) merge this PR and ship for a beta release.

Jared-Prime · 2018-04-02T14:03:54Z

I think we'd ultimately like to target a 0.13.0 release for this PR. This is a really awesome, but sophisticated feature that warrants some additional performance testing among other due diligence measures.

I agree with that choice of release target and your proposed steps towards shipping 👍 . I'll be able to give this branch more love this week.

separate server and client interceptors into different files create a serparate propagator for grpc usage test for bidirectional streaming support earlier inability to test this was due to a typo in the TestService class definition fix bug with GRPCPropagator A miswritten test masked a bug. The GRPCPropagator potentially supplied a value of 0 for a sampling priority when no sampling priority was declared [REFACTOR] implement PR review suggestions - different constants for gRPC metadata - supply tracer object via configuration - supply run_service helper method as part of a module - prefix trace name with "grpc" - use rspec rather than minitest some additional changes include - supply the protocol method name as the datadog resource - supply the grpc peer information as datadog tags - set metadata information as tags fix some testing errors use circleci tests, remove old minitest test

Jared-Prime · 2018-04-13T19:17:02Z

closing this in favor of improved implementation at #403

Jared-Prime mentioned this pull request Mar 20, 2018

Support gRPC #379

Closed

palazzem requested a review from delner March 21, 2018 08:44

palazzem assigned delner Mar 21, 2018

palazzem added integrations Involves tracing integrations community Was opened by a community member labels Mar 21, 2018

palazzem added this to the 0.12.0 milestone Mar 21, 2018

delner added the feature Involves a product feature label Mar 21, 2018

delner reviewed Mar 21, 2018

View reviewed changes

palazzem removed this from the 0.12.0 milestone Mar 28, 2018

Jared Davis added 2 commits April 2, 2018 10:43

allow specifying service name when configuring client or server

ebe657d

Jared-Prime force-pushed the grpc-interceptors branch from 57846c4 to ebe657d Compare April 3, 2018 14:27

delner added this to the 0.13.0 milestone Apr 5, 2018

delner changed the base branch from master to 0.13-dev April 13, 2018 17:32

Jared-Prime mentioned this pull request Apr 13, 2018

Support gRPC #403

Merged

Jared-Prime closed this Apr 13, 2018

Jared-Prime deleted the grpc-interceptors branch April 13, 2018 19:17

delner removed this from the 0.13.0 milestone May 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC Interceptors #378

gRPC Interceptors #378

Jared-Prime commented Mar 20, 2018

Jared-Prime commented Mar 21, 2018

delner commented Mar 21, 2018 •

edited

Loading

delner left a comment

delner Mar 21, 2018

delner Mar 21, 2018

delner Mar 21, 2018

delner Mar 21, 2018

Jared-Prime Mar 21, 2018 •

edited

Loading

delner Mar 21, 2018

delner Mar 21, 2018

delner Mar 21, 2018

delner Mar 21, 2018

Jared-Prime Mar 21, 2018

Jared-Prime commented Mar 21, 2018

delner commented Mar 28, 2018 •

edited

Loading

delner commented Mar 28, 2018 •

edited

Loading

delner commented Mar 28, 2018

Jared-Prime commented Apr 2, 2018

Jared-Prime commented Apr 13, 2018

gRPC Interceptors #378

gRPC Interceptors #378

Conversation

Jared-Prime commented Mar 20, 2018

Jared-Prime commented Mar 21, 2018

delner commented Mar 21, 2018 • edited Loading

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

Jared-Prime Mar 21, 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

Jared-Prime commented Mar 21, 2018

delner commented Mar 28, 2018 • edited Loading

delner commented Mar 28, 2018 • edited Loading

delner commented Mar 28, 2018

Jared-Prime commented Apr 2, 2018

Jared-Prime commented Apr 13, 2018

delner commented Mar 21, 2018 •

edited

Loading

Jared-Prime Mar 21, 2018 •

edited

Loading

delner commented Mar 28, 2018 •

edited

Loading

delner commented Mar 28, 2018 •

edited

Loading