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 sure bucket duration is not less the 20 minutes #385

Merged
merged 2 commits into from Feb 15, 2017

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Feb 14, 2017

Description

When reviewing the ad-hoc graphs we saw gaps in the graph, this gaps happen when data is collected in a slow rate (more then 5minutes per sample).
This PR makes it impossible for the user to chose a time bucket less then N minutes (N equels 20 for this PR)
We assume that if data is not collected for more then N minutes this is something user need to know about.

Secnd stage
Out of scope for this PR, get the N minutes value automatically from the ops team.

Screenshot
Interval between points is 20 minutes ( 03:29 -> 03:49 -> 04:09 ... ).
gifrecord_2017-02-15_160918

@yaacov
Copy link
Member Author

yaacov commented Feb 14, 2017

@simon3z @zeari @zgalor @moolitayer @himdel please review

@simon3z
Copy link
Contributor

simon3z commented Feb 14, 2017

@yaacov there are too many magic numbers that we don't know what they are, can you use constants with relevant names? (What is 1000? What is 300? For 60 seconds you can use 1.minute, etc.)

@miq-bot assign yaacov

@himdel
Copy link
Contributor

himdel commented Feb 14, 2017

There's no 1.minute in javascript unfortunately, but.. maybe we could keep everything in seconds?

Or.. at least, pick one common unit, mixing seconds, minutes and hours feels like a recipe for disaster :).

@simon3z
Copy link
Contributor

simon3z commented Feb 14, 2017

There's no 1.minute in javascript unfortunately, but.. maybe we could keep everything in seconds?

Or.. at least, pick one common unit, mixing seconds, minutes and hours feels like a recipe for disaster :).

@himdel of course I am not suggesting to use different units, in fact 1.minute is treated as 60 seconds.

I am saying that I don't know if there are 60 that are 60 because they're 1.minute or because there's something else that happen to be 60. That's why we need constant names.

@himdel
Copy link
Contributor

himdel commented Feb 14, 2017

@simon3z Agreed, sorry if it came out that way, I was just trying to clarify :) and noticed the current state of the code..

@yaacov
Copy link
Member Author

yaacov commented Feb 14, 2017

I think it's clear enough now :-)

@himdel @simon3z please re review

@simon3z
Copy link
Contributor

simon3z commented Feb 14, 2017

LGTM 👍

@miq-bot assign himdel

@miq-bot miq-bot assigned himdel and unassigned yaacov Feb 14, 2017
@@ -4,6 +4,10 @@
function($http, $window, miqService) {
var dash = this;
dash.tenant = '_ops';
dash.minBucketDurationInSecondes = 20 * 60;

var NUMBER_OF_MILISEC_IN_HOUR = 60 * 60 * 1000;
Copy link
Member

Choose a reason for hiding this comment

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

minor typo: MILISEC should MILLISEC or MILLISECONDS

var bucket_duration = parseInt(diff / 1000 / 200); // bucket duration is in seconds
var bucket_duration = parseInt(diff / NUMBER_OF_MILISEC_IN_SEC / numberOfBucketsInChart); // bucket duration is in seconds

// make sure backet duration is not smaller then minBucketDurationInSecondes seconds
Copy link
Member

Choose a reason for hiding this comment

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

minor typo: backet should be bucket

@yaacov
Copy link
Member Author

yaacov commented Feb 15, 2017

@chessbyte thanks, fixed.

@miq-bot
Copy link
Member

miq-bot commented Feb 15, 2017

Checked commits yaacov/manageiq-ui-classic@e58ba59~...97ee538 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks good. 🍪

@yaacov
Copy link
Member Author

yaacov commented Feb 15, 2017

@himdel please re review

@yaacov
Copy link
Member Author

yaacov commented Feb 15, 2017

p.s.

When testing, notice that changing the time_range behave mysteriously because of a problem/bug with patternfly line chart, patternfly/angular-patternfly#413

@himdel
Copy link
Contributor

himdel commented Feb 15, 2017

LGTM, would love to merge.. but please add info on where to find it in the UI to the description, no idea where to test.

@yaacov
Copy link
Member Author

yaacov commented Feb 15, 2017

@himdel yes sorry ... , added a screenshot

@himdel himdel added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 15, 2017
@himdel himdel merged commit 2f022cc into ManageIQ:master Feb 15, 2017
@himdel
Copy link
Contributor

himdel commented Feb 15, 2017

Works just fine, merged :)

(It is a bit counter-intuitive that you have to click Refresh after changing the date or number of days in the toolbar, and that Refresh is above that, but.. yeah, this PR works :).)

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.

None yet

5 participants