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

Make Widget run without timezones #14386

Merged
merged 5 commits into from
Mar 20, 2017
Merged

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Mar 17, 2017

Summary

A Widget is basically a Report that is customized for a user's security group and time zone.

Before

Some report data is different for each time zone.
So all reports are run for each user security group and each time zone.
Note: only time zones that are used by users in the security group are populated.

Change

But many reports just show tallies that are the same regardless of the user's time zone.

This PR marks widgets that are not affected by time zone, so they can be run once per time zone.

Conclusion

For my test customer:

50% less time is spent running these 3 reports.
The 10 minute savings per hour is 20% less load on the reporting worker.

Numbers

Widget runs before runs after ms/run rows/run before* after*
Top Storage Consumers 144 71 5,255.9 1,148 12.6m 6.2m
Vendor and Guest OS Chart 152 74 3,374.8 1,148 8.5m 4.2m
Guest OS Information 7 3 4,127.8 2,280 29s 12s

* before and after are estimated. Took too much time to run these.

Related items

@jrafanie
Copy link
Member

I like this approach @kbrock, very nice! Checkout the failing tests :trollface: Would you be able to create a test case that covers both the timezone_matters? and !timezone_matters case?

@kbrock
Copy link
Member Author

kbrock commented Mar 17, 2017

@jrafanie Failing test was one of the modified yaml files. (yay failing tests catching dev changes)

So I put in tests around generating content.

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.

Looks good, I have a few questions/suggestions.

def timezones_for_users(users)
users.to_miq_a.collect(&:get_timezone).uniq.sort
end

Copy link
Member

Choose a reason for hiding this comment

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

wow, we don't use this? 🥇

# options[:timezone_matters] == false will skip it
# TODO: detect date field in the report?
def timezone_matters?
options.try(:[], :timezone_matters) != false
Copy link
Member

Choose a reason for hiding this comment

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

is this a hash? Can we do options.fetch(:timezone_matters, true) instead?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's right, default to true

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, options can be nil.

  #A pr code:
    options.try(:[], :timezone_matters) != false
  #B using fetch:
    options ? options.fetch(:timezone_matters, true) : true
  #C using fetch 2:
    (options || {}).fetch(:timezone_matters, true)

Copy link
Member

Choose a reason for hiding this comment

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

Wow, good point @kbrock. Personally, I'd use a guard clause to make things very explicit:

Maybe something like this:

def timezone_matters?
  return true unless options
  options.fetch(:timezone_matters, true)
end

If we ever make options default to an empty hash, we could easily remove that guard clause.

Copy link
Member

Choose a reason for hiding this comment

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

I like choice C or @jrafanie's personally

Copy link
Member

Choose a reason for hiding this comment

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

why is options ever nil?

@jrafanie
Copy link
Member

@kbrock is there a BZ for this? does it need to be backported?

@kbrock
Copy link
Member Author

kbrock commented Mar 20, 2017

https://bugzilla.redhat.com/show_bug.cgi?id=1251259 - think it would be good in e, but 5.8 means f?

@miq-bot
Copy link
Member

miq-bot commented Mar 20, 2017

Checked commits kbrock/manageiq@1a4ce7f~...0a2c5f9 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🍪

@jrafanie jrafanie merged commit c9cf152 into ManageIQ:master Mar 20, 2017
@jrafanie jrafanie added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 20, 2017
@kbrock kbrock deleted the widget_timezones branch March 20, 2017 21:01
simaishi pushed a commit that referenced this pull request Mar 20, 2017
@simaishi
Copy link
Contributor

Euwe backport details:

$ git log -1
commit 102f688f4ef1af978a7f039a04fcb090d0508951
Author: Joe Rafaniello <[email protected]>
Date:   Mon Mar 20 16:52:26 2017 -0400

    Merge pull request #14386 from kbrock/widget_timezones
    
    Make Widget run without timezones
    (cherry picked from commit c9cf15216a416a654def816e4d79acc62e520000)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1434150

kbrock added a commit to kbrock/manageiq that referenced this pull request Mar 27, 2017
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.

7 participants