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

Add script name to resource for Sinatra if available #283

Merged
merged 2 commits into from
Dec 20, 2017

Conversation

delner
Copy link
Contributor

@delner delner commented Dec 19, 2017

This pull request prepends request.script_name to Sinatra trace resource names, if available and enabled. e.g. get '/endpoint' from a Sinatra app rooted in Rack via map '/foo' would change the trace resource from /endpoint to /foo/endpoint.

This new behavior is disabled by default. It can be enabled by adding the resource_script_names flag to the configuration. e.g.

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

Or via Datadog.configuration[:sinatra][:resource_script_names] = true

This flag is useful for particular cases where there are multiple Sinatra apps configured in a config.ru that have overlapping endpoints rooted at different namespaces. Take for example the following app:

config.ru:

require 'bundler'
Bundler.require

require './lib/sinatra/app'

map '/foo' do
  run FooApp
end

map '/bar' do
  run BarApp
end

lib/sinatra/app.rb:

require 'ddtrace'
require 'ddtrace/contrib/sinatra/tracer'
require 'sinatra'

require_relative 'app/foo'
require_relative 'app/bar'

lib/sinatra/app/foo.rb:

class FooApp < Sinatra::Base
  register Datadog::Contrib::Sinatra::Tracer

  get '/endpoint' do
    'Foo!'
  end
end

lib/sinatra/app/bar.rb:

class BarApp < Sinatra::Base
  register Datadog::Contrib::Sinatra::Tracer

  get '/endpoint' do
    'Bar!'
  end
end

If you were to call GET /foo/endpoint, then GET /bar/endpoint, you would hit each of the apps correctly on their respective paths. However, the two traces created from these requests would report the same resource as /endpoint in the UI, which could lead to some confusion (since they are not the same app.)

By adding request.script_name onto the front of a path (see Rack specification for more info), it allows these traces to be disambiguated as GET /foo/endpoint and GET /bar/endpoint for a less confusing user experience.

@delner delner self-assigned this Dec 19, 2017
@delner delner requested review from p-lambert and palazzem December 19, 2017 18:55
end

def setup
@writer = FauxWriter.new()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the empty ()? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can!

@@ -34,7 +34,7 @@ def route(verb, action, *)
# Keep track of the route name when the app is instantiated for an
# incoming request.
condition do
@datadog_route = action
@datadog_route = "#{request.script_name}#{action}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature seems kind of an advanced use-case IMO. Could we prepend this conditionally, using a configuration param? I think for most users the resource name without the script name would still be a better default.

Copy link
Contributor Author

@delner delner Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's reasonable. Any thoughts about what you'd call it? prepend_script_name_to_resource? resource_script_names? Something less wordy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

script_name_prefix?

@p-lambert
Copy link
Member

p-lambert commented Dec 19, 2017

Very good PR description, by the way :) 👏

@delner
Copy link
Contributor Author

delner commented Dec 19, 2017

Okay, modified per feedback. I added a resource_script_names flag to the Sinatra tracer, which defaults to false, disabling this new behavior by default. Also added tests to cover this flag.

p-lambert
p-lambert previously approved these changes Dec 19, 2017
@delner delner force-pushed the delner/add_script_name_to_sinatra_route branch from f32ec8a to b5c6cff Compare December 20, 2017 16:00
@delner delner merged commit 66cb67c into master Dec 20, 2017
@palazzem palazzem added this to the 0.11.0 milestone Jan 17, 2018
@delner delner deleted the delner/add_script_name_to_sinatra_route branch February 9, 2018 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants