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.

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

auto convert :auto to :prepend/:chain in config #1930

Merged
merged 7 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## dev

- **Feature: The agent configuration will now reflect whether method prepending or chaining was used per instrumentation**
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be up for changing "method prepending or chaining" to "module prepending or method chaining" in the title and the body copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly! Addressed with bf3b5ee


For `:'instrumentation.*'` configuration parameters that are set to :auto (the default), the agent will automatically determine whether to use method prepending or chaining. The agent will now update its in-memory configuration to give each relevant parameter a value of either :prepend or :chain so that the result of the determination can be introspected. This is intended to help 3rd party libraries that wish to further enhance the agent's instrumentation capabilities by prepending or chaining additional logic. Environment variable, YAML file, and server-side configuration based values are not impacted. [PR#1930](https://github.com/newrelic/newrelic-ruby-agent/pull/1930)

- **Feature: Deprecate memcached and memcache-client instrumentation**

Instrumentation for the memcached and memcache-client libraries is deprecated and will be removed during the next major release.
Expand Down
13 changes: 13 additions & 0 deletions lib/new_relic/agent/configuration/manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ module NewRelic
module Agent
module Configuration
class Manager
DEPENDENCY_DETECTION_VALUES = %i[prepend chain].freeze

# Defining these explicitly saves object allocations that we incur
# if we use Forwardable and def_delegators.
def [](key)
Expand Down Expand Up @@ -357,7 +359,18 @@ def reset_to_defaults
reset_cache
end

# reset the configuration hash, but do not replace previously auto
# determined dependency detection values with nil or 'auto'
def reset_cache
return new_cache unless defined?(@cache) && @cache

preserved = @cache.select { |_k, v| DEPENDENCY_DETECTION_VALUES.include?(v) }
new_cache
preserved.each { |k, v| @cache[k] = v unless @cache[k] && @cache[k] != 'auto' }
@cache
end

def new_cache
@cache = Hash.new { |hash, key| hash[key] = self.fetch(key) }
end

Expand Down
11 changes: 10 additions & 1 deletion lib/new_relic/dependency_detection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,15 @@ def fetch_config_value(key)
return valid_value
end

# update any :auto config value to be either :prepend or :chain after auto
# determination has selected one of those to use
def update_config_value(use_prepend)
if config_key && auto_configured?
NewRelic::Agent.config.instance_variable_get(:@cache)[config_key] = use_prepend ? :prepend : :chain
end
use_prepend
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this saying we are always returning true or false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Precisely. We hook into this existing method that returns a boolean, so here we make sure we still always return a boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it I see what is happening now. Thank you :)

end

def config_value
return AUTO_CONFIG_VALUE unless config_key

Expand All @@ -202,7 +211,7 @@ def conflicts_with_prepend(&block)
end

def use_prepend?
prepend_configured? || (auto_configured? && !prepend_conflicts?)
update_config_value(prepend_configured? || (auto_configured? && !prepend_conflicts?))
end

def prepend_conflicts?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ class DefaultSourceTest < Minitest::Test
def setup
@default_source = DefaultSource.new
@defaults = ::NewRelic::Agent::Configuration::DEFAULTS
NewRelic::Agent.config.send(:new_cache)
end

def test_default_values_have_a_public_setting
Expand Down
20 changes: 20 additions & 0 deletions test/new_relic/agent/configuration/manager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,26 @@ def test_apply_transformations_reraises_errors
end
end

def test_auto_determined_values_stay_cached
name = :knockbreck_manse

dd = DependencyDetection.defer do
named(name)
executes { use_prepend? }
end

key = :"instrumentation.#{name}"
with_config(key => 'auto') do
DependencyDetection.detect!

@manager.replace_or_add_config(ServerSource.new({}))

assert_equal :prepend, @manager.instance_variable_get(:@cache)[key]
end
end

private

def assert_parsed_labels(expected)
result = @manager.parsed_labels

Expand Down
36 changes: 36 additions & 0 deletions test/new_relic/dependency_detection_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -380,4 +380,40 @@ def test_log_and_instrument_uses_supportability_name_when_provided
assert_log_contains(log, /#{supportability_name}/)
assert_log_contains(log, /#{method}/)
end

# when an instrumentation value of :auto (the default) is present, the agent
# automatically determines whether to use :prepend or :chain. after the
# determination is made, the config value should be updated to be either
# :prepend or :chain so that the determined value can be introspected.
def test_prepend_or_chain_based_values_have_auto_converted_into_one_of_those
name = :bandu_gumbo

dd = DependencyDetection.defer do
named(name)
executes { use_prepend? }
end

with_config(:"instrumentation.#{name}" => 'auto') do
DependencyDetection.detect!

assert_equal :prepend, dd.config_value
end
end

# confirm that :auto becomes :chain when :chain is automatically determined
def test_auto_is_replaced_by_chain_when_chain_is_used
name = :blank_and_jones

dd = DependencyDetection.defer do
named(name)
conflicts_with_prepend { true }
executes { use_prepend? }
end

with_config(:"instrumentation.#{name}" => 'auto') do
DependencyDetection.detect!

assert_equal :chain, dd.config_value
end
end
end