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

Fix import when trace is missing from opentelemetry #2694

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

nicoloboschi
Copy link
Contributor

otel dependency is optional but if I try to use latest elastic client it fails to bootstrap due to

../../../vdp/config_verification/elastic_sink/elastic_config_verification.py:21: in <module>
    from elasticsearch import Elasticsearch
../../../../.venv/lib/python3.11/site-packages/elasticsearch/__init__.py:45: in <module>
    from ._async.client import AsyncElasticsearch as AsyncElasticsearch
../../../../.venv/lib/python3.11/site-packages/elasticsearch/_async/client/__init__.py:38: in <module>
    from ._base import (
../../../../.venv/lib/python3.11/site-packages/elasticsearch/_async/client/_base.py:48: in <module>
    from ..._otel import OpenTelemetry
../../../../.venv/lib/python3.11/site-packages/elasticsearch/_otel.py:25: in <module>
    from opentelemetry import trace
E   ImportError: cannot import name 'trace' from 'opentelemetry' (unknown location)

Fix

While is usually correct to catch ModuleNotFoundError, in case of opentelemetry this is a bit different since the actual "opentelemetry" package doesn't exist at all (we got opentelemetry-api and opentelemetry-sdk) and in that case Python (3.11 in my case) throws a more generic ImportError

Copy link

cla-checker-service bot commented Nov 11, 2024

💚 CLA has been signed

Copy link

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

Copy link
Member

@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.

While is usually correct to catch ModuleNotFoundError, in case of opentelemetry this is a bit different since the actual "opentelemetry" package doesn't exist at all (we got opentelemetry-api and opentelemetry-sdk) and in that case Python (3.11 in my case) throws a more generic ImportError

Actually, the distribution packages opentelemetry-api and opentelemetry-sdk can be accessed with the import package opentelemetry. See https://packaging.python.org/en/latest/discussions/distribution-package-vs-import-package/ to understand the difference. And indeed, if I run pip install opentelemetry-api, then I can run from opentelemetry import trace without error.

Most of our CI runs without opentelemetry-api installed, from Python 3.8 to 3.13. so catching ModuleNotFoundError is generally correct. What seems to happen is that you have an opentelemetry import package that does not define trace somehow. This is possible if you run pip install opentelemetry-sdk (which will also install opentelemetry-api) and then manually run pip uninstall opentelemetry-api. At this point, the import package opentelemetry still exists (and allows access to opentelemtry.sdk) but opentelemetry.trace no longer exists. I don't think the OpenTelemetry SDK can be used without the API, but this should not break the Elasticsearch client, so the pull request looks good to me. Thank you!

@pquentin
Copy link
Member

buildkite test this please

@pquentin pquentin changed the title fix: otel integration should be optional Fix import when trace is missing from opentelemetry Nov 12, 2024
@pquentin pquentin merged commit de6ed82 into elastic:main Nov 12, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 12, 2024
github-actions bot pushed a commit that referenced this pull request Nov 12, 2024
pquentin pushed a commit that referenced this pull request Nov 12, 2024
(cherry picked from commit de6ed82)

Co-authored-by: Nicolò Boschi <boschi1997@gmail.com>
pquentin pushed a commit that referenced this pull request Nov 12, 2024
(cherry picked from commit de6ed82)

Co-authored-by: Nicolò Boschi <boschi1997@gmail.com>
@pquentin
Copy link
Member

Thank you for your contribution, it is now released as part of 8.16.0: https://github.com/elastic/elasticsearch-py/releases/tag/v8.16.0

@nicoloboschi nicoloboschi deleted the import-otel branch November 13, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants