Skip to content

Grape support #562

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

Merged
merged 29 commits into from
Oct 22, 2019
Merged

Grape support #562

merged 29 commits into from
Oct 22, 2019

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Oct 11, 2019

This PR adds support for Grape. The changes are:

  • Defining a Grape.start method so that the agent can be hooked up to a Grape app
  • Normalizer for endpoint_run
  • Set config framework values for Grape in Grape.start
  • Documentation for Grape integration
  • Add framework name and version to transaction context when a grape event is initiated by a Rails app.

Further work:

  • Maybe add normalizer format_response
  • Consider not instrumenting the filter and validator endpoints, as they can be very noisy. There are a lot of internal filters and validators called and there's no way to distinguish between library and user-specified ones.

Todo:

  • Test in Jenkins

Closes #206, #321

@estolfo estolfo marked this pull request as ready for review October 14, 2019 10:00
@estolfo estolfo requested a review from mikker October 14, 2019 10:00
@estolfo estolfo changed the title Foundations for Grape support Grape support Oct 14, 2019
Copy link
Contributor

@mikker mikker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good!

We also need to add the public api (and the Rails.start one too) to docs/api.asciidoc btw. And a changelog entry.

@estolfo
Copy link
Contributor Author

estolfo commented Oct 14, 2019

@mikker btw, we already have documentation for Rails.start here

@codecov-io
Copy link

codecov-io commented Oct 14, 2019

Codecov Report

Merging #562 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
- Coverage   94.95%   94.84%   -0.11%     
==========================================
  Files          87       87              
  Lines        2695     2696       +1     
==========================================
- Hits         2559     2557       -2     
- Misses        136      139       +3
Impacted Files Coverage Δ
lib/elastic_apm/middleware.rb 97.22% <100%> (ø) ⬆️
lib/elastic_apm.rb 96.15% <100%> (+0.04%) ⬆️
lib/elastic_apm/config.rb 90.27% <0%> (-2.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3354604...42d30e4. Read the comment docs.

@@ -22,7 +22,7 @@ secret_token: <%= ENV["VERY_SECRET_TOKEN"] %>
Options are applied in the following order (last one wins):

1. Defaults
2. Arguments to `ElasticAPM.start` / `Config.new`
2. Arguments to `ElasticAPM.start` / `ElasticAPM::Grape.start` / `ElasticAPM::Rails.start` / `Config.new`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could Arguments to .start(…) be explanatory enough? I'm afraid mentioning too many different frameworks here will confuse more than help?

@estolfo
Copy link
Contributor Author

estolfo commented Oct 16, 2019

@mikker Since your approval, I've made significant changes to this PR. It'd be great if you could review the following:

Copy link
Contributor

@mikker mikker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to go 👍

@estolfo estolfo merged commit c92de1a into elastic:master Oct 22, 2019
@mikker mikker added this to the 7.5 milestone Oct 23, 2019
@estolfo estolfo mentioned this pull request Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Grape instrumentation using ActiveSupport::Notifications
3 participants