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

Correctly set rails.cache.backend span tag for multiple stores #3060

Merged
merged 9 commits into from
Aug 25, 2023
Merged

Correctly set rails.cache.backend span tag for multiple stores #3060

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0fdc187
wip
marcotc Aug 17, 2023
e9275e2
wip
marcotc Aug 17, 2023
42350a4
wip
marcotc Aug 18, 2023
37a2e22
Refresh comment
marcotc Aug 21, 2023
8dad19a
Even more
marcotc Aug 21, 2023
583fbcb
More comments
marcotc Aug 21, 2023
7b58122
Merge branch 'master' into as-cache
marcotc Aug 24, 2023
49887e5
Apply all suggestions
marcotc Aug 24, 2023
6315065
Document why the trivial method
marcotc Aug 25, 2023
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
166 changes: 48 additions & 118 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,122 +1,52 @@
source 'https://rubygems.org'

gemspec

# Development dependencies
gem 'addressable', '~> 2.4.0' # locking transitive dependency of webmock
if RUBY_VERSION < '2.3'
gem 'appraisal', '~> 2.2.0'
else
gem 'appraisal', '~> 2.4.0'
end
gem 'benchmark-ips', '~> 2.8'
gem 'benchmark-memory', '< 0.2' # V0.2 only works with 2.5+
gem 'builder'
gem 'climate_control', '~> 0.2.0'
# Leave it open as we also have it as an integration and want Appraisal to control the version under test.
if RUBY_VERSION >= '2.2.0'
gem 'concurrent-ruby'
else
gem 'concurrent-ruby', '< 1.1.10'
end
gem 'extlz4', '~> 0.3', '>= 0.3.3' if RUBY_PLATFORM != 'java' # Used to test lz4 compression done by libdatadog
gem 'json', '< 2.6' if RUBY_VERSION < '2.3.0'
gem 'json-schema', '< 3' # V3 only works with 2.5+
if RUBY_VERSION >= '2.3.0'
gem 'memory_profiler', '~> 0.9'
else
gem 'memory_profiler', '= 0.9.12'
end

gem 'os', '~> 1.1'
gem 'pimpmychangelog', '>= 0.1.2'
gem 'pry'
if RUBY_PLATFORM != 'java'
# There's a few incompatibilities between pry/pry-byebug on older Rubies
# There's also a few temproary incompatibilities with newer rubies
gem 'pry-byebug' if RUBY_VERSION >= '2.6.0' && RUBY_ENGINE != 'truffleruby' && RUBY_VERSION < '3.2.0'
gem 'pry-nav' if RUBY_VERSION < '2.6.0'
gem 'pry-stack_explorer' if RUBY_VERSION >= '2.5.0'
else
gem 'pry-debugger-jruby'
end
if RUBY_VERSION >= '2.2.0'
gem 'rake', '>= 10.5'
else
gem 'rake', '~> 12.3'
end
gem 'rake-compiler', '~> 1.1', '>= 1.1.1' # To compile native extensions
gem 'redcarpet', '~> 3.4' if RUBY_PLATFORM != 'java'
gem 'rspec', '~> 3.12'
gem 'rspec-collection_matchers', '~> 1.1'
gem 'rspec-wait', '~> 0'
if RUBY_VERSION >= '2.3.0'
gem 'rspec_junit_formatter', '>= 0.5.1'
else
# Newer versions do not support Ruby < 2.3.
gem 'rspec_junit_formatter', '<= 0.4.1'
end
gem 'rspec_n', '~> 1.3' if RUBY_VERSION >= '2.4.0'
gem 'ruby-prof', '~> 1.4' if RUBY_PLATFORM != 'java' && RUBY_VERSION >= '2.4.0'
if RUBY_VERSION >= '2.5.0'
# Merging branch coverage results does not work for old, unsupported rubies.
# We have a fix up for review, https://github.com/simplecov-ruby/simplecov/pull/972,
# but given it only affects unsupported version of Ruby, it might not get merged.
gem 'simplecov', git: 'https://github.com/DataDog/simplecov', ref: '3bb6b7ee58bf4b1954ca205f50dd44d6f41c57db'
gem 'simplecov-cobertura', '~> 2.1.0' # Used by codecov
else
# Compatible with older rubies. This version still produces compatible output
# with a newer version when the reports are merged.
gem 'simplecov', '~> 0.17'
end
gem 'simplecov-html', '~> 0.10.2' if RUBY_VERSION < '2.4.0'
gem 'warning', '~> 1' if RUBY_VERSION >= '2.5.0'
gem 'webmock', '>= 3.10.0'
if RUBY_VERSION < '2.3.0'
gem 'rexml', '< 3.2.5' # Pinned due to https://github.com/ruby/rexml/issues/69
end
gem 'webrick', '>= 1.7.0' if RUBY_VERSION >= '3.0.0' # No longer bundled by default since Ruby 3.0
if RUBY_VERSION >= '2.3.0'
gem 'yard', '~> 0.9'
else
gem 'yard', ['~> 0.9', '< 0.9.27'] # yard 0.9.27 starts pulling webrick as a gem dependency
end

