From e417000f9150355ad6ecf4b38f2ca760c9a73283 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Thu, 28 Sep 2023 17:07:54 -0700 Subject: [PATCH] Disable memcached command tag by default --- docs/GettingStarted.md | 1 + .../contrib/dalli/configuration/settings.rb | 6 ++++++ lib/datadog/tracing/contrib/dalli/ext.rb | 7 +++++++ .../tracing/contrib/dalli/instrumentation.rb | 6 ++++-- sig/datadog/tracing/contrib/dalli/ext.rbs | 2 ++ .../contrib/dalli/instrumentation_spec.rb | 21 ++++++++++++------- 6 files changed, 33 insertions(+), 10 deletions(-) diff --git a/docs/GettingStarted.md b/docs/GettingStarted.md index 4c2a9abf24c..3e0dfcb07f0 100644 --- a/docs/GettingStarted.md +++ b/docs/GettingStarted.md @@ -659,6 +659,7 @@ client.set('abc', 123) | Key | Env Var | Description | Default | |----------------|-------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-------------| +| `command_enabled` | `DD_TRACE_MEMCACHED_COMMAND_ENABLED` | Collect commands as the `memcached.command` tag. Command `keys` can potentially contain sensitive information. | `false` | | `service_name` | `DD_TRACE_DALLI_SERVICE_NAME` | Name of application running the `dalli` instrumentation. May be overridden by `global_default_service_name`. [See *Additional Configuration* for more details](#additional-configuration) | `memcached` | | `peer_service` | `DD_TRACE_DALLI_PEER_SERVICE` | Name of external service the application connects to | `nil` | diff --git a/lib/datadog/tracing/contrib/dalli/configuration/settings.rb b/lib/datadog/tracing/contrib/dalli/configuration/settings.rb index 77105d967c0..572b1972f44 100644 --- a/lib/datadog/tracing/contrib/dalli/configuration/settings.rb +++ b/lib/datadog/tracing/contrib/dalli/configuration/settings.rb @@ -42,6 +42,12 @@ class Settings < Contrib::Configuration::Settings o.type :string, nilable: true o.env Ext::ENV_PEER_SERVICE end + + option :command_enabled do |o| + o.type :bool + o.env Ext::ENV_COMMAND_ENABLED + o.default false + end end end end diff --git a/lib/datadog/tracing/contrib/dalli/ext.rb b/lib/datadog/tracing/contrib/dalli/ext.rb index 983b47a28bb..ecf51455f5c 100644 --- a/lib/datadog/tracing/contrib/dalli/ext.rb +++ b/lib/datadog/tracing/contrib/dalli/ext.rb @@ -7,9 +7,16 @@ module Dalli # Dalli integration constants # @public_api Changing resource names, tag names, or environment variables creates breaking changes. module Ext + # DEV: Even though this is the dalli integration, all spans are named `memcached.*`. + # DEV: This happens because such spans have special treatment in backend, with memcached-specific handling. + # DEV: If add support for the `memcached` gem (not popular as of 2023), we'll have issues with span naming + # DEV: conflicts. ENV_ENABLED = 'DD_TRACE_DALLI_ENABLED' ENV_ANALYTICS_ENABLED = 'DD_TRACE_DALLI_ANALYTICS_ENABLED' ENV_ANALYTICS_SAMPLE_RATE = 'DD_TRACE_DALLI_ANALYTICS_SAMPLE_RATE' + # DEV: This is named `*_MEMCACHED_*` because the spans it refer to are `memcached.*` and this variable + # DEV: is a global flag that affects all memcached spans generated by any Datadog product. + ENV_COMMAND_ENABLED = 'DD_TRACE_MEMCACHED_COMMAND_ENABLED' ENV_SERVICE_NAME = 'DD_TRACE_DALLI_SERVICE_NAME' ENV_PEER_SERVICE = 'DD_TRACE_DALLI_PEER_SERVICE' diff --git a/lib/datadog/tracing/contrib/dalli/instrumentation.rb b/lib/datadog/tracing/contrib/dalli/instrumentation.rb index 692053b41e7..332ddbc4d1c 100644 --- a/lib/datadog/tracing/contrib/dalli/instrumentation.rb +++ b/lib/datadog/tracing/contrib/dalli/instrumentation.rb @@ -50,8 +50,10 @@ def request(op, *args) span.set_tag(Contrib::Ext::DB::TAG_SYSTEM, Ext::TAG_SYSTEM) - cmd = Quantize.format_command(op, args) - span.set_tag(Ext::TAG_COMMAND, cmd) + if datadog_configuration[:command_enabled] + cmd = Quantize.format_command(op, args) + span.set_tag(Ext::TAG_COMMAND, cmd) + end Contrib::SpanAttributeSchema.set_peer_service!(span, Ext::PEER_SERVICE_SOURCES) super diff --git a/sig/datadog/tracing/contrib/dalli/ext.rbs b/sig/datadog/tracing/contrib/dalli/ext.rbs index 045cfc4139d..f96392a4ca5 100644 --- a/sig/datadog/tracing/contrib/dalli/ext.rbs +++ b/sig/datadog/tracing/contrib/dalli/ext.rbs @@ -3,6 +3,8 @@ module Datadog module Contrib module Dalli module Ext + ENV_COMMAND_ENABLED: ::String + ENV_ENABLED: "DD_TRACE_DALLI_ENABLED" ENV_ANALYTICS_ENABLED: "DD_TRACE_DALLI_ANALYTICS_ENABLED" diff --git a/spec/datadog/tracing/contrib/dalli/instrumentation_spec.rb b/spec/datadog/tracing/contrib/dalli/instrumentation_spec.rb index 868cf84d2f2..013f5ffc712 100644 --- a/spec/datadog/tracing/contrib/dalli/instrumentation_spec.rb +++ b/spec/datadog/tracing/contrib/dalli/instrumentation_spec.rb @@ -70,7 +70,7 @@ expect(span.name).to eq('memcached.command') expect(span.span_type).to eq('memcached') expect(span.resource).to eq('SET') - expect(span.get_tag('memcached.command')).to eq('set abc 123 0 0') + expect(span).to_not have_tag('memcached.command') expect(span.get_tag('out.host')).to eq(test_host) expect(span.get_tag('out.port')).to eq(test_port.to_f) expect(span.get_tag('db.system')).to eq('memcached') @@ -79,6 +79,11 @@ expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION)).to eq('command') end + context 'with command tag captured enabled' do + let(:configuration_options) { { command_enabled: true } } + it { expect(span.get_tag('memcached.command')).to eq('set abc 123 0 0') } + end + it_behaves_like 'a peer service span' do let(:peer_service_val) { ENV.fetch('TEST_MEMCACHED_HOST', '127.0.0.1') } let(:peer_service_source) { 'peer.hostname' } @@ -87,12 +92,7 @@ describe 'when multiplexed configuration is provided' do let(:service_name) { 'multiplex-service' } - - before do - Datadog.configure do |c| - c.tracing.instrument :dalli, describes: "#{test_host}:#{test_port}", service_name: service_name - end - end + let(:configuration_options) { { describes: "#{test_host}:#{test_port}", service_name: service_name } } context 'and #set is called' do before do @@ -106,7 +106,7 @@ expect(span.name).to eq('memcached.command') expect(span.span_type).to eq('memcached') expect(span.resource).to eq('SET') - expect(span.get_tag('memcached.command')).to eq('set abc 123 0 0') + expect(span).to_not have_tag('memcached.command') expect(span.get_tag('out.host')).to eq(test_host) expect(span.get_tag('out.port')).to eq(test_port.to_f) @@ -116,6 +116,11 @@ expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION)).to eq('command') end + context 'with command tag captured enabled' do + let(:configuration_options) { super().merge(command_enabled: true) } + it { expect(span.get_tag('memcached.command')).to eq('set abc 123 0 0') } + end + it_behaves_like 'a peer service span' do let(:peer_service_val) { ENV.fetch('TEST_MEMCACHED_HOST', '127.0.0.1') } let(:peer_service_source) { 'peer.hostname' }