[POC] Use ActiveSupport::Logger.broadcast instead of MulticastLogger #17227

NickLaMuro · 2018-03-29T01:15:09Z

Makes a switch to use ActiveSupport::Logger.broadcast which is already used to by the rails console to handle printing to both STDOUT and the log file for the rails development.log. This just makes use of the same functionality that is there that is pretty much being replicated in the MulticastLogger that we put together ourselves.

Minor difference that shouldn't matter: We now only control the level of the main logger, and that passes it to other loggers via super. That really didn't matter since we don't multicast to more than two loggers, and the top one is the one is the only one that matters. The $container_log will always be debug anyway, so no need for the level wrapper method that we had in MulticastLogger.

Links

Based off of Fix MulticastLogger#datetime_formatter #17223
Diff between this branch and the above: diff

Steps for Testing/QA

Right now, just using tests for QA, but we might want to spin up some containers and see if things are still working as expected.

The code for Ruby's `Logger#datetime_formatter` is as follows: def datetime_format @default_formatter.datetime_format end In the MulticastLogger, neither `@default_formatter` is set or `super` called to set this in the default manner, so this value is always `nil` when this method is called on `MulticastLogger` and thus returns an error. This fixes that by setting `@default_formatter` to `VMDBLogger::Formatter.new` even though it will never be used to log anything, simply because this was what is used by default in the other loggers. Set `@formatter` to `nil` as well as that is also what is done in ruby's source in `Logger#initialize` (to avoid potential warnings).

Fryguy · 2018-03-29T17:45:40Z

Very Interesting idea! @bdunne Please review.

Fryguy · 2018-03-29T17:47:52Z

lib/vmdb/loggers.rb

@@ -132,11 +132,9 @@ def self.create_loggers
    private_class_method :create_loggers

    def self.create_multicast_logger(log_file_path, logger_class = VMDBLogger)
-      logger_instance = logger_class.new(log_file_path).tap do |logger|
+      logger_class.new(log_file_path).tap do |logger|
        logger.level = Logger::DEBUG


Do we still need this settings of DEBUG line? If we create the logger and leave it with the default, it should Just Work™

Probably don't need this now. I can remove.

Fryguy · 2018-03-29T17:49:32Z

spec/lib/vmdb/loggers_spec.rb

+    before do
+      allow($container_log.logdev).to receive(:write)
+    end
+    before(:all) do


I think this before(:all) is not necessary, because we don't run the tests with ENV["CONTAINER"] set.

Oh, nvm. I completely misread this because of the shared examples being passed a logger. The structure of the test is confusing in that respect. Perhaps if logger is just replaced with a differently named variable like test_logger or something, then it is apparent it's not any built-in logger (which is how I read it).

Yeah, logger is coming in from the shared example bit. Changing it to test_logger or logger_being_tested (probably the former) is a simple enough change. That said I would probably have to change it in the tests I added in #17223 first.

Fryguy · 2018-03-29T17:54:10Z

spec/lib/vmdb/loggers_spec.rb

+    end
+
+    it "responds to #<<" do
+      expect(subject.respond_to?(:<<)).to be true


expect(subject).to respond_to(:<<)

Maybe?

Didn't know that was an rspec thing. TIL.

Can fix.

Fryguy · 2018-03-29T17:54:38Z

spec/lib/vmdb/loggers_spec.rb

+    end
+
+    describe "#datetime_format" do
+      it "works as expected" do


Don't think you need the outer describe here, though this test is kind of useless?

@Fryguy This was addressing/testing the issue that @djberg96 had where the method existed, but blew up because @default_formatter was not set.

Again, though, linking back to my reminder (and mentioned in the description) that this is based off of #17223, so these tests that you are commenting on really should be done in that PR. The point of this branch was to show the changes what was needed to swap this out, and that minimal changes to the tests that were needed to do so. The diff makes it obvious that there was very little in the tests that I needed to change to make this work.