if RUBY_VERSION >= '2.6.0'
gem 'rubocop', '~> 1.34.0', require: false
gem 'rubocop-packaging', '~> 0.5.2', require: false
gem 'rubocop-performance', '~> 1.9', require: false
gem 'rubocop-rspec', '~> 2.2', require: false
end

# Optional extensions
# TODO: Move this to Appraisals?
# dogstatsd v5, but lower than 5.2, has possible memory leak with ddtrace.
# @see https://github.com/DataDog/dogstatsd-ruby/issues/182
gem 'dogstatsd-ruby', '>= 3.3.0', '!= 5.0.0', '!= 5.0.1', '!= 5.1.0'
gem 'opentracing', '>= 0.4.1'

# Profiler optional dependencies
# NOTE: We're excluding versions 3.7.0 and 3.7.1 for the reasons documented in #1424.
# Since most of our customers won't have BUNDLE_FORCE_RUBY_PLATFORM=true, it's not something we want to add
# to our CI, so we just shortcut and exclude specific versions that were affecting our CI.
if RUBY_PLATFORM != 'java'
if RUBY_VERSION >= '2.7.0' # Bundler 1.x fails to find that versions >= 3.8.0 are not compatible because of binary gems
gem 'google-protobuf', ['~> 3.0', '!= 3.7.0', '!= 3.7.1']
elsif RUBY_VERSION >= '2.3.0'
gem 'google-protobuf', ['~> 3.0', '!= 3.7.0', '!= 3.7.1', '< 3.19.2']
else
gem 'google-protobuf', ['~> 3.0', '!= 3.7.0', '!= 3.7.1', '< 3.8.0']
end
end
# This file was generated by Appraisal

Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate on why the need for this change? Should we extract this change into a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, @GustavoCaso, because it's a draft PR, I make a bunch of mess before getting it ready.
Mucking about with the Gemfile makes my testing easier locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also push my changes at the end of day in case my laptop explodes overnight.

Copy link
Member

Choose a reason for hiding this comment

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

I also push my changes at the end of day in case my laptop explodes overnight.

Good call

source "https://rubygems.org"

gem "addressable", "~> 2.4.0"
gem "appraisal", "~> 2.4.0"
gem "benchmark-ips", "~> 2.8"
gem "benchmark-memory", "< 0.2"
gem "builder"
gem "climate_control", "~> 0.2.0"
gem "concurrent-ruby"
gem "extlz4", "~> 0.3", ">= 0.3.3"
gem "json-schema", "< 3"
gem "memory_profiler", "~> 0.9"
gem "os", "~> 1.1"
gem "pimpmychangelog", ">= 0.1.2"
gem "pry"
gem "pry-stack_explorer"
gem "rake", ">= 10.5"
gem "rake-compiler", "~> 1.1", ">= 1.1.1"
gem "redcarpet", "~> 3.4"
gem "rspec", "~> 3.12"
gem "rspec-collection_matchers", "~> 1.1"
gem "rspec-wait", "~> 0"
gem "rspec_junit_formatter", ">= 0.5.1"
gem "rspec_n", "~> 1.3"
gem "ruby-prof", "~> 1.4"
gem "simplecov", git: "https://github.com/DataDog/simplecov", ref: "3bb6b7ee58bf4b1954ca205f50dd44d6f41c57db"
gem "simplecov-cobertura", "~> 2.1.0"
gem "warning", "~> 1"
gem "webmock", ">= 3.10.0"
gem "webrick", ">= 1.7.0"
gem "yard", "~> 0.9"
gem "rubocop", "~> 1.34.0", require: false
gem "rubocop-packaging", "~> 0.5.2", require: false
gem "rubocop-performance", "~> 1.9", require: false
gem "rubocop-rspec", "~> 2.2", require: false
gem "dogstatsd-ruby", ">= 3.3.0", "!= 5.0.0", "!= 5.0.1", "!= 5.1.0"
gem "opentracing", ">= 0.4.1"
gem "google-protobuf", ["~> 3.0", "!= 3.7.0", "!= 3.7.1"]
gem "rails", "~> 6.1.0"
gem "pg", ">= 1.1", platform: :ruby
gem "activerecord-jdbcpostgresql-adapter", platform: :jruby
gem "sprockets", "< 4"
gem "lograge", "~> 0.11"
gem "net-smtp"

group :check do
if RUBY_VERSION >= '2.7.0' && RUBY_PLATFORM != 'java'
gem 'rbs', '~> 3.1.0', require: false
gem 'steep', '~> 1.4.0', require: false
end

end

gem 'docile', '~> 1.3.5' if RUBY_VERSION < '2.5'
gem 'ffi', '~> 1.12.2' if RUBY_VERSION < '2.3'
gem 'msgpack', '~> 1.3.3' if RUBY_VERSION < '2.4'
gemspec
45 changes: 33 additions & 12 deletions 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 @@ -56,11 +56,7 @@ def finish_trace_cache(payload)
return unless span && !span.finished?

