Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move logger initialization and configuration into plugins #20950

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Move logger initialization and configuration into plugins #20950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions .rubocop_local.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,31 +5,34 @@ AllCops:
Style/GlobalVars:
AllowedVariables:
# Loggers
- $ansible_tower_log
- $api_log
- $audit_log
- $aws_log
- $azure_log
- $azure_stack_log
- $cn_monitoring_log
- $container_log
- $datawarehouse_log
- $fog_log
- $gce_log
- $ibm_cloud_log
- $ibm_terraform_log
- $journald_log
- $kube_log
- $lenovo_log
- $log
- $miq_ae_logger
- $nsxt_log
- $nuage_log
- $policy_log
- $rails_log
- $redfish_log
- $remote_console_log
- $rhevm_log
- $scvmm_log
- $vcloud_log
- $vim_log
- $remote_console_log
- $nuage_log
- $nsxt_log
- $redfish_log
Rails/Exit:
Exclude:
- lib/workers/bin/*
Expand Down
2 changes: 2 additions & 0 deletions app/models/file_depot_s3.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ def connect(options = {})
# Note: The hard-coded aws_region will be removed after manageiq-ui-class implements region selection
aws_region = options[:region] || "us-east-1"

$aws_log ||= Vmdb::Loggers.create_logger("aws.log")

Aws::S3::Resource.new(
:access_key_id => username,
:secret_access_key => ManageIQ::Password.try_decrypt(password),
Expand Down
8 changes: 8 additions & 0 deletions lib/generators/manageiq/plugin/templates/lib/%plugin_path%/engine.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,13 @@ def self.vmdb_plugin?
def self.plugin_name
_('<%= plugin_human_name %>')
end

def self.init_loggers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using this comment (and any other ones as well) for reference prior to giving my own thoughts:

#20950 (comment)

But one downside to this approach is that a plugin author can technically do whatever the heck they feel like in this section, and it could have nothing to do with loggers. Highly doubt it will be a problem, but just something that came to mind as I was looking at this.

Copy link
Member Author

@Fryguy Fryguy Jan 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we want them to do whatever they want (like set up custom loggers for client gems or something)

$<%= plugin_name %>_log ||= Vmdb::Loggers.create_logger("<%= plugin_name %>.log")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we default this line to be commented out to start? Just seems like it makes sense to have this opt in to start, and if a more isolated logger is required by the plugin author, the interface is easily adjusted in the code base.

Just trying to avoid "log-splosion" in the log/ dir, with a bunch of unused files and debugging turns into checking a bunch of files that in fact are just empty. But possibly this is a necessary concern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coincidentally, just talked about this with @agrare and yeah I agree. I am going to change this to commented out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think the ideal would be command line option for the provider generator like:

rails generate manageiq:plugin [--logger]/[--no-logger]

And then this could become:

<% if options[:logger] -%>
    def self.init_loggers
      ...
    end
<% end -%>

end

def self.apply_logger_config(config)
Vmdb::Loggers.apply_config_value(config, $<%= plugin_name %>_log, :level_<%= plugin_name %>)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you go with my suggestion above for commenting the contents of init_loggers out, you would probably want to do that here as well.

end
end
<% end %>
89 changes: 47 additions & 42 deletions lib/vmdb/loggers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,15 @@ def self.apply_config(config)
apply_config_value(config, $log, :level)
apply_config_value(config, $journald_log, :level) if $journald_log
apply_config_value(config, $rails_log, :level_rails)
apply_config_value(config, $ansible_tower_log, :level_ansible_tower)
apply_config_value(config, $policy_log, :level_policy)
apply_config_value(config, $remote_console_log, :level_remote_console)

# TODO: Move this into the manageiq-api plugin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because of https://github.com/ManageIQ/manageiq-api/blob/5683d9ebd1d708d0be3257296d081595c203f1ac/lib/manageiq/api/engine.rb#L9-L18, which I can solve eventually, but am skipping for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that this comment is here prior to the suggestion by @jrafanie to only do a subset of loggers first:

#20950 (comment)

And this is less relevant of a "FYI" now that we are really only moving out two of the loggers for this first pass, correct?

apply_config_value(config, $api_log, :level_api)
# TODO: Move this into the manageiq-automation_engine plugin
apply_config_value(config, $miq_ae_logger, :level_automation)
apply_config_value(config, $aws_log, :level_aws)
# TODO: Move these to their respective provider plugins
apply_config_value(config, $ansible_tower_log, :level_ansible_tower)
apply_config_value(config, $azure_log, :level_azure)
apply_config_value(config, $azure_stack_log, :level_azure_stack)
apply_config_value(config, $cn_monitoring_log, :level_cn_monitoring)
Expand All @@ -39,57 +44,59 @@ def self.apply_config(config)
apply_config_value(config, $lenovo_log, :level_lenovo)
apply_config_value(config, $nsxt_log, :level_nsxt)
apply_config_value(config, $nuage_log, :level_nuage)
apply_config_value(config, $policy_log, :level_policy)
apply_config_value(config, $redfish_log, :level_redfish)
apply_config_value(config, $rhevm_log, :level_rhevm)
apply_config_value(config, $scvmm_log, :level_scvmm)
apply_config_value(config, $vcloud_log, :level_vcloud)
apply_config_value(config, $vim_log, :level_vim)
apply_config_value(config, $remote_console_log, :level_remote_console)

Vmdb::Plugins.each { |p| p.try(:apply_logger_config, config) }
end

private_class_method def self.create_loggers
path_dir = ManageIQ.root.join("log")
def self.create_logger(log_file_name, logger_class = VMDBLogger)
log_file = ManageIQ.root.join("log", log_file_name)
logger_class.new(log_file).tap do |logger|
logger.extend(ActiveSupport::Logger.broadcast($container_log)) if $container_log
logger.extend(ActiveSupport::Logger.broadcast($journald_log)) if $journald_log
end
end

private_class_method def self.create_loggers
$container_log = create_container_logger
$journald_log = create_journald_logger
$log = create_multicast_logger(path_dir.join("evm.log"))
$rails_log = create_multicast_logger(path_dir.join("#{Rails.env}.log"))
$audit_log = create_multicast_logger(path_dir.join("audit.log"), AuditLogger)
$api_log = create_multicast_logger(path_dir.join("api.log"))
$ansible_tower_log = create_multicast_logger(path_dir.join("ansible_tower.log"))
$miq_ae_logger = create_multicast_logger(path_dir.join("automation.log"))
$aws_log = create_multicast_logger(path_dir.join("aws.log"))
$azure_log = create_multicast_logger(path_dir.join("azure.log"), ProviderSdkLogger)
$azure_stack_log = create_multicast_logger(path_dir.join("azure_stack.log"))
$cn_monitoring_log = create_multicast_logger(path_dir.join("container_monitoring.log"))
$datawarehouse_log = create_multicast_logger(path_dir.join("datawarehouse.log"))
$fog_log = create_multicast_logger(path_dir.join("fog.log"), FogLogger)
$gce_log = create_multicast_logger(path_dir.join("gce.log"))
$ibm_cloud_log = create_multicast_logger(path_dir.join("ibm_cloud.log"), ProviderSdkLogger)
$ibm_terraform_log = create_multicast_logger(path_dir.join("ibm_terraform.log"), ProviderSdkLogger)
$kube_log = create_multicast_logger(path_dir.join("kubernetes.log"))
$lenovo_log = create_multicast_logger(path_dir.join("lenovo.log"))
$nsxt_log = create_multicast_logger(path_dir.join("nsxt.log"))
$nuage_log = create_multicast_logger(path_dir.join("nuage.log"))
$policy_log = create_multicast_logger(path_dir.join("policy.log"))
$redfish_log = create_multicast_logger(path_dir.join("redfish.log"))
$rhevm_log = create_multicast_logger(path_dir.join("rhevm.log"))
$scvmm_log = create_multicast_logger(path_dir.join("scvmm.log"))
$vcloud_log = create_multicast_logger(path_dir.join("vcloud.log"))
$vim_log = create_multicast_logger(path_dir.join("vim.log"))
$remote_console_log = create_multicast_logger(path_dir.join("remote_console.log"))
$log = create_logger("evm.log")
$rails_log = create_logger("#{Rails.env}.log")
$audit_log = create_logger("audit.log", AuditLogger)
$policy_log = create_logger("policy.log")
$remote_console_log = create_logger("remote_console.log")

# TODO: Move this into the manageiq-api plugin
$api_log = create_logger("api.log")
# TODO: Move this into the manageiq-automation_engine plugin
$miq_ae_logger = create_logger("automation.log")
# TODO: Move these to their respective provider plugins
$ansible_tower_log = create_logger("ansible_tower.log")
$azure_log = create_logger("azure.log", ProviderSdkLogger)
$azure_stack_log = create_logger("azure_stack.log")
$cn_monitoring_log = create_logger("container_monitoring.log")
$datawarehouse_log = create_logger("datawarehouse.log")
$fog_log = create_logger("fog.log", FogLogger)
$gce_log = create_logger("gce.log")
$ibm_cloud_log = create_logger("ibm_cloud.log", ProviderSdkLogger)
$ibm_terraform_log = create_logger("ibm_terraform.log", ProviderSdkLogger)
$kube_log = create_logger("kubernetes.log")
$lenovo_log = create_logger("lenovo.log")
$nsxt_log = create_logger("nsxt.log")
$nuage_log = create_logger("nuage.log")
$redfish_log = create_logger("redfish.log")
$rhevm_log = create_logger("rhevm.log")
$scvmm_log = create_logger("scvmm.log")
$vcloud_log = create_logger("vcloud.log")
$vim_log = create_logger("vim.log")

configure_external_loggers
end

private_class_method def self.create_multicast_logger(log_file_path, logger_class = VMDBLogger)
logger_class.new(log_file_path).tap do |logger|
logger.extend(ActiveSupport::Logger.broadcast($container_log)) if $container_log
logger.extend(ActiveSupport::Logger.broadcast($journald_log)) if $journald_log
end
end

private_class_method def self.create_container_logger
return unless ENV["CONTAINER"]

Expand All @@ -106,7 +113,7 @@ def self.apply_config(config)
nil
end

def self.configure_external_loggers
private_class_method def self.configure_external_loggers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crystal fanboy-ism coming into play here... 😏 💎

require 'awesome_spawn'
AwesomeSpawn.logger = $log

Expand All @@ -116,7 +123,6 @@ def self.configure_external_loggers
require 'inventory_refresh'
InventoryRefresh.logger = $log
end
private_class_method :configure_external_loggers

def self.apply_config_value(config, logger, key)
old_level = logger.level
Expand All @@ -127,7 +133,6 @@ def self.apply_config_value(config, logger, key)
logger.level = new_level
end
end
private_class_method :apply_config_value
end
end

Expand Down
7 changes: 7 additions & 0 deletions lib/vmdb/plugins.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ def each(&block)

def init
load_inflections
init_loggers
register_models
end

Expand Down Expand Up @@ -89,6 +90,12 @@ def load_inflections
end
end

def init_loggers
each do |engine|
engine.try(:init_loggers)
end
end

def register_models
each do |engine|
# make sure STI models are recognized
Expand Down
14 changes: 7 additions & 7 deletions spec/lib/vmdb/loggers_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ def in_container_env(example)
$container_log = nil
end

describe "#create_multicast_logger (private)" do
describe ".create_logger" do
shared_examples "has basic logging functionality" do
subject { described_class.send(:create_multicast_logger, log_file) }
subject { described_class.create_logger(log_file) }

before do
# Hide the container logger output to STDOUT
Expand Down Expand Up @@ -121,14 +121,14 @@ def in_container_env(example)
end
end

describe "#apply_config_value (private)" do
describe ".apply_config_value" do
before do
allow($log).to receive(:info)
end

it "will update the main lower level logger instance" do
log = described_class.send(:create_multicast_logger, log_file)
described_class.send(:apply_config_value, {:foo => :info}, log, :foo)
log = described_class.create_logger(log_file)
described_class.apply_config_value({:foo => :info}, log, :foo)

expect(log.level).to eq(Logger::INFO)
end
Expand All @@ -137,8 +137,8 @@ def in_container_env(example)
around { |example| in_container_env(example) }

it "will always keep $container_log as DEBUG" do
log = described_class.send(:create_multicast_logger, log_file)
described_class.send(:apply_config_value, {:foo => :info}, log, :foo)
log = described_class.create_logger(log_file)
described_class.apply_config_value({:foo => :info}, log, :foo)

expect(log.level).to eq(Logger::INFO)
expect($container_log.level).to eq(Logger::DEBUG)
Expand Down