-
Notifications
You must be signed in to change notification settings - Fork 368
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 for Sinatra modular apps #1015
Conversation
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.
Overall looks pretty good; just some questions around the implementation of fetch
for the middleware span. Once I have a better understanding of that, I think I should have no problem signing off on this.
@@ -1511,7 +1513,40 @@ get '/' do | |||
end | |||
``` | |||
|
|||
Where `options` is an optional `Hash` that accepts the following parameters: | |||
#### Modular application |
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.
Improved documentation is nice!
@@ -32,53 +36,115 @@ def route(verb, action, *) | |||
end | |||
|
|||
def self.registered(app) | |||
::Sinatra::Base.module_eval do |
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.
👍 to getting rid of monkey patches.
return span if span | ||
|
||
tracer = configuration[:tracer] | ||
span = tracer.trace( |
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.
Out of curiosity, why auto-create the span on fetch
?
Is this somehow related to if the stack short-circuits before reaching route_eval
?
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, this is the part that create the span when we are sure this is the correct middleware layer to instrument.
It can only happen in two cases:
- When a route matches: we create it right before we instrument the route itself. This happens right before
route_eval
. - When not route matches: we create it on the
app.after
callback, before we return from the current middleware.
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.
Okay, seems good. Thanks @marcotc !
Fixes #486 and #913
Closes #491 (thank you @jpaulgs!)
This PR adds improved support for Sinatra modular applications.
We already had support for classic applications, but it did not translate correctly to modular apps.
We now support nested modular Sinatra apps, and added a new span measuring the specific application route that was matched for a request (
sinatra.route
).To help support modular apps, a new tag (
sinatra.app.name
) was added to the existingsinatra.request
span, to identify which modular app handled to the request.Action shots
The existing
sinatra.request
span, which measures the total Sinatra middleware time. Now including the application name:The new
sinatra.route
span, which measures only the user application time of a matched route: