-
Notifications
You must be signed in to change notification settings - Fork 375
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
Conversation
end | ||
|
||
def setup | ||
@writer = FauxWriter.new() |
There was a problem hiding this comment.
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 ()
? 🙏
There was a problem hiding this comment.
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}" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
script_name_prefix
?
Very good PR description, by the way :) 👏 |
Okay, modified per feedback. I added a |
f32ec8a
to
b5c6cff
Compare
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 viamap '/foo'
would change the traceresource
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.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:
lib/sinatra/app.rb:
lib/sinatra/app/foo.rb:
lib/sinatra/app/bar.rb:
If you were to call
GET /foo/endpoint
, thenGET /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 asGET /foo/endpoint
andGET /bar/endpoint
for a less confusing user experience.