From 54b8cb4a8b15a4fa6294117c166089a592d4259c Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Wed, 9 Feb 2022 20:48:16 +0530 Subject: [PATCH 1/6] feat(instrumentation-dbapi): add experimental sql commenter capability --- .../instrumentation/dbapi/__init__.py | 49 ++++++++++++++++--- .../tests/test_dbapi_integration.py | 17 +++++++ .../opentelemetry/instrumentation/utils.py | 36 ++++++++++++++ 3 files changed, 94 insertions(+), 8 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py index 762d2904d2..d4067a5107 100644 --- a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py @@ -45,11 +45,11 @@ from opentelemetry import trace as trace_api from opentelemetry.instrumentation.dbapi.version import __version__ -from opentelemetry.instrumentation.utils import unwrap +from opentelemetry.instrumentation.utils import unwrap, _generate_sql_comment from opentelemetry.semconv.trace import SpanAttributes -from opentelemetry.trace import SpanKind, TracerProvider, get_tracer +from opentelemetry.trace import Span, SpanKind, TracerProvider, get_tracer -logger = logging.getLogger(__name__) +_logger = logging.getLogger(__name__) def trace_integration( @@ -59,6 +59,7 @@ def trace_integration( connection_attributes: typing.Dict = None, tracer_provider: typing.Optional[TracerProvider] = None, capture_parameters: bool = False, + enable_commenter: bool = False, db_api_integration_factory=None, ): """Integrate with DB API library. @@ -84,6 +85,7 @@ def trace_integration( version=__version__, tracer_provider=tracer_provider, capture_parameters=capture_parameters, + enable_commenter=enable_commenter, db_api_integration_factory=db_api_integration_factory, ) @@ -97,6 +99,7 @@ def wrap_connect( version: str = "", tracer_provider: typing.Optional[TracerProvider] = None, capture_parameters: bool = False, + enable_commenter: bool = False, db_api_integration_factory=None, ): """Integrate with DB API library. @@ -132,6 +135,7 @@ def wrap_connect_( version=version, tracer_provider=tracer_provider, capture_parameters=capture_parameters, + enable_commenter=enable_commenter, ) return db_integration.wrapped_connection(wrapped, args, kwargs) @@ -140,7 +144,7 @@ def wrap_connect_( connect_module, connect_method_name, wrap_connect_ ) except Exception as ex: # pylint: disable=broad-except - logger.warning("Failed to integrate with DB API. %s", str(ex)) + _logger.warning("Failed to integrate with DB API. %s", str(ex)) def unwrap_connect( @@ -163,7 +167,8 @@ def instrument_connection( connection_attributes: typing.Dict = None, version: str = "", tracer_provider: typing.Optional[TracerProvider] = None, - capture_parameters=False, + capture_parameters: bool = False, + enable_commenter: bool = False, ): """Enable instrumentation in a database connection. @@ -180,7 +185,7 @@ def instrument_connection( An instrumented connection. """ if isinstance(connection, wrapt.ObjectProxy): - logger.warning("Connection already instrumented") + _logger.warning("Connection already instrumented") return connection db_integration = DatabaseApiIntegration( @@ -190,6 +195,7 @@ def instrument_connection( version=version, tracer_provider=tracer_provider, capture_parameters=capture_parameters, + enable_commenter=enable_commenter ) db_integration.get_connection_attributes(connection) return get_traced_connection_proxy(connection, db_integration) @@ -207,7 +213,7 @@ def uninstrument_connection(connection): if isinstance(connection, wrapt.ObjectProxy): return connection.__wrapped__ - logger.warning("Connection is not instrumented") + _logger.warning("Connection is not instrumented") return connection @@ -220,6 +226,7 @@ def __init__( version: str = "", tracer_provider: typing.Optional[TracerProvider] = None, capture_parameters: bool = False, + enable_commenter: bool = False, ): self.connection_attributes = connection_attributes if self.connection_attributes is None: @@ -237,6 +244,7 @@ def __init__( tracer_provider=tracer_provider, ) self.capture_parameters = capture_parameters + self.enable_commenter = enable_commenter self.database_system = database_system self.connection_props = {} self.span_attributes = {} @@ -313,8 +321,9 @@ def __exit__(self, *args, **kwargs): class CursorTracer: - def __init__(self, db_api_integration: DatabaseApiIntegration): + def __init__(self, db_api_integration: DatabaseApiIntegration) -> None: self._db_api_integration = db_api_integration + self._commenter_enabled = self._db_api_integration.enable_commenter def _populate_span( self, @@ -355,6 +364,20 @@ def get_statement(self, cursor, args): # pylint: disable=no-self-use return statement.decode("utf8", "replace") return statement + @staticmethod + def _generate_comment(span: Span) -> str: + span_context = span.get_span_context() + meta = {} + if span_context.is_valid: + meta.update({ + 'trace_id': span_context.trace_id, + 'span_id': span_context.span_id, + 'trace_flags': span_context.trace_flags, + 'trace_state': span_context.trace_state.to_header() + }) + ## TODO(schekuri): revisit to enrich with info such as route, db_driver etc... + return _generate_sql_comment(**meta) + def traced_execution( self, cursor, @@ -374,6 +397,16 @@ def traced_execution( name, kind=SpanKind.CLIENT ) as span: self._populate_span(span, cursor, *args) + if args and self._commenter_enabled: + try: + comment = self._generate_comment(span) + if isinstance(args[0], bytes): + comment = comment.encode("utf8") + args_list = list(args) + args_list[0] += comment + args = tuple(args_list) + except Exception as exc: + _logger.warning("Exception while generating sql comment: %s", exc) return query_method(*args, **kwargs) diff --git a/instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py b/instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py index df7f1870f3..377eafbb6a 100644 --- a/instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py +++ b/instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py @@ -228,6 +228,21 @@ def test_executemany(self): span.attributes[SpanAttributes.DB_STATEMENT], "Test query" ) + def test_executemany_comment(self): + db_integration = dbapi.DatabaseApiIntegration( + "testname", "testcomponent", enable_commenter=True + ) + mock_connection = db_integration.wrapped_connection( + mock_connect, {}, {} + ) + cursor = mock_connection.cursor() + cursor.executemany("Test query") + spans_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans_list), 1) + span = spans_list[0] + comment = dbapi.CursorTracer._generate_comment(span) + self.assertIn(comment, cursor.query) + def test_callproc(self): db_integration = dbapi.DatabaseApiIntegration( "testname", "testcomponent" @@ -317,6 +332,8 @@ def execute(self, query, params=None, throw_exception=False): def executemany(self, query, params=None, throw_exception=False): if throw_exception: raise Exception("Test Exception") + self.query = query + self.params = params # pylint: disable=unused-argument, no-self-use def callproc(self, query, params=None, throw_exception=False): diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py index e4b0b03ba6..62a60b072d 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import urllib.parse from typing import Dict, Sequence from wrapt import ObjectProxy @@ -115,3 +116,38 @@ def _start_internal_or_server_span( attributes=attributes, ) return span, token + + +_KEY_VALUE_DELIMITER = "," + + +def _generate_sql_comment(**meta): + """ + Return a SQL comment with comma delimited key=value pairs created from + **meta kwargs. + """ + if not meta: # No entries added. + return "" + + # Sort the keywords to ensure that caching works and that testing is + # deterministic. It eases visual inspection as well. + return ( + " /*" + + _KEY_VALUE_DELIMITER.join( + "{}={!r}".format(_url_quote(key), _url_quote(value)) + for key, value in sorted(meta.items()) + if value is not None + ) + + "*/" + ) + + +def _url_quote(s): + if not isinstance(s, (str, bytes)): + return s + quoted = urllib.parse.quote(s) + # Since SQL uses '%' as a keyword, '%' is a by-product of url quoting + # e.g. foo,bar --> foo%2Cbar + # thus in our quoting, we need to escape it too to finally give + # foo,bar --> foo%%2Cbar + return quoted.replace("%", "%%") From c800b21502ecaf66e7b1e1811202db0baf80efa9 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Thu, 10 Feb 2022 03:24:19 +0530 Subject: [PATCH 2/6] Update instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py Co-authored-by: Diego Hurtado --- .../src/opentelemetry/instrumentation/dbapi/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py index d4067a5107..fc4af67588 100644 --- a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py @@ -406,7 +406,7 @@ def traced_execution( args_list[0] += comment args = tuple(args_list) except Exception as exc: - _logger.warning("Exception while generating sql comment: %s", exc) + _logger.exception("Exception while generating sql comment: %s", exc) return query_method(*args, **kwargs) From 575cb1757bd0ee5ecf902e669f2b74e736647c28 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Thu, 10 Feb 2022 18:53:19 +0530 Subject: [PATCH 3/6] Fix lint --- .../instrumentation/dbapi/__init__.py | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py index d4067a5107..a0811dc903 100644 --- a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py @@ -45,7 +45,7 @@ from opentelemetry import trace as trace_api from opentelemetry.instrumentation.dbapi.version import __version__ -from opentelemetry.instrumentation.utils import unwrap, _generate_sql_comment +from opentelemetry.instrumentation.utils import _generate_sql_comment, unwrap from opentelemetry.semconv.trace import SpanAttributes from opentelemetry.trace import Span, SpanKind, TracerProvider, get_tracer @@ -195,7 +195,7 @@ def instrument_connection( version=version, tracer_provider=tracer_provider, capture_parameters=capture_parameters, - enable_commenter=enable_commenter + enable_commenter=enable_commenter, ) db_integration.get_connection_attributes(connection) return get_traced_connection_proxy(connection, db_integration) @@ -369,12 +369,14 @@ def _generate_comment(span: Span) -> str: span_context = span.get_span_context() meta = {} if span_context.is_valid: - meta.update({ - 'trace_id': span_context.trace_id, - 'span_id': span_context.span_id, - 'trace_flags': span_context.trace_flags, - 'trace_state': span_context.trace_state.to_header() - }) + meta.update( + { + "trace_id": span_context.trace_id, + "span_id": span_context.span_id, + "trace_flags": span_context.trace_flags, + "trace_state": span_context.trace_state.to_header(), + } + ) ## TODO(schekuri): revisit to enrich with info such as route, db_driver etc... return _generate_sql_comment(**meta) @@ -406,7 +408,9 @@ def traced_execution( args_list[0] += comment args = tuple(args_list) except Exception as exc: - _logger.warning("Exception while generating sql comment: %s", exc) + _logger.warning( + "Exception while generating sql comment: %s", exc + ) return query_method(*args, **kwargs) From 83ddcbbd9964fbf127d398daf4a28f2f3f2a02ad Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Thu, 10 Feb 2022 18:55:45 +0530 Subject: [PATCH 4/6] Add CHANGELOG entry --- CHANGELOG.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fad0c2fc7..8c755d76f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,14 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.9.1-0.28b1...HEAD) -- `opentelemetry-instrumentation-wsgi` WSGI: Conditionally create SERVER spans - ([#903](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/903)) +### Added + +- `opentelemetry-instrumentation-dbapi` add experimental sql commenter capability + ([#908](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/908)) ### Fixed - `opentelemetry-instrumentation-logging` retrieves service name defensively. ([#890](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/890)) +- `opentelemetry-instrumentation-wsgi` WSGI: Conditionally create SERVER spans + ([#903](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/903)) + ## [1.9.1-0.28b1](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.9.1-0.28b1) - 2022-01-29 From 14d67c52a0a7758414ed1b3c75465884c8654c13 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Thu, 10 Feb 2022 19:07:37 +0530 Subject: [PATCH 5/6] Fix lint --- .../src/opentelemetry/instrumentation/dbapi/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py index 246c43c2e6..537aaaf32e 100644 --- a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py @@ -377,7 +377,7 @@ def _generate_comment(span: Span) -> str: "trace_state": span_context.trace_state.to_header(), } ) - ## TODO(schekuri): revisit to enrich with info such as route, db_driver etc... + # TODO(schekuri): revisit to enrich with info such as route, db_driver etc... return _generate_sql_comment(**meta) def traced_execution( From addfb3ad01e2117ceebc0e691da7b9f5c057cc1f Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Thu, 10 Feb 2022 19:35:36 +0530 Subject: [PATCH 6/6] Fix lint again --- .../src/opentelemetry/instrumentation/dbapi/__init__.py | 2 +- .../tests/test_dbapi_integration.py | 4 ++++ .../src/opentelemetry/instrumentation/utils.py | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py index 537aaaf32e..1645a8a48e 100644 --- a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py @@ -407,7 +407,7 @@ def traced_execution( args_list = list(args) args_list[0] += comment args = tuple(args_list) - except Exception as exc: + except Exception as exc: # pylint: disable=broad-except _logger.exception( "Exception while generating sql comment: %s", exc ) diff --git a/instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py b/instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py index 377eafbb6a..0302824db4 100644 --- a/instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py +++ b/instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py @@ -323,6 +323,10 @@ def cursor(self): class MockCursor: + def __init__(self) -> None: + self.query = "" + self.params = None + # pylint: disable=unused-argument, no-self-use def execute(self, query, params=None, throw_exception=False): if throw_exception: diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py index 62a60b072d..b53b4f2308 100644 --- a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py +++ b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py @@ -142,7 +142,7 @@ def _generate_sql_comment(**meta): ) -def _url_quote(s): +def _url_quote(s): # pylint: disable=invalid-name if not isinstance(s, (str, bytes)): return s quoted = urllib.parse.quote(s)