From 1d2e7c9657c5b037093bbe356704b4624c032d85 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Tue, 29 Oct 2024 15:15:52 -0700 Subject: [PATCH 1/5] Set db.statement after create sqlcomment --- .../opentelemetry/instrumentation/sqlalchemy/engine.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index 172c1193f3..0f35e370bd 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -218,11 +218,6 @@ def _before_cur_exec( kind=trace.SpanKind.CLIENT, ) with trace.use_span(span, end_on_exit=False): - if span.is_recording(): - 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, @@ -241,6 +236,11 @@ def _before_cur_exec( } statement = _add_sql_comment(statement, **commenter_data) + if span.is_recording(): + 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) context._otel_span = span From ad8948a9b8b80115c02a4d997871d87070ee9303 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Tue, 29 Oct 2024 15:32:50 -0700 Subject: [PATCH 2/5] Add test --- .../tests/test_sqlcommenter.py | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py index f13c552bf4..e9026ca9b4 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py @@ -12,12 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +import re import pytest from sqlalchemy import create_engine from opentelemetry import context from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor +from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.test.test_base import TestBase @@ -56,6 +58,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("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( From b1fbeb7e376d6e42090f2602c1cf8b213b0b414c Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Tue, 29 Oct 2024 15:33:59 -0700 Subject: [PATCH 3/5] Changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7597e60641..e3a2bee8f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ 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-sqlalchemy` Add sqlcomment to `db.statement` attribute + ([#2937](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2937)) ### Fixed From e3ddea996242d2466e56cd78a061b945f810b288 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Mon, 4 Nov 2024 11:56:54 -0800 Subject: [PATCH 4/5] _add_sql_comment only if span.is_recording --- .../instrumentation/sqlalchemy/engine.py | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index 0f35e370bd..b64af796d1 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -218,25 +218,28 @@ def _before_cur_exec( kind=trace.SpanKind.CLIENT, ) with trace.use_span(span, end_on_exit=False): - 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) 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(): From 7b78383fee477ea60bde610661660a148a7d9955 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Wed, 13 Nov 2024 16:00:27 -0800 Subject: [PATCH 5/5] Fix test for sqlalchemy 2 --- .../tests/test_sqlcommenter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py index 921abdc600..8490721e3e 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlcommenter.py @@ -70,7 +70,7 @@ def test_sqlcommenter_enabled_matches_db_statement_attribute(self): commenter_options={"db_framework": False}, ) cnx = engine.connect() - cnx.execute("SELECT 1;").fetchall() + cnx.execute(text("SELECT 1;")).fetchall() query_log = self.caplog.records[-2].getMessage() self.assertRegex( query_log,