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.

Sign up for GitHub

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

Jump to bottom

Add cache_key_enabled option for ActiveSupport #4022

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Add cache_key_enabled option for ActiveSupport #4022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e8188fc
Add cache_key_enabled option for ActiveSupport
bouwkast Oct 23, 2024
e87ed63
Copy/paste tests
bouwkast Oct 23, 2024
1f31567
Use shorthand `configuration` access
bouwkast Oct 23, 2024
eb19f98
Limit tested behavior to just `cache_key` expects
bouwkast Oct 23, 2024
4197b1a
Wrap reads with it do
bouwkast Oct 23, 2024
1c823a0
Fix linting errors
bouwkast Oct 23, 2024
866c888
Revert "Use shorthand `configuration` access"
bouwkast Oct 23, 2024
45ef5e6
Reapply "Use shorthand `configuration` access"
bouwkast Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/datadog/tracing/contrib/active_support/cache/events/cache.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def on_start(span, event, _id, payload)

span.set_tag('EVENT', event)

set_cache_key(span, key, mapping[:multi_key])
set_cache_key(span, key, mapping[:multi_key]) if configuration[:cache_key_enabled]
rescue StandardError => e
Datadog.logger.error(e.message)
Datadog::Core::Telemetry::Logger.report(e)
Expand Down
4 changes: 3 additions & 1 deletion lib/datadog/tracing/contrib/active_support/cache/instrumentation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require_relative '../../../../core/utils'
require_relative '../../../metadata/ext'
require_relative '../ext'
require_relative 'event'

module Datadog
module Tracing
Expand Down Expand Up @@ -58,7 +59,8 @@ def trace(action, store, key: nil, multi_key: nil)
end

span.set_tag(Ext::TAG_CACHE_BACKEND, store) if store
set_cache_key(span, key, multi_key)

set_cache_key(span, key, multi_key) if configuration[:cache_key_enabled]

yield
end
Expand Down
5 changes: 5 additions & 0 deletions lib/datadog/tracing/contrib/active_support/configuration/settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ class Settings < Contrib::Configuration::Settings
)
end
end

option :cache_key_enabled do |o|
Copy link
Contributor

@delner delner Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I want to caution that any addition of configuration like this is an expansion of the public API, so in that sense, we're bound to this method, and other measures (in other integrations) should be consistent with this style.

Allowing this to be disabled is value-add in capability. But users may find it insufficient, and may also need a more sophisticated quantization feature that itself may require more configuration options.

If we stick with cache_key_enabled, and we wanted to add regex capability (for example), then we would likely have to do cache_key_exclude_pattern, cache_key_include_pattern, etc... this is OK, but idiomatically, it may be better to have a subgroup instead (e.g. cache_key.enabled, cache_key.include_pattern, cache_key.exclude_pattern.)

So the time to decide on this behavior is now. I would recommend nesting this setting in its own subgroup to produce the cache_key.enabled behavior, such that we have the option of idiomatically expanding the API later without introducing a breaking change down the line.

Copy link
Author

Choose a reason for hiding this comment

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

If we stick with cache_key_enabled, and we wanted to add regex capability (for example), then we would likely have to do cache_key_exclude_pattern, cache_key_include_pattern, etc... this is OK, but idiomatically, it may be better to have a subgroup instead (e.g. cache_key.enabled, cache_key.include_pattern, cache_key.exclude_pattern.)

I like this idea, it seems much more effective and better long-term so I'll refactor to that.

o.type :bool
o.default true
end
end
end
end
Expand Down
155 changes: 155 additions & 0 deletions spec/datadog/tracing/contrib/rails/cache_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,21 @@
expect(set.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION))
.to eq('cache')
end

context 'when cache_key_enabled is false' do
before do
Datadog.configuration.tracing[:active_support][:cache_key_enabled] = false
end

it do
expect(read).to eq(50)

expect(spans).to have(2).items
get = spans
expect(get.name).to eq('rails.cache')
expect(get.get_tag('rails.cache.key')).to be_nil
end
end
end

describe '#read_multi' do
Expand Down Expand Up @@ -149,6 +164,20 @@
.to eq('cache')
end
end

context 'when cache_key_enabled is false' do
before do
Datadog.configuration.tracing[:active_support][:cache_key_enabled] = false
end

