From 200e822d8620535c9e19eb2ad3e63a63110f0b8e Mon Sep 17 00:00:00 2001 From: Tom de Bruijn 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. 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: - When 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: - When Appsignal.start is called. - When `Appsignal.configure` has been called beforehand. - The `config/appsignal.rb` file is not required. - Scenario 4: - When Appsignal.configure is called. - The `config/appsignal.yml` file is present. - The `config/appsignal.yml` file is loaded. - `Appsignal.start` is called. - Scenario 5: - When Appsignal.configure is called. - The `config/appsignal.yml` file is present. - The `config/appsignal.yml` file is 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. 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. --- .../add-config-appsignal-rb-file-support.md | 19 ++ lib/appsignal.rb | 75 +++++++- lib/appsignal/config.rb | 33 +++- spec/lib/appsignal_spec.rb | 174 ++++++++++++++++++ spec/support/helpers/directory_helper.rb | 6 + spec/support/testing.rb | 2 + 6 files changed, 303 insertions(+), 6 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..6124e609b 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,7 +117,24 @@ def start # rubocop:disable Metrics/AbcSize internal_logger.debug("Loading AppSignal gem") - @config ||= Config.new(Config.determine_root_path, Config.determine_env) + context = Appsignal::Config::Context.new( + :env => Config.determine_env, + :root_path => Config.determine_root_path + ) + if config + if context.dsl_config_file? + internal_logger.warn( + "Ignoring `#{context.dsl_config_file}` file because " \ + "`Appsignal.configure` has already been called." + ) + end + else + load_dsl_config_file(context) + # Load config manually if no DSL config file was found + @config ||= Config.new(context.root_path, context.env) + end + # Disable AppSignal on config file error + Appsignal.config[:active] = false if config_file_error? @config.validate _start_logger @@ -244,7 +268,11 @@ 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. + :load_yaml_file => !config_file_context? ) end @@ -397,6 +425,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 +441,42 @@ 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(context) + return if defined?(@dsl_config_file_loaded) + return unless context.dsl_config_file? + + ENV["_APPSIGNAL_CONFIG_FILE_CONTEXT"] = "true" + @dsl_config_file_loaded = true + require context.dsl_config_file + rescue => error + @config_file_error = error + message = "Not starting AppSignal because an error occurred while " \ + "loading the AppSignal config file.\n" \ + "File: #{context.dsl_config_file.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..a8975b604 100644 --- a/spec/lib/appsignal_spec.rb +++ b/spec/lib/appsignal_spec.rb @@ -373,6 +373,141 @@ 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 "doesn't start if config/appsignal.rb raised an error" 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 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 +631,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!