Move logger initialization and configuration into plugins #20950

Fryguy · 2021-01-12T19:43:44Z

@agrare Please review.

This PR only accounts for the Amazon provider (ManageIQ/manageiq-providers-amazon#675) in order to work out any kinks before I blast out PRs to all of the individual plugins.

Part of #19440

Fryguy · 2021-01-12T19:56:24Z

lib/vmdb/loggers.rb

-      $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"))
+      # TODO: Move this into the manageiq-api plugin


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.

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?

Fryguy · 2021-01-12T20:04:21Z

The biggest change here is

now only the base loggers are set up initially when Vmdb::Loggers.init is called here
plugin loggers are not set up until Vmdb::Plugins.init is called here
All of the config changes are then applied to all loggers here

Fryguy · 2021-01-12T22:49:04Z

@jrafanie Can you also review here? I want to make sure that some of the primordial cases for having logs available aren't broken (like running rake tasks that need a logger, but not Rails env), but I'm not sure which one is a good candidate.

lib/vmdb/loggers.rb

Fryguy · 2021-01-13T21:33:14Z

@agrare Updated with your suggestion...WDYT?

Fryguy · 2021-01-13T21:37:28Z

Cool....I'll have to create a chain of PRs for all of the plugins, after which I can un-WIP

jrafanie · 2021-01-13T22:15:27Z

@jrafanie Can you also review here? I want to make sure that some of the primordial cases for having logs available aren't broken (like running rake tasks that need a logger, but not Rails env), but I'm not sure which one is a good candidate

It looks good so far. Have you tried a cross repo with the plugin changes? I think that will cover a lot of these cases including some of the rake tasks.

Fryguy · 2021-01-13T22:35:29Z

Have you tried a cross repo with the plugin changes?

Not yet...I have to make like 20 PRs to all of the plugins first.

jrafanie · 2021-01-13T22:42:02Z

Not yet...I have to make like 20 PRs to all of the plugins first.

hehe

Maybe you can try undoing some of the changes locally so only 1 or 2 plugins opt-in to this new behavior? I could be missing something but it seems like you could have it either internal/external just for sanity testing and then swap all of the external plugins to external for the final test and merge.

Fryguy · 2021-01-13T22:46:09Z

Yeah that's not a bad idea for the first pass. I wanted to avoid having to make a pair of PRs per plugin, but I could make this PR pass with a couple of plugins, then a follow-up PR with all of the rest.

Fryguy · 2021-01-13T22:51:20Z

Ok, @jrafanie Updated so that the only extracted logger is $aws_log. Ironically, I might have to undo that because I think $aws_log is used by FileDepotS3, so I might have to choose a different logger 😆

miq-bot · 2021-01-14T17:45:42Z

Checked commit Fryguy@297aa6f with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
4 files checked, 0 offenses detected
Everything looks fine. 🍰

Fryguy · 2021-01-14T17:52:16Z

Ironically, I might have to undo that because I think $aws_log is used by FileDepotS3, so I might have to choose a different logger 😆

I went a different approach and ensured that the logger was created. It's effectively a no-op, but in the future if we remove the provider plugin, then this code will continue to work.

Fryguy · 2021-01-14T17:54:32Z

@miq-bot cross_repo_test ManageIQ/manageiq-providers-amazon#675, manageiq-providers-azure, manageiq-api, manageiq-ui-classic

From Pull Request: ManageIQ/manageiq#20950

Fryguy · 2021-01-14T20:39:07Z

@jrafanie @agrare Cross repo is green

NickLaMuro

I am good with this concept. I have a few concerns, but honestly I think they are minor and/or probably not relevant.

NickLaMuro · 2021-01-14T20:58:47Z

lib/vmdb/loggers.rb

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

-    def self.configure_external_loggers
+    private_class_method def self.configure_external_loggers


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

NickLaMuro · 2021-01-14T21:16:36Z

lib/generators/manageiq/plugin/templates/lib/%plugin_path%/engine.rb

@@ -11,5 +11,13 @@ def self.vmdb_plugin?
    def self.plugin_name
      _('<%= plugin_human_name %>')
    end
+
+    def self.init_loggers
+      $<%= plugin_name %>_log ||= Vmdb::Loggers.create_logger("<%= plugin_name %>.log")


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.

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

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 -%>

NickLaMuro · 2021-01-14T21:19:09Z

lib/generators/manageiq/plugin/templates/lib/%plugin_path%/engine.rb

@@ -11,5 +11,13 @@ def self.vmdb_plugin?
    def self.plugin_name
      _('<%= plugin_human_name %>')
    end
+
+    def self.init_loggers


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.

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

NickLaMuro · 2021-01-14T21:20:50Z

lib/generators/manageiq/plugin/templates/lib/%plugin_path%/engine.rb

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


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.

Fryguy · 2021-01-14T22:13:58Z

@agrare @jrafanie @NickLaMuro Acutally, I'd like to do the commenting/removal of #20950 (comment) in the follow up PR when I blast this out to all of the other plugins. @agrare and I will be reviewing each PR and I think we'll have a better decision about defaulting to on or off based on our analysis of those PRs.

NickLaMuro · 2021-01-14T22:42:44Z

I am good with that. Seems like a good time/plan to determine if what I am saying is an edge case or a common occurrence.

I am cool with moving this forward as is.

Fryguy requested a review from gtanzillo as a code owner January 12, 2021 19:43

Fryguy added refactoring pluggability/providers pluggability and removed pluggability/providers labels Jan 12, 2021

Fryguy assigned agrare Jan 12, 2021

Fryguy mentioned this pull request Jan 12, 2021

Move logger initialization and configuration into plugins ManageIQ/manageiq-providers-amazon#675

Merged

miq-bot added the wip label Jan 12, 2021

Fryguy force-pushed the pluggable_loggers branch from b552043 to 51f25c7 Compare January 12, 2021 19:49

Fryguy requested a review from agrare as a code owner January 12, 2021 19:49

Fryguy commented Jan 12, 2021

View reviewed changes

agrare reviewed Jan 13, 2021

View reviewed changes

lib/vmdb/loggers.rb Outdated Show resolved Hide resolved

Fryguy force-pushed the pluggable_loggers branch from 51f25c7 to 5796a4e Compare January 13, 2021 21:32

agrare approved these changes Jan 13, 2021

View reviewed changes

Fryguy force-pushed the pluggable_loggers branch from 5796a4e to 092d277 Compare January 13, 2021 22:38

Fryguy force-pushed the pluggable_loggers branch from 092d277 to 3ff38e7 Compare January 13, 2021 22:42

Fryguy force-pushed the pluggable_loggers branch from 3ff38e7 to 2f6cbdd Compare January 13, 2021 22:50

Move logger initialization and configuration into plugins

297aa6f

Fryguy force-pushed the pluggable_loggers branch from 2f6cbdd to 297aa6f Compare January 14, 2021 17:44

Fryguy requested a review from NickLaMuro as a code owner January 14, 2021 17:44

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jan 14, 2021

Running tests for Fryguy

51b7373

From Pull Request: ManageIQ/manageiq#20950

miq-bot mentioned this pull request Jan 14, 2021

[BOT] Cross repo test for ManageIQ/manageiq#20950 ManageIQ/manageiq-cross_repo-tests#264

Closed

Fryguy changed the title ~~[WIP] Move logger initialization and configuration into plugins~~ Move logger initialization and configuration into plugins Jan 14, 2021

miq-bot removed the wip label Jan 14, 2021

NickLaMuro approved these changes Jan 14, 2021

View reviewed changes

agrare approved these changes Jan 15, 2021

View reviewed changes

agrare merged commit fb0610a into ManageIQ:master Jan 15, 2021

Fryguy deleted the pluggable_loggers branch January 15, 2021 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move logger initialization and configuration into plugins #20950

Move logger initialization and configuration into plugins #20950

Fryguy commented Jan 12, 2021 •

edited

Loading

Fryguy Jan 12, 2021

NickLaMuro Jan 14, 2021

Fryguy commented Jan 12, 2021 •

edited

Loading

Fryguy commented Jan 12, 2021

Fryguy commented Jan 13, 2021

Fryguy commented Jan 13, 2021

jrafanie commented Jan 13, 2021

Fryguy commented Jan 13, 2021

jrafanie commented Jan 13, 2021

Fryguy commented Jan 13, 2021

Fryguy commented Jan 13, 2021

miq-bot commented Jan 14, 2021

Fryguy commented Jan 14, 2021

Fryguy commented Jan 14, 2021

Fryguy commented Jan 14, 2021

NickLaMuro left a comment

NickLaMuro Jan 14, 2021

NickLaMuro Jan 14, 2021

Fryguy Jan 14, 2021

agrare Jan 15, 2021

NickLaMuro Jan 14, 2021

Fryguy Jan 14, 2021 •

edited

Loading

NickLaMuro Jan 14, 2021

Fryguy commented Jan 14, 2021

NickLaMuro commented Jan 14, 2021

Move logger initialization and configuration into plugins #20950

Move logger initialization and configuration into plugins #20950

Conversation

Fryguy commented Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jan 12, 2021 • edited Loading

Fryguy commented Jan 12, 2021

Fryguy commented Jan 13, 2021

Fryguy commented Jan 13, 2021

jrafanie commented Jan 13, 2021

Fryguy commented Jan 13, 2021

jrafanie commented Jan 13, 2021

Fryguy commented Jan 13, 2021

Fryguy commented Jan 13, 2021

miq-bot commented Jan 14, 2021

Fryguy commented Jan 14, 2021

Fryguy commented Jan 14, 2021

Fryguy commented Jan 14, 2021

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jan 14, 2021

NickLaMuro commented Jan 14, 2021

Fryguy commented Jan 12, 2021 •

edited

Loading

Fryguy commented Jan 12, 2021 •

edited

Loading

Fryguy Jan 14, 2021 •

edited

Loading