Add warnings for config file behaviors

Add warnings to stderr and the log file when some config combination is
used that may be expected to work, but does not. This is in preparation
of the YAML file removal and to encourage apps to use the
`config/appsignal.rb` config file and/or `Appsignal.configure` helper.

lib/appsignal.rb

@@ -160,11 +160,15 @@ def _load_config!(env_param = nil)
       # If there's a config/appsignal.rb file
       if context.dsl_config_file?
         if config
-          # Do not load it if a config is already present
-          internal_logger.warn(
-            "Ignoring `#{context.dsl_config_file}` file because " \
-              "`Appsignal.configure` has already been called."
-          )
+          # When calling `Appsignal.configure` from an app, not the
+          # `config/appsignal.rb` file, with also a Ruby config file present.
+          message = "The `Appsignal.configure` helper is called from within an " \
+            "app while a `#{context.dsl_config_file}` file is present. " \
+            "The `config/appsignal.rb` file is ignored when the " \
+            "config is loaded with `Appsignal.configure` from within an app. " \
+            "We recommend moving all config to the `config/appsignal.rb` file " \
+            "or the `Appsignal.configure` helper in the app."
+          Appsignal::Utils::StdoutAndLoggerMessage.warning(message)
         else
           # Load it when no config is present
           load_dsl_config_file(context.dsl_config_file, env_param)
@@ -289,6 +293,19 @@ def configure(env_param = nil, root_path: nil)
         )
       end
 
+      # When calling `Appsignal.configure` from a Rails initializer and a YAML
+      # file is present. We will not load the YAML file in the future.
+      if !config_file_context? && config.yml_config_file?
+        message = "The `Appsignal.configure` helper is called while a " \
+          "`config/appsignal.yml` file is present. In future versions the " \
+          "`config/appsignal.yml` file will be ignored when loading the " \
+          "config. We recommend moving all config to the " \
+          "`config/appsignal.rb` file, or the `Appsignal.configure` helper " \
+          "in Rails initializer file, and remove the " \
+          "`config/appsignal.yml` file."
+        Appsignal::Utils::StdoutAndLoggerMessage.warning(message)
+      end
+
       config_dsl = Appsignal::Config::ConfigDSL.new(config)
       return unless block_given?
 

lib/appsignal/config.rb

@@ -298,6 +298,16 @@ def load_config
       if @load_yaml_file
         @file_config = load_from_disk || {}
         merge(file_config)
+      elsif yml_config_file?
+        # When in a `config/appsignal.rb` file and it detects a
+        # `config/appsignal.yml` file.
+        # Only logged and printed on `Appsignal.start`.
+        message = "Both a Ruby and YAML configuration file are found. " \
+          "The `config/appsignal.yml` file is ignored when the " \
+          "config is loaded from `config/appsignal.rb`. Move all config to " \
+          "the `config/appsignal.rb` file and remove the " \
+          "`config/appsignal.yml` file."
+        Appsignal::Utils::StdoutAndLoggerMessage.warning(message)
       end
 
       # Load config from environment variables
@@ -463,6 +473,13 @@ def freeze
       config_hash.transform_values(&:freeze)
     end
 
+    # @api private
+    def yml_config_file?
+      return false unless config_file
+
+      File.exist?(config_file)
+    end
+
     private
 
     def logger
@@ -486,7 +503,7 @@ def detect_from_system
     end
 
     def load_from_disk
-      return if !config_file || !File.exist?(config_file)
+      return unless yml_config_file?
 
       read_options = YAML::VERSION >= "4.0.0" ? { :aliases => true } : {}
       configurations = YAML.load(ERB.new(File.read(config_file)).result, **read_options)

spec/lib/appsignal_spec.rb

@@ -5,9 +5,18 @@
   let(:transaction) { http_request_transaction }
 
   describe ".configure" do
-    context "when active" do
+    let(:root_path) { tmp_dir }
+    before do
+      log_dir = File.join(root_path, "log")
+      FileUtils.mkdir_p(log_dir)
+    end
+
+    context "when started" do
       it "doesn't update the config" do
-        start_agent
+        start_agent(
+          :root_path => root_path,
+          :options => { :active => true, :push_api_key => "dummy" }
+        )
         Appsignal::Testing.store[:config_called] = false
         expect do
           Appsignal.configure do |_config|
@@ -18,7 +27,10 @@
       end
 
       it "logs a warning" do
-        start_agent
+        start_agent(
+          :root_path => tmp_dir,
+          :options => { :active => true, :push_api_key => "dummy" }
+        )
         logs =
           capture_logs do
             Appsignal.configure do |_config|
@@ -34,7 +46,7 @@
 
     context "with config but not started" do
       it "reuses the already loaded config if no env arg is given" do
-        Appsignal.configure(:my_env, :root_path => project_fixture_path) do |config|
+        Appsignal.configure(:my_env, :root_path => root_path) do |config|
           config.ignore_actions = ["My action"]
         end
 
@@ -56,7 +68,7 @@
       end
 
       it "reuses the already loaded config if the env is the same" do
-        Appsignal.configure(:my_env, :root_path => project_fixture_path) do |config|
+        Appsignal.configure(:my_env, :root_path => root_path) do |config|
           config.ignore_actions = ["My action"]
         end
 
