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

Containers Dashboard: Show hourly and realtime trends #519

Merged
merged 1 commit into from
Mar 3, 2017

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Mar 1, 2017

In case that daily trends are not available, we would like to display hourly trends (last 24 hours) if available, or realtime trends (last 10 minutes) instead.
For Image Usage and Pod Creation and Deletion cards I added only hourly trends as we still don't collect the relevant realtime data in the Metric table. This can be added in a follow-up PR once we have the backend functionality.

dashours

real

@zakiva zakiva force-pushed the containers_dashboard branch 3 times, most recently from 7392ace to 4574849 Compare March 1, 2017 14:24
@zakiva
Copy link
Contributor Author

zakiva commented Mar 1, 2017

@miq-bot add_label enhancement, compute/containers

@zakiva
Copy link
Contributor Author

zakiva commented Mar 1, 2017

cc @zeari @simon3z @himdel

@simon3z
Copy link
Contributor

simon3z commented Mar 1, 2017

@zakiva I see that in the screenshot you have an example of last 24 hours (constructed from hourly I suppose). Do you have also a screenshot of the graphs constructed from the "realtime" metrics?

@zakiva
Copy link
Contributor Author

zakiva commented Mar 1, 2017

@zakiva I see that in the screenshot you have an example of last 24 hours (constructed from hourly I suppose).

Correct.

Do you have also a screenshot of the graphs constructed from the "realtime" metrics?

Sure, I'll add it in the description.

@simon3z
Copy link
Contributor

simon3z commented Mar 1, 2017

@zeari @yaacov can you review?

if (data.ems_utilization.interval_name != "daily") {
$scope.cpuUsageSparklineConfig.tooltipFn = chartsMixin.hourlyTimeTooltip;
$scope.memoryUsageSparklineConfig.tooltipFn = chartsMixin.hourlyTimeTooltip;
if (data.ems_utilization.interval_name == "hourly") {
Copy link
Member

Choose a reason for hiding this comment

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

nested if's are not nice :-( can you use something like ?
a. a hash { 'daily': { __('Last ...'), __( ... ), 'hourly': { ...} ... } }
b. switch (data.ems_utilization.interval_name) ...
c one level if's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

dataName : __('KBps'),
tooltipFn : hourlyTimeTooltip
},
realtimeNetworkUsageConfig: {
Copy link
Member

Choose a reason for hiding this comment

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

can we use here the same NetworkUsageConfig struct and just change timeFrame when we know witch one we use ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks

}
end

def hourly_ems_utilization
Copy link
Member

Choose a reason for hiding this comment

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

the realtime_ems_utilization and hourly_ems_utilization methods look suspiciously the same, can you do one function that take the time_frame (hour or minute) as a parameter ?

Copy link
Member

Choose a reason for hiding this comment

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

Looked at it again with @zakiva, merging this two function will be equally problematic ... 👍 i'm ok with the way it is :-)

@zakiva zakiva force-pushed the containers_dashboard branch from 4574849 to 1f63deb Compare March 2, 2017 12:48
@miq-bot
Copy link
Member

miq-bot commented Mar 2, 2017

Checked commit zakiva@1f63deb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 4 offenses detected

app/services/container_dashboard_service.rb

@himdel
Copy link
Contributor

himdel commented Mar 2, 2017

So.. do we have any providers where you can actually see this?

I do have a few container providers but the dashboard always looks like this:

dashboard

@yaacov
Copy link
Member

yaacov commented Mar 2, 2017

So.. do we have any providers where you can actually see this?

@himdel

My way of testing this screens is using a script to fill up random data:
https://github.com/yaacov/miq-scripts/blob/master/set-miq-metric.rb

Or a script to read real metrics from providers:
https://github.com/yaacov/miq-scripts/blob/master/collect-miq-metrics.rb

@zakiva @simon3z testing the UI parts that depend on metrics collection is a problem,
We need to fill the Metric and MetricRollup tables with data that will show the relevant data in the page. For reports we also need to fill the vpor table :-( . Do you know how the QE team does that ?

@himdel
Copy link
Contributor

himdel commented Mar 2, 2017

@yaacov aah, you and your cool scripts again :) Thanks!

(Though, bugreport: collect crashes as soon as you have an unreachable server ;) .)

So.. set-miq-metric worked for me, seems like everything works...

Are we waiting for other reviews or should I just merge?

@yaacov
Copy link
Member

yaacov commented Mar 2, 2017

@himdel :-)

Are we waiting for other reviews or should I just merge?

@simon3z ?

@simon3z
Copy link
Contributor

simon3z commented Mar 2, 2017

Relevant BZ is: https://bugzilla.redhat.com/show_bug.cgi?id=1425221

Are we waiting for other reviews or should I just merge?

LGTM 👍 ready for merge
Thanks @himdel

@miq-bot assign himdel

@himdel himdel merged commit 6147ed2 into ManageIQ:master Mar 3, 2017
@himdel himdel added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 3, 2017
@himdel himdel added the euwe/no label Mar 3, 2017
@simon3z
Copy link
Contributor

simon3z commented Mar 3, 2017

@zakiva it seems that in the case of realtime metrics you're having too many squares in the nodes heatmaps CPU/Memory (check the number of nodes and the number of squares):

screenshot from 2017-03-03 18-07-55

This issue was present also in your screenshot above.

Let's fix this before becomes a bug.

@zakiva
Copy link
Contributor Author

zakiva commented Mar 7, 2017

@zakiva it seems that in the case of realtime metrics you're having too many squares in the nodes heatmaps CPU/Memory (check the number of nodes and the number of squares):

@simon3z I sent a fix in #608

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.

5 participants