Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DBAPI: Add param to control capturing of db.statement.parameters #1247

Closed
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- Stop capturing query parameters by default
([#1247](https://github.com/open-telemetry/opentelemetry-python/pull/1247))
## Version 0.13b0

Released 2020-09-17
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ def trace_integration(
database_type: str = "",
connection_attributes: typing.Dict = None,
tracer_provider: typing.Optional[TracerProvider] = None,
capture_parameters: bool = False,
):
"""Integrate with DB API library.
https://www.python.org/dev/peps/pep-0249/
Expand All @@ -76,6 +77,7 @@ def trace_integration(
user in Connection object.
tracer_provider: The :class:`opentelemetry.trace.TracerProvider` to
use. If ommited the current configured one is used.
capture_parameters: Configure if db.statement.parameters should be captured.
"""
wrap_connect(
__name__,
Expand All @@ -86,6 +88,7 @@ def trace_integration(
connection_attributes,
version=__version__,
tracer_provider=tracer_provider,
capture_parameters=capture_parameters,
)


Expand All @@ -98,6 +101,7 @@ def wrap_connect(
connection_attributes: typing.Dict = None,
version: str = "",
tracer_provider: typing.Optional[TracerProvider] = None,
capture_parameters: bool = True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default should be False for all instrumentations that use dbapi.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

):
"""Integrate with DB API library.
https://www.python.org/dev/peps/pep-0249/
Expand All @@ -111,6 +115,8 @@ def wrap_connect(
database_type: The Database type. For any SQL database, "sql".
connection_attributes: Attribute names for database, port, host and
user in Connection object.
capture_parameters: Configure if db.statement.parameters should be captured.

"""

# pylint: disable=unused-argument
Expand All @@ -127,6 +133,7 @@ def wrap_connect_(
connection_attributes=connection_attributes,
version=version,
tracer_provider=tracer_provider,
capture_parameters=capture_parameters,
)
return db_integration.wrapped_connection(wrapped, args, kwargs)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe instrument_connection needs to have this parameter as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense.

Expand Down Expand Up @@ -211,6 +218,7 @@ def __init__(
connection_attributes=None,
version: str = "",
tracer_provider: typing.Optional[TracerProvider] = None,
capture_parameters: bool = True,
):
self.connection_attributes = connection_attributes
if self.connection_attributes is None:
Expand All @@ -223,6 +231,7 @@ def __init__(
self._name = name
self._version = version
self._tracer_provider = tracer_provider
self.capture_parameters = capture_parameters
self.database_component = database_component
self.database_type = database_type
self.connection_props = {}
Expand Down Expand Up @@ -327,7 +336,7 @@ def _populate_span(
) in self._db_api_integration.span_attributes.items():
span.set_attribute(attribute_key, attribute_value)

if len(args) > 1:
if self._db_api_integration.capture_parameters and len(args) > 1:
span.set_attribute("db.statement.parameters", str(args[1]))

def traced_execution(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,49 @@ def test_span_succeeded(self):
span.status.status_code, trace_api.status.StatusCode.UNSET,
)
stschenk marked this conversation as resolved.
Show resolved Hide resolved

def test_span_succeeded_without_capture_of_statement_parameters(self):
connection_props = {
"database": "testdatabase",
"server_host": "testhost",
"server_port": 123,
"user": "testuser",
}
connection_attributes = {
"database": "database",
"port": "server_port",
"host": "server_host",
"user": "user",
}
db_integration = dbapi.DatabaseApiIntegration(
self.tracer,
"testcomponent",
"testtype",
connection_attributes,
capture_parameters=False,
)
mock_connection = db_integration.wrapped_connection(
mock_connect, {}, connection_props
)
cursor = mock_connection.cursor()
cursor.execute("Test query", ("param1Value", False))
spans_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans_list), 1)
span = spans_list[0]
self.assertEqual(span.name, "testcomponent.testdatabase")
self.assertIs(span.kind, trace_api.SpanKind.CLIENT)

stschenk marked this conversation as resolved.
Show resolved Hide resolved
self.assertEqual(span.attributes["component"], "testcomponent")
self.assertEqual(span.attributes["db.type"], "testtype")
self.assertEqual(span.attributes["db.instance"], "testdatabase")
self.assertEqual(span.attributes["db.statement"], "Test query")
self.assertFalse("db.statement.parameters" in span.attributes)
self.assertEqual(span.attributes["db.user"], "testuser")
self.assertEqual(span.attributes["net.peer.name"], "testhost")
self.assertEqual(span.attributes["net.peer.port"], 123)
self.assertIs(
span.status.status_code, trace_api.status.StatusCode.UNSET,
)

def test_span_not_recording(self):
connection_props = {
"database": "testdatabase",
Expand Down