feat(pymemcached): add DD_TRACE_MEMCACHED_COMMAND_ENABLED (#7092)

This adds environment variable `DD_TRACE_MEMCACHED_COMMAND_ENABLED` to
configure collection of memcached commands from pymemcache integration.
By default, this is disabled. For those that require it and for whom the
contents of keys are not an issue, they can retain the existing behavior
by setting `DD_TRACE_MEMCACHED_COMMAND_ENABLED=true`.

## Motivation

According to the [memcached
protocol](https://github.com/memcached/memcached/blob/master/doc/protocol.txt),
a client sends messages to the server with commands along with several
arguments, one of which can be the key which uniquely identifies the
data stored and retrieved by a client. These keys can have sensitive
information that uniquely identifies a client's data.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

---------

Co-authored-by: Emmett Butler <[email protected]>

Author: majorgreys

ddtrace/contrib/pymemcache/client.py

@@ -1,3 +1,4 @@
+import os
 import sys
 from typing import Iterable
 
@@ -28,12 +29,21 @@
 from ...internal.compat import reraise
 from ...internal.logger import get_logger
 from ...internal.schema import schematize_cache_operation
+from ...internal.utils.formats import asbool
 from ...pin import Pin
 
 
 log = get_logger(__name__)
 
 
+config._add(
+    "pymemcache",
+    {
+        "command_enabled": asbool(os.getenv("DD_TRACE_MEMCACHED_COMMAND_ENABLED", default=False)),
+    },
+)
+
+
 # keep a reference to the original unpatched clients
 _Client = Client
 _HashClient = HashClient
@@ -317,9 +327,10 @@ def _trace(func, p, method_name, *args, **kwargs):
         # with the application
         try:
             span.set_tags(p.tags)
-            vals = _get_query_string(args)
-            query = "{}{}{}".format(method_name, " " if vals else "", vals)
-            span.set_tag_str(memcachedx.QUERY, query)
+            if config.pymemcache.command_enabled:
+                vals = _get_query_string(args)
+                query = "{}{}{}".format(method_name, " " if vals else "", vals)
+                span.set_tag_str(memcachedx.QUERY, query)
         except Exception:
             log.debug("Error setting relevant pymemcache tags")
 

releasenotes/notes/pymemcached-command-b0eaed2bfa1dcdc8.yaml

@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    pymemcache: add ``DD_TRACE_MEMCACHED_COMMAND_ENABLED`` environment variable for configuring the collection of memcached commands. This feature is disabled by default.
+upgrade:
+  - |
+    pymemcache: The ``memcached.query`` span tag will no longer be set by the integration. This command includes keys that can potentially contain sensitive information. If you require this span tag, you can retain the existing functionality by setting ``DD_TRACE_MEMCACHED_COMMAND_ENABLED=true``. This span tag can be redacted using ``DD_APM_REPLACE_TAGS`` in your Agent configuration.

tests/contrib/pymemcache/test_client.py

@@ -17,6 +17,7 @@
 from ddtrace.internal.schema import DEFAULT_SPAN_SERVICE_NAME
 from tests.utils import DummyTracer
 from tests.utils import TracerTestCase
+from tests.utils import override_config
 
 from .test_client_mixin import PYMEMCACHE_VERSION
 from .test_client_mixin import PymemcacheClientTestCaseMixin
@@ -29,6 +30,13 @@
 _Client = pymemcache.client.base.Client
 
 
+# Manually configure pymemcached to collect command
+@pytest.fixture(scope="module", autouse=True)
+def command_enabled():
+    with override_config("pymemcache", dict(command_enabled=True)):
+        yield
+
+
 class PymemcacheClientTestCase(PymemcacheClientTestCaseMixin):
     """Tests for a patched pymemcache.client.base.Client."""
 

tests/contrib/pymemcache/test_client_defaults.py

@@ -0,0 +1,36 @@
+# 3p
+import pymemcache
+import pytest
+
+# project
+from ddtrace import Pin
+from ddtrace.contrib.pymemcache.patch import patch
+from ddtrace.contrib.pymemcache.patch import unpatch
+from tests.utils import override_config
+
+from .test_client_mixin import TEST_HOST
+from .test_client_mixin import TEST_PORT
+from .utils import MockSocket
+
+
+@pytest.fixture()
+def client(tracer):
+    try:
+        patch()
+        Pin.override(pymemcache, tracer=tracer)
+        with override_config("pymemcache", dict(command_enabled=False)):
+            client = pymemcache.client.base.Client((TEST_HOST, TEST_PORT))
+            yield client
+    finally:
+        unpatch()
+
+
+def test_query_default(client, tracer):
+    client.sock = MockSocket([b"STORED\r\n"])
+    result = client.set(b"key", b"value", noreply=False)
+    assert result is True
+
+    traces = tracer.pop_traces()
+    assert 1 == len(traces)
+    assert 1 == len(traces[0])
+    assert traces[0][0].get_tag("memcached.query") is None

tests/contrib/pymemcache/test_client_mixin.py

@@ -1,5 +1,6 @@
 # 3p
 import pymemcache
+import pytest
 
 # project
 from ddtrace import Pin
@@ -10,6 +11,7 @@
 from ddtrace.ext import net
 from tests.utils import DummyTracer
 from tests.utils import TracerTestCase
+from tests.utils import override_config
 
 from .utils import MockSocket
 
@@ -22,6 +24,13 @@
 PYMEMCACHE_VERSION = tuple(int(_) for _ in pymemcache.__version__.split("."))
 
 
+# Manually configure pymemcached to collect command
+@pytest.fixture(scope="module", autouse=True)
+def command_enabled():
+    with override_config("pymemcache", dict(command_enabled=True)):
+        yield
+
+
 class PymemcacheClientTestCaseMixin(TracerTestCase):
     """Tests for a patched pymemcache.client.base.Client."""