From e631993057bfdf440656459fdf74dccd9a00ad52 Mon Sep 17 00:00:00 2001 From: fallwith Date: Mon, 30 Sep 2024 17:17:03 -0700 Subject: [PATCH 01/15] type coercion for configuration parameters An initial pass at reworking the processing of configuration parameters to address #2852. - nils to produce errors when user-supplied for params that don't support them - introduce a common shared type coercion layer for all config values so that things like environment variable and YAML values are both consistently coerced - boolean coercion is no longer special cased and handled by the coercion layer - use `#call` instead of `instance_eval` on procs (benchmark supported) --- .../agent/configuration/default_source.rb | 88 +---------- .../agent/configuration/environment_source.rb | 44 ++---- lib/new_relic/agent/configuration/manager.rb | 140 ++++++++++++------ 3 files changed, 113 insertions(+), 159 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 246af67829..d656410cbe 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -35,15 +35,6 @@ def self.===(o) end class DefaultSource - BOOLEAN_MAP = { - 'true' => true, - 'yes' => true, - 'on' => true, - 'false' => false, - 'no' => false, - 'off' => false - }.freeze - attr_reader :defaults extend Forwardable @@ -73,12 +64,6 @@ def self.allowlist_for(key) value_from_defaults(key, :allowlist) end - def self.boolean_for(key, value) - string_value = (value.respond_to?(:call) ? value.call : value).to_s - - BOOLEAN_MAP.fetch(string_value, nil) - end - def self.default_for(key) value_from_defaults(key, :default) end @@ -227,53 +212,14 @@ def self.api_host end end - def self.convert_to_regexp_list(raw_value) - value_list = convert_to_list(raw_value) - value_list.map do |value| - /#{value}/ - end - end - - def self.convert_to_list(value) - case value - when String - value.split(/\s*,\s*/) - when Array - value - else - raise ArgumentError.new("Config value '#{value}' couldn't be turned into a list.") - end - end - - def self.convert_to_hash(value) - return value if value.is_a?(Hash) - - if value.is_a?(String) - return value.split(',').each_with_object({}) do |item, hash| - key, value = item.split('=') - hash[key] = value - end - end - - raise ArgumentError.new( - "Config value '#{value}' of " \ - "class #{value.class} couldn't be turned into a Hash." - ) - end - - SEMICOLON = ';'.freeze - def self.convert_to_list_on_semicolon(value) - case value - when Array then value - when String then value.split(SEMICOLON) - else NewRelic::EMPTY_ARRAY - end + def self.convert_to_regexp_list(string_array) + string_array.map { |value| /#{value}/ } end - def self.convert_to_constant_list(raw_value) - return NewRelic::EMPTY_ARRAY if raw_value.nil? || raw_value.empty? + def self.convert_to_constant_list(string_array) + return string_array if string_array.empty? - constants = convert_to_list(raw_value).map! do |class_name| + constants = string_array.map! do |class_name| const = ::NewRelic::LanguageSupport.constantize(class_name) NewRelic::Agent.logger.warn("Ignoring invalid constant '#{class_name}' in #{raw_value}") unless const const @@ -361,7 +307,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => String, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list_on_semicolon), + :transform => proc { |str| str.split(';') }, :description => 'Specify the [application name](/docs/apm/new-relic-apm/installation-configuration/name-your-application) used to aggregate data in the New Relic UI. To report data to [multiple apps at the same time](/docs/apm/new-relic-apm/installation-configuration/using-multiple-names-app), specify a list of names separated by a semicolon `;`. For example, `MyApp` or `MyStagingApp;Instance1`.' }, :license_key => { @@ -868,7 +814,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :default => {}, :public => true, :type => Hash, - :transform => DefaultSource.method(:convert_to_hash), :allowed_from_server => false, :description => 'A hash with key/value pairs to add as custom attributes to all log events forwarded to New Relic. If sending using an environment variable, the value must be formatted like: "key1=value1,key2=value2"' }, @@ -914,7 +859,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to exclude from all destinations. Allows `*` as wildcard at end.' }, :'attributes.include' => { @@ -922,7 +866,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to include in all destinations. Allows `*` as wildcard at end.' }, :'browser_monitoring.attributes.enabled' => { @@ -937,7 +880,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to exclude from browser monitoring. Allows `*` as wildcard at end.' }, :'browser_monitoring.attributes.include' => { @@ -945,7 +887,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to include in browser monitoring. Allows `*` as wildcard at end.' }, :'error_collector.attributes.enabled' => { @@ -960,7 +901,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to exclude from error collection. Allows `*` as wildcard at end.' }, :'error_collector.attributes.include' => { @@ -968,7 +908,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to include in error collection. Allows `*` as wildcard at end.' }, :'span_events.attributes.enabled' => { @@ -983,7 +922,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to exclude from span events. Allows `*` as wildcard at end.' }, :'span_events.attributes.include' => { @@ -991,7 +929,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to include on span events. Allows `*` as wildcard at end.' }, :'transaction_events.attributes.enabled' => { @@ -1006,7 +943,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to exclude from transaction events. Allows `*` as wildcard at end.' }, :'transaction_events.attributes.include' => { @@ -1014,7 +950,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to include in transaction events. Allows `*` as wildcard at end.' }, :'transaction_segments.attributes.enabled' => { @@ -1029,7 +964,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to exclude from transaction segments. Allows `*` as wildcard at end.' }, :'transaction_segments.attributes.include' => { @@ -1037,7 +971,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to include on transaction segments. Allows `*` as wildcard at end.' }, :'transaction_tracer.attributes.enabled' => { @@ -1052,7 +985,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to exclude from transaction traces. Allows `*` as wildcard at end.' }, :'transaction_tracer.attributes.include' => { @@ -1060,7 +992,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Prefix of attributes to include in transaction traces. Allows `*` as wildcard at end.' }, # Audit log @@ -1468,7 +1399,6 @@ def self.notify :public => true, :type => Array, :allowed_from_server => false, - :transform => DefaultSource.method(:convert_to_list), :description => 'Ordinarily the agent reports dyno names with a trailing dot and process ID (for example, `worker.3`). You can remove this trailing data by specifying the prefixes you want to report without trailing data (for example, `worker`).' }, # Infinite tracing @@ -1874,7 +1804,6 @@ def self.notify type: Array, dynamic_name: true, allowed_from_server: false, - :transform => DefaultSource.method(:convert_to_list), :description => <<~DESCRIPTION An array of strings to specify which keys inside a Stripe event's `user_data` hash should be reported to New Relic. Each string in this array will be turned into a regular expression via `Regexp.new` to @@ -1887,7 +1816,6 @@ def self.notify type: Array, dynamic_name: true, allowed_from_server: false, - :transform => DefaultSource.method(:convert_to_list), :description => <<~DESCRIPTION An array of strings to specify which keys and/or values inside a Stripe event's `user_data` hash should not be reported to New Relic. Each string in this array will be turned into a regular expression via @@ -2147,9 +2075,9 @@ def self.notify :description => 'If true, the agent strips messages from all exceptions except those in the [allowlist](#strip_exception_messages-allowlist). Enabled automatically in [high security mode](/docs/accounts-partnerships/accounts/security/high-security).' }, :'strip_exception_messages.allowed_classes' => { - :default => '', + :default => NewRelic::EMPTY_ARRAY, :public => true, - :type => String, + :type => Array, :allowed_from_server => false, :transform => DefaultSource.method(:convert_to_constant_list), :description => 'Specify a list of exceptions you do not want the agent to strip when [strip_exception_messages](#strip_exception_messages-enabled) is `true`. Separate exceptions with a comma. For example, `"ImportantException,PreserveMessageException"`.' diff --git a/lib/new_relic/agent/configuration/environment_source.rb b/lib/new_relic/agent/configuration/environment_source.rb index fe6cb791a1..a58c536291 100644 --- a/lib/new_relic/agent/configuration/environment_source.rb +++ b/lib/new_relic/agent/configuration/environment_source.rb @@ -13,6 +13,7 @@ class EnvironmentSource < DottedHash /^NEW_RELIC_METADATA_/ # read by NewRelic::Agent::Connect::RequestBuilder ] + # TODO: remove type_map attr_accessor :alias_map, :type_map def initialize @@ -20,9 +21,11 @@ def initialize set_config_file @alias_map = {} + # TODO: remove type_map @type_map = {} DEFAULTS.each do |config_setting, value| + # TODO: remove type_map self.type_map[config_setting] = value[:type] set_aliases(config_setting, value) end @@ -70,43 +73,14 @@ def set_values_from_new_relic_environment_variables nr_env_var_keys.each do |key| next if SPECIAL_CASE_KEYS.any? { |pattern| pattern === key.upcase } - set_value_from_environment_variable(key) - end - end - - def set_value_from_environment_variable(key) - config_key = convert_environment_key_to_config_key(key) - set_key_by_type(config_key, key) - end + config_key = convert_environment_key_to_config_key(key) - def set_key_by_type(config_key, environment_key) - value = ENV[environment_key] - type = self.type_map[config_key] - - if type == String - self[config_key] = value - elsif type == Integer - self[config_key] = value.to_i - elsif type == Float - self[config_key] = value.to_f - elsif type == Symbol - self[config_key] = value.to_sym - elsif type == Array - self[config_key] = if DEFAULTS[config_key].key?(:transform) - DEFAULTS[config_key][:transform].call(value) - else - value.split(/\s*,\s*/) - end - elsif type == NewRelic::Agent::Configuration::Boolean - if /false|off|no/i.match?(value) - self[config_key] = false - elsif !value.nil? - self[config_key] = true + unless DEFAULTS.key?(config_key) || serverless? + ::NewRelic::Agent.logger.info("#{key} does not have a corresponding configuration setting (#{config_key} does not exist).") + ::NewRelic::Agent.logger.info('Run `rake newrelic:config:docs` or visit https://docs.newrelic.com/docs/apm/agents/ruby-agent/configuration/ruby-agent-configuration to see a list of available configuration settings.') end - elsif !serverless? - ::NewRelic::Agent.logger.info("#{environment_key} does not have a corresponding configuration setting (#{config_key} does not exist).") - ::NewRelic::Agent.logger.info('Run `rake newrelic:config:docs` or visit https://docs.newrelic.com/docs/apm/agents/ruby-agent/configuration/ruby-agent-configuration to see a list of available configuration settings.') - self[config_key] = value + + self[config_key] = ENV[key] end end diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index f1c5b6bb24..c5a3ac4318 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -17,6 +17,27 @@ module Configuration class Manager DEPENDENCY_DETECTION_VALUES = %i[prepend chain unsatisfied].freeze + BOOLEAN_MAP = { + 'true' => true, + 'yes' => true, + 'on' => true, + 'false' => false, + 'no' => false, + 'off' => false + }.freeze + + INSTRUMENTATION_VALUES = %w[chain prepend unsatisfied] + + TYPE_COERCIONS = {Integer => {pattern: /^\d+$/, proc: proc { |s| s.to_i }}, + Float => {pattern: /^\d+\.\d+$/, proc: proc { |s| s.to_f }}, + Symbol => {proc: proc { |s| s.to_sym }}, + Array => {proc: proc { |s| s.split(/\s*,\s*/) }}, + Hash => {proc: proc { |s| s.split(/\s*,\s*/).each_with_object({}) { |i, h| k, v = i.split(/\s*=\s*/); h[k] = v } }}, + NewRelic::Agent::Configuration::Boolean => {pattern: /^(?:#{BOOLEAN_MAP.keys.join('|')})$/, + proc: proc { |s| BOOLEAN_MAP[s] }}}.freeze + + USER_CONFIG_CLASSES = [NewRelic::Agent::Configuration::EnvironmentSource, NewRelic::Agent::Configuration::YamlSource] + # Defining these explicitly saves object allocations that we incur # if we use Forwardable and def_delegators. def [](key) @@ -116,72 +137,103 @@ def fetch(key) next unless config accessor = key.to_sym + next unless config.has_key?(accessor) - if config.has_key?(accessor) - begin - return evaluate_and_apply_transformations(accessor, config[accessor]) - rescue - next - end + begin + return evaluate_and_apply_transformations(accessor, config[accessor], nr_supplied?(config.class)) + rescue + next end end nil end - def evaluate_procs(value) - if value.respond_to?(:call) - instance_eval(&value) - else - value - end + def nr_supplied?(klass) + !USER_CONFIG_CLASSES.include?(klass) end - def evaluate_and_apply_transformations(key, value) - evaluated = evaluate_procs(value) - default = enforce_allowlist(key, evaluated) - return default if default - - boolean = enforce_boolean(key, value) - return boolean if [true, false].include?(boolean) + def evaluate_and_apply_transformations(key, value, nr_supplied) + evaluated = value.respond_to?(:call) ? value.call : value + evaluated = type_coerce(key, evaluated, nr_supplied) + evaluated = enforce_allowlist(key, evaluated) apply_transformations(key, evaluated) end - def apply_transformations(key, value) - if transform = transform_from_default(key) - begin - transform.call(value) - rescue => e - NewRelic::Agent.logger.error("Error applying transformation for #{key}, pre-transform value was: #{value}.", e) - raise e - end - else - value - end + def boolean?(type, value) + return false unless type == NewRelic::Agent::Configuration::Boolean + + value.class == TrueClass || value.class == FalseClass end - def enforce_allowlist(key, value) - return unless allowlist = default_source.allowlist_for(key) - return if allowlist.include?(value) + # auto-instrumentation configuration params can be symbols or strings + # and unless we want to refactor the configuration hash to support both + # types, we handle the special case here + def instrumentation?(type, value) + return false unless type == String || type == Symbol + return true if INSTRUMENTATION_VALUES.include?(value.to_s) - default = default_source.default_for(key) - NewRelic::Agent.logger.warn "Invalid value '#{value}' for #{key}, applying default value of '#{default}'" - default + false end - def enforce_boolean(key, value) - type = default_source.value_from_defaults(key, :type) - return unless type == Boolean + def type_coerce(key, value, nr_supplied) + return validate_nil(key, nr_supplied) unless value + + type = DEFAULTS.dig(key, :type) + return value if value.is_a?(type) || boolean?(type, value) || instrumentation?(type, value) + + if value.class != String + return default_with_warning(key, value, "Expected to receive a value of type #{type} but " \ + "received #{value.class}.") + end + + pattern = TYPE_COERCIONS.dig(type, :pattern) + if pattern && value !~ pattern + return default_with_warning(key, value, "Expected to receive a value of type #{type} matching " \ + "pattern '#{pattern}'.") + end - bool_value = default_source.boolean_for(key, value) - return bool_value unless bool_value.nil? + procedure = TYPE_COERCIONS.dig(type, :proc) + return value unless procedure + + procedure.call(value) + end - default = default_source.default_for(key) - NewRelic::Agent.logger.warn "Invalid value '#{value}' for #{key}, applying default value of '#{default}'" + def default_with_warning(key, value, msg) + default = default_without_warning(key) + NewRelic::Agent.logger.warn "Received an invalid '#{value}' value for the '#{key}' configuration " \ + "parameter! #{msg} Using the default value of '#{default}'." default end + def default_without_warning(key) + DEFAULTS[key][:default] + end + + def validate_nil(key, nr_supplied = false) + return if DEFAULTS.dig(key, :allow_nil) + return default_without_warning(key) if nr_supplied + + default_with_warning(key, nil, 'Nil values are not permitted for the parameter.') + end + + def apply_transformations(key, value) + return value unless transform = default_source.transform_for(key) + + transform.call(value) + rescue => e + default_with_warning(key, value, "Error encountered while applying transformation: >>#{e}<<") + end + + def enforce_allowlist(key, value) + return value unless allowlist = default_source.allowlist_for(key) + return value if allowlist.include?(value) + + default_with_warning(key, value, 'Expected to receive a value found on the following list: ' \ + ">>#{allowlist}<<, but received '#{value}'.") + end + def transform_from_default(key) default_source.transform_for(key) end @@ -244,7 +296,7 @@ def flattened thawed_layer = layer.to_hash.dup thawed_layer.each do |k, v| begin - thawed_layer[k] = instance_eval(&v) if v.respond_to?(:call) + thawed_layer[k] = v.call if v.respond_to?(:call) rescue => e NewRelic::Agent.logger.debug("#{e.class.name} : #{e.message} - when accessing config key #{k}") thawed_layer[k] = nil From 4e0e06f1f80be96cb7ee5c0e7130dabdf472081e Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 1 Oct 2024 20:55:59 -0700 Subject: [PATCH 02/15] type coercion fixups - fix `raw_value` -> `string_array` variable rename needed for one remaining instance - anticipate a default value being a proc and call it before yielding the value - explicitly check for `#nil?` instead of truthiness, as `false` is certainly a valid non-nil config value --- lib/new_relic/agent/configuration/default_source.rb | 2 +- lib/new_relic/agent/configuration/manager.rb | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index d656410cbe..68e9968022 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -221,7 +221,7 @@ def self.convert_to_constant_list(string_array) constants = string_array.map! do |class_name| const = ::NewRelic::LanguageSupport.constantize(class_name) - NewRelic::Agent.logger.warn("Ignoring invalid constant '#{class_name}' in #{raw_value}") unless const + NewRelic::Agent.logger.warn("Ignoring invalid constant '#{class_name}' in #{string_array}") unless const const end constants.compact! diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index c5a3ac4318..5e1fc56c0d 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -178,7 +178,7 @@ def instrumentation?(type, value) end def type_coerce(key, value, nr_supplied) - return validate_nil(key, nr_supplied) unless value + return validate_nil(key, nr_supplied) if value.nil? type = DEFAULTS.dig(key, :type) return value if value.is_a?(type) || boolean?(type, value) || instrumentation?(type, value) @@ -208,7 +208,8 @@ def default_with_warning(key, value, msg) end def default_without_warning(key) - DEFAULTS[key][:default] + default = DEFAULTS[key][:default] + default.respond_to?(:call) ? default.call : default end def validate_nil(key, nr_supplied = false) From e4a18ed16d3d719cf9d963cb061924ea1b904de5 Mon Sep 17 00:00:00 2001 From: fallwith Date: Thu, 3 Oct 2024 22:01:44 -0700 Subject: [PATCH 03/15] config tweaks for legacy reasons... 1. permit tests to specify invalid configuration 2. permit `manual_start` to specify invalid configuration 3. permit int values for float based params and vice versa 4. permit string values for symbol based params and vice versa and also... 5. fix calls to `evaluate_and_apply_transformations` that were broken when the method was updated to accept a config category argument --- lib/new_relic/agent/configuration/manager.rb | 52 +++++++++++++++----- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 5e1fc56c0d..4f4854d233 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -27,6 +27,8 @@ class Manager }.freeze INSTRUMENTATION_VALUES = %w[chain prepend unsatisfied] + NUMERIC_TYPES = [Integer, Float] + STRINGLIKE_TYPES = [String, Symbol] TYPE_COERCIONS = {Integer => {pattern: /^\d+$/, proc: proc { |s| s.to_i }}, Float => {pattern: /^\d+\.\d+$/, proc: proc { |s| s.to_f }}, @@ -140,7 +142,7 @@ def fetch(key) next unless config.has_key?(accessor) begin - return evaluate_and_apply_transformations(accessor, config[accessor], nr_supplied?(config.class)) + return evaluate_and_apply_transformations(accessor, config[accessor], config_category(config.class)) rescue next end @@ -149,13 +151,17 @@ def fetch(key) nil end - def nr_supplied?(klass) - !USER_CONFIG_CLASSES.include?(klass) + def config_category(klass) + return :user if USER_CONFIG_CLASSES.include?(klass) + return :test if [DottedHash, Hash].include?(klass) + return :manual if klass == ManualSource + + return :nr end - def evaluate_and_apply_transformations(key, value, nr_supplied) + def evaluate_and_apply_transformations(key, value, category) evaluated = value.respond_to?(:call) ? value.call : value - evaluated = type_coerce(key, evaluated, nr_supplied) + evaluated = type_coerce(key, evaluated, category) evaluated = enforce_allowlist(key, evaluated) apply_transformations(key, evaluated) @@ -177,11 +183,30 @@ def instrumentation?(type, value) false end - def type_coerce(key, value, nr_supplied) - return validate_nil(key, nr_supplied) if value.nil? + def handle_nil_type(key, value, category) + return value if %i[manual test].include?(category) + + default_without_warning(key) + end + + # permit an int to be supplied for a float based param and vice versa + def numeric_conversion(value) + value.is_a?(Integer) ? value.to_f : value.round + end + + # permit a symbol to be supplied for a string based param and vice versa + def string_conversion(value) + value.is_a?(Symbol) ? value.to_s : value.to_sym + end + + def type_coerce(key, value, category) + return validate_nil(key, category) if value.nil? type = DEFAULTS.dig(key, :type) + return handle_nil_type(key, value, category) unless type return value if value.is_a?(type) || boolean?(type, value) || instrumentation?(type, value) + return numeric_conversion(value) if NUMERIC_TYPES.include?(type) && NUMERIC_TYPES.include?(value.class) + return string_conversion(value) if STRINGLIKE_TYPES.include?(type) && STRINGLIKE_TYPES.include?(value.class) if value.class != String return default_with_warning(key, value, "Expected to receive a value of type #{type} but " \ @@ -208,13 +233,13 @@ def default_with_warning(key, value, msg) end def default_without_warning(key) - default = DEFAULTS[key][:default] + default = DEFAULTS.dig(key, :default) default.respond_to?(:call) ? default.call : default end - def validate_nil(key, nr_supplied = false) - return if DEFAULTS.dig(key, :allow_nil) - return default_without_warning(key) if nr_supplied + def validate_nil(key, category) + return if DEFAULTS.dig(key, :allow_nil) || category == :test # tests are free to specify nil + return default_without_warning(key) unless category == :user # only user supplied config raises a warning default_with_warning(key, nil, 'Nil values are not permitted for the parameter.') end @@ -250,13 +275,14 @@ def register_callback(key, &proc) def invoke_callbacks(direction, source) return unless source + return if source.respond_to?(:empty?) && source.empty? source.keys.each do |key| begin # we need to evaluate and apply transformations for the value to deal with procs as values # this is usually done by the fetch method when accessing config, however the callbacks bypass that - evaluated_cache = evaluate_and_apply_transformations(key, @cache[key]) - evaluated_source = evaluate_and_apply_transformations(key, source[key]) + evaluated_cache = evaluate_and_apply_transformations(key, @cache[key], :nr) + evaluated_source = evaluate_and_apply_transformations(key, source[key], config_category(source.class)) rescue next end From ba7d79bef00480702e1c01f4ea2b4ce4201189c2 Mon Sep 17 00:00:00 2001 From: fallwith Date: Fri, 4 Oct 2024 19:12:36 -0700 Subject: [PATCH 04/15] config tweaks - treat :app_name as a special case. technically it should be classified as an `Array` type with a custom (semicolon) delimiter override but for backwards compatibility just make the transform dynamic - allow unit tests to do screwy things users shouldn't be permitted to do --- lib/new_relic/agent/configuration/default_source.rb | 2 +- lib/new_relic/agent/configuration/manager.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 68e9968022..8d7709d641 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -307,7 +307,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => String, :allowed_from_server => false, - :transform => proc { |str| str.split(';') }, + :transform => proc { |v| v.is_a?(String) ? v.split(';') : v }, :description => 'Specify the [application name](/docs/apm/new-relic-apm/installation-configuration/name-your-application) used to aggregate data in the New Relic UI. To report data to [multiple apps at the same time](/docs/apm/new-relic-apm/installation-configuration/using-multiple-names-app), specify a list of names separated by a semicolon `;`. For example, `MyApp` or `MyStagingApp;Instance1`.' }, :license_key => { diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 4f4854d233..449cf42193 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -209,6 +209,8 @@ def type_coerce(key, value, category) return string_conversion(value) if STRINGLIKE_TYPES.include?(type) && STRINGLIKE_TYPES.include?(value.class) if value.class != String + return value if category == :test + return default_with_warning(key, value, "Expected to receive a value of type #{type} but " \ "received #{value.class}.") end From 97e1ea8f59bb2045de85b448b81efff1d46f3c7a Mon Sep 17 00:00:00 2001 From: fallwith Date: Mon, 7 Oct 2024 11:50:13 -0700 Subject: [PATCH 05/15] config tweaks * remove orphaned type map content from environment source * for bools convert symbols to strings before converting to true/false * rework unit tests to match lib changes --- .../agent/configuration/environment_source.rb | 7 +- lib/new_relic/agent/configuration/manager.rb | 2 + lib/new_relic/control/instance_methods.rb | 6 +- .../configuration/default_source_test.rb | 47 ++-------- .../configuration/environment_source_test.rb | 93 +++++++------------ 5 files changed, 42 insertions(+), 113 deletions(-) diff --git a/lib/new_relic/agent/configuration/environment_source.rb b/lib/new_relic/agent/configuration/environment_source.rb index a58c536291..04a9aaa35c 100644 --- a/lib/new_relic/agent/configuration/environment_source.rb +++ b/lib/new_relic/agent/configuration/environment_source.rb @@ -13,20 +13,15 @@ class EnvironmentSource < DottedHash /^NEW_RELIC_METADATA_/ # read by NewRelic::Agent::Connect::RequestBuilder ] - # TODO: remove type_map - attr_accessor :alias_map, :type_map + attr_accessor :alias_map def initialize set_log_file set_config_file @alias_map = {} - # TODO: remove type_map - @type_map = {} DEFAULTS.each do |config_setting, value| - # TODO: remove type_map - self.type_map[config_setting] = value[:type] set_aliases(config_setting, value) end diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 449cf42193..38220ded11 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -208,6 +208,8 @@ def type_coerce(key, value, category) return numeric_conversion(value) if NUMERIC_TYPES.include?(type) && NUMERIC_TYPES.include?(value.class) return string_conversion(value) if STRINGLIKE_TYPES.include?(type) && STRINGLIKE_TYPES.include?(value.class) + # convert bool to string for regex usage and bool hash lookup + value = value.to_s if type == Boolean if value.class != String return value if category == :test diff --git a/lib/new_relic/control/instance_methods.rb b/lib/new_relic/control/instance_methods.rb index c64dee0297..2209d6d1d8 100644 --- a/lib/new_relic/control/instance_methods.rb +++ b/lib/new_relic/control/instance_methods.rb @@ -98,10 +98,8 @@ def configure_agent(env, options) manual = Agent::Configuration::ManualSource.new(options) Agent.config.replace_or_add_config(manual) - # if manual config sees serverless mode enabled, then the proc - # must have returned 'true'. don't bother with YAML and high security - # in a serverless context - return if Agent.config[:'serverless_mode.enabled'] + # don't bother with YAML and high security in a serverless context + return if Agent.config[:'serverless_mode.enabled'] || env == 'serverless' yaml_source = Agent::Configuration::YamlSource.new(config_file_path, env) log_yaml_source_failures(yaml_source) if yaml_source.failed? diff --git a/test/new_relic/agent/configuration/default_source_test.rb b/test/new_relic/agent/configuration/default_source_test.rb index dd68ce405d..fdc979fc12 100644 --- a/test/new_relic/agent/configuration/default_source_test.rb +++ b/test/new_relic/agent/configuration/default_source_test.rb @@ -101,24 +101,6 @@ def test_transform_for_returns_nil_for_settings_that_do_not_have_a_transform assert_nil DefaultSource.transform_for(:ca_bundle_path) end - def test_convert_to_list - result = DefaultSource.convert_to_list('Foo,Bar,Baz') - - assert_equal %w[Foo Bar Baz], result - end - - def test_convert_to_list_returns_original_argument_given_array - result = DefaultSource.convert_to_list(['Foo']) - - assert_equal ['Foo'], result - end - - def test_convert_to_list_raises_on_totally_wrong_object - assert_raises(ArgumentError) do - DefaultSource.convert_to_list(Object.new) - end - end - def test_rules_ignore_converts_comma_delimited_string_to_array with_config(:'rules.ignore_url_regexes' => 'Foo,Bar,Baz') do assert_equal [/Foo/, /Bar/, /Baz/], NewRelic::Agent.config[:'rules.ignore_url_regexes'] @@ -248,25 +230,6 @@ def test_instrumentation_logger_matches_application_logging_disabled end end - def test_convert_to_hash_returns_hash - result = {'key1' => 'value1', 'key2' => 'value2'} - - assert_equal(DefaultSource.convert_to_hash(result), result) - end - - def test_convert_to_hash_with_string - value = 'key1=value1,key2=value2' - result = {'key1' => 'value1', 'key2' => 'value2'} - - assert_equal(DefaultSource.convert_to_hash(value), result) - end - - def test_convert_to_hash_raises_error_with_wrong_data_type - value = [1, 2, 3] - - assert_raises(ArgumentError) { DefaultSource.convert_to_hash(value) } - end - def test_allowlist_permits_valid_values valid_value = 'info' key = :'application_logging.forwarding.log_level' @@ -324,7 +287,7 @@ def test_boolean_configs_accepts_yes_on_and_true_as_symbols config_array.each do |value| with_config(key => value) do - assert NewRelic::Agent.config[key], "The '#{value}' value failed to evaluate as truthy!" + assert NewRelic::Agent.config[key], "The ':#{value}' value failed to evaluate as truthy!" end end end @@ -344,12 +307,12 @@ def test_boolean_configs_accepts_no_off_and_false_as_strings_as_symbols %i[no off false].each do |value| with_config(key => value) do - refute NewRelic::Agent.config[key], "The '#{value}' value failed to evaluate as falsey!" + refute NewRelic::Agent.config[key], "The ':#{value}' value failed to evaluate as falsey!" end end end - def test_enforce_boolean_uses_defult_on_invalid_value + def test_enforce_boolean_uses_default_on_invalid_value key = :'send_data_on_exit' # default value is `true` with_config(key => 'invalid_value') do @@ -362,7 +325,9 @@ def test_enforce_boolean_logs_warning_on_invalid_value default = ::NewRelic::Agent::Configuration::DefaultSource.default_for(key) with_config(key => 'yikes!') do - expects_logging(:warn, includes("Invalid value 'yikes!' for #{key}, applying default value of '#{default}'")) + NewRelic::Agent::Configuration::Manager.stub_const(:USER_CONFIG_CLASSES, [DottedHash]) do + expects_logging(:warn, includes('Expected to receive a value of type NewRelic::Agent::Configuration::Boolean matching pattern ')) + end end end diff --git a/test/new_relic/agent/configuration/environment_source_test.rb b/test/new_relic/agent/configuration/environment_source_test.rb index 056d889995..8020a2c1be 100644 --- a/test/new_relic/agent/configuration/environment_source_test.rb +++ b/test/new_relic/agent/configuration/environment_source_test.rb @@ -41,26 +41,26 @@ def test_environment_symbols_are_applied define_method("test_environment_booleans_truths_are_applied_to_#{var}") do ENV[var] = 'true' - assert EnvironmentSource.new[:enabled] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'true', true) ENV[var] = 'on' - assert EnvironmentSource.new[:enabled] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'on', true) ENV[var] = 'yes' - assert EnvironmentSource.new[:enabled] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'yes', true) ENV.delete(var) end define_method("test_environment_booleans_falsehoods_are_applied_to_#{var}") do ENV[var] = 'false' - refute EnvironmentSource.new[:enabled] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'false', false) ENV[var] = 'off' - refute EnvironmentSource.new[:enabled] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'off', false) ENV[var] = 'no' - refute EnvironmentSource.new[:enabled] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'no', false) ENV.delete(var) end end @@ -69,26 +69,26 @@ def test_environment_symbols_are_applied define_method("test_environment_booleans_truths_are_applied_to_#{var}") do ENV[var] = 'true' - assert EnvironmentSource.new[:disable_harvest_thread] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'true', true) ENV[var] = 'on' - assert EnvironmentSource.new[:disable_harvest_thread] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'on', true) ENV[var] = 'yes' - assert EnvironmentSource.new[:disable_harvest_thread] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'yes', true) ENV.delete(var) end define_method("test_environment_booleans_falsehoods_are_applied_to_#{var}") do ENV[var] = 'false' - refute EnvironmentSource.new[:disable_harvest_thread] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'false', false) ENV[var] = 'off' - refute EnvironmentSource.new[:disable_harvest_thread] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'off', false) ENV[var] = 'no' - refute EnvironmentSource.new[:disable_harvest_thread] + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'no', false) ENV.delete(var) end end @@ -132,48 +132,6 @@ def test_set_values_from_new_relic_environment_variables_ignores_NEW_RELIC_LOG @environment_source.set_values_from_new_relic_environment_variables end - def test_set_value_from_environment_variable - ENV['NEW_RELIC_LICENSE_KEY'] = 'super rad' - @environment_source.set_value_from_environment_variable('NEW_RELIC_LICENSE_KEY') - - assert_equal 'super rad', @environment_source[:license_key] - end - - def test_set_key_by_type_uses_the_default_type - ENV['NEW_RELIC_TEST'] = 'true' - @environment_source.set_key_by_type(:enabled, 'NEW_RELIC_TEST') - - assert @environment_source[:enabled] - end - - def test_set_key_by_type_converts_comma_lists_to_array - ENV['NEW_RELIC_ATTRIBUTES_INCLUDE'] = 'hi,bye' - @environment_source.set_key_by_type(:'attributes.include', 'NEW_RELIC_ATTRIBUTES_INCLUDE') - - assert_equal %w[hi bye], @environment_source[:'attributes.include'] - end - - def test_set_key_by_type_converts_comma_lists_with_spaces_to_array - ENV['NEW_RELIC_ATTRIBUTES_INCLUDE'] = 'hi, bye' - @environment_source.set_key_by_type(:'attributes.include', 'NEW_RELIC_ATTRIBUTES_INCLUDE') - - assert_equal %w[hi bye], @environment_source[:'attributes.include'] - end - - def test_array_based_params_use_the_transform_proc_when_present - skip_unless_minitest5_or_above - - arr = %w[James Jessie Meowth] - - env_var = 'NEW_RELIC_AUTOMATIC_CUSTOM_INSTRUMENTATION_METHOD_LIST' - param = :automatic_custom_instrumentation_method_list - ENV.stub(:[], arr.join(','), [env_var]) do - @environment_source.set_key_by_type(param, env_var) - end - - assert_equal arr, @environment_source[param] - end - def test_set_key_with_new_relic_prefix assert_applied_string('NEW_RELIC_LICENSE_KEY', :license_key) end @@ -195,7 +153,7 @@ def test_convert_environment_key_to_config_key end def test_convert_environment_key_to_config_key_respects_aliases - assert_applied_boolean('NEWRELIC_ENABLE', :enabled) + assert_applied_boolean('NEWRELIC_ENABLE', :enabled, 'true', true) end def test_convert_environment_key_to_config_key_allows_underscores_as_dots @@ -220,31 +178,42 @@ def test_does_not_warn_for_new_relic_env_environment_variable end def assert_applied_string(env_var, config_var) - ENV[env_var] = 'test value' + value = 'test value' + ENV[env_var] = value + expected = env_var.end_with?('_APP_NAME') ? [value] : value - assert_equal 'test value', EnvironmentSource.new[config_var.to_sym] + assert_equal expected, refreshed_config_value_for(config_var) + ensure ENV.delete(env_var) end def assert_applied_symbol(env_var, config_var) ENV[env_var] = 'test value' - assert_equal :'test value', EnvironmentSource.new[config_var.to_sym] + assert_equal :'test value', refreshed_config_value_for(config_var) + ensure ENV.delete(env_var) end def assert_applied_fixnum(env_var, config_var) ENV[env_var] = '3000' - assert_equal 3000, EnvironmentSource.new[config_var.to_sym] + assert_equal 3000, refreshed_config_value_for(config_var) + ensure ENV.delete(env_var) end - def assert_applied_boolean(env_var, config_var) - ENV[env_var] = 'true' + def assert_applied_boolean(env_var, config_var, value, expected) + ENV[env_var] = value - assert EnvironmentSource.new[config_var.to_sym] + assert_equal expected, refreshed_config_value_for(config_var) + ensure ENV.delete(env_var) end + + def refreshed_config_value_for(var) + NewRelic::Agent.config.reset_to_defaults + NewRelic::Agent.config[var.to_sym] + end end end From 18651e2473b0528059739869b1e04c889ef6e24b Mon Sep 17 00:00:00 2001 From: fallwith Date: Mon, 7 Oct 2024 20:42:38 -0700 Subject: [PATCH 06/15] config tweaks * revert from `call` back to `instance_eval` so that the original intended scope is preserved * update tests to match lib changes --- lib/new_relic/agent/configuration/manager.rb | 15 ++---- .../agent/configuration/manager_test.rb | 52 ++++--------------- 2 files changed, 14 insertions(+), 53 deletions(-) diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 38220ded11..5e1fbb1c12 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -160,7 +160,7 @@ def config_category(klass) end def evaluate_and_apply_transformations(key, value, category) - evaluated = value.respond_to?(:call) ? value.call : value + evaluated = value.respond_to?(:call) ? instance_eval(&value) : value evaluated = type_coerce(key, evaluated, category) evaluated = enforce_allowlist(key, evaluated) @@ -264,10 +264,6 @@ def enforce_allowlist(key, value) ">>#{allowlist}<<, but received '#{value}'.") end - def transform_from_default(key) - default_source.transform_for(key) - end - def default_source NewRelic::Agent::Configuration::DefaultSource end @@ -283,11 +279,10 @@ def invoke_callbacks(direction, source) source.keys.each do |key| begin - # we need to evaluate and apply transformations for the value to deal with procs as values - # this is usually done by the fetch method when accessing config, however the callbacks bypass that - evaluated_cache = evaluate_and_apply_transformations(key, @cache[key], :nr) + evaluated_cache = evaluate_and_apply_transformations(key, @cache[key], :manual) evaluated_source = evaluate_and_apply_transformations(key, source[key], config_category(source.class)) - rescue + rescue => e + NewRelic::Agent.logger.warn("Error evaluating callback for direction '#{direction}' with key '#{key}': #{e}") next end @@ -327,7 +322,7 @@ def flattened thawed_layer = layer.to_hash.dup thawed_layer.each do |k, v| begin - thawed_layer[k] = v.call if v.respond_to?(:call) + thawed_layer[k] = instance_eval(&v) if v.respond_to?(:call) rescue => e NewRelic::Agent.logger.debug("#{e.class.name} : #{e.message} - when accessing config key #{k}") thawed_layer[k] = nil diff --git a/test/new_relic/agent/configuration/manager_test.rb b/test/new_relic/agent/configuration/manager_test.rb index b574690a98..e88d25de8b 100644 --- a/test/new_relic/agent/configuration/manager_test.rb +++ b/test/new_relic/agent/configuration/manager_test.rb @@ -414,21 +414,6 @@ def test_fetch_with_a_transform_returns_the_transformed_value end end - def test_fetch_skips_the_config_layer_if_transformation_raises_error - bomb = proc do |value| - if value == 'boom' - raise StandardError.new - else - value - end - end - @manager.stubs(:transform_from_default).returns(bomb) - - with_config(:'rules.ignore_url_regexes' => 'boom') do - assert_empty(@manager.fetch(:'rules.ignore_url_regexes')) - end - end - def test_prepend_key_absent_to_instrumentation_value_of with_config({}) do result = @manager.fetch(:'instrumentation.net_http') @@ -461,34 +446,15 @@ def test_default_to_value_of_only_happens_at_defaults end end - def test_evaluate_procs_returns_evaluated_value_if_it_responds_to_call - callable = proc { 'test' } - - assert_equal 'test', @manager.evaluate_procs(callable) - end - - def test_evaluate_procs_returns_original_value_if_it_does_not_respond_to_call - assert_equal 'test', @manager.evaluate_procs('test') - end - - def test_apply_transformations_logs_error_if_transformation_fails - bomb = proc { raise StandardError.new } - @manager.stubs(:transform_from_default).returns(bomb) - - expects_logging(:error, includes('Error applying transformation'), any_parameters) - - assert_raises StandardError do - @manager.apply_transformations(:test_key, 'test_value') - end - end - - def test_apply_transformations_reraises_errors - bomb = proc { raise StandardError.new } - @manager.stubs(:transform_from_default).returns(bomb) - - assert_raises StandardError do - @manager.apply_transformations(:test_key, 'test_value') - end + def test_apply_transformations_logs_warning_if_transformation_fails + key = :test_key + bomb = proc { raise 'kaboom' } + ds = Minitest::Mock.new + ds.expect :transform_for, bomb, [key] + @manager.stubs(:default_source).returns(ds) + expects_logging(:warn, includes('Error encountered while applying transformation'), any_parameters) + @manager.apply_transformations(key, 'test_value') + ds.verify end def test_auto_determined_values_stay_cached From 585632a396cfb82ebcec3a884985d88b81f349fe Mon Sep 17 00:00:00 2001 From: fallwith Date: Mon, 7 Oct 2024 21:08:41 -0700 Subject: [PATCH 07/15] update tests to match new config behavior trust the config manager tests to handle these situations --- test/new_relic/agent/hostname_test.rb | 8 -------- test/new_relic/agent/local_log_decorator_test.rb | 2 +- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/test/new_relic/agent/hostname_test.rb b/test/new_relic/agent/hostname_test.rb index 1a1cc2152f..26c0c892b9 100644 --- a/test/new_relic/agent/hostname_test.rb +++ b/test/new_relic/agent/hostname_test.rb @@ -80,14 +80,6 @@ def test_shortens_to_prefixes_with_empty_string end end - def test_shortens_to_prefixes_with_unsupported_object - with_dyno_name('Imladris.1', :'heroku.use_dyno_names' => true, :'heroku.dyno_name_prefixes_to_shorten' => Object.new) do - expects_logging(:error, includes('heroku.dyno_name_prefixes_to_shorten'), instance_of(ArgumentError)) - - assert_equal 'Imladris.1', NewRelic::Agent::Hostname.get - end - end - def test_local_predicate_true_when_host_local hosts = %w[localhost 0.0.0.0 127.0.0.1 0:0:0:0:0:0:0:1 0:0:0:0:0:0:0:0 ::1 ::] diff --git a/test/new_relic/agent/local_log_decorator_test.rb b/test/new_relic/agent/local_log_decorator_test.rb index a3208fedd2..77a80edead 100644 --- a/test/new_relic/agent/local_log_decorator_test.rb +++ b/test/new_relic/agent/local_log_decorator_test.rb @@ -94,7 +94,7 @@ def test_URI_encodes_entity_name end def test_safe_without_entity_name - with_config(app_name: nil) do + with_config(app_name: []) do decorated_message = LocalLogDecorator.decorate(MESSAGE) assert_includes decorated_message, '||' From 8c339c180a60f6f0d4f414744b2a894857e9118b Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 8 Oct 2024 23:48:56 -0700 Subject: [PATCH 08/15] config tweaks config manager test updates --- lib/new_relic/agent/configuration/manager.rb | 1 - .../agent/configuration/manager_test.rb | 216 ++++++++++++++++++ 2 files changed, 216 insertions(+), 1 deletion(-) diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 5e1fbb1c12..bca7fdb258 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -32,7 +32,6 @@ class Manager TYPE_COERCIONS = {Integer => {pattern: /^\d+$/, proc: proc { |s| s.to_i }}, Float => {pattern: /^\d+\.\d+$/, proc: proc { |s| s.to_f }}, - Symbol => {proc: proc { |s| s.to_sym }}, Array => {proc: proc { |s| s.split(/\s*,\s*/) }}, Hash => {proc: proc { |s| s.split(/\s*,\s*/).each_with_object({}) { |i, h| k, v = i.split(/\s*=\s*/); h[k] = v } }}, NewRelic::Agent::Configuration::Boolean => {pattern: /^(?:#{BOOLEAN_MAP.keys.join('|')})$/, diff --git a/test/new_relic/agent/configuration/manager_test.rb b/test/new_relic/agent/configuration/manager_test.rb index e88d25de8b..7b4ead33fe 100644 --- a/test/new_relic/agent/configuration/manager_test.rb +++ b/test/new_relic/agent/configuration/manager_test.rb @@ -516,6 +516,222 @@ def phony_cache.dup; self[:dup_called] = true; self; end @manager.new_cache end + def test_type_coercion_of_an_integer_from_a_string + key = :max_chocolate_chips + defaults = {key => {default: 0, type: Integer}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, '1138', :manual) + + assert_equal 1138, value + end + end + + def test_type_coercion_of_a_float_from_a_string + key = :slice_dice_ratio + defaults = {key => {default: 0.0, type: Float}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, '867.5309', :manual) + + assert_equal 867.5309, value # rubocop:disable Minitest/AssertInDelta + end + end + + def test_type_coercion_of_a_string_from_a_symbol + key = :alert_highlight_color + defaults = {key => {default: 'beige', type: String}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, :'forest green', :manual) + + assert_equal 'forest green', value + end + end + + def test_type_coercion_of_a_symbol_from_a_string + key = :sampling_direction + defaults = {key => {default: :forwards, type: Symbol}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, 'backwards', :manual) + + assert_equal :backwards, value + end + end + + def test_type_coercion_of_an_array_from_a_string + key = :allowlisted_job_params + defaults = {key => {default: [], type: Array}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, 'beans, rice, cheese', :manual) + + assert_equal %w[beans rice cheese], value + end + end + + def test_type_coercion_of_a_hash_from_a_string + key = :id_map + defaults = {key => {default: {}, type: Hash}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, '19 = 81, Blind = Chance', :manual) + + assert_equal({'19' => '81', 'Blind' => 'Chance'}, value) + end + end + + def test_type_coercion_of_a_boolean_from_a_string + key = :vm_performance_analysis + defaults = {key => {default: false, type: NewRelic::Agent::Configuration::Boolean}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, 'on', :manual) + + assert_equal true, value # rubocop:disable Minitest/AssertTruthy + end + end + + def test_type_coercion_of_an_integer_from_a_float + key = :applied_jigawatts + defaults = {key => {default: 0, type: Integer}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, 1.21, :manual) + + assert_equal 1, value + end + end + + def test_type_coercion_of_a_float_from_an_int + key = :refresh_sampling_ratio + defaults = {key => {default: 12.5, type: Float}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, 25, :manual) + + assert_equal 25.0, value # rubocop:disable Minitest/AssertInDelta + end + end + + def test_type_coercion_rejects_invalid_input_and_falls_back_to_the_default + key = :error_rate_threshold + default = 50 + defaults = {key => {default: default, type: Integer}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + expects_logging(:warn, includes('Expected to receive a value of type Integer matching pattern ')) + + value = @manager.type_coerce(key, 'seventy-five', :manual) + + assert_equal default, value + end + end + + def test_type_coercion_anticipates_an_invalid_type_without_a_proc_defined + key = :unexpected + input = 'original' + defaults = {key => {default: nil, type: Class}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + value = @manager.type_coerce(key, input, :manual) + + assert_equal input, value + end + end + + def test_add_config_for_testing_enforces_an_input_class_allowlist + error = assert_raises RuntimeError do + @manager.add_config_for_testing(nil) + end + + assert_equal 'Invalid config type for testing', error.message + end + + def test_validate_nil_expects_nils_from_tests + expects_no_logging(:warn) + + refute @manager.validate_nil(:a_key, :test) + end + + def test_validates_nil_allows_nil_if_the_config_param_has_allow_nil_set + key = :optional_tolerance + defaults = {key => {default: '', type: String, allow_nil: true}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + expects_no_logging(:warn) + + refute @manager.validate_nil(key, :nr) + end + end + + def test_validate_nil_warns_users + key = :required_tolerance + default = 1138 + defaults = {key => {default: default, type: String}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + expects_logging(:warn, includes('Nil values are not permitted')) + + value = @manager.validate_nil(key, :user) + + assert_equal default, value + end + end + + def test_validate_nil_does_not_warn_for_nr_internal_category + key = :required_tolerance + default = 1138 + defaults = {key => {default: default, type: String}} + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + expects_no_logging(:warn) + + value = @manager.validate_nil(key, :nr) + + assert_equal default, value + end + end + + def test_enforce_allowlist_only_operates_on_params_with_allowlists + key = :unguarded + + default_source = Object.new + default_source.stubs(:allowlist_for).returns(nil) + @manager.stubs(:default_source).returns(default_source) + + expects_no_logging(:warn) + + value = @manager.enforce_allowlist(key, 9) + + assert_equal 9, value + end + + def test_enforce_allowlist_does_not_warn_if_the_input_value_is_on_the_allowlist + key = :guarded + default = 1138 + allowlist = [default, 11, 38] + defaults = {key => {default: default, allowlist: allowlist}} + + default_source = Object.new + default_source.stubs(:allowlist_for).returns(allowlist) + @manager.stubs(:default_source).returns(default_source) + + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + expects_no_logging(:warn) + + value = @manager.enforce_allowlist(key, 11) + + assert_equal 11, value + end + end + + def test_enforce_allowlist_warns_if_the_input_value_is_not_on_the_allowlist + key = :guarded + default = 1138 + allowlist = [default, 11, 38] + defaults = {key => {default: default, allowlist: allowlist}} + + default_source = Object.new + default_source.stubs(:allowlist_for).returns(allowlist) + @manager.stubs(:default_source).returns(default_source) + + NewRelic::Agent::Configuration::Manager.stub_const(:DEFAULTS, defaults) do + expects_logging(:warn, includes('Expected to receive a value found on the following list')) + + value = @manager.enforce_allowlist(key, 9) + + assert_equal default, value + end + end + private def assert_parsed_labels(expected) From 279e118533946df52eccd9cd0d15f10dfd561c85 Mon Sep 17 00:00:00 2001 From: fallwith Date: Wed, 9 Oct 2024 12:35:44 -0700 Subject: [PATCH 09/15] rake test: use an int for an int param float args for int params will be rounded, so use an int to help with setting accurate expectations --- test/multiverse/suites/rake/instrumentation_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/multiverse/suites/rake/instrumentation_test.rb b/test/multiverse/suites/rake/instrumentation_test.rb index 41af5537a9..d334bb5f34 100644 --- a/test/multiverse/suites/rake/instrumentation_test.rb +++ b/test/multiverse/suites/rake/instrumentation_test.rb @@ -10,7 +10,7 @@ class TesterClass include NewRelic::Agent::Instrumentation::Rake::Tracer def name; 'Snake'; end - def timeout; 140.85; end + def timeout; 140; end end class ErrorClass < StandardError; end From 1da3a0e055fac9c8368ae1a0b6f7a6649e0b06db Mon Sep 17 00:00:00 2001 From: fallwith Date: Wed, 9 Oct 2024 19:56:06 -0700 Subject: [PATCH 10/15] config callback optmisations - don't bother with calculations if a callback isn't registered - assume the cached value has been evaluated already --- lib/new_relic/agent/configuration/manager.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index bca7fdb258..b2d69bfdb4 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -277,14 +277,16 @@ def invoke_callbacks(direction, source) return if source.respond_to?(:empty?) && source.empty? source.keys.each do |key| + next unless @callbacks.key?(key) + begin - evaluated_cache = evaluate_and_apply_transformations(key, @cache[key], :manual) evaluated_source = evaluate_and_apply_transformations(key, source[key], config_category(source.class)) rescue => e NewRelic::Agent.logger.warn("Error evaluating callback for direction '#{direction}' with key '#{key}': #{e}") next end + evaluated_cache = @cache.fetch(key, nil) if evaluated_cache != evaluated_source @callbacks[key].each do |proc| if direction == :add From a06bcfa51e396bd5fad1a8ce1b87446832feb800 Mon Sep 17 00:00:00 2001 From: fallwith Date: Wed, 9 Oct 2024 23:18:11 -0700 Subject: [PATCH 11/15] config tweaks * optimisation: don't bother with empty manual source opts hashes * restoration of legacy behavior: permit config params like :web_transactions_apdex to behave as expected (permit them to not have any entry in default source for things like a class type or a default value --- lib/new_relic/agent/configuration/manager.rb | 13 ++++++++++--- lib/new_relic/control/instance_methods.rb | 6 ++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index b2d69bfdb4..1eceb69c90 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -101,6 +101,8 @@ def remove_config(source) end def replace_or_add_config(source) + return if source.respond_to?(:empty?) && source.empty? + source.freeze was_finished = finished_configuring? @@ -185,7 +187,12 @@ def instrumentation?(type, value) def handle_nil_type(key, value, category) return value if %i[manual test].include?(category) - default_without_warning(key) + # TODO: identify all config params such as :web_transactions_apdex + # that can exist in the @config hash without having an entry + # in the DEFAULTS hash. then warn here when a key is in play + # that is not on that allowlist. for now, just permit any key + # and return the value. + default_without_warning(key) || value end # permit an int to be supplied for a float based param and vice versa @@ -335,8 +342,8 @@ def flattened end def apply_mask(hash) - MASK_DEFAULTS \ - .select { |_, proc| proc.call } \ + MASK_DEFAULTS + .select { |_, proc| proc.call } .each { |key, _| hash.delete(key) } hash end diff --git a/lib/new_relic/control/instance_methods.rb b/lib/new_relic/control/instance_methods.rb index 2209d6d1d8..62a3cf1387 100644 --- a/lib/new_relic/control/instance_methods.rb +++ b/lib/new_relic/control/instance_methods.rb @@ -95,8 +95,10 @@ def determine_env(options) end def configure_agent(env, options) - manual = Agent::Configuration::ManualSource.new(options) - Agent.config.replace_or_add_config(manual) + unless options.empty? + manual = Agent::Configuration::ManualSource.new(options) + Agent.config.replace_or_add_config(manual) + end # don't bother with YAML and high security in a serverless context return if Agent.config[:'serverless_mode.enabled'] || env == 'serverless' From 61c96187194d956c74c8c60a83fa25719b49066f Mon Sep 17 00:00:00 2001 From: fallwith Date: Thu, 10 Oct 2024 00:02:58 -0700 Subject: [PATCH 12/15] config tweaks * use `#[]` instead of `#fetch` on the @cache hash to light up its functionality properly * update default source test to perform a cache lookup call (previously a now-skipped callback call would have performed it) --- lib/new_relic/agent/configuration/manager.rb | 2 +- test/new_relic/agent/configuration/default_source_test.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 1eceb69c90..696f6693c4 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -293,7 +293,7 @@ def invoke_callbacks(direction, source) next end - evaluated_cache = @cache.fetch(key, nil) + evaluated_cache = @cache[key] if evaluated_cache != evaluated_source @callbacks[key].each do |proc| if direction == :add diff --git a/test/new_relic/agent/configuration/default_source_test.rb b/test/new_relic/agent/configuration/default_source_test.rb index fdc979fc12..abe15af5c0 100644 --- a/test/new_relic/agent/configuration/default_source_test.rb +++ b/test/new_relic/agent/configuration/default_source_test.rb @@ -327,6 +327,7 @@ def test_enforce_boolean_logs_warning_on_invalid_value with_config(key => 'yikes!') do NewRelic::Agent::Configuration::Manager.stub_const(:USER_CONFIG_CLASSES, [DottedHash]) do expects_logging(:warn, includes('Expected to receive a value of type NewRelic::Agent::Configuration::Boolean matching pattern ')) + NewRelic::Agent.config[key] end end end From b47be2b00b968e3c7f1195d5c45ed49bbff3cf50 Mon Sep 17 00:00:00 2001 From: fallwith Date: Thu, 10 Oct 2024 00:24:34 -0700 Subject: [PATCH 13/15] config: permit empty hashes rejecting empty hashes apparently wipes previously set values from non-empty hashes, so for now allow the empties to flow through as before --- lib/new_relic/agent/configuration/manager.rb | 2 -- lib/new_relic/control/instance_methods.rb | 6 ++---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 696f6693c4..b6d52e3a64 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -101,8 +101,6 @@ def remove_config(source) end def replace_or_add_config(source) - return if source.respond_to?(:empty?) && source.empty? - source.freeze was_finished = finished_configuring? diff --git a/lib/new_relic/control/instance_methods.rb b/lib/new_relic/control/instance_methods.rb index 62a3cf1387..2209d6d1d8 100644 --- a/lib/new_relic/control/instance_methods.rb +++ b/lib/new_relic/control/instance_methods.rb @@ -95,10 +95,8 @@ def determine_env(options) end def configure_agent(env, options) - unless options.empty? - manual = Agent::Configuration::ManualSource.new(options) - Agent.config.replace_or_add_config(manual) - end + manual = Agent::Configuration::ManualSource.new(options) + Agent.config.replace_or_add_config(manual) # don't bother with YAML and high security in a serverless context return if Agent.config[:'serverless_mode.enabled'] || env == 'serverless' From e80d1661c478ce71ac6215fb2e078aae1a7ffcaf Mon Sep 17 00:00:00 2001 From: fallwith Date: Thu, 10 Oct 2024 13:47:29 -0700 Subject: [PATCH 14/15] config: permit values to skip transformation if the given value is already of the type the transform is expected to produce, then just let it pass through --- lib/new_relic/agent/configuration/default_source.rb | 9 +++++++++ lib/new_relic/agent/configuration/manager.rb | 6 +++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 9696cfeaaa..da1ca33365 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -308,6 +308,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :type => String, :allowed_from_server => false, :transform => proc { |v| v.is_a?(String) ? v.split(';') : v }, + :transformed_type => Array, :description => 'Specify the [application name](/docs/apm/new-relic-apm/installation-configuration/name-your-application) used to aggregate data in the New Relic UI. To report data to [multiple apps at the same time](/docs/apm/new-relic-apm/installation-configuration/using-multiple-names-app), specify a list of names separated by a semicolon `;`. For example, `MyApp` or `MyStagingApp;Instance1`.' }, :license_key => { @@ -463,6 +464,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => true, :type => String, :allowed_from_server => false, + :transformed_type => Hash, # NOTE: :labels is a special case and transformed in manager.rb without a :transform key :description => 'A dictionary of [label names](/docs/data-analysis/user-interface-functions/labels-categories-organize-your-apps-servers) and values that will be applied to the data sent from this agent. May also be expressed as a semicolon-delimited `;` string of colon-separated `:` pairs. For example, `Server:One;Data Center:Primary`.' }, :log_file_name => { @@ -1008,6 +1010,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :type => Array, :allowed_from_server => false, :transform => DefaultSource.method(:convert_to_regexp_list), + :transformed_type => Array, :description => 'List of allowed endpoints to include in audit log.' }, :'audit_log.path' => { @@ -1089,6 +1092,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :type => Array, :allowed_from_server => false, :transform => proc { |arr| NewRelic::Agent.add_automatic_method_tracers(arr) }, + :transformed_type => Array, :description => <<~DESCRIPTION An array of `CLASS#METHOD` (for instance methods) and/or `CLASS.METHOD` (for class methods) strings representing Ruby methods that the agent can automatically add custom instrumentation to. This doesn't require any modifications of the source code that defines the methods. @@ -1587,6 +1591,7 @@ def self.notify :type => Array, :allowed_from_server => false, :transform => DefaultSource.method(:convert_to_regexp_list), + :transformed_type => Array, :description => %Q(Specifies a list of hostname patterns separated by commas that will match gRPC hostnames that traffic is to be ignored by New Relic for. New Relic's gRPC client instrumentation will ignore traffic streamed to a host matching any of these patterns, and New Relic's gRPC server instrumentation will ignore traffic for a server running on a host whose hostname matches any of these patterns. By default, no traffic is ignored when gRPC instrumentation is itself enabled. For example, `"private.com$,exception.*"`) }, :'instrumentation.grpc_server' => { @@ -1933,6 +1938,7 @@ def self.notify :type => Array, :allowed_from_server => false, :transform => DefaultSource.method(:convert_to_regexp_list), + :transformed_type => Array, :description => 'Specify an Array of Rake tasks to automatically instrument. ' \ 'This configuration option converts the Array to a RegEx list. If you\'d like ' \ 'to allow all tasks by default, use `rake.tasks: [.+]`. No rake tasks will be ' \ @@ -1953,6 +1959,7 @@ def self.notify :type => Array, :allowed_from_server => true, :transform => DefaultSource.method(:convert_to_regexp_list), + :transformed_type => Array, :description => 'Define transactions you want the agent to ignore, by specifying a list of patterns matching the URI you want to ignore. For more detail, see [the docs on ignoring specific transactions](/docs/agents/ruby-agent/api-guides/ignoring-specific-transactions/#config-ignoring).' }, # Serverless @@ -1962,6 +1969,7 @@ def self.notify :type => Boolean, :allowed_from_server => false, :transform => proc { |bool| NewRelic::Agent::ServerlessHandler.env_var_set? || bool }, + :transformed_type => Boolean, :description => 'If `true`, the agent will operate in a streamlined mode suitable for use with short-lived ' \ 'serverless functions. NOTE: Only AWS Lambda functions are supported currently and this ' \ "option is not intended for use without [New Relic's Ruby Lambda layer](https://docs.newrelic.com/docs/serverless-function-monitoring/aws-lambda-monitoring/get-started/monitoring-aws-lambda-serverless-monitoring/) offering." @@ -2080,6 +2088,7 @@ def self.notify :type => Array, :allowed_from_server => false, :transform => DefaultSource.method(:convert_to_constant_list), + :transformed_type => Array, :description => 'Specify a list of exceptions you do not want the agent to strip when [strip_exception_messages](#strip_exception_messages-enabled) is `true`. Separate exceptions with a comma. For example, `"ImportantException,PreserveMessageException"`.' }, # Thread profiler diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index b6d52e3a64..b9b2192a46 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -215,7 +215,7 @@ def type_coerce(key, value, category) # convert bool to string for regex usage and bool hash lookup value = value.to_s if type == Boolean if value.class != String - return value if category == :test + return value if category == :test || likely_transformed_already?(key, value) return default_with_warning(key, value, "Expected to receive a value of type #{type} but " \ "received #{value.class}.") @@ -233,6 +233,10 @@ def type_coerce(key, value, category) procedure.call(value) end + def likely_transformed_already?(key, value) + DEFAULTS.dig(key, :transformed_type) == value.class + end + def default_with_warning(key, value, msg) default = default_without_warning(key) NewRelic::Agent.logger.warn "Received an invalid '#{value}' value for the '#{key}' configuration " \ From 5c7a532d420de5e991ebfab327340ce33c1b0de4 Mon Sep 17 00:00:00 2001 From: fallwith Date: Thu, 10 Oct 2024 14:00:09 -0700 Subject: [PATCH 15/15] config logging test: expect a default value update the test to expect a default value to be used --- test/multiverse/suites/agent_only/logging_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/multiverse/suites/agent_only/logging_test.rb b/test/multiverse/suites/agent_only/logging_test.rb index 4e057d83f3..e6528fb59f 100644 --- a/test/multiverse/suites/agent_only/logging_test.rb +++ b/test/multiverse/suites/agent_only/logging_test.rb @@ -31,7 +31,7 @@ def test_logs_app_name def test_logs_error_with_bad_app_name running_agent_writes_to_log( {:app_name => false}, - 'No application name configured.' + 'Using the default value' ) end