Update elastic search spy to check for new gem name by jaggederest · Pull Request #1257 · elastic/apm-agent-ruby

jaggederest · 2022-05-05T22:59:44Z

What does this pull request do?

There is an error in the tests, and likely no data being captured, with the new elasticsearch-ruby gem. This fixes that issue.

Why is it important?

elasticsearch-ruby relies on a separate gem to provide transport functionality, and that gem is what we instrument. Last year, in preparation for the 8.0 release, that gem was renamed and separated from the core elasticsearch-ruby repo into a new repo, and renamed elastic-transport. Our spy still works, but it was no longer checking the right dependency name to install itself, and therefore was not capturing data correctly.

Checklist

I have signed the Contributor License Agreement.
My code follows the style guidelines of this project (See .rubocop.yml)
I have rebased my changes on top of the latest main branch

I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

I have made corresponding changes to the documentation
I have updated CHANGELOG.asciidoc
I have updated supported-technologies.asciidoc
Added an API method or config option? Document in which version this will be introduced

Related issues

ghost · 2022-05-05T23:04:38Z

💔 Tests Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-05-12T09:05:59.836+0000
Duration: 34 min 46 sec

Test stats 🧪

Test	Results
Failed	6
Passed	39072
Skipped	71
Total	39149

Test errors

Expand to view the tests failures

`Tests / Tests / Ruby:ruby:3.0#sinatra-2.0 / [empty] – ruby-agent-junit.xml`

no error details

Expand to view the stacktrace

 Test report file /var/lib/jenkins/workspace/_ruby_apm-agent-ruby-mbp_PR-1257/src/github.com/elastic/apm-agent-ruby/spec/junit-reports/ruby-agent-junit.xml was length 0

`Tests / Main Tests frameworks / Ruby:ruby:3.0#rails-main / [empty] – ruby-agent-junit.xml`

no error details

Expand to view the stacktrace

 Test report file /var/lib/jenkins/workspace/_ruby_apm-agent-ruby-mbp_PR-1257/src/github.com/elastic/apm-agent-ruby/spec/junit-reports/ruby-agent-junit.xml was length 0

`Tests / Main Tests frameworks / Ruby:ruby:3.0#sinatra-master / [empty] – ruby-agent-junit.xml`

no error details

Expand to view the stacktrace

 Test report file /var/lib/jenkins/workspace/_ruby_apm-agent-ruby-mbp_PR-1257/src/github.com/elastic/apm-agent-ruby/spec/junit-reports/ruby-agent-junit.xml was length 0

`Tests / Tests / Ruby:ruby:3.0#rails-6.1 / [empty] – ruby-agent-junit.xml`

no error details

Expand to view the stacktrace

 Test report file /var/lib/jenkins/workspace/_ruby_apm-agent-ruby-mbp_PR-1257/src/github.com/elastic/apm-agent-ruby/spec/junit-reports/ruby-agent-junit.xml was length 0

`Tests / Tests / Ruby:ruby:3.0#rails-6.0 / [empty] – ruby-agent-junit.xml`

no error details

Expand to view the stacktrace

 Test report file /var/lib/jenkins/workspace/_ruby_apm-agent-ruby-mbp_PR-1257/src/github.com/elastic/apm-agent-ruby/spec/junit-reports/ruby-agent-junit.xml was length 0

`Tests / Main Tests frameworks / Ruby:ruby:3.0#grape-master / [empty] – ruby-agent-junit.xml`

no error details

Expand to view the stacktrace

 Test report file /var/lib/jenkins/workspace/_ruby_apm-agent-ruby-mbp_PR-1257/src/github.com/elastic/apm-agent-ruby/spec/junit-reports/ruby-agent-junit.xml was length 0

Steps errors

Expand to view the steps failures

Show only the first 10 steps failures

`Shell Script`

Took 1 min 5 sec . View more details here
Description: ./spec/scripts/spec.sh ruby:3.0 rails-main

