fix(redis): don't use unbound variable [backport 1.20] (#7025)

Backport ce9d9c3 from #6994 to 1.20.

The exception handling in Redis monkey patch in completely bugged,
making Redis+ddtrace broken.
This fixes it.

Fixes #6993

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

Co-authored-by: Mehdi ABAAKOUK <[email protected]>

Author: github-actions[bot]

ddtrace/contrib/redis/asyncio_patch.py

@@ -34,19 +34,17 @@ async def traced_async_execute_pipeline(func, instance, args, kwargs):
 
 
 async def _run_redis_command_async(span, func, args, kwargs):
+    parsed_command = stringify_cache_args(args)
+    redis_command = parsed_command.split(" ")[0]
     try:
-        parsed_command = stringify_cache_args(args)
-        redis_command = parsed_command.split(" ")[0]
-
         result = await func(*args, **kwargs)
+        if redis_command in ROW_RETURNING_COMMANDS:
+            determine_row_count(redis_command=redis_command, span=span, result=result)
         return result
     except Exception:
         if redis_command in ROW_RETURNING_COMMANDS:
             span.set_metric(db.ROWCOUNT, 0)
         raise
-    finally:
-        if redis_command in ROW_RETURNING_COMMANDS:
-            determine_row_count(redis_command=redis_command, span=span, result=result)
 
 
 async def traced_async_execute_cluster_pipeline(func, instance, args, kwargs):

ddtrace/contrib/trace_utils_redis.py

@@ -76,19 +76,17 @@ def determine_row_count(redis_command, span, result):
 
 
 def _run_redis_command(span, func, args, kwargs):
+    parsed_command = stringify_cache_args(args)
+    redis_command = parsed_command.split(" ")[0]
     try:
-        parsed_command = stringify_cache_args(args)
-        redis_command = parsed_command.split(" ")[0]
-
         result = func(*args, **kwargs)
+        if redis_command in ROW_RETURNING_COMMANDS:
+            determine_row_count(redis_command=redis_command, span=span, result=result)
         return result
     except Exception:
         if redis_command in ROW_RETURNING_COMMANDS:
             span.set_metric(db.ROWCOUNT, 0)
         raise
-    finally:
-        if redis_command in ROW_RETURNING_COMMANDS:
-            determine_row_count(redis_command=redis_command, span=span, result=result)
 
 
 @contextmanager

ddtrace/internal/compat.py

@@ -46,6 +46,11 @@
 PY2 = sys.version_info[0] == 2
 PY3 = sys.version_info[0] == 3
 
+try:
+    from unittest import mock
+except ImportError:
+    import mock  # type: ignore  # noqa
+
 try:
     from builtin import TimeoutError
 except ImportError:

releasenotes/notes/fix-redis-exception-handling-32964d6cc673c7bc.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    redis: Resolves ``UnboundLocalError`` raised when a traced redis command raises an exception. 

tests/contrib/redis/test_redis.py

@@ -1,11 +1,13 @@
 # -*- coding: utf-8 -*-
+import pytest
 import redis
 
 import ddtrace
 from ddtrace import Pin
 from ddtrace.constants import ANALYTICS_SAMPLE_RATE_KEY
 from ddtrace.contrib.redis.patch import patch
 from ddtrace.contrib.redis.patch import unpatch
+from ddtrace.internal.compat import mock
 from ddtrace.internal.schema import DEFAULT_SPAN_SERVICE_NAME
 from tests.opentracer.utils import init_tracer
 from tests.utils import DummyTracer
@@ -137,6 +139,15 @@ def test_basics(self):
         assert span.resource == "GET cheese"
         assert span.get_metric(ANALYTICS_SAMPLE_RATE_KEY) is None
 
+    def test_connection_error(self):
+        with mock.patch.object(
+            redis.connection.ConnectionPool,
+            "get_connection",
+            side_effect=redis.exceptions.ConnectionError("whatever"),
+        ):
+            with pytest.raises(redis.exceptions.ConnectionError):
+                self.r.get("foo")
+
     def test_analytics_without_rate(self):
         with self.override_config("redis", dict(analytics_enabled=True)):
             us = self.r.get("cheese")

tests/contrib/redis/test_redis_asyncio.py

@@ -1,4 +1,5 @@
 import typing
+from unittest import mock
 
 import pytest
 import redis
@@ -77,6 +78,18 @@ async def test_unicode_request(redis_client):
     assert val is None
 
 
+@pytest.mark.asyncio
+@pytest.mark.snapshot(wait_for_num_traces=1, ignores=["meta.error.stack"])
+async def test_connection_error(redis_client):
+    with mock.patch.object(
+        redis.asyncio.connection.ConnectionPool,
+        "get_connection",
+        side_effect=redis.exceptions.ConnectionError("whatever"),
+    ):
+        with pytest.raises(redis.exceptions.ConnectionError):
+            await redis_client.get("foo")
+
+
 @pytest.mark.asyncio
 @pytest.mark.snapshot(wait_for_num_traces=2)
 async def test_decoding_non_utf8_args(redis_client):

tests/snapshots/tests.contrib.redis.test_redis_asyncio.test_connection_error.json

@@ -0,0 +1,38 @@
+[[
+  {
+    "name": "redis.command",
+    "service": "redis",
+    "resource": "GET foo",
+    "trace_id": 0,
+    "span_id": 1,
+    "parent_id": 0,
+    "type": "redis",
+    "error": 1,
+    "meta": {
+      "_dd.base_service": "",
+      "_dd.p.dm": "-0",
+      "component": "redis",
+      "db.system": "redis",
+      "error.message": "whatever",
+      "error.stack": "Traceback (most recent call last):\n  File \"/root/project/ddtrace/contrib/trace_utils_redis.py\", line 117, in _trace_redis_cmd\n    yield span\n  File \"/root/project/ddtrace/contrib/redis/asyncio_patch.py\", line 22, in traced_async_execute_command\n    return await _run_redis_command_async(span=span, func=func, args=args, kwargs=kwargs)\n  File \"/root/project/ddtrace/contrib/redis/asyncio_patch.py\", line 41, in _run_redis_command_async\n    result = await func(*args, **kwargs)\n  File \"/root/project/.riot/venv_py31011_mock_pytest_pytest-mock_coverage_pytest-cov_opentracing_hypothesis6451_pytest-asyncio_redis~41/lib/python3.10/site-packages/redis/asyncio/client.py\", line 509, in execute_command\n    conn = self.connection or await pool.get_connection(command_name, **options)\n  File \"/root/.pyenv/versions/3.10.11/lib/python3.10/unittest/mock.py\", line 2234, in _execute_mock_call\n    raise effect\nredis.exceptions.ConnectionError: whatever\n",
+      "error.type": "redis.exceptions.ConnectionError",
+      "language": "python",
+      "out.host": "127.0.0.1",
+      "redis.raw_command": "GET foo",
+      "runtime-id": "dc59875580884b52bebd2f9c402238f8",
+      "span.kind": "client"
+    },
+    "metrics": {
+      "_dd.measured": 1,
+      "_dd.top_level": 1,
+      "_dd.tracer_kr": 1.0,
+      "_sampling_priority_v1": 1,
+      "db.row_count": 0,
+      "network.destination.port": 6379,
+      "out.redis_db": 0,
+      "process_id": 2340,
+      "redis.args_length": 2
+    },
+    "duration": 935417,
+    "start": 1695409673533997174
+  }]]