Skip to content

Adds async Elasticsearch support #1309

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

Closed
wants to merge 2 commits into from

Conversation

fagnerzulin
Copy link
Contributor

Overview

This Pull Request instruments the Elasticsearch python library with asynchronous implementation. Basically, I reused what was already implemented for the synchronous version, however, I ran into some issues with the unit tests in the following files:

  • tests/datastore_elasticsearch/test_async_connection.py
  • tests/datastore_elasticsearch/test_async_mget.py
  • tests/datastore_elasticsearch/test_async_multiple_dbs.py

If anyone can help me understand what’s going on, I would be grateful.

Related Github Issue

#1204

Testing

The agent includes a suite of 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 most contributions it is strongly recommended to add additional tests which
exercise your changes.

@fagnerzulin fagnerzulin requested a review from a team as a code owner February 20, 2025 17:09
@CLAassistant
Copy link

CLAassistant commented Feb 20, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ fagnerzulin
❌ mergify[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@TimPansino TimPansino left a comment

Choose a reason for hiding this comment

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

Reviewed just the instrumentation so far but these are the issues I immediately see. I'll take a pass at fixing them and redesigning the instance_info section for async, and we'll see if that fixes the rest of the tests.

except Exception:
instance_info = ("unknown", "unknown", None)

transaction._nr_datastore_instance_info = instance_info
Copy link
Contributor

Choose a reason for hiding this comment

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

This wrapper is mostly right, but unfortunately it doesn't work in async contexts. When connecting to multiple databases at once there's a race condition that gets introduced if we store instance_info on the transaction object directly. We'll have to instead work out a different design where we stash it elsewhere, or call current_trace() and carefully insert it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any suggestions on how to do this?

@TimPansino
Copy link
Contributor

Update from our side, I'm going to take a pass at this next week. We had a bit of crunch to finish a few things by the end of this quarter and I haven't been able to spend any time on this.

@TimPansino
Copy link
Contributor

Closing in favor of #1362

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