begin
# discard parameters from the cache_store configuration
if defined?(::Rails)
store, = *Array.wrap(::Rails.configuration.cache_store).flatten
span.set_tag(Ext::TAG_CACHE_BACKEND, store)
end
span.set_tag(Ext::TAG_CACHE_BACKEND, payload[:store]) if payload[:store]

normalized_key = ::ActiveSupport::Cache.expand_cache_key(payload.fetch(:key))
cache_key = Core::Utils.truncate(normalized_key, Ext::QUANTIZE_CACHE_MAX_KEY_SIZE)
Expand Down Expand Up @@ -112,7 +108,26 @@ def read(*args, &block)
payload = {
action: Ext::RESOURCE_CACHE_GET,
key: args[0],
tracing_context: {}
tracing_context: {},
# The name of the store is never saved anywhere.
# ActiveSupport looks up stores by converting a symbol into a 'require' path,
# then "camelizing" it for a `const_get` call:
# ```
# require "active_support/cache/#{store}"
# ActiveSupport::Cache.const_get(store.to_s.camelize)
# ```
# @see https://github.com/rails/rails/blob/261975dbef77731d2c76f907f1076c5132ebc0e4/activesupport/lib/active_support/cache.rb#L139-L149
#
# As `self` is the store object, we can reverse engineer
# the original symbol by converting the class name to snake case:
# e.g. ActiveSupport::Cache::RedisStore -> active_support/cache/redis_store
# In this case `redis_store` is the store name.
#
# Because there's no API retrieve only the class name
# (only `RedisStore`, and not `ActiveSupport::Cache::RedisStore`)
# the easiest way to retrieve the store symbol is to convert the fully qualified
# name using Rails-provided methods, then extracting the last part.
store: self.class.name.underscore.match(/active_support\/cache\/(.*)/)[1]
Copy link
Member

@GustavoCaso GustavoCaso Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Would using regex for each command executed on the cache cause performance issues for the customer's application?

We could memoize the result on an instance variable

def store_name
  return @store_name if defined?(@store_name)
  @store_name = self.class.name.underscore.match(/active_support\/cache\/(.*)/)[1]
  @store_name
end

Also, we always are getting the last portion of the match. Could we use split(''/'').last rather than a regex?

def store_name
  return @store_name if defined?(@store_name)
  @store_name = self.class.name.underscore.split('/').last
  @store_name
end

Copy link
Member

Choose a reason for hiding this comment

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

Should we add a comment to explain that underscore is from Rails::ActiveSupport and that this codes works because is executed within the context of Rails

Copy link
Member Author

Choose a reason for hiding this comment

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

Caching is great, I made those changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding "split+last" vs regex, I'm keeping the regex because it's the version that more closely mirrors ActiveSupport's logic (require "active_support/cache/#{store}").

And because we are only doing it once, as we are now caching, the performance impact will be almost nil.

}

begin
Expand All @@ -135,7 +150,8 @@ def read_multi(*keys, &block)
payload = {
action: Ext::RESOURCE_CACHE_MGET,
keys: keys,
tracing_context: {}
tracing_context: {},
store: self.class.name.underscore.match(/active_support\/cache\/(.*)/)[1]
}

begin
Expand All @@ -158,7 +174,8 @@ def fetch(*args, &block)
payload = {
action: Ext::RESOURCE_CACHE_GET,
key: args[0],
tracing_context: {}
tracing_context: {},
store: self.class.name.underscore.match(/active_support\/cache\/(.*)/)[1]
}

begin
Expand All @@ -183,7 +200,8 @@ def fetch_multi(*args, &block)
payload = {
action: Ext::RESOURCE_CACHE_MGET,
keys: keys,
tracing_context: {}
tracing_context: {},
store: self.class.name.underscore.match(/active_support\/cache\/(.*)/)[1]
}

begin
Expand All @@ -206,7 +224,8 @@ def write(*args, &block)
payload = {
action: Ext::RESOURCE_CACHE_SET,
key: args[0],
tracing_context: {}
tracing_context: {},
store: self.class.name.underscore.match(/active_support\/cache\/(.*)/)[1]
}

begin
Expand All @@ -229,7 +248,8 @@ def write_multi(hash, options = nil)
payload = {
action: Ext::RESOURCE_CACHE_MSET,
keys: hash.keys,
tracing_context: {}
tracing_context: {},
store: self.class.name.underscore.match(/active_support\/cache\/(.*)/)[1]
}

begin
Expand All @@ -252,7 +272,8 @@ def delete(*args, &block)
payload = {
action: Ext::RESOURCE_CACHE_DELETE,
key: args[0],
tracing_context: {}
tracing_context: {},
store: self.class.name.underscore.match(/active_support\/cache\/(.*)/)[1]
}

begin
Expand Down