Skip to content
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

Add elasticsearch instrumentation #1525

Merged
merged 61 commits into from
Oct 14, 2022
Merged

Conversation

tannalynn
Copy link
Contributor

@tannalynn tannalynn commented Oct 5, 2022

Before contributing, please read our contributing guidelines and code of conduct.

Overview

Adds instrumentation for the elasticsearch gem 7.0+.
Also updates the docker-compose file and the CI to run elasticsearch 7 and 8 to support our tests.
The Mongo sql obfuscator was moved to be used more generally for nosql obfuscation.

New configuration options:

  • instrumentation.elasticsearch
  • elasticsearch.capture_queries
  • elasticsearch.obfuscate_queries

Submitter Checklist:

  • Include a link to the related GitHub issue, if applicable
  • Include a security review link, if applicable

Testing

The agent includes a suite of unit and functional tests which should be used to
verify your changes don't break existing functionality. These tests will run with
GitHub Actions when a pull request is made. More details on running the tests locally can be found
here for our unit tests,
and here for our functional tests.
For most contributions it is strongly recommended to add additional tests which
exercise your changes.

Reviewer Checklist

  • Perform code review
  • Add performance label
  • Perform appropriate level of performance testing
  • Confirm all checks passed
  • Add version label prior to acceptance

tannalynn and others added 13 commits September 28, 2022 15:28
* Change the naming of the instrumentation method from _with_tracing
to _with_new_relic
* Add alias for renaming the original method
* Add yield to instrumented method
The template was missing the reflection of the alias method
to rename the original :perform_request method
The condition previously used ::Elastic, this caused a load error
for Elasticsearch 7.x
This first pass at the data segment finds dynamic values
for some attributes and hard codes others.

This misses any reporting on the arguments passed to perform_request
We should find a way to include those arguments in the segment
This commit adds the ability to capture queries and optionally obfuscate them
and add some associated tests
Other datastore segments notice the queries instead of adding them as
attributes this change follows that pattern
@tannalynn tannalynn force-pushed the add_elasticsearch_instrumentation branch from 1f47acb to 00d2d01 Compare October 5, 2022 19:01
.github/workflows/ci.yml Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
lib/new_relic/agent/instrumentation/elasticsearch.rb Outdated Show resolved Hide resolved
test/multiverse/lib/multiverse/runner.rb Outdated Show resolved Hide resolved
@segment = txn.segments[1]
end

# TODO! TEST METRIC GENERATION!!!
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this TODO has either been satisfied or can be tracked elsewhere outside of the code now.

fallwith
fallwith previously approved these changes Oct 14, 2022
Copy link
Contributor

@fallwith fallwith left a comment

Choose a reason for hiding this comment

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

All of my new questions / concerns for the stuff I hadn't yet synced up with you on from before have now been addressed. The instrumentation itself is still looking great and I'm excited to get it out into users' hands. Great work!

fallwith
fallwith previously approved these changes Oct 14, 2022
fallwith
fallwith previously approved these changes Oct 14, 2022
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Hooray for new instrumentation! Thanks for your leadership, Tanna!

@github-actions
Copy link
Contributor

SimpleCov Report

Coverage Threshold
Line 93.28% 93%
Branch 84.16% 84%

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.

3 participants