Fix MulticastLogger#datetime_formatter

[NickLaMuro]

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.

Links

• https://github.com/ruby/ruby/blob/c1718e988/lib/logger.rb#L300-L302
• https://github.com/ruby/ruby/blob/c1718e988/lib/logger.rb#L382

Steps for Testing/QA

1. Open up a Rails console
2. Run $azure_log.datetime_format

On master, it should blow up. With this patch, it should return nil.

[djberg96]

👍

[NickLaMuro]

The alternative for this is we could call super, but incase that would break other things, we decided it would be better to start with this change for now, since it will modify less things in doing so (since less things would be called in that super call). Up for debate though.

[NickLaMuro]

@miq-bot add_label bug

cc @bdunne @Fryguy

[bdunne]

This seems like the less intrusive change. Where did you notice this? Is there anything we can add tests for?

[Fryguy]

I'm going to 👎 this. I feel like adding more complexity is worse, and this is the second issue like this. Instead of playing whack-a-mole with missing methods, can we get rid of MulticastLogger < Logger and just delegate all calls instead?

[NickLaMuro]

Where did you notice this?

@djberg96 made note of it in the provider's channel, specifically here:

https://gitter.im/ManageIQ/manageiq/providers?at=5abab883f3f6d24c68c194fa

Is there anything we can add tests for?

Yes, sounds good... though, @Fryguy chimed in as I was typing here... so a difference of opinions has clearly surfaced...

[NickLaMuro]

Instead of playing whack-a-mole with missing methods, can we get rid of MulticastLogger < Logger and just delegate all calls instead?

So we (@Fryguy and myself) had a longer conversation about this in the gitter channel (shortly after the links in the description), but I think the general theme is that we probably want to move away from the MulticastLogger in it's current form.

My 2 cents: I think we should do that eventually, but I think that possibly merging and backporting this to G as a surgical fix, and making this change in prep for H makes the most sense to me (I think making a larger fix and trying to backport that is much riskier). That said, I think this plan making sense hinges on the use case that @djberg96 was having issues with that caused this PR to be open in the first place.

</2cents>

[NickLaMuro]

@bdunne Added some specs, a lot actually.

Based the other conversations, tried to make them "integration-y", where they are testing the global constants themselves, instead of a newly created instance to make sure what is used in the app are responding as expected.

This means they might be a little more brittle, and I had to do a decent amount of jerry-rigging to make them idempotent (cross your fingers I succeeded in that...), with the goal that it will hopefully allow us to swap out the underlying implementation of the MulticastLogger in the future if we decide to go that route, but still keep these tests as a safety net.

Let me know if you think I should test anything else while I am at it, as I don't think these tests are exhaustive by any means.

cc/ @Fryguy @djberg96

[NickLaMuro]

Update: Added #17227 as a POC for using ActiveSupport::Logger.broadcast instead of MulticastLogger. On the fence about it, and not sure it is a good idea, but just throwing that out there as a option for the #future™.

[bdunne]

Honestly, I'm less interested in refactoring and redesigning this. I think in master we can go the path that @jrafanie prefers and just delete this code. It was originally added to bridge the gap where we need to write both to a log file (for appliances) and STDOUT (for containers). Is there any reason we still need to log to files on master?

[djberg96]

@bdunne Does that mean you favor #17227? If not, what do you suggest?

[miq-bot]

Checked commits NickLaMuro/manageiq@26ed45e~...2a30a53 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. 🏆

[Fryguy]

Is there any reason we still need to log to files on master?

Because we're not going fully podified?

[Fryguy]

I favor #17227 personally.

[NickLaMuro]

I favor #17227 personally.

For master, I agree. Again, my #2cents™ above was more suggesting that this should be a backport PR (but get it into master) to then be overwritten by #17227 . For what it is worth, #17227 uses the commits in this PR as well, along with a LOT of the tests.

That said, #17228 by @djberg96 might address his concerns just fine, which his issues were the main reason I opened this up, and this might only be tangentially related (not sure myself since I am not looking at it from a usage perspective like Dan is).