grape icon indicating copy to clipboard operation
grape copied to clipboard

Request for a method to wrap executing @block in Endpoint

Open estolfo opened this issue 6 years ago • 5 comments

Hi, I work on the Elastic APM Ruby agent. We've recently added support for instrumenting Grape apps and it will be released in our next minor version.

Right now, we do not have backtraces for spans with Grape because using caller in our ActiveSupport::Notifications subscriber will not show a backtrace leading to the user's code. It instead shows this (note this isn't the complete backtrace, just the top ~20 lines):

/Users/emily/.rvm/gems/ruby-2.6.2/gems/activesupport-6.0.1/lib/active_support/notifications/fanout.rb:156:in `start'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/activesupport-6.0.1/lib/active_support/notifications/fanout.rb:58:in `block in start'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/activesupport-6.0.1/lib/active_support/notifications/fanout.rb:58:in `each'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/activesupport-6.0.1/lib/active_support/notifications/fanout.rb:58:in `start'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/activesupport-6.0.1/lib/active_support/notifications/instrumenter.rb:36:in `start'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/activesupport-6.0.1/lib/active_support/notifications/instrumenter.rb:22:in `instrument'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/activesupport-6.0.1/lib/active_support/notifications.rb:180:in `instrument'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/endpoint.rb:243:in `run'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/endpoint.rb:318:in `block in build_stack'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/middleware/base.rb:31:in `call!'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/middleware/base.rb:24:in `call'
/Users/emily/Code/apm-agent-ruby/fork/apm-agent-ruby/lib/elastic_apm/middleware.rb:20:in `call'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/middleware/error.rb:37:in `block in call!'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/middleware/error.rb:36:in `catch'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/middleware/error.rb:36:in `call!'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/middleware/base.rb:24:in `call'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/rack-2.0.7/lib/rack/head.rb:12:in `call'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/endpoint.rb:227:in `call!'
/Users/emily/.rvm/gems/ruby-2.6.2/gems/grape-1.2.4/lib/grape/endpoint.rb:221:in `call'

You call the user's code on this line so I'm asking if you can maybe wrap that in another method. That way, we can alias the method, grab the stacktrace with caller, then call the original method. If you have other ideas on how to expose the stacktrace, we are open to hearing them. Let me know what you'd prefer and I'm happy to open a PR. Thanks in advance!

estolfo avatar Nov 11 '19 13:11 estolfo

I don't see a problem with moving that whole block into a method @estolfo, but since it's not an interface it will be hard to guarantee this moving forward. How do others solve this? NewRelic agent is OSS too. Do you really need the stacktrace - users want to know which API this is I think and it's something you can get from ENV or context.

dblock avatar Nov 11 '19 14:11 dblock

Hi @dblock, thanks for your response. New Relic doesn't use ActiveSupport::Notifications (perhaps because they support back to a version before Grape introduced it?). They instead use the alias strategy that we also use for other frameworks. I could be missing it, but it doesn't appear that they take stacktraces either.

We realize the instability of relying on internal gem methods but sometimes it's all we can do = )

One option for us is to get a single-item stacktrace that consists of the source_location from the payload[:endpoint].source. That would be a compromise for us if there is no other way to get to the @block.call(self) line. This is used to show the source code to the user. What do you think?

estolfo avatar Nov 11 '19 15:11 estolfo

One option for us is to get a single-item stacktrace that consists of the source_location from the payload[:endpoint].source. That would be a compromise for us if there is no other way to get to the @block.call(self) line. This is used to show the source code to the user. What do you think?

I think you should do both this for older versions and PR a change so we can help you with future versions. Also open for something that looks more like an interface with tests so we don't break it moving forward.

dblock avatar Nov 11 '19 16:11 dblock

Ok, I think what we'll do is:

  1. Have a single-line stacktrace for older versions as proposed above.
  2. Open a PR to move @block.call(self) to its own method so we can overwrite it.
  3. Later, open a PR proposing an interface, as approach (2) is not sustainable.

Let me know if that sounds good. Thanks!

estolfo avatar Nov 12 '19 10:11 estolfo

sounds good to me

dblock avatar Nov 12 '19 13:11 dblock