Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support gRPC #379

Closed
Jared-Prime opened this issue Mar 20, 2018 · 10 comments
Closed

Support gRPC #379

Jared-Prime opened this issue Mar 20, 2018 · 10 comments
Labels
feature-request A request for a new feature or change to an existing one integrations Involves tracing integrations

Comments

@Jared-Prime
Copy link
Contributor

Jared-Prime commented Mar 20, 2018

As a user of gRPC services/clients and of Datadog APM, I would like to have the gem support a grpc plugin.

Proposed Implementation: #378

@delner delner added integrations Involves tracing integrations feature-request A request for a new feature or change to an existing one labels Apr 9, 2018
@Jared-Prime
Copy link
Contributor Author

Jared-Prime commented Apr 12, 2018

@delner I'm going to pull the higher-level feedback and considerations into this issue. I have many of the changes you've requested implemented, though not yet pushed up. I'm thinking of opening a new PR with a simpler, more complete integration and closing #378. My intention is to get all the items below completed by end of this week, early next week at the latest. I'll be testing some of our newer internal services at KennaSecurity on my branch as well as providing a simple demo application for inspection.


TODO & Followup

Interception Patterns:

  • server-side
    • request response
    • client streamer
    • server streamer
    • bidirectional streamer
  • client-side
    • request response
    • client streamer
    • server streamer
    • bidirectional streamer

Testing:

  • unit test coverage
  • integration test (rspec)
  • integration test (manual, demo project)
  • performance benchmark

Datadog Features:

  • parent / child span associations
  • configurable service name
  • pinnable client configurations (eg. "strategy to support GRPC calls as different services")

@delner
Copy link
Contributor

delner commented Apr 12, 2018

@Jared-Prime Great! Thanks for putting this together. Looking forward to getting this wrapped up.

@Jared-Prime
Copy link
Contributor Author

Jared-Prime commented Apr 13, 2018

demo app + performance benchmark: https://github.com/Jared-Prime/grpc-demo

@Jared-Prime
Copy link
Contributor Author

I feel like #403 is in a good place. I think I've gotten the Pin correct from the clients' perspective; the main process will be identified as whatever has been set in the configuration block and specific client stubs will supply additional resource information while tracing.

@delner
Copy link
Contributor

delner commented Apr 13, 2018

@Jared-Prime It's looking pretty good, but I don't think the use of Pin is quite right.

You have this to set the pin:

def add_pin
  Pin.new(
    get_option(:service_name),
    app: 'grpc',
    app_type: 'grpc',
    tracer: get_option(:tracer)
  ).onto(::GRPC)
end

And this to get the pin back:

def pin
  Pin.get_from(::GRPC)
end

This effectively globalizes the configuration settings by attaching them to that constant, meaning if you do gRPC calls to two different services, you'd end up seeing them traced as one service (which is not ideal.)

I suspect you probably were using an older integration in our library for inspiration? The pin used to be the primary means of apply configuration prior to our configuration and Patcher API. We now use it only internally for this specific scenario where we want to trace two requests within the same integration with different settings.

The tricky part here is putting the pin on the right thing... what the "right" thing is might depend on how this gRPC library works. Usually the "right" thing would be something unique to a service, e.g. a client object configured with specific server settings. Such that in the intercept function, you could use some property of the request to retrieve the configuration specific for that kind of request.

I don't know if there's anything like that here; it looks like a middleware pattern, that may or may not expose a "client" object or some other suitable attribute of the request.

@Jared-Prime
Copy link
Contributor Author

Jared-Prime commented Apr 16, 2018

This is tricky. In gRPC Ruby, the developer retrieves client classes using the rpc_sub_class method on the service itself; specific configurations pass via the initialization method. Here's an example in the demo app.

The gRPC Go package for DataDog supplying the intereceptor when setting up the client.

// from the godocs at https://godoc.org/github.com/DataDog/dd-trace-go/contrib/google.golang.org/grpc
// Create the client interceptor using the grpc trace package.
i := grpctrace.UnaryClientInterceptor(
    grpctrace.WithServiceName("my-grpc-client"),
    grpctrace.WithTracer(tracer.DefaultTracer),
)

// Create initialization options for dialing into a server. Make sure
// to include the created interceptor.
opts := []grpc.DialOption{
    grpc.WithInsecure(),
    grpc.WithUnaryInterceptor(i),
}

// Dial in...
conn, err := grpc.Dial("localhost:50051", opts...)
if err != nil {
    log.Fatal(err)
}
defer conn.Close()

// And continue using the connection as normal.

In Ruby, that would look like:

Demo::Echo::Service.rpc_stub_class
                                  .new('localhost:50051',
                                           :this_channel_is_insecure,
                                           :interceptors => [Datadog::Contrib::GRPC::DatadogInterceptor::Client.new])

Pin information may then be set on the interceptor instance. What do you think of that approach? Would that also mean there's no need for a global pin or monkey patching the InterceptionContext?

@delner
Copy link
Contributor

delner commented Apr 16, 2018

Ah, if you can initialize and assign a separate interceptor instance per service, that could work well.

The only question left would be how are users expected to configure this behavior? If at all possible we'd like to provide a default Datadog.configure { |c| c.use :grpc } behavior to get quick out-of-the-box functionality for a single service gRPC setup. For the multiple service scenario, it would be reasonable that they're expected to undertake some more sophisticated configuration within their gRPC setup.

Could you provide example code for 1) basic, single service setup and 2) multi-service setup? As in, what it would look like for a user to simply use the gRPC tracing function we're building here?

@Jared-Prime
Copy link
Contributor Author

The only question left would be how are users expected to configure this behavior?

Here's what I came up with in 9573b5d

Datadog.configure do |c|
  c.use :grpc, service_name: "Default"
end

default_client = Demo::Echo::Service.rpc_stub_class.new(
  'localhost:50051',
  :this_channel_is_insecure
)

configured_interceptor = Datadog::Contrib::GRPC::DatadogInterceptor::Client.new do |c|
  c.service_name = "Alternate"
end

alternate_client = Demo::Echo::Service.rpc_stub_class.new(
  'localhost:50051',
  :this_channel_is_insecure,
  :interceptors => [configured_interceptor]
)

Spans sent by default_client will be under the service "Default" and spans sent by the alternate_client will be under the service "Alternate".

@delner
Copy link
Contributor

delner commented Apr 17, 2018

@Jared-Prime Awesome, I think that looks good! It should allow for the multiple service scenario.

@delner
Copy link
Contributor

delner commented May 11, 2018

This has been merged to 0.13-dev and was released with 0.13.0.beta1. Thanks @Jared-Prime !

@delner delner closed this as completed May 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A request for a new feature or change to an existing one integrations Involves tracing integrations
Projects
None yet
Development

No branches or pull requests

2 participants