Add Redis instrumentation query sanitization #1572
Conversation
Add a query sanitizer to the Redis instrumentation. This can be disabled with the `sanitize_query = False` config option. Given the query `SET key value`, the sanitized query becomes `SET ? ?`. Both the keys and values are sanitized, as both can contain PII data. The Redis queries are sanitized by default. This changes the default behavior of this instrumentation. Previously it reported unsanitized Redis queries. This was previously discussed in the previous implementation of this PR in PR open-telemetry#1571 Closes open-telemetry#1548
...tion/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-redis/tests/test_redis.py
Outdated
Show resolved
Hide resolved
|
this looks great, |
Changes suggested in open-telemetry#1572 (comment)
The Redis test that performs the tests with the default options, doesn't need to uninstrument and then instrument the instrumentor. This commit removes the unnecessary setup code. The setup code is already present at the top of the file.
|
I fixed the lint issue that failed the build previously. I couldn't get the linter to run locally first, but I confirmed it now passes locally. |
...entation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
Outdated
Show resolved
Hide resolved
|
I'm not sure why to change the default behavior, |
This was briefly discussed in the previous implementation of this, in PR #1571. I think sanitizing the query is a good default. It's better because users won't send any Personal Identifying Information by default, and need to explicitly enable full query reporting if they really need it.
Okay, sounds good. I'm just running into another issue where I can't run this test suite. I'm getting connection issues for something. I'll have a look. |
- Update the sanitizer to also account for a max `db.statement` attribute value length. No longer than 1000 characters. - Update the functional tests to assume the queries are sanitized by default. - Add new tests that test the behavior with sanitization turned off. Only for the tests in the first test class. I don't think it's needed to duplicate this test for the clustered and async setup combinations.
|
@shalevr I've updated the functional tests, added new tests to test |
Change the Redis functional tests so that they test the unsanitized query by default, and test the sanitized query results in the separate test functions. This is a partial revert of the previous commit 8d56c2f
|
@srikanthccv with the discussion about query sanitization by default ongoing in the spec repo, shall I change this PR to not sanitize the queries by default, but make the option available to sanitize query opt-in? |
|
Yes, please do. |
Update the Redis instrumentation library to not change the default behavior for the Redis instrumentation. This can be enabled at a later time when the spec discussion about this topic has concluded. open-telemetry/opentelemetry-specification#3104
|
@srikanthccv I've changed the |
Remove else statement.
|
@srikanthccv I pushed the fix for pylint issue.
|
...entation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
Outdated
Show resolved
Hide resolved
[ci skip] Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
Check the length of the args array and return an empty string if there are no args. That way it won't cause an IndexError if the args array is empty and it tries to fetch the first element, which should be the Redis command.
Description
Add a query sanitizer to the Redis instrumentation. This can be disabled
with the
sanitize_query = Falseconfig option.Given the query
SET key value, the sanitized query becomesSET ? ?.Both the keys and values are sanitized, as both can contain PII data.
The Redis queries are sanitized by default. This changes the default
behavior of this instrumentation. Previously it reported unsanitized
Redis queries.
This was previously discussed in the previous implementation of this PR
in PR #1571
Closes #1548
Type of change
Please delete options that are not relevant.
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
tox -e test-instrumentation-redisDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.