Add RequestLogger from AutomationEngine

[agrare]

Add a Vmdb::Loggers::RequestLogger based on the AutomationEngine::Logger

• Commit 1: straight copy from automation_engine/logger just copying it into Vmdb::Loggers::RequestLogger
• Commit 2: rename automation_log_wrapper to just log_wrapper, drop the .create_log_wrapper class method since that was hard-coded to use the automation logger and it makes sense to just do this in a subclass
• Commit 3: rubocop -A on request_logger.rb

Follow-ups:

• Can we set resource_id in the initializer or do we need to log to different resources on each call?
• Can we use ManageIQ::Loggers.wrap

Related:

• Set logger for each workflow run manageiq-providers-workflows#128

[agrare]

@miq-bot cross-repo-test ManageIQ/manageiq-automation_engine#569

[agrare]

Okay this is passing cross-repo

[agrare]

NOTE one thing I don't like about this approach is that it requires that the caller modify all calls to the logger, instead of passing it a logger instance that has everything already set up.

If instead we had e.g. a logger instance created with the object we're referencing then we can use normal logger.info calls. We can also use standard wrapped loggers and pass the request logger into code that otherwise wouldn't have to know about it (aka Floe)

[Fryguy]

Yeah, that's what we ended up having to do, but I don't think we can actually do the other way around as you're proposing.

[agrare]

Hm that might be a bit of an issue for using this more generally then. I was playing with overriding the automate subclass of this logger to log using this style maybe I go further down that path.

[kbrock]

I feel strongly about using a standard interface.

Would really prefer having the resource_id hardcoded in the logger (initializer) Like I did in #23406

This current implementation doesn't work for me

[Fryguy]

Can we set resource_id in the initializer or do we need to log to different resources on each call?

No, the way it was built is that it was a single logger than could handle an extra parameter. When the request_id parameter was sent as part of the log message it writes to both the request logs table and the automation log, otherwise it only writes to the automation log.

This was because the request code jumps all over the place including different workers, and goes through serialization and deserialization such as through the workspace. As such, the only constant was the request id. I think it would be weird if we had to initialize a new logger with a request id just to call a single info call and then throw it away.

[agrare]

No, the way it was built is that it was a single logger than could handle an extra parameter. When the request_id parameter was sent as part of the log message it writes to both the request logs table and the automation log, otherwise it only writes to the automation log.

This was because the request code jumps all over the place including different workers, and goes through serialization and deserialization such as through the workspace. As such, the only constant was the request id. I think it would be weird if we had to initialize a new logger with a request id just to call a single info call and then throw it away.

Yeah no issue with the request_id being the constant because of serialize/deserialize, not sure if instantiating a wrapped logger when you want to use it is weird since having an extra log parameter that changes if it logs to one place or two places is also kinda weird... but if it is too odd then maybe we leave the automate one the way it is and have this one in core work a different way if we had to

[Fryguy]

An example might be helpful: https://github.com/ManageIQ/manageiq-automation_engine/blob/master/lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_method.rb#L91

So here, this is a class method that invokes automate. We have the obj.workspace, and we pull out the miq_request_id out of it, then pass it over to the automation logger. I guess we could put that logger inside the workspace and call obj.workloger.logger.info and then we wouldn't need the request_id. I'm not certain if all of the other callers go through the workspace - we'd have to investigate them.

[Fryguy]

BTW, I'm not against your idea of flipping it around - it was gross to me at the beginning having to change all these caller in the automation engine to be request-aware, but I couldn't figure it out at the time.

[kbrock]

good news: half of logging asks the Workspace for the request_id. Instead we can ask for Workspace#request_logger and just use the standard logging interface.

This feels like a simple solution to get us closer to using the standard logging interface.

For this to happen, we would need to change this logger to setting the request_id in the initializer, use the standard info/debug/warn parameters, and the logger would be responsible for passing that onto the database.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[agrare]

Replaced by #23426