From 41f9e9d5143273f584e1ad5f044e274a8d78cb15 Mon Sep 17 00:00:00 2001
From: Tom de Bruijn <tom@tomdebruijn.com>
Date: Thu, 31 Oct 2024 14:32:04 +0100
Subject: [PATCH] Add config/appsignal.rb config file

Add `config/appsignal.rb` config file support. This is a replacement for
the `config/appsignal.yml` config file.

Load the `config/appsignal.rb` file automatically on `Appsignal.start`.
For most apps, like Rails apps, this will work the same way as
`config/appsignal.yml` does now.

If `Appsignal.configure` is called manually in the app, like a Rails
initializer, the `config/appsignal.rb` file is not loaded. Instead, we
recommend moving the `Appsignal.configure` call to the
`config/appsignal.rb` file.

## New behavior

With this change the config is loaded as described in these scenarios:

- Scenario 1:
  - `Appsignal.start` is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.rb` is required and configures the gem.
- Scenario 2:
  - `Appsignal.start` is called.
  - No config is loaded yet.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.rb` is required and configures the gem using
    the `Appsignal.configure` helper.
  - The `config/appsignal.yml` is not loaded.
- Scenario 3:
  - `Appsignal.configure` is called.
  - `Appsignal.start` is called.
  - The `config/appsignal.rb` file is not required.
- Scenario 4:
  - `Appsignal.configure` is called.
  - The `config/appsignal.yml` file is present.
  - The `config/appsignal.yml` file is loaded.
  - `Appsignal.start` is called.
- Scenario 5:
  - `Appsignal.configure` is called.
  - The `config/appsignal.rb` file is present.
  - The `config/appsignal.rb` file is not loaded.
  - `Appsignal.start` is called.

This is more complex that I would have liked. In hindsight I shouldn't
have made `Appsignal.configure` load the `config/appsignal.yml` file.
That would have made it a bit more predictable. But I don't want
`Appsignal.configure` in the `config/appsignal.rb` file to also load the
`config/appsignal.yml` file, because then it just gets more confusing.

In the next major version I want to remove `config/appsignal.yml`
support, so this is the first step.

## App/root path

I remember some of our users configured the Config class's `root_path`
to specify where to load the config file from.
If an app configures itself via a `config/appsignal.rb`, this makes it
impossible to customize the root path. For that purpose I've added the
`APPSIGNAL_APP_PATH` environment variable to customize the config path.
Which I've also used to make it easier to test the different scenarios.

---

Part of #1324
---
 .../add-config-appsignal-rb-file-support.md   |  19 ++
 lib/appsignal.rb                              |  85 ++++++-
 lib/appsignal/config.rb                       |  33 ++-
 spec/lib/appsignal_spec.rb                    | 234 ++++++++++++++++++
 spec/support/helpers/directory_helper.rb      |   6 +
 spec/support/testing.rb                       |   2 +
 6 files changed, 371 insertions(+), 8 deletions(-)
 create mode 100644 .changesets/add-config-appsignal-rb-file-support.md

diff --git a/.changesets/add-config-appsignal-rb-file-support.md b/.changesets/add-config-appsignal-rb-file-support.md
new file mode 100644
index 000000000..514374630
--- /dev/null
+++ b/.changesets/add-config-appsignal-rb-file-support.md
@@ -0,0 +1,19 @@
+---
+bump: minor
+type: add
+---
+
+Add `config/appsignal.rb` config file support. When a `config/appsignal.rb` file is present in the app, the Ruby gem will automatically load it when `Appsignal.start` is called.
+
+The `config/appsignal.rb` config file is a replacement for the `config/appsignal.yml` config file. When both files are present, only the `config/appsignal.rb` config file is loaded.
+
+Example `config/appsignal.rb` config file:
+
+```ruby
+# config/appsignal.rb
+Appsignal.configure do |config|
+  config.name = "My app name"
+end
+```
+
+It is not possible to configure multiple environments in the `config/appsignal.rb` config file as it was possible in the YAML version.
diff --git a/lib/appsignal.rb b/lib/appsignal.rb
index 1147efd02..668cd6bbd 100644
--- a/lib/appsignal.rb
+++ b/lib/appsignal.rb
@@ -92,7 +92,7 @@ def testing?
     #
     # @return [void]
     # @since 0.7.0