Don't think you need the outer describe here...

⚠️ Lengthy pedantic justification detected! ⚠️

This one is more of a preference in how I like to write specs, but there are two points to it:

the alternative of it "#datetime_format works as expected" reads super dump, and I think it is stupid (sorry, not sugar coating my opinion on this one).

While this is "more to type", it lends it self better to working with multiple checks for a given method and branching contexts.

To my discredit, I did this one poorly. This really should have been two tests (or more), so instead of "works as expected", I would/should have done "doesn't raise and error" and "returns nil". In the document format for rspec, it then reads as:

#date_format doesn't raise an error returns nil

If I had a context for @default_formatter.datetime_format = "%Y%m%d%H%M", then I would read:

#date_format doesn't raise an error returns nil when @default_formatter.datetime_format = "%Y%m%d%H%M" returns "%Y%m%d%H%M"

Personally, I would rather maintain a style of specs that can be added to without refactoring, and work in a consistent manner, regardless if I am testing with one it block or many for a particular method. Also sets up a consistent framework when writing tests that a new comer can walk in on and add to easily, compared it "#datetime_format works as expected" which just promotes chaos.

I was more coming from the viewpoint of "if there's only one test in a describe, and the only test has it 'works'", then I think it's useless to have the outer describe, when it could just be the "it". But, if the intent was to have more tests, then this is fine.

NickLaMuro · 2018-03-29T17:55:08Z

Just a reminder that this is built off of #17223, so looking at the diff of the two branches might make this a little easier to grok.

Adds specs for each of the global loggers around the multicast logger based loggers. Written in a way where we should be able to swap out what we are using under the hood for Multicast logger, and the tests should be a litmus test if we are doing everything correct. Borrowed a decent number of tests from lib/vmdb/loggers/multicast_logger_spec.rb since that has the potential to go away, but we want to test the functionality from that spec file on all of the existing loggers. Hopefully most of the specs here are idempotent, but we will see.

miq-bot · 2018-03-29T21:26:45Z

Checked commits NickLaMuro/manageiq@26ed45e~...6910718 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

Makes a switch to use `ActiveSupport::Logger.broadcast` which is already used to by the `rails console` to handle printing to both STDOUT and the terminal for the rails log. This just makes use of the same functionality that is there that is pretty much being replicated in the MulticastLogger that we put together ourselves. Minor difference that shouldn't matter: We now only control the level of the main logger, but that really didn't matter since we don't multicast to more than two loggers, and the top one is the one is the only one that matters. The `$container_log` will always be debug anyway, so no need for the level wrap that we had in `MulticastLogger`.

Fryguy · 2018-04-02T16:16:53Z

Any reason this is still WIP/POC? Seems like a valid approach

NickLaMuro · 2018-04-02T16:45:11Z

@Fryguy From @bdunne: #17223 (comment)

But, I can un-WIP at this point I guess.

Fryguy · 2018-04-03T21:20:25Z

@NickLaMuro That comment ignores the fact that we still have to support appliances, and thus log files. We can't just delete the code. My terse response was here #17223 (comment) 😄

bdunne

Since we can't delete it yet, let's give this a try 👍

NickLaMuro · 2018-05-25T16:16:33Z

@Fryguy @bdunne Going over some of my old PRs and trying to get them cleaned up, this being one of them

From where it looks like we left this one off a month ago, we were basically cool with merging this. That said, in my comment in the related PR, I think merging should happen after merging #17223 since that one is better suited for a backport (if ever necessary).

That said, this PR is built off of this one, so it will probably get auto merged anyway with this one.

¯\(°_o)/¯

djberg96 · 2018-05-29T12:26:47Z

👍

kbrock

Nice ✂️ ✂️ ✂️

miq-bot changed the title ~~[POC][WIP] Use ActiveSupport::Logger.broadcast instead of MulticastLogger~~ [WIP] [POC][WIP] Use ActiveSupport::Logger.broadcast instead of MulticastLogger Mar 29, 2018