`Shell Script`

Took 1 min 5 sec . View more details here
Description: ./spec/scripts/spec.sh ruby:3.0 rails-main

`Archive the artifacts`

Took 0 min 0 sec . View more details here
Description: [2022-05-12T09:23:19.885Z] Archiving artifacts [2022-05-12T09:23:20.183Z] ‘coverage/matrix_result

`Shell Script`

Took 1 min 31 sec . View more details here
Description: ./spec/scripts/spec.sh ruby:3.0 sinatra-master

`Shell Script`

Took 1 min 31 sec . View more details here
Description: ./spec/scripts/spec.sh ruby:3.0 sinatra-master

`Archive the artifacts`

Took 0 min 0 sec . View more details here
Description: [2022-05-12T09:23:24.166Z] Archiving artifacts [2022-05-12T09:23:24.413Z] ‘coverage/matrix_result

`Shell Script`

Took 1 min 35 sec . View more details here
Description: ./spec/scripts/spec.sh ruby:3.0 grape-master

`Shell Script`

Took 1 min 36 sec . View more details here
Description: ./spec/scripts/spec.sh ruby:3.0 grape-master

`Archive the artifacts`

Took 0 min 0 sec . View more details here
Description: [2022-05-12T09:23:56.420Z] Archiving artifacts [2022-05-12T09:23:56.675Z] ‘coverage/matrix_result

`Error signal`

Took 0 min 0 sec . View more details here
Description: Ruby:ruby:3.0#rails-6.1 tests failed : hudson.AbortException: script returned exit code 134

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run benchmark tests : Run the benchmark test.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

ghost · 2022-05-10T18:48:14Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`1/1`)	💚
Files	99.2% (`124/125`)	❕
Classes	99.2% (`124/125`)	❕
Lines	59.47% (`2493/4192`)	❕
Conditionals	100.0% (`0/0`)	💚

estolfo

I think we should keep this PR to fixing the elastisearch client instrumentation. Then, let's open a separate PR to resolve the other test failures (sneakers version and json seg fault)

estolfo · 2022-05-11T07:43:05Z

lib/elastic_apm/spies/elasticsearch.rb


      def install
-        ::Elasticsearch::Transport::Client.prepend(Ext)
+        if defined?(::ElasticSearch::Transport::Client)


Why do we have this condition and clause?

There are 3 different constants in use through the versions of elastic transport ruby - Elastic::Transport is the current one, the older one is ElasticSearch::Transport, and the oldest one is Elasticsearch::Transport. So this if statement makes sure we instrument the correct one of those three, based on which one is defined. I should probably make it Elastic::Transport first, now that I think about it 👍

Where did you find that it was ElasticSearch previously?

Huh, maybe it wasn't. That was just the constant I used when I fixed the test errors. I will remove it and if it doesn't cause any additional test errors, no harm no foul I guess.

spec/elastic_apm/spies/sneakers_spec.rb

spec/scripts/spec.sh

jaggederest · 2022-05-12T00:43:50Z

lib/elastic_apm/spies/elasticsearch.rb


      def install
-        ::Elasticsearch::Transport::Client.prepend(Ext)
+        if defined?(::ElasticSearch::Transport::Client)


There are 3 different constants in use through the versions of elastic transport ruby - Elastic::Transport is the current one, the older one is ElasticSearch::Transport, and the oldest one is Elasticsearch::Transport. So this if statement makes sure we instrument the correct one of those three, based on which one is defined. I should probably make it Elastic::Transport first, now that I think about it 👍

Gemfile

spec/elastic_apm/config/wildcard_pattern_list_spec.rb

spec/elastic_apm/spies/sneakers_spec.rb

…(pre 2.5)

jaggederest · 2022-05-12T01:05:19Z

@estolfo This PR should be ready for re-review, thanks for reviewing it!

