Add config to disable obfuscation in memcached queries#2725
Add config to disable obfuscation in memcached queries#2725pablomartinezbernardo merged 2 commits intomasterfrom
Conversation
BenchmarksBenchmark execution time: 2024-06-20 15:12:56 Comparing candidate commit 8e2b1b9 in PR branch Found 2 performance improvements and 1 performance regressions! Performance is the same for 175 metrics, 0 unstable metrics. scenario:EmptyFileBench/benchEmptyFileOverhead
scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
scenario:PDOBench/benchPDOBaseline-opcache
|
| CONFIG(STRING, DD_VERSION, "", .ini_change = ddtrace_alter_dd_version, \ | ||
| .env_config_fallback = ddtrace_conf_otel_resource_attributes_version) \ | ||
| CONFIG(STRING, DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP, DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP_DEFAULT) \ | ||
| CONFIG(BOOL, DD_TRACE_MEMCACHED_OBFUSCATION, "true") \ |
There was a problem hiding this comment.
Q: In the APMS ticket, it appears that Ruby and Python both choose to use DD_TRACE_MEMCACHED_COMMAND_ENABLED - Is there a reason for using a different name?
There was a problem hiding this comment.
Yes: Ruby and Python's behavior before those envs were implemented was to not include the command at all. The env includes the command, rather than unobfuscating it.
| $queryParams = dd_trace_env_config("DD_TRACE_MEMCACHED_OBFUSCATION") ? | ||
| Obfuscation::toObfuscatedString($args[0]) : $args[0]; | ||
| $span->meta['memcached.query'] = $command . ' ' . $queryParams; |
There was a problem hiding this comment.
My feeling is that since all the changes have the same structure, we could do a standard function for this. At least, if we ever have an issue, this would prevent us from having to look at five different places. This could be considered a nit :-)
pablomartinezbernardo
deleted the
pmartinez/memcached-disable-obfuscation
branch
2 participants
Description
Add config to disable obfuscation in memcached queries. Keeps old behavior by default. Added on both memcache and memcached. Tests for memached cover only one path, but I didn't want to duplicate the number of integration tests.
Reviewer checklist