-    def start # rubocop:disable Metrics/AbcSize
+    def start # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity, Metrics/MethodLength, Metrics/CyclomaticComplexity
       if ENV.fetch("_APPSIGNAL_DIAGNOSE", false)
         internal_logger.warn("Skipping start in diagnose context")
         return
@@ -103,6 +103,13 @@ def start # rubocop:disable Metrics/AbcSize
         return
       end
 
+      if config_file_context?
+        internal_logger.warn(
+          "Ignoring call to Appsignal.start in config file context."
+        )
+        return
+      end
+
       unless extension_loaded?
         internal_logger.info("Not starting AppSignal, extension is not loaded")
         return
@@ -110,12 +117,35 @@ def start # rubocop:disable Metrics/AbcSize
 
       internal_logger.debug("Loading AppSignal gem")
 
-      @config ||= Config.new(Config.determine_root_path, Config.determine_env)
-      @config.validate
+      context = Appsignal::Config::Context.new(
+        :env => Config.determine_env,
+        :root_path => Config.determine_root_path
+      )
+      # 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."
+          )
+        else
+          # Load it when no config is present
+          load_dsl_config_file(context.dsl_config_file)
+          # Disable AppSignal on config file error
+          config[:active] = false if config && config_file_error?
+        end
+      else
+        # Load config if no config file was found and no config is present yet
+        # This will load the config/appsignal.yml file automatically
+        @config ||= Config.new(context.root_path, context.env)
+      end
+      # Validate the config, if present
+      config&.validate
 
       _start_logger
 
-      if config.valid?
+      if config&.valid?
         if config.active?
           @started = true
           internal_logger.info "Starting AppSignal #{Appsignal::VERSION} " \
@@ -244,7 +274,12 @@ def configure(env_param = nil, root_path: nil)
       else
         @config = Config.new(
           root_path_param || Config.determine_root_path,
-          Config.determine_env(env_param)
+          Config.determine_env(env_param),
+          # If in the context of an `config/appsignal.rb` config file, do not
+          # load the `config/appsignal.yml` file.
+          # The `.rb` file is a replacement for the `.yml` file so it shouldn't
+          # load both.
+          :load_yaml_file => !config_file_context?
         )
       end
 
@@ -397,6 +432,11 @@ def active?
       config&.active? && extension_loaded?
     end
 
+    # @api private
+    def dsl_config_file_loaded?
+      defined?(@dsl_config_file_loaded) ? true : false
+    end
+
     private
 
     def params_match_loaded_config?(env_param, root_path_param)
@@ -408,6 +448,41 @@ def params_match_loaded_config?(env_param, root_path_param)
         (root_path_param.nil? || config.root_path == root_path_param)
     end
 
+    # Load the `config/appsignal.rb` config file, if present.
+    #
+    # If the config file has already been loaded once and it's trying to be
+    # loaded more than once, which should never happen, it will not do
+    # anything.
+    def load_dsl_config_file(path)
+      return if defined?(@dsl_config_file_loaded)
+
+      ENV["_APPSIGNAL_CONFIG_FILE_CONTEXT"] = "true"
+      @dsl_config_file_loaded = true
+      require path
+    rescue => error
+      @config_file_error = error
+      message = "Not starting AppSignal because an error occurred while " \
+        "loading the AppSignal config file.\n" \
+        "File: #{path.inspect}\n" \
+        "#{error.class.name}: #{error}"
+      Kernel.warn "appsignal ERROR: #{message}"
+      internal_logger.error "#{message}\n#{error.backtrace.join("\n")}"
+    ensure
+      ENV.delete("_APPSIGNAL_CONFIG_FILE_CONTEXT")
+    end
+
+    # Returns true if we're currently in the `config/appsignal.rb` file
+    # context.
+    def config_file_context?
+      ENV.fetch("_APPSIGNAL_CONFIG_FILE_CONTEXT", nil) == "true"
+    end
+
+    # Returns true if loading the `config/appsignal.rb` file resulted in an
+    # error.
+    def config_file_error?
+      defined?(@config_file_error) ? true : false
+    end
+
     def start_internal_stdout_logger
       @internal_logger = Appsignal::Utils::IntegrationLogger.new($stdout)
       internal_logger.formatter = log_formatter("appsignal")