it do
expect(read_multi).to eq(Hash[multi_keys.zip([51, 52, 53])])
expect(spans).to have(1 + multi_keys.size).items
get = spans[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

⚪ Code Quality Violation

Suggested change
get = spans[0]
get = spans.first
Improve readability with first (...read more)

This rule encourages the use of first and last methods over array indexing to access the first and last elements of an array, respectively. The primary reason behind this rule is to improve code readability. Using first and last makes it immediately clear that you are accessing the first or last element of the array, which might not be immediately obvious with array indexing, especially for developers who are new to Ruby.

The use of these methods also helps to make your code more idiomatic, which is a crucial aspect of writing effective Ruby code. Idiomatic code is easier to read, understand, and maintain. It also tends to be more efficient, as idioms often reflect patterns that are optimized for the language.

To adhere to this rule, replace the use of array indexing with first or last methods when you want to access the first and last elements of an array. For instance, instead of arr[0] use arr.first and instead of arr[-1] use arr.last. However, note that this rule should be applied only when reading values. When modifying the first or last elements, array indexing should still be used. For example, arr[0] = 'new_value' and arr[-1] = 'new_value'.

View in Datadog  Leave us feedback  Documentation

expect(get.name).to eq('rails.cache')
expect(get.get_tag('rails.cache.keys')).to be_nil
end
end
end

describe '#write' do
Expand Down Expand Up @@ -191,6 +220,20 @@
expect(span.get_tag('rails.cache.key')).to eq('custom-key/x/y/User:3')
end
end

context 'when cache_key_enabled is false' do
before do
Datadog.configuration.tracing[:active_support][:cache_key_enabled] = false
end

let(:key) { ['custom-key', %w[x y], user] }
let(:user) { double('User', cache_key: 'User:3') }

it 'does not expand key using ActiveSupport when cache_key_enabled false' do
write
expect(span.get_tag('rails.cache.key')).to be_nil
end
end
end

describe '#write_multi' do
Expand Down Expand Up @@ -240,6 +283,27 @@
expect(span.get_tag('rails.cache.keys')).to eq('["custom-key/x/y/User:3"]')
end
end

context 'when cache_key_enabled is false' do
before do
Datadog.configuration.tracing[:active_support][:cache_key_enabled] = false
end

it do
write_multi
expect(span.name).to eq('rails.cache')
expect(span.type).to eq('cache')
expect(span.resource).to eq('MSET')
expect(span.service).to eq('rails-cache')
expect(span.get_tag('rails.cache.backend')).to eq('file_store')
expect(span.get_tag('rails.cache.keys')).to be_nil

expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT))
.to eq('active_support')
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION))
.to eq('cache')
end
end
end

context 'when the method is not defined' do
Expand Down Expand Up @@ -278,6 +342,27 @@
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION))
.to eq('cache')
end

context 'when cache_key_enabled is false' do
before do
Datadog.configuration.tracing[:active_support][:cache_key_enabled] = false
end

it do
delete
expect(span.name).to eq('rails.cache')
expect(span.type).to eq('cache')
expect(span.resource).to eq('DELETE')
expect(span.service).to eq('rails-cache')
expect(span.get_tag('rails.cache.backend')).to eq('file_store')
expect(span.get_tag('rails.cache.key')).to be_nil

expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT))
.to eq('active_support')
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION))
.to eq('cache')
end
end
end

describe '#fetch' do
Expand Down Expand Up @@ -306,6 +391,32 @@
.to eq('cache')
end
end

context 'when cache_key_enabled is false' do
before do
Datadog.configuration.tracing[:active_support][:cache_key_enabled] = false
end

subject(:fetch) { cache.fetch('exception') { raise 'oops' } }

it do
expect { fetch }.to raise_error(StandardError)

expect(span.name).to eq('rails.cache')
expect(span.type).to eq('cache')
expect(span.resource).to eq('GET')
expect(span.service).to eq('rails-cache')
expect(span.get_tag('rails.cache.backend')).to eq('file_store')
expect(span.get_tag('rails.cache.key')).to be_nil
expect(span.get_tag('error.type')).to eq('RuntimeError')
expect(span.get_tag('error.message')).to eq('oops')

expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT))
.to eq('active_support')
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION))
.to eq('cache')
end
end
end

describe '#fetch_multi' do
Expand Down Expand Up @@ -340,6 +451,30 @@
.to eq('cache')
end
end

context 'with exception and when cache_key_enabled is false' do
before do
Datadog.configuration.tracing[:active_support][:cache_key_enabled] = false
end
subject(:fetch_multi) { cache.fetch_multi('exception', 'another', 'one') { raise 'oops' } }

it do
expect { fetch_multi }.to raise_error(StandardError)
expect(span.name).to eq('rails.cache')
expect(span.type).to eq('cache')
expect(span.resource).to eq('MGET')
expect(span.service).to eq('rails-cache')
expect(span.get_tag('rails.cache.backend')).to eq('file_store')
expect(span.get_tag('rails.cache.keys')).to be_nil
expect(span.get_tag('error.type')).to eq('RuntimeError')
expect(span.get_tag('error.message')).to eq('oops')

expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT))
.to eq('active_support')
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION))
.to eq('cache')
end
end
end

context 'when the method is not defined' do
Expand Down Expand Up @@ -378,4 +513,24 @@
.to eq('cache')
end
end

context 'with very large cache key and when cache_key_enabled is false' do
before do
Datadog.configuration.tracing[:active_support][:cache_key_enabled] = false
end
it 'truncates key too large' do
max_key_size = Datadog::Tracing::Contrib::ActiveSupport::Ext::QUANTIZE_CACHE_MAX_KEY_SIZE
large_key = ''.ljust(max_key_size * 2, SecureRandom.hex)
cache.write(large_key, 'foobar')

expect(large_key.size).to be > max_key_size
expect(span.name).to eq('rails.cache')
expect(span.get_tag('rails.cache.key')).to be_nil

expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT))
.to eq('active_support')
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION))
.to eq('cache')
end
end
end
Loading