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.

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

Adding request id to evm log #17013

Merged
merged 5 commits into from
Mar 28, 2018
Merged

Adding request id to evm log #17013

merged 5 commits into from
Mar 28, 2018

Conversation

pkomanek
Copy link
Contributor

@pkomanek pkomanek commented Feb 16, 2018

We are adding the letter r plus request_id as a prefix of tracking_label. With this change we are able to find all related tasks in the log by a single request_id.

https://bugzilla.redhat.com/show_bug.cgi?id=1535229

Logs:

before:
screenshot from 2018-03-28 05-51-11
screenshot from 2018-03-28 05-51-29

after:
screenshot from 2018-03-28 04-59-27
screenshot from 2018-03-28 04-59-45
filter command: grep "Q-task_id(\[.*\])" log/evm.log

Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

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

@pkomanek As we discussed we want to make the same change to the tracking_label at the request level as well.

Example log snippet:

Q-task_id([service_template_provision_request_r99000000000003]) and
Q-task_id([r99000000000003_service_template_provision_task_99000000000004])

@@ -3,7 +3,7 @@ module MiqRequestTask::StateMachine
delegate :my_zone, :to => :source, :allow_nil => true

def my_task_id
"#{self.class.base_model.name.underscore}_#{id}"
"r#{miq_request_id}_#{self.class.base_model.name.underscore}_#{id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek Can my_task_id be changed to tracking_id or tracking_label_id? the my_task_id function conflicts with MiqTask. If its used outside of here maybe we could add an alias.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I mentioned this on the call this morning. Need to check if it is used elsewhere, but this seems like a good refactoring.

Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@pkomanek Besides changing the name of the my_task_id method, it looks good. This change is awesome and is going to make debugging customer provisioning issues so much easier.

@gmcculloug
Copy link
Member

cc @jcarter12 @math3matical

@@ -398,7 +398,7 @@ def execute
:method_name => "create_request_tasks",
:zone => options.fetch(:miq_zone, my_zone),
:role => my_role,
:tracking_label => "#{self.class.name.underscore}_#{id}",
:tracking_label => "#{self.class.name.underscore}_r#{id}",
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 be moved into a tracking_label_id method like we do for the task. It can live in the miq_request.rb file.

@gmcculloug
Copy link
Member

The code changes look good. Still would like to review the concept with @jcarter12 and @math3matical to get their feedback before merging.

@pkomanek pkomanek changed the title [WIP]Adding request id to evm log Adding request id to evm log Mar 2, 2018
@miq-bot miq-bot removed the wip label Mar 2, 2018
@kbrock
Copy link
Member

kbrock commented Mar 16, 2018

ping @jcarter12 @math3matical Look good to you?

@tinaafitz
Copy link
Member

@kbrock @jcarter12 and @math3matical are good with the changes. Because this is an important change, I've reached out to the field for feedback and expect to hear back shortly.

@tinaafitz
Copy link
Member

@pkomanek We requested community feedback because this is a really important change. One great suggestion we'd like to implement is add the tracking label to the request line as well:
before:
Q-task_id([service_template_provision_request_r99000000000003]) and
Q-task_id([r99000000000003_service_template_provision_task_99000000000004])

after:
Q-task_id([r99000000000003_service_template_provision_request_99000000000003]) and
Q-task_id([r99000000000003_service_template_provision_task_99000000000004])

Can you implement this change?

@@ -126,7 +127,7 @@ def service_resource_id(index, scaling_max)
:method_name => 'execute',
:role => 'ems_operations',
:zone => 'a_zone',
:tracking_label => "service_template_provision_task_#{@task_0.id}",
:tracking_label => "r#{@request.id}_service_template_provision_task_#{@task_0.id}",
Copy link
Member

Choose a reason for hiding this comment

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

You can de-dup this tracking_label string by adding it to a let block. I'd recommend placing it just above the def create_stp define.

let(:tracking_label) { "r#{@request.id}_service_template_provision_task_#{@task_0.id}" }

Then the two expect(MiqQueue) methods change to use :tracking_label => tracking_label

@miq-bot
Copy link
Member

miq-bot commented Mar 23, 2018

Checked commits pkomanek/manageiq@fbeab34~...5fb68f7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. ⭐

@gmcculloug
Copy link
Member

@tinaafitz PTAL

@tinaafitz
Copy link
Member

@pkomanek Can you update the screen shot to reflect the request log changes?

@pkomanek
Copy link
Contributor Author

@tinaafitz sure, is it ok?

@tinaafitz
Copy link
Member

@pkomanek Looks great. :-)
@gmcculloug Can we merge this?

@gmcculloug gmcculloug merged commit 84aae96 into ManageIQ:master Mar 28, 2018
@gmcculloug gmcculloug added this to the Sprint 83 Ending Apr 9, 2018 milestone Mar 28, 2018
@pkomanek pkomanek deleted the adding_request_id_to_evm_log branch March 29, 2018 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants