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

Move graph refresh internals logging to debug #16442

Merged
merged 1 commit into from
Nov 16, 2017
Merged

Move graph refresh internals logging to debug #16442

merged 1 commit into from
Nov 16, 2017

Conversation

agrare
Copy link
Member

@agrare agrare commented Nov 10, 2017

Move all logging except for the top level Saving... messages to debug.

When loglevel is info this is what will be logged from ManagerRefresh:

INFO -- : MIQ(ManagerRefresh::SaveInventory.save_inventory) EMS: [fsimonce-ocp], id: [1] Saving EMS Inventory...
INFO -- : MIQ(ManagerRefresh::SaveInventory.save_inventory) EMS: [fsimonce-ocp], id: [1] Saving EMS Inventory...Complete

Fixes: #16374

@agrare
Copy link
Member Author

agrare commented Nov 10, 2017

cc @Ladas @Fryguy

@agrare agrare force-pushed the reduce_graph_refresh_logging branch from 05b9d4b to 670b776 Compare November 10, 2017 16:22
@miq-bot
Copy link
Member

miq-bot commented Nov 10, 2017

Checked commit agrare@670b776 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. ⭐

Ladas
Ladas reviewed Nov 13, 2017
View reviewed changes
app/models/manager_refresh/save_collection/saver/base.rb
"created=#{inventory_collection.created_records.count}, "\
"updated=#{inventory_collection.updated_records.count}, "\
"deleted=#{inventory_collection.deleted_records.count} *************")
_log.debug("Processing #{inventory_collection}, "\
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep at least this one on info? That should help us to see how many&how long, in a case of perf issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

which goes with (not enough logging for old refresh made it hard to pinpoint the source of issue)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think some of these are useful but not sure if they all are, for instance a single refresh of an openshift provider gave us 45 of just these created/updated/deleted lines. I think these are great for top level objects (Container, ContainerGroup, ContainerImage) but maybe not ContainerEnvVar, CustomAttribute? @Fryguy WDYT?

in a case of perf issues.

This is when we'd turn debug on and re-collect the logs IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's ok to expect that people will run debug first, then I am fine with it. :-) I just know it was biting us a lot, that old refresh has no logging for figuring out where is the perf problem.

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 I think if we can configurably log info for certain collections I'd love to see ContainerGroup, ContainerImage, Vm, Host, etc...

Ladas
Ladas approved these changes Nov 13, 2017
View reviewed changes
Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

Ok, seems it's normal to ask for debug logs in a case of problems, so we can leave it all debug.

@Fryguy Fryguy merged commit 1a73cd2 into ManageIQ:master Nov 16, 2017
@Fryguy Fryguy added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 16, 2017
@agrare agrare deleted the reduce_graph_refresh_logging branch November 16, 2017 19:37
simaishi pushed a commit that referenced this pull request Nov 17, 2017
@Fryguy @simaishi
Merge pull request #16442 from agrare/reduce_graph_refresh_logging
1fed527 
Move graph refresh internals logging to debug
(cherry picked from commit 1a73cd2)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 1fed5270523007c6fbe0232b0ba2c8864b92add3
Author: Jason Frey <[email protected]>
Date:   Thu Nov 16 14:12:35 2017 -0500

    Merge pull request #16442 from agrare/reduce_graph_refresh_logging
    
    Move graph refresh internals logging to debug
    (cherry picked from commit 1a73cd2183d89ad132a0f103d61f3a6201dd7b7d)

@cben
Copy link
Contributor

cben commented Nov 20, 2017 •
edited
Loading

Perhaps just Benchmark.realtime_block-based timing could be a useful compromise (optionally with per-collection block)

OTOH, as Doc Brown said about graph refresh, "Where we're going, we don't need timings" 🏎️ 🔥 ;-)
/me ducks

@agrare
Copy link
Member Author

agrare commented Nov 20, 2017

Perhaps just benchmark_block-based timing could be a useful compromise (optionally with per-collection block)

👍 agreed this would be nice. I thought we had this at some point where the key was the inventory_collection name but I don't see it (maybe it was just a dream who knows)

This was referenced Nov 23, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas approved these changes

Assignees

@Fryguy Fryguy

Labels
gaprindashvili/backported providers/inventory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@agrare @miq-bot @simaishi @cben @Ladas @Fryguy