From e7228580c0ca06374d77c52244bdafd1587a039e Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Tue, 28 Nov 2023 12:48:17 -0500 Subject: [PATCH 1/7] Refactor create_logger to remove || --- lib/vmdb/loggers.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/vmdb/loggers.rb b/lib/vmdb/loggers.rb index 50b51806654..9f5ed4d5c32 100644 --- a/lib/vmdb/loggers.rb +++ b/lib/vmdb/loggers.rb @@ -29,9 +29,13 @@ def self.apply_config(config) end def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) - create_container_logger(log_file_name, logger_class) || - create_journald_logger(log_file_name, logger_class) || + if MiqEnvironment::Command.is_container? + create_container_logger(log_file_name, logger_class) + elsif MiqEnvironment::Command.supports_systemd? + create_journald_logger(log_file_name, logger_class) + else create_file_logger(log_file_name, logger_class) + end end private_class_method def self.create_loggers @@ -59,8 +63,6 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) end private_class_method def self.create_raw_container_logger - return unless ENV["CONTAINER"] - require "manageiq/loggers/container" ManageIQ::Loggers::Container.new end @@ -72,8 +74,6 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) end private_class_method def self.create_raw_journald_logger - return unless MiqEnvironment::Command.supports_systemd? - require "manageiq/loggers/journald" ManageIQ::Loggers::Journald.new rescue LoadError From 9f8ddcdb2d257f6c8e53c312a032376e17a83ee0 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Tue, 28 Nov 2023 13:37:53 -0500 Subject: [PATCH 2/7] Extract log_file path building to common method --- lib/vmdb/loggers.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/vmdb/loggers.rb b/lib/vmdb/loggers.rb index 9f5ed4d5c32..83fff8e840b 100644 --- a/lib/vmdb/loggers.rb +++ b/lib/vmdb/loggers.rb @@ -49,8 +49,7 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) end private_class_method def self.create_file_logger(log_file, logger_class) - log_file = Pathname.new(log_file) if log_file.kind_of?(String) - log_file = ManageIQ.root.join("log", log_file) if log_file.try(:dirname).to_s == "." + log_file = log_path_from_file(log_file) progname = log_file.try(:basename, ".*").to_s logger_class.new(log_file, :progname => progname) @@ -80,13 +79,18 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) nil end + private_class_method def self.log_path_from_file(log_file) + log_file = Pathname.new(log_file) if log_file.kind_of?(String) + log_file = ManageIQ.root.join("log", log_file) if log_file.try(:dirname).to_s == "." + log_file + end + private_class_method def self.progname_from_file(log_file_name) File.basename(log_file_name, ".*") end private_class_method def self.create_wrapper_logger(log_file, logger_class, wrapped_logger) - log_file = Pathname.new(log_file) if log_file.kind_of?(String) - log_file = ManageIQ.root.join("log", log_file) if log_file.try(:dirname).to_s == "." + log_file = log_path_from_file(log_file) progname = log_file.try(:basename, ".*").to_s logger_class.new(nil, :progname => progname).tap do |logger| From 6ed92ad158ca517c9553475e54ea2b72d9d365cf Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Tue, 28 Nov 2023 13:50:19 -0500 Subject: [PATCH 3/7] Add a log_path_from_file helper method --- lib/vmdb/loggers.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/vmdb/loggers.rb b/lib/vmdb/loggers.rb index 83fff8e840b..1d9597c70f4 100644 --- a/lib/vmdb/loggers.rb +++ b/lib/vmdb/loggers.rb @@ -50,7 +50,7 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) private_class_method def self.create_file_logger(log_file, logger_class) log_file = log_path_from_file(log_file) - progname = log_file.try(:basename, ".*").to_s + progname = progname_from_file(log_file) logger_class.new(log_file, :progname => progname) end @@ -58,7 +58,8 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) private_class_method def self.create_container_logger(log_file_name, logger_class) return nil unless (logger = create_raw_container_logger) - create_wrapper_logger(log_file_name, logger_class, logger) + progname = progname_from_file(log_file_name) + create_wrapper_logger(progname, logger_class, logger) end private_class_method def self.create_raw_container_logger @@ -69,7 +70,8 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) private_class_method def self.create_journald_logger(log_file_name, logger_class) return nil unless (logger = create_raw_journald_logger) - create_wrapper_logger(log_file_name, logger_class, logger) + progname = progname_from_file(log_file_name) + create_wrapper_logger(progname, logger_class, logger) end private_class_method def self.create_raw_journald_logger @@ -86,13 +88,11 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) end private_class_method def self.progname_from_file(log_file_name) - File.basename(log_file_name, ".*") + log_file = Pathname.new(log_file) if log_file.kind_of?(String) + log_file.try(:basename, ".*").to_s end - private_class_method def self.create_wrapper_logger(log_file, logger_class, wrapped_logger) - log_file = log_path_from_file(log_file) - progname = log_file.try(:basename, ".*").to_s - + private_class_method def self.create_wrapper_logger(progname, logger_class, wrapped_logger) logger_class.new(nil, :progname => progname).tap do |logger| # HACK: In order to access the wrapped logger in test, we inject it as an instance var. if Rails.env.test? From 86b127a5329767fe35176c10973da87702d6063b Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Tue, 28 Nov 2023 13:55:30 -0500 Subject: [PATCH 4/7] Set the logger progname in the main methods --- lib/vmdb/loggers.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/vmdb/loggers.rb b/lib/vmdb/loggers.rb index 1d9597c70f4..4adb7757410 100644 --- a/lib/vmdb/loggers.rb +++ b/lib/vmdb/loggers.rb @@ -59,6 +59,8 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) return nil unless (logger = create_raw_container_logger) progname = progname_from_file(log_file_name) + logger.progname = progname + create_wrapper_logger(progname, logger_class, logger) end @@ -71,6 +73,8 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) return nil unless (logger = create_raw_journald_logger) progname = progname_from_file(log_file_name) + logger.progname = progname + create_wrapper_logger(progname, logger_class, logger) end @@ -104,8 +108,6 @@ def logger.wrapped_logger end logger.extend(ActiveSupport::Logger.broadcast(wrapped_logger)) - - wrapped_logger.progname = progname end end From b8aabc675acdf1555ebfe44756a5a759c69ca1a9 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Tue, 28 Nov 2023 14:20:11 -0500 Subject: [PATCH 5/7] Drop the raw_create_* logger methods --- lib/vmdb/loggers.rb | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/lib/vmdb/loggers.rb b/lib/vmdb/loggers.rb index 4adb7757410..b19cc4574ce 100644 --- a/lib/vmdb/loggers.rb +++ b/lib/vmdb/loggers.rb @@ -56,7 +56,8 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) end private_class_method def self.create_container_logger(log_file_name, logger_class) - return nil unless (logger = create_raw_container_logger) + require "manageiq/loggers/container" + logger = ManageIQ::Loggers::Container.new progname = progname_from_file(log_file_name) logger.progname = progname @@ -64,13 +65,9 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) create_wrapper_logger(progname, logger_class, logger) end - private_class_method def self.create_raw_container_logger - require "manageiq/loggers/container" - ManageIQ::Loggers::Container.new - end - private_class_method def self.create_journald_logger(log_file_name, logger_class) - return nil unless (logger = create_raw_journald_logger) + require "manageiq/loggers/journald" + logger = ManageIQ::Loggers::Journald.new progname = progname_from_file(log_file_name) logger.progname = progname @@ -78,13 +75,6 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) create_wrapper_logger(progname, logger_class, logger) end - private_class_method def self.create_raw_journald_logger - require "manageiq/loggers/journald" - ManageIQ::Loggers::Journald.new - rescue LoadError - nil - end - private_class_method def self.log_path_from_file(log_file) log_file = Pathname.new(log_file) if log_file.kind_of?(String) log_file = ManageIQ.root.join("log", log_file) if log_file.try(:dirname).to_s == "." From 811ea6707c373dcabb5d242f1bb832da5cb59dc8 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Tue, 28 Nov 2023 14:29:22 -0500 Subject: [PATCH 6/7] Return the raw logger unless logger_class is present --- lib/vmdb/loggers.rb | 7 ++++- spec/lib/vmdb/loggers_spec.rb | 57 +++++------------------------------ 2 files changed, 13 insertions(+), 51 deletions(-) diff --git a/lib/vmdb/loggers.rb b/lib/vmdb/loggers.rb index b19cc4574ce..14c4ee2ff3e 100644 --- a/lib/vmdb/loggers.rb +++ b/lib/vmdb/loggers.rb @@ -28,7 +28,7 @@ def self.apply_config(config) Vmdb::Plugins.each { |p| p.try(:apply_logger_config, config) } end - def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) + def self.create_logger(log_file_name, logger_class = nil) if MiqEnvironment::Command.is_container? create_container_logger(log_file_name, logger_class) elsif MiqEnvironment::Command.supports_systemd? @@ -52,6 +52,7 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) log_file = log_path_from_file(log_file) progname = progname_from_file(log_file) + logger_class ||= ManageIQ::Loggers::Base logger_class.new(log_file, :progname => progname) end @@ -62,6 +63,8 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) progname = progname_from_file(log_file_name) logger.progname = progname + return logger if logger_class.nil? + create_wrapper_logger(progname, logger_class, logger) end @@ -72,6 +75,8 @@ def self.create_logger(log_file_name, logger_class = ManageIQ::Loggers::Base) progname = progname_from_file(log_file_name) logger.progname = progname + return logger if logger_class.nil? + create_wrapper_logger(progname, logger_class, logger) end diff --git a/spec/lib/vmdb/loggers_spec.rb b/spec/lib/vmdb/loggers_spec.rb index 53153d9ae28..8525924d2cf 100644 --- a/spec/lib/vmdb/loggers_spec.rb +++ b/spec/lib/vmdb/loggers_spec.rb @@ -32,13 +32,6 @@ def in_container_env(example) subject { described_class.create_logger(log_file) } - let(:container_log) { subject.try(:wrapped_logger) } - - before do - # Hide the container logger output to STDOUT - allow(container_log.logdev).to receive(:write) if container_log - end - it "responds to #<<" do expect(subject).to respond_to(:<<) end @@ -64,11 +57,7 @@ def in_container_env(example) end it "#logdev" do - if container_log - expect(subject.logdev).to be_nil - else - expect(subject.logdev).to be_a Logger::LogDevice - end + expect(subject.logdev).to be_a Logger::LogDevice end describe "#datetime_format" do @@ -90,20 +79,8 @@ def in_container_env(example) subject.level = old_level end - it "forwards to the other loggers" do - expect(subject).to receive(:add).with(1, nil, "test message").and_call_original - expect(container_log).to receive(:add).with(1, nil, "test message").and_call_original if container_log - - subject.info("test message") - end - it "only forwards the message if the severity is correct" do - if container_log - expect(subject.logdev).to be_nil - expect(container_log.logdev).not_to receive(:write).with("test message") - else - expect(subject.logdev).not_to receive(:write).with("test message") - end + expect(subject.logdev).not_to receive(:write).with("test message") subject.debug("test message") end @@ -128,8 +105,7 @@ def in_container_env(example) context "#<<" do it "forwards to the other loggers" do - expect(subject).to receive(:<<).with("test message").and_call_original - expect(container_log).to receive(:<<).with("test message").and_call_original if container_log + expect(subject).to receive(:<<).with("test message")#.and_call_original subject << "test message" end @@ -139,12 +115,9 @@ def in_container_env(example) let(:log_file) { StringIO.new } it "logs correctly" do - expect(subject).to receive(:add).with(1, nil, "test message").and_call_original - expect(container_log).to receive(:add).with(1, nil, "test message").and_call_original if container_log + expect(subject).to receive(:add).with(1, nil, "test message") subject.info("test message") - - expect(log_file.string).to include("test message") unless container_log end end @@ -154,12 +127,9 @@ def in_container_env(example) after { log_file.delete if log_file.exist? } it "logs correctly" do - expect(subject).to receive(:add).with(1, nil, "test message").and_call_original - expect(container_log).to receive(:add).with(1, nil, "test message").and_call_original if container_log + expect(subject).to receive(:add).with(1, nil, "test message") subject.info("test message") - - expect(log_file.read).to include("test message") unless container_log end end @@ -169,31 +139,20 @@ def in_container_env(example) after { File.delete(log_file) if File.exist?(log_file) } it "logs correctly" do - expect(subject).to receive(:add).with(1, nil, "test message").and_call_original - expect(container_log).to receive(:add).with(1, nil, "test message").and_call_original if container_log + expect(subject).to receive(:add).with(1, nil, "test message") subject.info("test message") - - expect(File.read(log_file)).to include("test message") unless container_log end end end context "in a non-container environment" do - it "does not have a container logger" do - expect(container_log).to be_nil - end - include_examples "has basic logging functionality" end context "in a container environment" do around { |example| in_container_env(example) } - it "has a container logger" do - expect(container_log).to_not be_nil - end - include_examples "has basic logging functionality" end end @@ -212,12 +171,10 @@ def in_container_env(example) it "will honor the log level in the container logger" do log = described_class.create_logger(log_file_name) - container_log = log.wrapped_logger described_class.apply_config_value({:level_foo => :error}, log, :level_foo) - expect(log.level).to eq(Logger::ERROR) - expect(container_log.level).to eq(Logger::ERROR) + expect(log.level).to eq(Logger::ERROR) end end end From 515eefef294657babbd5df92a403bb9e88a9437b Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Tue, 28 Nov 2023 14:37:53 -0500 Subject: [PATCH 7/7] Remove wrapped_logger hack --- lib/vmdb/loggers.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lib/vmdb/loggers.rb b/lib/vmdb/loggers.rb index 14c4ee2ff3e..035d3658e15 100644 --- a/lib/vmdb/loggers.rb +++ b/lib/vmdb/loggers.rb @@ -93,15 +93,6 @@ def self.create_logger(log_file_name, logger_class = nil) private_class_method def self.create_wrapper_logger(progname, logger_class, wrapped_logger) logger_class.new(nil, :progname => progname).tap do |logger| - # HACK: In order to access the wrapped logger in test, we inject it as an instance var. - if Rails.env.test? - logger.instance_variable_set(:@wrapped_logger, wrapped_logger) - - def logger.wrapped_logger - @wrapped_logger - end - end - logger.extend(ActiveSupport::Logger.broadcast(wrapped_logger)) end end