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

pymemcache query sanitization #1576

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

avzis
Copy link
Contributor

@avzis avzis commented Jan 12, 2023

Description

Added a query sanitizer to the Pymemcache instrumentation.
Default False, can be enabled with sanitize_query = True

This will allow users to sanitize the query.
PymemcacheInstrumentor().instrument(sanitize_query=True)

Fixes #1546

How Has This Been Tested?

  • Original tests remained un-changed, and they will validate the original queries.
  • Added a test that validates a sanitized query.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@avzis avzis marked this pull request as ready for review January 12, 2023 16:28
@avzis avzis requested a review from a team January 12, 2023 16:28
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Until now the default behavior was to present the original query,
and now the default behavior is to present a sanitized query.

Is this generally the thing that users prefer?

@avzis
Copy link
Contributor Author

avzis commented Jan 15, 2023

Until now the default behavior was to present the original query,
and now the default behavior is to present a sanitized query.

Is this generally the thing that users prefer?

actually this is a good question, i opened an issue in the spec repository in order to clarify this:
open-telemetry/opentelemetry-specification#3104

avzis and others added 3 commits January 22, 2023 18:26
# Conflicts:
#	CHANGELOG.md
#	instrumentation/opentelemetry-instrumentation-pymemcache/tests/test_pymemcache.py
@@ -44,6 +44,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#1592](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1592))
- `opentelemetry-instrumentation-django` Allow explicit `excluded_urls` configuration through `instrument()`
([#1618](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1618))
- Added query sanitization for pymemcache
([#1576](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1576))
Copy link
Member

Choose a reason for hiding this comment

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

Move this to Unreleased section

with tracer.start_as_current_span(
cmd, kind=SpanKind.CLIENT, attributes={}
) as span:
try:
if span.is_recording():
if not args:
if not args or sanitize_query:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the approach we follow elsewhere. The values get replaced with something like ? or similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement sensitive data sanitization for pymemcache instrumentation
3 participants