Skip to content

Grape support#562

Merged
estolfo merged 29 commits into
elastic:masterfrom
estolfo:grape-support
Oct 22, 2019
Merged

Grape support#562
estolfo merged 29 commits into
elastic:masterfrom
estolfo:grape-support

Conversation

@estolfo

@estolfo estolfo commented Oct 11, 2019

Copy link
Copy Markdown
Contributor

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
Comment thread lib/elastic_apm/grape.rb Outdated
@estolfo estolfo changed the title Foundations for Grape support Grape support Oct 14, 2019

@mikker mikker left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread lib/elastic_apm/grape.rb Outdated
Comment thread lib/elastic_apm/grape.rb
@estolfo

estolfo commented Oct 14, 2019

Copy link
Copy Markdown
Contributor Author

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

@codecov-io

codecov-io commented Oct 14, 2019

Copy link
Copy Markdown

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.

Comment thread docs/api.asciidoc
Comment thread docs/configuration.asciidoc Outdated

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

Copy link
Copy Markdown
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?

Comment thread docs/configuration.asciidoc Outdated
Comment thread Gemfile
Comment thread lib/elastic_apm/middleware.rb
@estolfo

estolfo commented Oct 16, 2019

Copy link
Copy Markdown
Contributor Author

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

Comment thread Gemfile
Comment thread docs/configuration.asciidoc Outdated
Comment thread lib/elastic_apm/middleware.rb
Comment thread spec/elastic_apm/grape_spec.rb Outdated
Comment thread spec/integration/grape_spec.rb Outdated
Comment thread spec/integration/rails_grape_spec.rb Outdated
Comment thread spec/integration/rails_grape_spec.rb Outdated
Comment thread lib/elastic_apm/normalizers/grape/endpoint_run.rb
Comment thread docs/configuration.asciidoc Outdated

@mikker mikker left a comment

Copy link
Copy Markdown
Contributor

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