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

Jump to bottom

Add logging to RequestLog model #516

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Add logging to RequestLog model #516

merged 1 commit into from
Feb 14, 2023

Conversation

DavidResende0
Copy link
Member

Parent Issue: ManageIQ/manageiq#21188
Continuation of: #503

@miq-bot miq-bot added the wip label Jan 6, 2023
@DavidResende0 DavidResende0 force-pushed the request_logs branch 3 times, most recently from 5e44217 to 0094f34 Compare January 23, 2023 18:50
@DavidResende0
Copy link
Member Author

@miq-bot assign @Fryguy

This was referenced Jan 23, 2023
Closed
Closed
@DavidResende0 DavidResende0 force-pushed the request_logs branch 2 times, most recently from 59fe421 to 216592a Compare February 1, 2023 20:49
@DavidResende0 DavidResende0 mentioned this pull request Feb 2, 2023
Merged
@DavidResende0 DavidResende0 changed the title [WIP] Adding Logs for Requests Adding Logs for Requests Feb 7, 2023
@miq-bot miq-bot removed the wip label Feb 7, 2023
@DavidResende0 DavidResende0 changed the title Adding Logs for Requests [WIP] Adding Logs for Requests Feb 7, 2023
@miq-bot miq-bot added the wip label Feb 7, 2023
@DavidResende0 DavidResende0 changed the title [WIP] Adding Logs for Requests Adding Logs for Requests Feb 8, 2023
@miq-bot miq-bot removed the wip label Feb 8, 2023
@DavidResende0 DavidResende0 force-pushed the request_logs branch 4 times, most recently from cabe711 to dce93fd Compare February 9, 2023 22:43
@Fryguy
Copy link
Member

Fryguy commented Feb 10, 2023

Discussed with @DavidResende0 and we are skipping the bot suggestion because the current code stays consistent with the other trivial accessors in the file.

Fryguy
Fryguy reviewed Feb 10, 2023
View reviewed changes
lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_method.rb Outdated
@@ -218,10 +233,14 @@ def self.run_method(cmd)
yield stdin if block_given?
stdin.close
threads << Thread.new do
stdout.each_line { |msg| $miq_ae_logger.info("Method STDOUT: #{msg.strip}") }
stdout.each_line do |msg|
$request_log.request_info("Method STDOUT: #{msg.strip}", {:resource_id => miq_request_id}, false)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm - this didn't come out the way I'd expect and I think I led you down the wrong path.

I think maybe instead of RequestLogger, we should have modified the AutomateLogger to accept a request_id. Then the AutomateLogger would additionally write to the Request table (or a simpler RequestLogger).

@Fryguy Fryguy force-pushed the request_logs branch 2 times, most recently from d657a69 to 04a905b Compare February 11, 2023 04:31
Fryguy
Fryguy reviewed Feb 11, 2023
View reviewed changes
lib/miq_automation_engine/engine/miq_ae_engine.rb
@@ -78,7 +78,6 @@ def self.deliver(*args)
begin
miq_task&.state_active
object_name = "#{options[:object_type]}.#{options[:object_id]}"
_log.info("Delivering #{ManageIQ::Password.sanitize_string(options[:attrs].inspect)} for object [#{object_name}] with state [#{state}] to Automate")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this one - I'm concerned it's going to be logged too late if we do it after resolve_automation_object

Copy link
Member

Choose a reason for hiding this comment

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

We my want to keep this line as "Attempting to Deliver". Then later between the create_automation_object and resolve_automation_object, we can try to get the request_id from the uri.

@Fryguy
Copy link
Member

Fryguy commented Feb 11, 2023 •
edited
Loading

@DavidResende0 I updated a bunch of stuff here - Going to look at the core changes and regular $log changes tomorrow, but I think I might keep the $request_log, but instead make it just delegate to $log.

Too tired tonight though 😴

Fryguy
Fryguy reviewed Feb 13, 2023
View reviewed changes
lib/manageiq/automation_engine/logger.rb
message = progname
progname = @progname
end
end
Copy link
Member

Choose a reason for hiding this comment

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

I wish that was extracted in upstream Ruby - I'm considering pull this out into a private method upstream.

Copy link
Member

@Fryguy Fryguy Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, what they're probably going to suggest is to create a different LogDevice and Formatter. I might consider that.

@Fryguy
Copy link
Member

Fryguy commented Feb 13, 2023

@DavidResende0 I'm seeing some issues locally caused by #519, but those are unrelated. Hoping to get that merged first, then I can rebase this.

@Fryguy Fryguy changed the title Adding Logs for Requests Add logging to RequestLog model Feb 13, 2023
@Fryguy Fryguy force-pushed the request_logs branch 2 times, most recently from b2bff93 to 8442a27 Compare February 13, 2023 22:31
@akhilkr128 @DavidResende0 @Fryguy
Add logging to RequestLog model
f28f0a2 
Co-Authored-By: DavidResende0 <david.resende@ibm.com>
Co-Authored-By: Jason Frey <fryguy9@gmail.com>
@miq-bot
Copy link
Member

miq-bot commented Feb 14, 2023

Checked commit DavidResende0@f28f0a2 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
18 files checked, 13 offenses detected

lib/manageiq/automation_engine/logger.rb

lib/miq_automation_engine/engine/miq_ae_engine.rb

spec/miq_ae_service_spec.rb

jrafanie
jrafanie reviewed Feb 14, 2023
View reviewed changes
lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_workspace_runtime.rb
miq_request_id = nil
if args['MiqRequest::miq_request']
miq_request_id = args['MiqRequest::miq_request']
elsif args['vmdb_object_type'].to_s.include?("task")
Copy link
Member

Choose a reason for hiding this comment

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

Can you verify the vmdb_object_type is lowercase all the time?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah just checked and can confirm that vmdb_object_type is always lowercase. It always looks something like vm_retire_task, vm_retire_request, or vm

jrafanie
jrafanie reviewed Feb 14, 2023
View reviewed changes
lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_workspace_runtime.rb
elsif !defined?(root.attributes[root.attributes['vmdb_object_type']].object.miq_request_id).nil?
root.attributes[root.attributes['vmdb_object_type']].object.miq_request_id
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a rescue here so at the very worst, we log something without a resource_id. We probably don't want to blow up automation when trying to cut a record and log it.

jrafanie
jrafanie reviewed Feb 14, 2023
View reviewed changes
lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb
@@ -270,6 +270,19 @@ def respond_to_missing?(method_name, include_private = false)
super
end

def self.find_miq_request_id(object)
if !defined?(object.type).nil?
if object.type.to_s.match?("RequestEvent")
Copy link
Member

Choose a reason for hiding this comment

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

use include? here

jrafanie
jrafanie approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM, let's tackle any changes in a followup.

(the defensive rescue on failing to extract the resource_id comes to mind)

@jrafanie jrafanie merged commit 3afdfa9 into ManageIQ:master Feb 14, 2023
@jrafanie jrafanie assigned jrafanie and unassigned Fryguy Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@jrafanie jrafanie jrafanie approved these changes

Assignees

@jrafanie jrafanie

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DavidResende0 @Fryguy @miq-bot @jrafanie @akhilkr128