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 standard output for report generator tool #18640

Merged
merged 2 commits into from
May 14, 2019
Merged

Add logging to standard output for report generator tool #18640

merged 2 commits into from
May 14, 2019

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Apr 9, 2019

I added parsing with https://github.com/ManageIQ/optimist in first commit.

example of output with logging:

./tools/generate_report.rb  -n 'vms' -l debug

Logging on standard output, log level set to: debug
Generating report... vms
I, [2019-04-09T12:30:37.853104 #48837]  INFO -- : <AuditSuccess> MIQ(Generator.queue_generate_table) userid: [admin] - Generate Report: 'vms', successfully initiated
I, [2019-04-09T12:30:37.853170 #48837]  INFO -- : MIQ(MiqTask#update_status) Task: [366203] [Queued] [Ok] [Task has been queued]
I, [2019-04-09T12:30:37.921521 #48837]  INFO -- : MIQ(MiqQueue.put) Message id: [123135345],  id: [], Zone: [], Role: [reporting], Server: [], MiqTask id: [], Ident: [generic], Target id: [], Instance id: [525], Task id: [], Command: [MiqReport._async_generate_table], Timeout: [3600], Priority: [100], State: [ready], Deliver On: [], Data: [], Args: [366203, {:userid=>"admin", :mode=>"async", :report_source=>"Requested by user"}]
I, [2019-04-09T12:30:37.925455 #48837]  INFO -- : MIQ(MiqTask#update_status) Task: [366203] [Active] [Ok] [Generating report]
I, [2019-04-09T12:30:42.472756 #48837]  INFO -- : MIQ(MiqReport#build_create_results) Creating report results with hash: [{:name=>"vms1", :userid=>"admin", :report_source=>"Requested by user", :db=>"ManageIQ::Providers::InfraManager::Vm", :last_run_on=>2019-04-09 10:30:42 UTC, :last_accessed_on=>2019-04-09 10:30:42 UTC, :miq_report_id=>525, :miq_group_id=>nil}]
I, [2019-04-09T12:30:44.616015 #48837]  INFO -- : MIQ(MiqReport#build_create_results) Finished creating report result with id [20906] for report id: [525], name: [vms]
I, [2019-04-09T12:30:44.636990 #48837]  INFO -- : MIQ(MiqTask#update_status) Task: [366203] [Finished] [Ok] [Generating report complete]

cc @gtanzillo

@miq-bot assign @jrafanie
@miq-bot add_label report, tools

@miq-bot
Copy link
Member

miq-bot commented Apr 9, 2019

@lpichler Cannot apply the following label because they are not recognized: report

@miq-bot miq-bot added the tools label Apr 9, 2019
Fryguy
Fryguy reviewed Apr 9, 2019
View reviewed changes
tools/generate_report.rb Outdated
banner "USAGE: #{__FILE__} -u <name of userid which runs report, default: admin> \n" \
" #{__FILE__} -n <name of report or id of report, default: last created report> \n" \
" #{__FILE__} -l <log level, if this option is set log messages are forwarded to \n" \
" standard output, allowed levels are debug, info, warn, error> \n" \
Copy link
Member

Choose a reason for hiding this comment

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

This should not be necessary...optimist builds all of this for you.

Fryguy
Fryguy reviewed Apr 9, 2019
View reviewed changes
tools/generate_report.rb Outdated

include ActionDispatch::Routing::UrlFor
include Rails.application.routes.url_helpers

USER_ID = "admin".freeze
REPORT_PARAMS = {:userid => USER_ID, :mode => "async", :report_source => "Requested by user"}.freeze
options = Optimist.options(ARGV) do
Copy link
Member

Choose a reason for hiding this comment

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

ARGV is the default, so no need to pass it.

Fryguy
Fryguy reviewed Apr 9, 2019
View reviewed changes
tools/generate_report.rb Outdated
if ARGV.empty?
opt :user, "userid", :short => "u", :type => String, :default => "admin"
opt :report_name_or_id, "report name or report id", :short => "n", :type => :string
opt :log_level, "log level (debug, info, warn, error)", :short => "l", :type => :string
Copy link
Member

@Fryguy Fryguy Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

All of the short options are automatic, so no need to define them

EDIT: (You do need the short option for report_name_or_id)

Fryguy
Fryguy reviewed Apr 9, 2019
View reviewed changes
tools/generate_report.rb Outdated

def report_from_args
if ARGV.empty?
opt :user, "userid", :short => "u", :type => String, :default => "admin"
Copy link
Member

Choose a reason for hiding this comment

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

The type is not necessary because you provided a default. If you want to leave it for consistency with other options, use the symbol style like you did in the other ones.

Fryguy
Fryguy reviewed Apr 9, 2019
View reviewed changes
tools/generate_report.rb Outdated
end

if options[:log_level]
Optimist.die "Log level #{options[:log_level]} is not supported, supported levels are: debug, info, warn, error" unless Vmdb::LogProxy::LEVELS.include?(options[:log_level].to_sym)
Copy link
Member

Choose a reason for hiding this comment

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

Prefer building the message from the same list you are checking from, if possible.. that is, instead of hardcoding debug, info, etc. do Vmdb::LogProxy::LEVELS.join(", ")

Fryguy
Fryguy reviewed Apr 9, 2019
View reviewed changes
tools/generate_report.rb Outdated

if options[:log_level]
Optimist.die "Log level #{options[:log_level]} is not supported, supported levels are: debug, info, warn, error" unless Vmdb::LogProxy::LEVELS.include?(options[:log_level].to_sym)
$log = Logger.new(STDOUT)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a VmdbLogger or similar?

Copy link
Contributor Author

@lpichler lpichler May 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

yes, VMDBLogger (we have it in other scripts in /tools)

@lpichler lpichler force-pushed the add_loggint_to_standart_output_for_report_generator_tool branch from 96d3ce0 to 5557675 Compare May 2, 2019 09:57
@lpichler
Copy link
Contributor Author

lpichler commented May 2, 2019

I updated PR according to the suggestions. thanks for the review @Fryguy 👍

@lpichler lpichler force-pushed the add_loggint_to_standart_output_for_report_generator_tool branch from 5557675 to 6a5794d Compare May 2, 2019 10:04
@lpichler
Copy link
Contributor Author

lpichler commented May 2, 2019

@miq-bot add_label changelog/yes

@miq-bot
Copy link
Member

miq-bot commented May 2, 2019

Checked commits lpichler/manageiq@cd34aa0~...6a5794d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@gtanzillo gtanzillo added this to the Sprint 112 Ending May 27, 2019 milestone May 14, 2019
@gtanzillo gtanzillo merged commit 8d21952 into ManageIQ:master May 14, 2019
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

@gtanzillo gtanzillo gtanzillo approved these changes

Assignees

@jrafanie jrafanie

Labels
changelog/yes tools
Projects
None yet
Milestone
No milestone
Development

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