* Update elastic search spy to check for new gem name * Update changelog for pr #1257 * Fix case where we are using older version of elasticsearch-transport (pre 2.5) * Omit extra bundler install in scripts/spec * Remove erroneous ElasticSearch constant check

Update elastic search spy to check for new gem name

f23884f

jaggederest requested a review from estolfo May 5, 2022 22:59

jaggederest self-assigned this May 5, 2022

github-actions bot added the agent-ruby label May 5, 2022

Update changelog for pr #1257

e3f00a2

estolfo approved these changes May 6, 2022

View reviewed changes

estolfo requested changes May 11, 2022

View reviewed changes

jaggederest commented May 12, 2022

View reviewed changes

Justin George added 2 commits May 11, 2022 17:48

Fix case where we are using older version of elasticsearch-transport …

4f943af

…(pre 2.5)

Omit extra bundler install in scripts/spec

7002e71

jaggederest requested a review from estolfo May 12, 2022 08:22

Remove erroneous ElasticSearch constant check

42e28e6

estolfo approved these changes May 12, 2022

View reviewed changes

jaggederest merged commit 1d6f89f into elastic:main May 12, 2022

jaggederest deleted the elastic_transport_spy branch May 12, 2022 10:33

Conversation

jaggederest commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this pull request do?

Why is it important?

Checklist

Related issues

ghost commented May 5, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Tests Failed

Build stats

Test stats 🧪

Test errors

Tests / Tests / Ruby:ruby:3.0#sinatra-2.0 / [empty] – ruby-agent-junit.xml

Tests / Main Tests frameworks / Ruby:ruby:3.0#rails-main / [empty] – ruby-agent-junit.xml

Tests / Main Tests frameworks / Ruby:ruby:3.0#sinatra-master / [empty] – ruby-agent-junit.xml

Tests / Tests / Ruby:ruby:3.0#rails-6.1 / [empty] – ruby-agent-junit.xml

Tests / Tests / Ruby:ruby:3.0#rails-6.0 / [empty] – ruby-agent-junit.xml

Tests / Main Tests frameworks / Ruby:ruby:3.0#grape-master / [empty] – ruby-agent-junit.xml

Steps errors

Shell Script

Shell Script

Archive the artifacts

Shell Script

Shell Script

Archive the artifacts

Shell Script

Shell Script

Archive the artifacts

Error signal

🤖 GitHub comments

ghost commented May 10, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

estolfo left a comment

Choose a reason for hiding this comment

estolfo May 11, 2022

Choose a reason for hiding this comment

jaggederest May 12, 2022

Choose a reason for hiding this comment

estolfo May 12, 2022

Choose a reason for hiding this comment

jaggederest May 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaggederest May 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaggederest commented May 12, 2022

Labels

2 participants

jaggederest commented May 5, 2022 •

edited

Loading

ghost commented May 5, 2022 •

edited by ghost

Loading

`Tests / Tests / Ruby:ruby:3.0#sinatra-2.0 / [empty] – ruby-agent-junit.xml`

`Tests / Main Tests frameworks / Ruby:ruby:3.0#rails-main / [empty] – ruby-agent-junit.xml`

`Tests / Main Tests frameworks / Ruby:ruby:3.0#sinatra-master / [empty] – ruby-agent-junit.xml`

`Tests / Tests / Ruby:ruby:3.0#rails-6.1 / [empty] – ruby-agent-junit.xml`

`Tests / Tests / Ruby:ruby:3.0#rails-6.0 / [empty] – ruby-agent-junit.xml`

`Tests / Main Tests frameworks / Ruby:ruby:3.0#grape-master / [empty] – ruby-agent-junit.xml`

`Shell Script`

`Shell Script`

`Archive the artifacts`

`Shell Script`

`Shell Script`

`Archive the artifacts`

`Shell Script`

`Shell Script`

`Archive the artifacts`

`Error signal`

ghost commented May 10, 2022 •

edited by ghost

Loading