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

elasticsearch: don't produce spans if native elasticsearch support is enabled #2524

Merged
merged 9 commits into from
May 24, 2024

Conversation

xrmx
Copy link
Contributor

@xrmx xrmx commented May 15, 2024

Description

If elasticsearch is found with native OTel support enabled just call the wrapped function without creating our own spans.

Fixes #2393

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • tox -e py310-test-instrumentation-elasticsearch-2

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@xrmx xrmx added the Skip Changelog PRs that do not require a CHANGELOG.md entry label May 15, 2024
@xrmx xrmx requested a review from a team May 15, 2024 15:03
@xrmx
Copy link
Contributor Author

xrmx commented May 15, 2024

cc @pquentin

@xrmx xrmx force-pushed the test-with-es8-native-otel branch from 86a4fe5 to 0101e91 Compare May 15, 2024 15:59
@xrmx xrmx requested review from a team and removed request for a team May 15, 2024 16:00
Copy link
Contributor

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks for this Riccardo! I wonder, though, should we add a warning in that case?

The native instrumentation is more correct, following the semantic conventions, but also very different. If you install this package and elasticsearch-py 8.13+, you're going to be very surprised. So a warning saying to uninstall opentelemetry-instrumentation-elasticsearch because it does not cover elasticsearch-py 8.13+ would be useful IMO.

tox.ini Show resolved Hide resolved
@xrmx
Copy link
Contributor Author

xrmx commented May 16, 2024

Thanks for this Riccardo! I wonder, though, should we add a warning in that case?

I can add that only if we have a rate limited logger because we are still checking the span at each perform request call

The native instrumentation is more correct, following the semantic conventions, but also very different. If you install this package and elasticsearch-py 8.13+, you're going to be very surprised. So a warning saying to uninstall opentelemetry-instrumentation-elasticsearch because it does not cover elasticsearch-py 8.13+ would be useful IMO.

Will add something to the documentation

@pquentin
Copy link
Contributor

I can add that only if we have a rate limited logger because we are still checking the span at each perform request call

Warnings are deduplicated already, you can run this example to see for yourself:

import warnings

def deprecated_api(message):
    warnings.warn(message, Warning, stacklevel=2)

for i in range(10):
    deprecated_api("test")

… enabled

If elasticsearch is found with native OTel support enabled just call the
wrapped function without creating our own spans.

Fix open-telemetry#2393
@xrmx xrmx force-pushed the test-with-es8-native-otel branch from 0101e91 to fefb005 Compare May 16, 2024 08:56
@xrmx
Copy link
Contributor Author

xrmx commented May 16, 2024

Updated PR: fixed typo in env variables, added warning to the docs and code, limited tests with native Otel to elasticsearch > 8.13

@xrmx xrmx requested a review from pquentin May 16, 2024 09:12
@xrmx xrmx removed the Skip Changelog PRs that do not require a CHANGELOG.md entry label May 16, 2024
@xrmx xrmx requested review from tammy-baylis-swi and pquentin and removed request for pquentin May 16, 2024 09:59
@xrmx xrmx force-pushed the test-with-es8-native-otel branch from 9dd86c9 to 3f71d2d Compare May 16, 2024 13:28
Copy link
Contributor

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

…c/opentelemetry/instrumentation/elasticsearch/__init__.py
Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi left a comment

Choose a reason for hiding this comment

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

Thanks for comments as well

@lzchen lzchen merged commit eb8e456 into open-telemetry:main May 24, 2024
314 checks passed
shadchin pushed a commit to shadchin/opentelemetry-python-contrib that referenced this pull request May 29, 2024
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.

Handle elasticsearch client native instrumentation
4 participants