-
Notifications
You must be signed in to change notification settings - Fork 179
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
feat: Make Rack install optional for sinatra #1019
feat: Make Rack install optional for sinatra #1019
Conversation
1ccab8b
to
2b8f182
Compare
This allows for scenarios when there are multiple Rack Applications mounted in the same builder and you want to avoid installing the Rack Events middleware multiple times in the stack. A common example is a Rails application that mounts a Sinatra App in the routes file: ```ruby Rails.application.routes.draw do mount Sinatra::Application, at: '/sinatra' end ``` This results in the Rack middleware being installed multiple times. Once by the Rails ActionPack instrumentation and then by the Sinatra application leading to multiple Rack spans being created in the same trace. This change allows you to avoid this by setting the install_rack option to false when using Sinatra instrumentation and allowing users to manually configure the middleware in the stack as they see fit.
2b8f182
to
dd3e3b0
Compare
Working through some inconsistent and unexpected behavior in tests while trying to make sure the app is installed properly. |
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.
Could we get an example of what using this looks like in the wild? Maybe an example of some real middleware that would cause this case would be helpful?
Also, would it be possible to detect that the Rack instrumentation is already installed, and emit a warning that it's going to be installed again, unless install_rack: false
is set?
# Sinatra hook after extension is registered | ||
def self.registered(app) | ||
# Create tracing `render` method | ||
::Sinatra::Base.prepend(RenderPatches) |
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 appreciate the use of a module
we can prepend
rather than module_eval
. The later always causes me a little anxiety, and to go re-read the docs; probably trauma from over-use and being overly-clever back in the Ruby meta-programming heydays. 😆
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 is great as-is. Thanks for taking on this problem. My only suggestion would be to add some sort of documentation for the new config, perhaps in the README?
@stevenharman thank you for the feedback! I will incorporate those changes. I had mentioned that I do not like this solution in during the SIG meeting and it's a bit of a hack to help unblock some teams turn on Sinatra tracing. Ideally, I would like the registry to handle dependencies and installation order but that is a bit too much for me to handle right now. @mwear suggested something along the lines of using a combination of declarative dependency management in the @kaylareopelle that was a bit irresponsible of me. I do need to think about current users and future self. |
@kaylareopelle I have added docs @stevenharman I added a diagnostic message and example output. Please let me know if that is what you are looking for. |
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.
🚢
@stevenharman I have a follow up question for you then. If the Rack is not already installed, should this instrumentation basically fail the installation rather than partially install the template rendering? Now that I am thinking about it, it will seem so weird to generate incomplete instrumentation spans where rendering a template may be the ingress span. |
So, the situation would be that someone has, for some reason explicitly set I cannot think of a reason that would be desirable, but perhaps it is? That said, I'd also prefer keep things simple (at least conceptually, if not also technically) until there's an actual use case/need for the carrying cost of the added complexity. i.e., maybe it should error because at least then folks won't be accidentally half-installing the instrumentation and then confused why spans are missing? |
Yes.
Yes. I think it would be strange for Sinatra think it is installed, though its dependency was not. We will still have the scenario where the middleware is missing but at least this takes one use case out of the equation. |
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.
Two small typos, looks great otherwise. Thanks for adding the documentation!
instrumentation/sinatra/lib/opentelemetry/instrumentation/sinatra/instrumentation.rb
Outdated
Show resolved
Hide resolved
instrumentation/sinatra/test/opentelemetry/instrumentation/sinatra_test.rb
Outdated
Show resolved
Hide resolved
…tra/instrumentation.rb Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
…atra_test.rb Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
@stevenharman On second thought, I am going to leave this as is. If someone intentionally disables the Rack dependency, they may install it at any time much later in the process and everything will still work as expected. |
This allows for scenarios when there are multiple Rack Applications mounted in the same builder and you want to avoid installing the Rack Events middleware multiple times in the stack.
A common example is a Rails application that mounts a Sinatra App in the routes file:
This results in the Rack middleware being installed multiple times.
Once by the Rails ActionPack instrumentation and then by the Sinatra application leading to multiple Rack spans being created in the same trace.
This change allows you to avoid this by setting the
install_rack
option tofalse
when using Sinatra instrumentation and allowing users to manually configure the middleware in the stack as they see fit.Example output
Manual configuration of Rack
Automatic configuration of Rack
Sinatra with
install_rack: false
and not middleware configured:With rack explicitly disabled