diff --git a/lib/appsignal/config.rb b/lib/appsignal/config.rb
index e2a9cfe85..d3cf1b35d 100644
--- a/lib/appsignal/config.rb
+++ b/lib/appsignal/config.rb
@@ -53,6 +53,9 @@ def self.determine_env(initial_env = nil)
     # Determine which root path AppSignal should initialize with.
     # @api private
     def self.determine_root_path
+      app_path_env_var = ENV.fetch("APPSIGNAL_APP_PATH", nil)
+      return app_path_env_var if app_path_env_var
+
       loader_defaults.reverse.each do |loader_defaults|
         root_path = loader_defaults[:root_path]
         return root_path if root_path
@@ -61,6 +64,26 @@ def self.determine_root_path
       Dir.pwd
     end
 
+    # @api private
+    class Context
+      DSL_FILENAME = "config/appsignal.rb"
+
+      attr_reader :env, :root_path
+
+      def initialize(env: nil, root_path: nil)
+        @env = env
+        @root_path = root_path
+      end
+
+      def dsl_config_file
+        File.join(root_path, DSL_FILENAME)
+      end
+
+      def dsl_config_file?
+        File.exist?(dsl_config_file)
+      end
+    end
+
     # @api private
     DEFAULT_CONFIG = {
       :activejob_report_errors => "all",
@@ -213,8 +236,10 @@ def self.determine_root_path
     #   How to integrate AppSignal manually
     def initialize(
       root_path,
-      env
+      env,
+      load_yaml_file: true
     )
+      @load_yaml_file = load_yaml_file
       @root_path = root_path.to_s
       @config_file_error = false
       @config_file = config_file
@@ -269,8 +294,10 @@ def load_config
       @initial_config[:env] = @env
 
       # Load the config file if it exists
-      @file_config = load_from_disk || {}
-      merge(file_config)
+      if @load_yaml_file
+        @file_config = load_from_disk || {}
+        merge(file_config)
+      end
 
       # Load config from environment variables
       @env_config = load_from_environment
diff --git a/spec/lib/appsignal_spec.rb b/spec/lib/appsignal_spec.rb
index 222a69c78..84fd742fc 100644
--- a/spec/lib/appsignal_spec.rb
+++ b/spec/lib/appsignal_spec.rb
@@ -373,6 +373,201 @@ def on_load
 
         expect(Appsignal.config.env).to eq("env_env")
       end
+
+      it "reads the config/appsignal.rb file if present" do
+        test_path = File.join(tmp_dir, "config_file_test_1")
+        FileUtils.mkdir_p(test_path)
+        Dir.chdir test_path do
+          config_contents =
+            <<~CONFIG
+              Appsignal.configure do |config|
+                config.active = false
+                config.name = "DSL app"
+                config.push_api_key = "config_file_push_api_key"
+                config.ignore_actions << "Test"
+              end
+            CONFIG
+          write_file(File.join(test_path, "config", "appsignal.rb"), config_contents)
+        end
+
+        ENV["APPSIGNAL_APP_PATH"] = test_path
+        Appsignal.start
+
+        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)
+        expect(Appsignal.config[:name]).to eq("DSL app")
+        expect(Appsignal.config[:push_api_key]).to eq("config_file_push_api_key")
+        expect(Appsignal.config[:ignore_actions]).to include("Test")
+      ensure
+        FileUtils.rm_rf(test_path)
+      end
+
+      it "ignores calls to Appsignal.start from config/appsignal.rb" do
+        test_path = File.join(tmp_dir, "config_file_test_2")
+        FileUtils.mkdir_p(test_path)
+        Dir.chdir test_path do
+          config_contents =
+            <<~CONFIG
+              Appsignal.configure do |config|
+                config.active = false
+                config.name = "DSL app"
+              end
+              Appsignal.start
+            CONFIG
+          write_file(File.join(test_path, "config", "appsignal.rb"), config_contents)
+        end
+
+        ENV["APPSIGNAL_APP_PATH"] = test_path
+        logs = capture_logs { Appsignal.start }
+
+        expect(logs)
+          .to contains_log(:warn, "Ignoring call to Appsignal.start in config file context.")
+        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)
+        expect(Appsignal.config[:name]).to eq("DSL app")
+      ensure
+        FileUtils.rm_rf(test_path)
+      end
+
+      it "only reads from config/appsignal.rb if it and config/appsignal.yml are present" do
+        test_path = File.join(tmp_dir, "config_file_test_3")
+        FileUtils.mkdir_p(test_path)
+        Dir.chdir test_path do
+          config_contents =
+            <<~CONFIG
+              Appsignal.configure(:test) do |config|
+                config.active = false
+                config.name = "DSL app"
+                config.push_api_key = "config_file_push_api_key"
+              end
+            CONFIG
+          write_file(File.join(test_path, "config", "appsignal.rb"), config_contents)
+
+          yaml_contents =
+            <<~YAML
+              test:
+                active: true
+                name: "YAML app"
+                ignore_errors: ["YAML error"]
+            YAML
+          write_file(File.join(test_path, "config", "appsignal.yml"), yaml_contents)
+        end
+
+        ENV["APPSIGNAL_APP_PATH"] = test_path
+        Appsignal.start
+
+        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)
+        expect(Appsignal.config[:name]).to eq("DSL app")
+        expect(Appsignal.config[:push_api_key]).to eq("config_file_push_api_key")
+        expect(Appsignal.config[:ignore_errors]).to_not include("YAML error")
+      ensure
+        FileUtils.rm_rf(test_path)
+      end
+
+      it "only reads from config/appsignal.rb even if it's empty" do
+        test_path = File.join(tmp_dir, "config_file_test_3")
+        FileUtils.mkdir_p(test_path)
+        Dir.chdir test_path do
+          config_contents = "# I am empty!"
+          write_file(File.join(test_path, "config", "appsignal.rb"), config_contents)
+
+          yaml_contents =
+            <<~YAML
+              test:
+                active: true
+                name: "YAML app"
+                ignore_errors: ["YAML error"]
+            YAML
+          write_file(File.join(test_path, "config", "appsignal.yml"), yaml_contents)
+        end
+
+        ENV["APPSIGNAL_APP_PATH"] = test_path
+        Appsignal.start
+
+        expect(Appsignal.dsl_config_file_loaded?).to be(true)
+        # No Appsignal.configure was called, so it's misconfigured, but it
+        # shouldn't fall back on the YAML file.
+        expect(Appsignal.config).to be_nil
+      ensure
+        FileUtils.rm_rf(test_path)
+      end
+
+      it "options set in config/appsignal.rb are leading" do
+        test_path = File.join(tmp_dir, "config_file_test_4")
+        FileUtils.mkdir_p(test_path)
+        Dir.chdir test_path do
+          config_contents =
+            <<~CONFIG
+              Appsignal.configure(:test) do |config|
+                config.active = true
+                config.name = "DSL app"
+                config.push_api_key = "config_file_push_api_key"
+              end
+            CONFIG
+          write_file(File.join(test_path, "config", "appsignal.rb"), config_contents)
+        end
+
+        ENV["APPSIGNAL_APP_PATH"] = test_path
+        # These env vars should not be used as the config option values
+        ENV["APPSIGNAL_APP_ENV"] = "env_env"
+        ENV["APPSIGNAL_APP_NAME"] = "env_name"
+        ENV["APPSIGNAL_PUSH_API_KEY"] = "env_push_api_key"
+        Appsignal.start
+
+        expect(Appsignal.dsl_config_file_loaded?).to be(true)
+        expect(Appsignal.config.root_path).to eq(test_path)
+        expect(Appsignal.config.env).to eq("test")
+        expect(Appsignal.config[:active]).to be(true)
+        expect(Appsignal.config[:name]).to eq("DSL app")
+        expect(Appsignal.config[:push_api_key]).to eq("config_file_push_api_key")
+      ensure
+        FileUtils.rm_rf(test_path)
+      end
+
+      it "doesn't start if config/appsignal.rb raised an error" do
+        test_path = File.join(tmp_dir, "config_file_test_5")
+        FileUtils.mkdir_p(test_path)
+        Dir.chdir test_path do
+          config_contents =
+            <<~CONFIG
+              Appsignal.configure do |config|
+                config.active = true
+                config.name = "DSL app"
+                config.push_api_key = "config_file_push_api_key"
+              end
+              raise "uh oh" # Deliberatly crash
+            CONFIG
+          write_file(File.join(test_path, "config", "appsignal.rb"), config_contents)
+        end
+
+        ENV["APPSIGNAL_APP_PATH"] = test_path
+        err_stream = std_stream
+        logs =
+          capture_std_streams(std_stream, err_stream) do
+            capture_logs do
+              Appsignal.start
+            end
+          end
+
+        message =
+          "Not starting AppSignal because an error occurred while loading the " \
+            "AppSignal config file.\n" \
+            "File: \"#{File.join(test_path, "config/appsignal.rb")}\"\n" \
+            "RuntimeError: uh oh\n"
+        expect(logs).to contains_log(:error, message)
+        expect(err_stream.read).to include("appsignal ERROR: #{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) # Disables the config on error
+        expect(Appsignal.config[:name]).to eq("DSL app")
+        expect(Appsignal.config[:push_api_key]).to eq("config_file_push_api_key")
+      ensure
+        FileUtils.rm_rf(test_path)
+      end
     end
 
     context "when config is loaded" do
@@ -496,6 +691,45 @@ def on_start
           end
         end
       end
+
+      it "doesn't load config/appsignal.rb if Appsignal.configure was called beforehand" do
+        Appsignal.configure do |config|
+          config.active = false
+          config.name = "DSL app"
+          config.push_api_key = "dsl_push_api_key"
+        end
+
+        test_path = File.join(tmp_dir, "config_file_test_5")
+        FileUtils.mkdir_p(test_path)
+        config_file_path = File.join(test_path, "config", "appsignal.rb")
+        Dir.chdir test_path do
+          config_contents =
+            <<~CONFIG
+              Appsignal.configure do |config|
+                config.active = false
+                config.name = "DSL app"
+                config.push_api_key = "config_file_push_api_key"
+              end
+            CONFIG
+          write_file(config_file_path, config_contents)
+        end
+
+        ENV["APPSIGNAL_APP_PATH"] = test_path
+        logs = capture_logs { Appsignal.start }
+
+        expect(logs).to contains_log(
+          :warn,
+          "Ignoring `#{config_file_path}` file because " \
+            "`Appsignal.configure` has already been called."
+        )
+        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)
+        expect(Appsignal.config[:name]).to eq("DSL app")
+        expect(Appsignal.config[:push_api_key]).to eq("dsl_push_api_key")
+      ensure
+        FileUtils.rm_rf(test_path)
+      end
     end
 
     context "when already started" do
diff --git a/spec/support/helpers/directory_helper.rb b/spec/support/helpers/directory_helper.rb
index 109e8e8ac..2380b2fcc 100644
--- a/spec/support/helpers/directory_helper.rb
+++ b/spec/support/helpers/directory_helper.rb
@@ -17,6 +17,12 @@ def tmp_dir
     @tmp_dir ||= File.join(spec_dir, "tmp")
   end
 
+  def write_file(path, contents)
+    parent_dir = File.dirname(path)
+    FileUtils.mkdir_p(parent_dir)
+    File.write(path, contents)
+  end
+
   def fixtures_dir
     @fixtures_dir ||= File.join(support_dir, "fixtures")
   end
diff --git a/spec/support/testing.rb b/spec/support/testing.rb
index 8fbb184e7..3e980a64c 100644
--- a/spec/support/testing.rb
+++ b/spec/support/testing.rb
@@ -24,6 +24,8 @@ def clear_config!
 
     # @api private
     def clear!
+      remove_instance_variable(:@dsl_config_file_loaded) if defined?(@dsl_config_file_loaded)
+      remove_instance_variable(:@config_file_error) if defined?(@config_file_error)
       Appsignal.internal_logger = nil
 
       clear_started!