Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions .github/workflows/test-integrations-dbs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,19 @@ jobs:
--health-retries 5
ports:
- 3306:3306
elasticsearch:
image: docker.elastic.co/elasticsearch/elasticsearch:9.1.4
env:
discovery.type: single-node
xpack.security.enabled: false
ES_JAVA_OPTS: -Xms512m -Xmx512m
options: >-
--health-cmd "curl -f http://localhost:9200/_cluster/health"
--health-interval 10s
--health-timeout 5s
--health-retries 10
ports:
- 9200:9200
env:
# 3.6/3.7 run in the python:X.Y container; tell uv to use that system Python.
TOX_UV_PYTHON_PREFERENCE: ${{ (matrix.python-version == '3.6' || matrix.python-version == '3.7') && 'only-system' || 'managed' }}
Expand All @@ -52,6 +65,7 @@ jobs:
SENTRY_PYTHON_TEST_POSTGRES_PASSWORD: sentry
SENTRY_PYTHON_TEST_MYSQL_USER: root
SENTRY_PYTHON_TEST_MYSQL_PASSWORD: sentry
SENTRY_PYTHON_TEST_ES_HOST: ${{ (matrix.python-version == '3.6' || matrix.python-version == '3.7') && 'elasticsearch' || 'localhost' }}
container: ${{ (matrix.python-version == '3.6' || matrix.python-version == '3.7') && format('python:{0}', matrix.python-version) || null }}
steps:
- uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
Expand Down Expand Up @@ -81,6 +95,10 @@ jobs:
run: |
set -x # print commands that are executed
./scripts/runtox.sh "py${{ matrix.python-version }}-clickhouse_driver"
- name: Test elasticsearch
run: |
set -x # print commands that are executed
./scripts/runtox.sh "py${{ matrix.python-version }}-elasticsearch"
- name: Test pymongo
run: |
set -x # print commands that are executed
Expand Down
7 changes: 7 additions & 0 deletions scripts/populate_tox/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@
"package": "dramatiq",
"num_versions": 2,
},
"elasticsearch": {
"package": "elasticsearch",
"deps": {
"*": ["pytest-asyncio", "aiohttp"],
},
"include": ">=8",
},
"falcon": {
"package": "falcon",
"python": "<3.13",
Expand Down
6 changes: 6 additions & 0 deletions scripts/split_tox_gh_actions/split_tox_gh_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@
"clickhouse_driver",
}

FRAMEWORKS_NEEDING_ES = {
"elasticsearch",
}

FRAMEWORKS_NEEDING_DOCKER = {
"aws_lambda",
}
Expand Down Expand Up @@ -106,6 +110,7 @@
"aiomysql",
"asyncpg",
"clickhouse_driver",
"elasticsearch",
"pymongo",
"redis",
"redis_py_cluster_legacy",
Expand Down Expand Up @@ -334,6 +339,7 @@ def render_template(group, frameworks, py_versions):
"group": group,
"frameworks": frameworks,
"needs_clickhouse": bool(set(frameworks) & FRAMEWORKS_NEEDING_CLICKHOUSE),
"needs_es": bool(set(frameworks) & FRAMEWORKS_NEEDING_ES),
"needs_docker": bool(set(frameworks) & FRAMEWORKS_NEEDING_DOCKER),
"needs_mysql": bool(set(frameworks) & FRAMEWORKS_NEEDING_MYSQL),
"needs_postgres": bool(set(frameworks) & FRAMEWORKS_NEEDING_POSTGRES),
Expand Down
20 changes: 19 additions & 1 deletion scripts/split_tox_gh_actions/templates/test_group.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
python-version: [{{ py_versions|join(",") }}]
os: [ubuntu-22.04]

