From 8c6c75d350f468fa3b0686d017e602c23e929c5a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marcin=20Po=C5=BAniak?= <marcin.pozniak@intel.com>
Date: Fri, 30 Aug 2024 07:12:32 -0700
Subject: [PATCH] Change name to _set_span_attribute_if_value, use constant for
 _FIELD_TYPES, return when zero returned docs

---
 .../instrumentation/redis/__init__.py         | 50 +++++++++----------
 .../instrumentation/redis/util.py             |  2 +-
 2 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
index 51963b75b1..c4578ea817 100644
--- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
+++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
@@ -102,7 +102,7 @@ def response_hook(span, instance, response):
 from opentelemetry.instrumentation.redis.util import (
     _extract_conn_attributes,
     _format_command_args,
-    _set_span_attribute,
+    _set_span_attribute_if_value,
     _value_or_none,
 )
 from opentelemetry.instrumentation.redis.version import __version__
@@ -128,6 +128,8 @@ def response_hook(span, instance, response):
 _REDIS_CLUSTER_VERSION = (4, 1, 0)
 _REDIS_ASYNCIO_CLUSTER_VERSION = (4, 3, 2)
 
+_FIELD_TYPES = ["NUMERIC", "TEXT", "GEO", "TAG", "VECTOR"]
+
 
 def _set_connection_attributes(span, conn):
     if not span.is_recording() or not hasattr(conn, "connection_pool"):
@@ -239,7 +241,7 @@ def _traced_execute_pipeline(func, instance, args, kwargs):
         return response
 
     def _add_create_attributes(span, args):
-        _set_span_attribute(
+        _set_span_attribute_if_value(
             span, "redis.create_index.index", _value_or_none(args, 1)
         )
         # According to: https://github.com/redis/redis-py/blob/master/redis/commands/search/commands.py#L155 schema is last argument for execute command
@@ -251,23 +253,22 @@ def _add_create_attributes(span, args):
         field_attribute = ""
         # Schema in format:
         # [first_field_name, first_field_type, first_field_some_attribute1, first_field_some_attribute2, second_field_name, ...]
-        field_types = ["NUMERIC", "TEXT", "GEO", "TAG", "VECTOR"]
-        for index in range(len(schema)):
-            if schema[index] in field_types:
-                field_attribute += (
-                    f"Field(name: {schema[index - 1]}, type: {schema[index]});"
-                )
-        _set_span_attribute(
+        field_attribute = "".join(
+            f"Field(name: {schema[index - 1]}, type: {schema[index]});"
+            for index in range(1, len(schema))
+            if schema[index] in _FIELD_TYPES
+        )
+        _set_span_attribute_if_value(
             span,
             "redis.create_index.fields",
             field_attribute,
         )
 
     def _add_search_attributes(span, response, args):
-        _set_span_attribute(
+        _set_span_attribute_if_value(
             span, "redis.search.index", _value_or_none(args, 1)
         )
-        _set_span_attribute(
+        _set_span_attribute_if_value(
             span, "redis.search.query", _value_or_none(args, 2)
         )
         # Parse response from search
@@ -277,24 +278,21 @@ def _add_search_attributes(span, response, args):
         # Returned documents in array format:
         # [first_field_name, first_field_value, second_field_name, second_field_value ...]
         number_of_returned_documents = _value_or_none(response, 0)
-        _set_span_attribute(
+        _set_span_attribute_if_value(
             span, "redis.search.total", number_of_returned_documents
         )
-        if "NOCONTENT" in args:
+        if "NOCONTENT" in args or not number_of_returned_documents:
             return
-        if number_of_returned_documents:
-            for document_number in range(number_of_returned_documents):
-                document_index = _value_or_none(
-                    response, 1 + 2 * document_number
-                )
-                if document_index:
-                    document = response[2 + 2 * document_number]
-                    for attribute_name_index in range(0, len(document), 2):
-                        _set_span_attribute(
-                            span,
-                            f"redis.search.xdoc_{document_index}.{document[attribute_name_index]}",
-                            document[attribute_name_index + 1],
-                        )
+        for document_number in range(number_of_returned_documents):
+            document_index = _value_or_none(response, 1 + 2 * document_number)
+            if document_index:
+                document = response[2 + 2 * document_number]
+                for attribute_name_index in range(0, len(document), 2):
+                    _set_span_attribute_if_value(
+                        span,
+                        f"redis.search.xdoc_{document_index}.{document[attribute_name_index]}",
+                        document[attribute_name_index + 1],
+                    )
 
     pipeline_class = (
         "BasePipeline" if redis.VERSION < (3, 0, 0) else "Pipeline"
diff --git a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
index a69cafc0fe..4d6982f1ad 100644
--- a/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
+++ b/instrumentation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
@@ -70,7 +70,7 @@ def _format_command_args(args):
     return out_str
 
 
-def _set_span_attribute(span, name, value):
+def _set_span_attribute_if_value(span, name, value):
     if value is not None and value != "":
         span.set_attribute(name, value)
     return