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.

Sign up for GitHub

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
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0031952
add param to control capturing of db.statement.parameters
stschenk Oct 15, 2020
0d838bd
update change log
stschenk Oct 15, 2020
b8268c5
Merge branch 'master' into dbapi-skip-params-by-default
stschenk Oct 19, 2020
097eaf2
Merge branch 'master' into dbapi-skip-params-by-default
stschenk Nov 2, 2020
cc1902e
Add parameter to disable collection of db.statement.parameters for DBAPI
stschenk Nov 2, 2020
4a6293a
Set DBAPI default capture of db.statement.parameters to False
stschenk Nov 2, 2020
7373a20
Set DBApiIntegration capture of db.statement.parameters to False
stschenk Nov 3, 2020
32d34bb
Merge branch 'master' into dbapi-skip-params-by-default
stschenk Nov 3, 2020
684ab0b
Merge branch 'master' into dbapi-skip-params-by-default
stschenk Nov 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion instrumentation/opentelemetry-instrumentation-aiopg/tests/test_aiopg_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,11 @@ def test_span_succeeded(self):
"user": "user",
}
db_integration = AiopgIntegration(
self.tracer, "testcomponent", "testtype", connection_attributes
self.tracer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't there a lot of other instrumentations that use dbapi as well (mysql, psycopg2, etc)? I think tests would need to be added for those as well because this is a breaking change.

Copy link
Contributor Author

@stschenk stschenk Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would argue that since the the other instrumentations already are not testing for "db.statement.parameters", we would not need to modify the test to do so now.

Also db.statement.parameters is not even part of the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md#call-level-attributes, why is it being collected at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would argue that since the the other instrumentations already are not testing for "db.statement.parameters", we would not need to modify the test to do so now.

To ensure completeness of the feature, tests should be added for each instrumentation. The original authors should have tested for this, but since your change will effect the behavior of these instrumentations, it would be good to have them there as well.

why is it being collected at all?

That is a good question. Perhaps this should be the discussion topic to address first? @codeboten

"testcomponent",
"testtype",
connection_attributes,
capture_parameters=True,
)
mock_connection = async_call(
Copy link
Contributor

Choose a reason for hiding this comment

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

Explicitly check the span attribute existence in this test as well.

db_integration.wrapped_connection(
Expand Down
2 changes: 2 additions & 0 deletions instrumentation/opentelemetry-instrumentation-dbapi/CHANGELOG.md
View file
Open in desktop
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
15 changes: 13 additions & 2 deletions ...n/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py
View file
Open in desktop
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 = False,
):
"""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 @@ -159,6 +166,7 @@ def instrument_connection(
connection_attributes: typing.Dict = None,
version: str = "",
tracer_provider: typing.Optional[TracerProvider] = None,
capture_parameters=False,
):
"""Enable instrumentation in a database connection.

Expand All @@ -170,7 +178,7 @@ def instrument_connection(
database_type: The Database type. For any SQL database, "sql".
connection_attributes: Attribute names for database, port, host and
user in a connection object.

capture_parameters: Configure if db.statement.parameters should be captured.
Returns:
An instrumented connection.
"""
Expand All @@ -181,6 +189,7 @@ def instrument_connection(
connection_attributes=connection_attributes,
version=version,
tracer_provider=tracer_provider,
capture_parameters=capture_parameters,
)
db_integration.get_connection_attributes(connection)
return get_traced_connection_proxy(connection, db_integration)
Expand Down Expand Up @@ -211,6 +220,7 @@ def __init__(
connection_attributes=None,
version: str = "",
tracer_provider: typing.Optional[TracerProvider] = None,
capture_parameters: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

How can this be configured by the user? As well as the instrumentations that depend on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I did not make this configurable for the user. I initially though I was going to but then came across this PR that removes the capturing of "db.statement.parameters" for AsyncPGInstrumentor
#854 Since the approach was accepted I thought it would be fine for DBAPI too.

Would it be better to not have the constructor param and instead introduce a user settable property?

Thanks!

Copy link
Contributor

@lzchen lzchen Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Having users be able to configure it is fine. The reason why the AsyncPGInstumentor works is because the configuration property is exposed to the users (because they can call AsyncPGInstrumentor(capture_parameters =True) constructor to instrument). However for this case it is in DatabaseApiIntegration, which users do not interact with (for dbapi, they use trace_integration(...) or for something like mysql, they use MySQLInstrumentor()`.

):
self.connection_attributes = connection_attributes
if self.connection_attributes is None:
Expand All @@ -223,6 +233,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 +338,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
43 changes: 43 additions & 0 deletions instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,49 @@ def test_span_succeeded(self):
self.assertEqual(span.name, "testcomponent.testdatabase")
self.assertIs(span.kind, trace_api.SpanKind.CLIENT)

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,
)
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=True,
)
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")
Expand Down