Merge pull request #2878 from newrelic/presto_chango

type coercion for configuration parameters

lib/new_relic/agent/configuration/environment_source.rb

@@ -13,17 +13,15 @@ class EnvironmentSource < DottedHash
           /^NEW_RELIC_METADATA_/ # read by NewRelic::Agent::Connect::RequestBuilder
         ]
 
-        attr_accessor :alias_map, :type_map
+        attr_accessor :alias_map
 
         def initialize
           set_log_file
           set_config_file
 
           @alias_map = {}
-          @type_map = {}
 
           DEFAULTS.each do |config_setting, value|
-            self.type_map[config_setting] = value[:type]
             set_aliases(config_setting, value)
           end
 
@@ -70,43 +68,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
 

lib/new_relic/agent/configuration/manager.rb

@@ -17,6 +17,28 @@ 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]
+        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 }},
+                          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,74 +138,138 @@ 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], config_category(config.class))
+            rescue
+              next
             end
           end
 
           nil
         end
 
-        def evaluate_procs(value)
-          if value.respond_to?(:call)
-            instance_eval(&value)
-          else
-            value
-          end
-        end
+        def config_category(klass)
+          return :user if USER_CONFIG_CLASSES.include?(klass)
+          return :test if [DottedHash, Hash].include?(klass)
+          return :manual if klass == ManualSource
 
-        def evaluate_and_apply_transformations(key, value)
-          evaluated = evaluate_procs(value)
-          default = enforce_allowlist(key, evaluated)
-          return default if default
+          return :nr
+        end
 
-          boolean = enforce_boolean(key, value)
-          return boolean if [true, false].include?(boolean)
+        def evaluate_and_apply_transformations(key, value, category)
+          evaluated = value.respond_to?(:call) ? instance_eval(&value) : value
+          evaluated = type_coerce(key, evaluated, category)
+          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
+        def boolean?(type, value)
+          return false unless type == NewRelic::Agent::Configuration::Boolean
+
+          value.class == TrueClass || value.class == FalseClass
+        end
+
+        # 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)
+
+          false
+        end
+
+        def handle_nil_type(key, value, category)
+          return value if %i[manual test].include?(category)
+
+          # 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
+        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)
+
+          # 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 || likely_transformed_already?(key, value)
+
+            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
+
+          procedure = TYPE_COERCIONS.dig(type, :proc)
+          return value unless procedure
+
+          procedure.call(value)
         end
 
-        def enforce_allowlist(key, value)
-          return unless allowlist = default_source.allowlist_for(key)
-          return if allowlist.include?(value)
+        def likely_transformed_already?(key, value)
+          DEFAULTS.dig(key, :transformed_type) == value.class
+        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 enforce_boolean(key, value)
-          type = default_source.value_from_defaults(key, :type)
-          return unless type == Boolean
+        def default_without_warning(key)
+          default = DEFAULTS.dig(key, :default)
+          default.respond_to?(:call) ? default.call : default
+        end
 
-          bool_value = default_source.boolean_for(key, value)
-          return bool_value unless bool_value.nil?
+        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 = default_source.default_for(key)
-          NewRelic::Agent.logger.warn "Invalid value '#{value}' for #{key}, applying default value of '#{default}'"
-          default
+          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 transform_from_default(key)
-          default_source.transform_for(key)
+        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 default_source
@@ -197,17 +283,19 @@ 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|
+            next unless @callbacks.key?(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])
-            rescue
+              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[key]
             if evaluated_cache != evaluated_source
               @callbacks[key].each do |proc|
                 if direction == :add
@@ -256,8 +344,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

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?

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
 

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