Add distributed_tracing option to Sinatra

[delner]

Currently Sinatra doesn't support distributed tracing out of the box. It required the user to configure Rack as a service first, e.g.:

Datadog.configure do |c|
  c.use :sinatra, resource_script_names: true, service_name: 'knowledge-api',
  c.use :rack, distributed_tracing: true
end

This pull request implements distributed_tracing as an option for Sinatra, so you only have to:

Datadog.configure do |c|
  c.use :sinatra, distributed_tracing: true
end

[palazzem]

Not sure about that. Sinatra can play with the Rack integration, and if c.use :rack is activated, our Context should have already a Rack span. If we do this change without checking, we may lose data here. I don't know why we can't automatically activate the Rack patch like we did for Rails:

dd-trace-rb/lib/ddtrace/contrib/rails/framework.rb

Lines 28 to 33 in f6e83ca

	Datadog.configuration.use(
	:rack,
	tracer: tracer,
	service_name: config[:service_name],
	distributed_tracing: config[:distributed_tracing]
	)

Anyway, since we didn't use the Rack integration under the hood, the approach that doesn't break anything is checking if we have spans before replacing the context. In that case, I would say to not do anything.

Side-effects:

• if you're using Rack, Rack should activate distributed_tracing
• if you're using something else on top of Sinatra, well it should be a supported framework to enable distributed tracing

What do you think?

[palazzem]

After our discussion, it's a matter of tradeoffs. Since Sinatra doesn't use Rack under the hood, we should keep this logic. The only problem with this approach, is that you may detach Sinatra and Rack traces, if you enable both and activate Sinatra distributed tracing.

The behavior is supported if you activate distributed_tracing only on Rack, that makes sense and is the lesser error prone approach. Let's keep it that way.

[palazzem]

Since we're not using Rack with Sinatra, the lesser error prone approach is this one. Good to go.

[palazzem]

After our discussion, it's a matter of tradeoffs. Since Sinatra doesn't use Rack under the hood, we should keep this logic. The only problem with this approach, is that you may detach Sinatra and Rack traces, if you enable both and activate Sinatra distributed tracing.

The behavior is supported if you activate distributed_tracing only on Rack, that makes sense and is the lesser error prone approach. Let's keep it that way.

[delner]

Discussed a possible issue when both Rack and Sinatra are activated together with @palazzem. If you activate Rack without distributed tracing, then activate Sinatra with distributed tracing, e.g.:

Datadog.configure do |c|
  c.use :rack
  c.use :sinatra, distributed_tracing: true
end

...then we could see dropped Rack spans, because Sinatra will override trace context with the trace info it receives from the request. This obviously isn't great, but it's a compromise that needs to be made because without a little aid from the configuration, it's very difficult to identify which integration is the "root" one handling inbound distributed tracing, and subsequently override trace contexts properly. The compromise made here is that we expect users who are configuring their application to apply the distributed_tracing: true option exclusively to the root most integration, where traces enter the application.

In the case of an application with both Rack and Sinatra, then it should look something like:

Datadog.configure do |c|
  c.use :rack, distributed_tracing: true
  c.use :sinatra
end

For applications with just Sinatra + distributed tracing, it would be:

Datadog.configure do |c|
  c.use :sinatra, distributed_tracing: true
end