SQLAlchemy instrumentor populates span after sqlcomment creation, not before (#2937)

Author: tammy-baylis-swi

CHANGELOG.md

@@ -17,6 +17,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#2976](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2976))
   - Add `opentelemetry-instrumentation-openai-v2` to `opentelemetry-bootstrap`
   ([#2996](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2996))
+  - `opentelemetry-instrumentation-sqlalchemy` Add sqlcomment to `db.statement` attribute
+  ([#2937](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2937))
+  - `opentelemetry-instrumentation-dbapi` Add sqlcomment to `db.statement` attribute
+  ([#2935](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2935))
 
 ### Fixed
 
@@ -44,8 +48,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#2635](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2635))
 - `opentelemetry-instrumentation` Add support for string based dotted module paths in unwrap
   ([#2919](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2919))
-- `opentelemetry-instrumentation-dbapi` Add sqlcomment to `db.statement` attribute
-  ([#2935](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2935))
 
 ### Fixed
 

instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py

@@ -219,28 +219,31 @@ def _before_cur_exec(
         )
         with trace.use_span(span, end_on_exit=False):
             if span.is_recording():
+                if self.enable_commenter:
+                    commenter_data = {
+                        "db_driver": conn.engine.driver,
+                        # Driver/framework centric information.
+                        "db_framework": f"sqlalchemy:{sqlalchemy.__version__}",
+                    }
+
+                    if self.commenter_options.get(
+                        "opentelemetry_values", True
+                    ):
+                        commenter_data.update(**_get_opentelemetry_values())
+
+                    # Filter down to just the requested attributes.
+                    commenter_data = {
+                        k: v
+                        for k, v in commenter_data.items()
+                        if self.commenter_options.get(k, True)
+                    }
+
+                    statement = _add_sql_comment(statement, **commenter_data)
+
                 span.set_attribute(SpanAttributes.DB_STATEMENT, statement)
                 span.set_attribute(SpanAttributes.DB_SYSTEM, self.vendor)
                 for key, value in attrs.items():
                     span.set_attribute(key, value)
-            if self.enable_commenter:
-                commenter_data = {
-                    "db_driver": conn.engine.driver,
-                    # Driver/framework centric information.
-                    "db_framework": f"sqlalchemy:{sqlalchemy.__version__}",
-                }
-
-                if self.commenter_options.get("opentelemetry_values", True):
-                    commenter_data.update(**_get_opentelemetry_values())
-
-                # Filter down to just the requested attributes.
-                commenter_data = {
-                    k: v
-                    for k, v in commenter_data.items()
-                    if self.commenter_options.get(k, True)
-                }
-
-                statement = _add_sql_comment(statement, **commenter_data)
 
         context._otel_span = span
 

instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py

@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
+import re
 
 import pytest
 from sqlalchemy import (
@@ -21,6 +22,7 @@
 
 from opentelemetry import context
 from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor
+from opentelemetry.semconv.trace import SpanAttributes
 from opentelemetry.test.test_base import TestBase
 
 
@@ -59,6 +61,38 @@ def test_sqlcommenter_enabled(self):
             r"SELECT  1 /\*db_driver='(.*)',traceparent='\d{1,2}-[a-zA-Z0-9_]{32}-[a-zA-Z0-9_]{16}-\d{1,2}'\*/;",
         )
 
+    def test_sqlcommenter_enabled_matches_db_statement_attribute(self):
+        engine = create_engine("sqlite:///:memory:")
+        SQLAlchemyInstrumentor().instrument(
+            engine=engine,
+            tracer_provider=self.tracer_provider,
+            enable_commenter=True,
+            commenter_options={"db_framework": False},
+        )
+        cnx = engine.connect()
+        cnx.execute(text("SELECT  1;")).fetchall()
+        query_log = self.caplog.records[-2].getMessage()
+        self.assertRegex(
+            query_log,
+            r"SELECT  1 /\*db_driver='(.*)',traceparent='\d{1,2}-[a-zA-Z0-9_]{32}-[a-zA-Z0-9_]{16}-\d{1,2}'\*/;",
+        )
+        spans = self.memory_exporter.get_finished_spans()
+        self.assertEqual(len(spans), 2)
+        # first span is connection to db
+        self.assertEqual(spans[0].name, "connect")
+        # second span is query itself
+        query_span = spans[1]
+        self.assertRegex(
+            query_span.attributes[SpanAttributes.DB_STATEMENT],
+            r"SELECT  1 /\*db_driver='(.*)',traceparent='\d{1,2}-[a-zA-Z0-9_]{32}-[a-zA-Z0-9_]{16}-\d{1,2}'\*/;",
+        )
+        cnx_span_id = re.search(r"[a-zA-Z0-9_]{16}", query_log).group()
+        db_statement_span_id = re.search(
+            r"[a-zA-Z0-9_]{16}",
+            query_span.attributes[SpanAttributes.DB_STATEMENT],
+        ).group()
+        self.assertEqual(cnx_span_id, db_statement_span_id)
+
     def test_sqlcommenter_enabled_otel_values_false(self):
         engine = create_engine("sqlite:///:memory:")
         SQLAlchemyInstrumentor().instrument(