{% if needs_docker or needs_postgres or needs_redis or needs_mysql %}
{% if needs_docker or needs_postgres or needs_redis or needs_mysql or needs_es %}
services:
{% if needs_docker %}
docker:
Expand Down Expand Up @@ -54,6 +54,21 @@
ports:
- 3306:3306
{% endif %}
{% if needs_es %}
elasticsearch:
image: docker.elastic.co/elasticsearch/elasticsearch:9.1.4
env:
discovery.type: single-node
xpack.security.enabled: false
ES_JAVA_OPTS: -Xms512m -Xmx512m
options: >-
--health-cmd "curl -f http://localhost:9200/_cluster/health"
--health-interval 10s
--health-timeout 5s
--health-retries 10
ports:
- 9200:9200
{% endif %}
{% endif %}
env:
# 3.6/3.7 run in the python:X.Y container; tell uv to use that system Python.
Expand All @@ -70,6 +85,9 @@
SENTRY_PYTHON_TEST_MYSQL_USER: root
SENTRY_PYTHON_TEST_MYSQL_PASSWORD: sentry
{% endif %}
{% if needs_es %}
SENTRY_PYTHON_TEST_ES_HOST: {% raw %}${{ (matrix.python-version == '3.6' || matrix.python-version == '3.7') && 'elasticsearch' || 'localhost' }}{% endraw %}
{% endif %}
container: {% raw %}${{ (matrix.python-version == '3.6' || matrix.python-version == '3.7') && format('python:{0}', matrix.python-version) || null }}{% endraw %}
steps:
- uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
Expand Down
2 changes: 2 additions & 0 deletions sentry_sdk/integrations/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ def iter_default_integrations(
"sentry_sdk.integrations.clickhouse_driver.ClickhouseDriverIntegration",
"sentry_sdk.integrations.cohere.CohereIntegration",
"sentry_sdk.integrations.django.DjangoIntegration",
"sentry_sdk.integrations.elasticsearch.ElasticsearchIntegration",
"sentry_sdk.integrations.falcon.FalconIntegration",
"sentry_sdk.integrations.fastapi.FastApiIntegration",
"sentry_sdk.integrations.flask.FlaskIntegration",
Expand Down Expand Up @@ -132,6 +133,7 @@ def iter_default_integrations(
"cohere": (5, 4, 0),
"django": (1, 8),
"dramatiq": (1, 9),
"elasticsearch": (8, 0),
"falcon": (1, 4),
"fastapi": (0, 79, 0),
"flask": (1, 1, 4),
Expand Down
199 changes: 199 additions & 0 deletions sentry_sdk/integrations/elasticsearch.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
import functools
import json
from contextlib import contextmanager
from typing import TYPE_CHECKING, TypeVar

import sentry_sdk
from sentry_sdk.consts import OP, SPANDATA
from sentry_sdk.integrations import DidNotEnable, Integration, _check_minimum_version
from sentry_sdk.scope import should_send_default_pii
from sentry_sdk.traces import StreamedSpan
from sentry_sdk.tracing_utils import has_span_streaming_enabled
from sentry_sdk.utils import capture_internal_exceptions

# Hack to get new Python features working in older versions
# without introducing a hard dependency on `typing_extensions`
# from: https://stackoverflow.com/a/71944042/300572
if TYPE_CHECKING:
from collections.abc import Iterator
from typing import Any, Awaitable, Callable, Mapping, Optional, ParamSpec, Union

from sentry_sdk.tracing import Span
else:
# Fake ParamSpec
class ParamSpec:
def __init__(self, _):
self.args = None
self.kwargs = None

# Callable[anything] will return None
class _Callable:
def __getitem__(self, _):
return None

# Make instances
Callable = _Callable()

try:
from elasticsearch import ( # type: ignore[import-not-found]
__version__ as ES_VERSION,
)
from elasticsearch._sync.client._base import ( # type: ignore[import-not-found]
BaseClient,
)
except ImportError:
raise DidNotEnable("elasticsearch not installed.")

try:
from elasticsearch._async.client._base import ( # type: ignore[import-not-found]
BaseClient as AsyncBaseClient,
)
except ImportError:
# The async client's transport has optional dependencies. Missing them
# should not disable instrumentation of the sync client.
AsyncBaseClient = None


class ElasticsearchIntegration(Integration):
identifier = "elasticsearch"
origin = f"auto.db.{identifier}"

@staticmethod
def setup_once() -> None:
_check_minimum_version(ElasticsearchIntegration, ES_VERSION)

# Every API method of both the sync and the async client funnels its
# HTTP call through its class's `perform_request`, so patching the two
# of them covers the whole API surface, including raw
# `client.perform_request()` calls.
BaseClient.perform_request = _wrap_perform_request(BaseClient.perform_request)
if AsyncBaseClient is not None:
AsyncBaseClient.perform_request = _wrap_perform_request_async(
AsyncBaseClient.perform_request
)


P = ParamSpec("P")
T = TypeVar("T")


def _wrap_perform_request(f: "Callable[P, T]") -> "Callable[P, T]":
@functools.wraps(f)
def _inner(*args: "P.args", **kwargs: "P.kwargs") -> "T":
client = sentry_sdk.get_client()
if client.get_integration(ElasticsearchIntegration) is None:
return f(*args, **kwargs)

with _es_span(client, args, kwargs):
return f(*args, **kwargs)

return _inner


def _wrap_perform_request_async(
f: "Callable[P, Awaitable[T]]",
) -> "Callable[P, Awaitable[T]]":
@functools.wraps(f)
async def _inner(*args: "P.args", **kwargs: "P.kwargs") -> "T":
client = sentry_sdk.get_client()
if client.get_integration(ElasticsearchIntegration) is None:
return await f(*args, **kwargs)

with _es_span(client, args, kwargs):
return await f(*args, **kwargs)

return _inner


@contextmanager
def _es_span(
client: "sentry_sdk.client.BaseClient", args: "Any", kwargs: "Any"
) -> "Iterator[None]":
instance = args[0]
method = args[1] if len(args) > 1 else kwargs.get("method")
path = args[2] if len(args) > 2 else kwargs.get("path")
# `endpoint_id` (e.g. "search", "indices.refresh") and `path_parts` (e.g.
# {"index": "my-index"}) are only passed by elasticsearch >= 8.12.
endpoint_id = kwargs.get("endpoint_id")
path_parts = kwargs.get("path_parts")

name = str(endpoint_id) if endpoint_id else "{} {}".format(method, path)

if has_span_streaming_enabled(client.options):
span = sentry_sdk.traces.start_span(
name=name,
attributes={
"sentry.op": OP.DB,
"sentry.origin": ElasticsearchIntegration.origin,
SPANDATA.DB_SYSTEM_NAME: "elasticsearch",
},
)
_set_db_data(span, instance, method, path, endpoint_id, path_parts, kwargs)
try:
yield
finally:
span.end()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Streamed spans miss error status

Medium Severity

When span streaming is enabled, _es_span ends the span with span.end() instead of exiting via the context manager. That path never sets SpanStatus.ERROR on exceptions, so failed Elasticsearch calls are recorded as successful spans.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 63524c4. Configure here.

Comment on lines +132 to +135

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: A failing Elasticsearch streaming operation does not mark its Sentry span as an error because span.end() is used instead of a with statement.
Severity: MEDIUM

Suggested Fix

Replace the try...finally block with a with sentry_sdk.start_span(...) as span: block for the streaming operation. This will ensure the span's __exit__ method is invoked with exception details, allowing it to correctly set the span status to 'error' upon failure.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: sentry_sdk/integrations/elasticsearch.py#L132-L135

Potential issue: When an Elasticsearch operation fails in the streaming path, the
associated Sentry span's status remains 'ok' instead of being set to 'error'. This
occurs because the code uses a `try...finally` block where `span.end()` is called in the
`finally` clause. The `span.end()` method does not receive information about exceptions
from the `try` block. In contrast, the non-streaming path correctly uses a `with
sentry_sdk.start_span(...)` block, which ensures the span's `__exit__` method is called
with exception details, allowing the status to be set correctly on failure.

Did we get this right? 👍 / 👎 to inform future reviews.

else:
with sentry_sdk.start_span(
op=OP.DB,
name=name,
origin=ElasticsearchIntegration.origin,
) as span:
span.set_data(SPANDATA.DB_SYSTEM, "elasticsearch")
_set_db_data(span, instance, method, path, endpoint_id, path_parts, kwargs)

try:
yield
finally:
with capture_internal_exceptions():
if span.scope is not None:
span.scope.add_breadcrumb(
message=name, category="query", data=dict(span._data)
)


def _set_db_data(
span: "Union[Span, StreamedSpan]",
instance: "Any",
method: "Optional[str]",
path: "Optional[str]",
endpoint_id: "Optional[str]",
path_parts: "Optional[Mapping[str, Any]]",
kwargs: "Any",
) -> None:
if isinstance(span, StreamedSpan):
set_on_span = span.set_attribute
db_operation_key = SPANDATA.DB_OPERATION_NAME
else:
set_on_span = span.set_data
db_operation_key = SPANDATA.DB_OPERATION

if endpoint_id:
set_on_span(db_operation_key, str(endpoint_id))

if method:
set_on_span("http.request.method", str(method))

if path:
set_on_span("url.path", str(path))

if path_parts:
with capture_internal_exceptions():
for key, value in path_parts.items():
set_on_span("db.elasticsearch.path_parts.{}".format(key), str(value))

with capture_internal_exceptions():
# The client only knows which node served a request at the transport
# layer, so only report the server when there is no ambiguity.
nodes = list(instance.transport.node_pool.all())
if len(nodes) == 1:
set_on_span(SPANDATA.SERVER_ADDRESS, nodes[0].config.host)
set_on_span(SPANDATA.SERVER_PORT, nodes[0].config.port)

body = kwargs.get("body")
if body is not None and should_send_default_pii():
if isinstance(span, StreamedSpan):
with capture_internal_exceptions():
set_on_span(SPANDATA.DB_QUERY_TEXT, json.dumps(body, default=str))
else:
set_on_span(SPANDATA.DB_QUERY_TEXT, body)
3 changes: 3 additions & 0 deletions tests/integrations/elasticsearch/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import pytest

pytest.importorskip("elasticsearch")
Loading