The Wayback Machine - https://web.archive.org/web/20210106160144/https://github.com/ruby-grape/grape/issues/1929
Skip to content

/ grape

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

Jump to bottom

Request for a method to wrap executing @block in Endpoint #1929

Open
estolfo opened this issue Nov 11, 2019 · 5 comments
Open

Request for a method to wrap executing @block in Endpoint #1929

estolfo opened this issue Nov 11, 2019 · 5 comments
Labels
discuss! feature request needs info

Comments

@estolfo
Copy link
Contributor

@estolfo estolfo commented Nov 11, 2019

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!
@dblock
Copy link
Member

@dblock dblock commented Nov 11, 2019

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.
@estolfo
Copy link
Contributor Author

@estolfo estolfo commented Nov 11, 2019 •
edited

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?
@dblock
Copy link
Member

@dblock dblock commented Nov 11, 2019

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.
@estolfo
Copy link
Contributor Author

@estolfo estolfo commented Nov 12, 2019 •
edited

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!
@dblock
Copy link
Member

@dblock dblock commented Nov 12, 2019

sounds good to me
@estolfo estolfo mentioned this issue Nov 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
discuss! feature request needs info
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
@dblock @estolfo
You can’t perform that action at this time.