@@ -76,7 +88,7 @@
       end
 
       it "loads a new config if the env is not the same" do
-        Appsignal.configure(:my_env, :root_path => project_fixture_path) do |config|
+        Appsignal.configure(:my_env, :root_path => root_path) do |config|
           config.name = "Some name"
           config.push_api_key = "Some key"
           config.ignore_actions = ["My action"]
@@ -104,7 +116,7 @@
           config.ignore_actions = ["My action"]
         end
 
-        Appsignal.configure(:my_env, :root_path => project_fixture_path) do |config|
+        Appsignal.configure(:my_env, :root_path => root_path) do |config|
           expect(config.ignore_actions).to be_empty
           config.active = true
           config.name = "My app"
@@ -162,21 +174,41 @@
       end
 
       it "uses the given root path to read the config file" do
-        Appsignal.configure(:test, :root_path => project_fixture_path)
+        err_stream = std_stream
+        logs =
+          capture_logs do
+            capture_std_streams(std_stream, err_stream) do
+              Appsignal.configure(:test, :root_path => project_fixture_path)
+            end
+          end
         Appsignal.start
 
+        message = "The `Appsignal.configure` helper is called while a `config/appsignal.yml` " \
+          "file is present."
+        expect(logs).to contains_log(:warn, message)
+        expect(err_stream.read).to include("appsignal WARNING: #{message}")
         expect(Appsignal.config.env).to eq("test")
         expect(Appsignal.config[:push_api_key]).to eq("abc")
         # Ensure it loads from the config file in the given path
         expect(Appsignal.config.file_config).to_not be_empty
       end
 
-      it "loads the config without a block being given" do
-        Dir.chdir project_fixture_path do
-          Appsignal.configure(:test)
-        end
+      it "loads the config from the YAML file" do
+        err_stream = std_stream
+        logs =
+          capture_logs do
+            capture_std_streams(std_stream, err_stream) do
+              Dir.chdir project_fixture_path do
+                Appsignal.configure(:test)
+              end
+            end
+          end
         Appsignal.start
 
+        message = "The `Appsignal.configure` helper is called while a `config/appsignal.yml` " \
+          "file is present."
+        expect(logs).to contains_log(:warn, message)
+        expect(err_stream.read).to include("appsignal WARNING: #{message}")
         expect(Appsignal.config.env).to eq("test")
         expect(Appsignal.config[:push_api_key]).to eq("abc")
         # Ensure it loads from the config file in the current working directory
@@ -202,14 +234,6 @@
         end
       end
 
-      it "loads the config from the YAML file" do
-        Dir.chdir project_fixture_path do
-          Appsignal.configure(:test) do |config|
-            expect(config.name).to eq("TestApp")
-          end
-        end
-      end
-
       it "recognizes valid config" do
         Appsignal.configure(:my_env) do |config|
           config.push_api_key = "key"
@@ -243,6 +267,18 @@
         expect(Appsignal.config.env).to eq("env_env")
       end
 
+      it "reads config options from the environment" do
+        ENV["APPSIGNAL_APP_ENV"] = "env_env"
+        ENV["APPSIGNAL_APP_NAME"] = "AppNameFromEnv"
+        Appsignal.configure do |config|
+          expect(config.env).to eq("env_env")
+          expect(config.name).to eq("AppNameFromEnv")
+        end
+
+        expect(Appsignal.config.env).to eq("env_env")
+        expect(Appsignal.config[:name]).to eq("AppNameFromEnv")
+      end
+
       it "reads the environment from a loader default" do
         clear_integration_env_vars!
         define_loader(:loader_env) do
@@ -456,8 +492,17 @@ def on_load
         end
 
         ENV["APPSIGNAL_APP_PATH"] = test_path
-        Appsignal.start
+        err_stream = std_stream
+        logs =
+          capture_logs do
+            capture_std_streams(std_stream, err_stream) do
+              Appsignal.start
+            end
+          end
 
+        warning_message = "Both a Ruby and YAML configuration file are found."
+        expect(logs).to contains_log(:warn, warning_message)
+        expect(err_stream.read).to include("appsignal WARNING: #{warning_message}")
         expect(Appsignal.dsl_config_file_loaded?).to be(true)
         expect(Appsignal.config.root_path).to eq(test_path)
         expect(Appsignal.config[:active]).to be(false)
@@ -717,13 +762,18 @@ def on_start
         end
 
         ENV["APPSIGNAL_APP_PATH"] = test_path
-        logs = capture_logs { Appsignal.start }
+        err_stream = std_stream
+        logs =
+          capture_logs do
+            capture_std_streams(std_stream, err_stream) do
+              Appsignal.start
+            end
+          end
 
-        expect(logs).to contains_log(
-          :warn,
-          "Ignoring `#{config_file_path}` file because " \
-            "`Appsignal.configure` has already been called."
-        )
+        message = "The `Appsignal.configure` helper is called from within an " \
+          "app while a `#{config_file_path}` file is present."
+        expect(logs).to contains_log(:warn, message)
+        expect(err_stream.read).to include("appsignal WARNING: #{message}")
         expect(Appsignal.dsl_config_file_loaded?).to be(false)
         expect(Appsignal.config.root_path).to eq(project_fixture_path)
         expect(Appsignal.config[:active]).to be(false)