DBAPI: Add param to control capturing of db.statement.parameters by stschenk · Pull Request #1247 · open-telemetry/opentelemetry-python

stschenk · 2020-10-15T21:40:32Z

Description

Switched DBAPI so that it will not capture parameterized query parameters. Capturing this would be a problem in production environments since they will contain sensitive information such as session tokens and hashed passwords.

Have added a parameter to the instrumentation constructor to allow for this feature to be turned on.

I will mark this as a breaking change since I am removing the db.statement.parameter span attribute and something might be depending on it.

Fixes # (issue)
https://github.com/open-telemetry/opentelemetry-python/issues/1215

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Unit Tests

Checklist:

[ x] Followed the style guidelines of this project
[ x] Changelogs have been updated
[ x] Unit tests have been added
Documentation has been updated

NathanielRN · 2020-10-22T04:54:58Z

Hello! I have a PR to move some files you have in this PR to the Contrib repo, please let me know if this gets merged before the PR in the Contrib repo. Please see https://github.com/open-telemetry/opentelemetry-python-contrib/pulls/87

instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py

lzchen · 2020-10-22T14:41:34Z

instrumentation/opentelemetry-instrumentation-aiopg/tests/test_aiopg_integration.py

+            connection_attributes,
+            capture_parameters=True,
        )
        mock_connection = async_call(


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

lzchen · 2020-10-22T14:45:30Z

instrumentation/opentelemetry-instrumentation-aiopg/tests/test_aiopg_integration.py

        }
        db_integration = AiopgIntegration(
-            self.tracer, "testcomponent", "testtype", connection_attributes
+            self.tracer,


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.

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?

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

lzchen · 2020-10-22T14:46:02Z

...tion/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py

        connection_attributes=None,
        version: str = "",
        tracer_provider: typing.Optional[TracerProvider] = None,
+        capture_parameters: bool = False,


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

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!

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()`.

lzchen

Some comments.

stschenk · 2020-10-29T19:30:45Z

@lzchen / @owais - I have put together a different more conservative and backward compatible PR that will allow for the user to turn off the collection of db.statement.parameters through the use of an env var: #1306. If this one seems fine, I will go ahead and abandon this PR.

* feat: adding json over http for collector exporter * feat: updating readme and adding headers options in config for json over http * chore: reviews and few small cleanups * chore: aligning type for headers * chore: fixing doc * chore: unifying types for headers * chore: reviews * chore: adding validation for headers, and making the types correct this time * chore: linting * chore: linting * chore: fixes after merge * chore: reviews * chore: merge branch 'master' into collector_json

lzchen · 2020-11-02T01:28:16Z

@stschenk
How come we can't go with the same mechanism that AsyncPGInstrumentor is using? With the env vars, now we have two different ways of setting the same feature, which is confusing for the user.

stschenk · 2020-11-02T22:00:55Z

@lzchen - Good question... I misinterpreted your previous comment around needing to have the ability for the user to configure the capture of db.statement.parameters as meaning there should be an env prop.

I have changed this PR a bit by doing the following:

Added the ability to configure the feature through the trace_integration method. Making the capture off by default for DBAPI
Left the other DB instrumentations as they are... They will continue to capture db.statement.parameters by default.

lzchen · 2020-11-03T15:34:40Z

...tion/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py

+            capture_parameters=capture_parameters,
        )
        return db_integration.wrapped_connection(wrapped, args, kwargs)



I believe instrument_connection needs to have this parameter as well.

makes sense.

lzchen · 2020-11-03T15:35:03Z

...tion/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py

    connection_attributes: typing.Dict = None,
    version: str = "",
    tracer_provider: typing.Optional[TracerProvider] = None,
+    capture_parameters: bool = True,


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

lzchen · 2020-11-05T18:48:41Z

@stschenk
We are moving all of our instrumentation code into the contrib repo. Do you mind opening up this PR there and I can close this one?

stschenk · 2020-11-06T00:10:30Z

Sure, will do it first thing tomorrow

stschenk · 2020-11-06T17:42:55Z

This has been moved to open-telemetry/opentelemetry-python-contrib#156.

I will go ahead and close this PR

add param to control capturing of db.statement.parameters

0031952

stschenk requested review from a team, lzchen and owais and removed request for a team October 15, 2020 21:40

stschenk and others added 2 commits October 15, 2020 14:43

update change log

0d838bd

Merge branch 'master' into dbapi-skip-params-by-default

b8268c5

owais approved these changes Oct 22, 2020

View reviewed changes

lzchen reviewed Oct 22, 2020

View reviewed changes

instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py Show resolved Hide resolved

lzchen reviewed Oct 22, 2020

View reviewed changes

instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py Show resolved Hide resolved

lzchen reviewed Oct 22, 2020

View reviewed changes

lzchen requested changes Oct 22, 2020

View reviewed changes

stschenk and others added 3 commits November 2, 2020 12:59

Merge branch 'master' into dbapi-skip-params-by-default

097eaf2

Add parameter to disable collection of db.statement.parameters for DBAPI

cc1902e

Set DBAPI default capture of db.statement.parameters to False

4a6293a

lzchen reviewed Nov 3, 2020

View reviewed changes

stschenk and others added 3 commits November 3, 2020 08:56

Set DBApiIntegration capture of db.statement.parameters to False

7373a20

Merge branch 'master' into dbapi-skip-params-by-default

32d34bb

Merge branch 'master' into dbapi-skip-params-by-default

684ab0b

codeboten added the instrumentation Related to the instrumentation of third party libraries or frameworks label Nov 5, 2020

stschenk closed this Nov 6, 2020

Conversation

stschenk commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

NathanielRN commented Oct 22, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stschenk Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen left a comment

Choose a reason for hiding this comment

Uh oh!

stschenk commented Oct 29, 2020

Uh oh!

lzchen commented Nov 2, 2020

Uh oh!

stschenk commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen commented Nov 5, 2020

Uh oh!

stschenk commented Nov 6, 2020

Uh oh!

stschenk commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stschenk commented Oct 15, 2020 •

edited

Loading

stschenk Oct 26, 2020 •

edited

Loading

lzchen Oct 28, 2020 •

edited

Loading

stschenk commented Nov 2, 2020 •

edited

Loading