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

Set Elasticsearch operation dynamically #1543

Conversation

kaylareopelle
Copy link
Contributor

@kaylareopelle kaylareopelle commented Oct 14, 2022

Overview

  • Set Elasticsearch datastore segment's operation attribute dynamically from caller_locations
  • Elasticsearch 8 Playground tested here: https://staging.onenr.io/0dBj3VPrvRX (internal)
  • Elasticsearch 7 Playground tested here: https://staging.onenr.io/0dBj3VWe2RX (internal)
  • Previously, the operations were all query, the fallback value is proposed to become perform_request since the dynamic operations are set to the methods called, it seems odd to have the operation default be unrelated to any Elasticsearch method name. Totally down to revert this change, however!
  • BONUS: Reorganizes and indents the instrumentation.elasticsearch config value in newrelic.yml

Relates to #1525

end

def test_segment_operation_returns_OPERATION_when_method_nil
skip('How can I stub only the Array#index method?')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this means that the following line trying to stub isn't working how you want. I'm not sure how to do it with minitest stubs as I'm less familiar with that, but maybe James has some ideas since he's more familiar that.
I've used mocha more often so I'm more familiar with it. I'm pretty sure we can do this with mocha stubs though, using any_instance.
Array.any_instance.stubs(:index).returns(nil)
any_instance isn't really recommended generally, but we still do use it quite a lot in the agent tests already. If we do it this way, we'd also want to make sure mocha_teardown gets added to the teardown method in here as well, so the stub doesn't leak past the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I'm bummed I missed this until now! I readjusted the code and the tests a bit to make this case work in a different way. I'd be curious to see what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

oh its cool, I'm glad you figured out another way to test it!

@kaylareopelle kaylareopelle force-pushed the set_elasticsearch_operation_dynamically branch from c94232a to a6f901d Compare October 14, 2022 18:05
@kaylareopelle kaylareopelle marked this pull request as ready for review October 14, 2022 18:14
Copy link
Contributor

@tannalynn tannalynn left a comment

Choose a reason for hiding this comment

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

This awesome, thank you!

@kaylareopelle kaylareopelle merged commit 6e44b20 into add_elasticsearch_instrumentation Oct 14, 2022
@kaylareopelle kaylareopelle deleted the set_elasticsearch_operation_dynamically branch October 14, 2022 20:12
@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