miq-bot added the wip label Mar 29, 2018

NickLaMuro changed the title ~~[WIP] [POC][WIP] Use ActiveSupport::Logger.broadcast instead of MulticastLogger~~ [WIP] [POC] Use ActiveSupport::Logger.broadcast instead of MulticastLogger Mar 29, 2018

NickLaMuro mentioned this pull request Mar 29, 2018

Fix MulticastLogger#datetime_formatter #17223

Merged

Fryguy requested a review from bdunne March 29, 2018 17:44

Fryguy reviewed Mar 29, 2018

View reviewed changes

NickLaMuro force-pushed the swap_out_multicast_logger_for_active_support_logger_broadcast branch 3 times, most recently from 6c5c3f9 to 6910718 Compare March 29, 2018 21:24

NickLaMuro added 2 commits March 29, 2018 16:26

Removes MulticastLogger

6910718

NickLaMuro mentioned this pull request Apr 2, 2018

Add specs to show save! supports string keys #17238

Merged

NickLaMuro changed the title ~~[WIP] [POC] Use ActiveSupport::Logger.broadcast instead of MulticastLogger~~ [POC] Use ActiveSupport::Logger.broadcast instead of MulticastLogger Apr 2, 2018

miq-bot removed the wip label Apr 2, 2018

bdunne approved these changes Apr 6, 2018

View reviewed changes

kbrock approved these changes May 29, 2018

View reviewed changes

bdunne merged commit a2d4fe1 into ManageIQ:master Jul 25, 2018

bdunne self-assigned this Jul 25, 2018

bdunne added the gaprindashvili/no label Jul 25, 2018

bdunne added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] Use ActiveSupport::Logger.broadcast instead of MulticastLogger #17227

[POC] Use ActiveSupport::Logger.broadcast instead of MulticastLogger #17227

NickLaMuro commented Mar 29, 2018 •

edited

Fryguy commented Mar 29, 2018

Fryguy Mar 29, 2018

NickLaMuro Mar 29, 2018

Fryguy Mar 29, 2018

Fryguy Mar 29, 2018

NickLaMuro Mar 29, 2018

Fryguy Mar 29, 2018

NickLaMuro Mar 29, 2018

Fryguy Mar 29, 2018

NickLaMuro Mar 29, 2018 •

edited

NickLaMuro Mar 29, 2018 •

edited

Fryguy Mar 29, 2018

NickLaMuro commented Mar 29, 2018

miq-bot commented Mar 29, 2018

Fryguy commented Apr 2, 2018

NickLaMuro commented Apr 2, 2018

Fryguy commented Apr 3, 2018 •

edited

bdunne left a comment

NickLaMuro commented May 25, 2018

djberg96 commented May 29, 2018

kbrock left a comment

[POC] Use ActiveSupport::Logger.broadcast instead of MulticastLogger #17227

[POC] Use ActiveSupport::Logger.broadcast instead of MulticastLogger #17227

Conversation

NickLaMuro commented Mar 29, 2018 • edited

Links

Steps for Testing/QA

Fryguy commented Mar 29, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro Mar 29, 2018 • edited

Choose a reason for hiding this comment

NickLaMuro Mar 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented Mar 29, 2018

miq-bot commented Mar 29, 2018

Fryguy commented Apr 2, 2018

NickLaMuro commented Apr 2, 2018

Fryguy commented Apr 3, 2018 • edited

bdunne left a comment

Choose a reason for hiding this comment

NickLaMuro commented May 25, 2018

djberg96 commented May 29, 2018

kbrock left a comment

Choose a reason for hiding this comment

NickLaMuro commented Mar 29, 2018 •

edited

NickLaMuro Mar 29, 2018 •

edited

NickLaMuro Mar 29, 2018 •

edited

Fryguy commented Apr 3, 2018 •

edited