CLOUDSTACK-9783: Improve metrics view performance #1944
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-492 |
Member
Author
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
borisstoyanov
approved these changes
Feb 16, 2017
Contributor
borisstoyanov
left a comment
borisstoyanov
left a comment
There was a problem hiding this comment.
Thanks Rohit, this really improves the load time of the infrastructure page.
LGTM
Member
Author
|
Thanks for testing/reviewing @borisstoyanov |
|
Trillian test result (tid-833)
|
Contributor
|
Just triggered Trillian env to check the test failure. |
yadvr
force-pushed
the
4.9-metrics-enhancement
branch
from
3aba6ad to
3881d24
Compare
Contributor
|
I've created a Trillian env out of the PR and run the test and it has passed: |
Member
Author
|
Thanks @borisstoyanov test results LGTM. |
Member
Author
|
Requesting for review - @abhinandanprateek @DaanHoogland @rashmidixit @karuturi @koushik-das |
Contributor
|
seen it work in practice: LGTM |
|
@rhtyd Will be trying this out today - will post my comments. |
Member
Author
|
Thanks @rashmidixit awaiting your review |
marcaurele
reviewed
Feb 27, 2017
| </dependency> | ||
| </dependencies> | ||
| <build> | ||
| <plugins> |
Member
There was a problem hiding this comment.
Would be nice to follow maven standards on new modules. Since you cannot inherit from cloud-maven-standard you have to put all those lines:
<build>
<sourceDirectory>${basedir}/src/main/java</sourceDirectory>
<scriptSourceDirectory>${basedir}/src/main/scripts</scriptSourceDirectory>
<testSourceDirectory>${basedir}/src/test/java</testSourceDirectory>
<outputDirectory>${basedir}/target/classes</outputDirectory>
<testOutputDirectory>${basedir}/target/test-classes</testOutputDirectory>
<resources>
<resource>
<directory>${basedir}/src/main/resources</directory>
</resource>
</resources>
<testResources>
<testResource>
<directory>${basedir}/src/test/resources</directory>
</testResource>
</testResources>
...
</build>
|
@rhtyd I pulled the code and was able to run it as well. Otherwise LGTM. |
Member
Author
|
@borisstoyanov can you see the failure in Travis with test_list_infrastructure_metrics test case? From the UI, to remove this service they can modify the core metrics widget to do no-operation (in ui/scripts/ui-custom/metricsView.js) and the rendered button can be modified to be hidden, alternatively metricsView section can be removed across the resources (zones, cluster, host, vms, volumes and storage pool). |
|
@rhtyd On the contrary, I think you adding metrics as pluggable API is a good idea. However, to seriously make the feature pluggable the UI component should also load based on a plugin based flag. For example (I am not sure if the current design makes this possible), the plugin should return a flag in the listCapabilities API which tells the UI that metrics is enabled. If this flag is present, the metrics button should appear, else not. |
Member
Author
|
@rashmidixit the current UI does not support support pluggability of components within UI like the backend does. We can have a separate tab but not the implementation that the current metrics view UI has i.e. embedded across resource views. It's a general purpose feature that works for all deployments/environments and I think most users would want to keep it so we never implemented a way to disable it. |
|
@rhtyd I guess this is fine for now. Thanks! |
Member
Author
|
Thanks @rashmidixit @karuturi this is ready for merge, as soon as Travis passes. Thanks. |
This improves the metrics view feature by improving the rendering performance
of metrics view tables, by reimplementing the logic at the backend and data
served via APIs. In large environments, the older implementation would
make several API calls that increases both network and database load.
List of APIs introduced for improving the performance:
listClustersMetrics
listHostsMetrics
listInfrastructure
listStoragePoolsMetrics
listVMsMetrics
listVolumesMetrics
listZonesMetrics
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
yadvr
force-pushed
the
4.9-metrics-enhancement
branch
from
6950feb to
4022535
Compare
Member
Author
|
@karuturi ping? can this be merged as well, this greatly improves the UI performance for the existing metrics view feature as well as improves the load time of the infrastructure tab |
Member
Author
|
@karuturi this is merge-able and much needed enhancement to improve UI performance for large environments |
Contributor
|
Hi @rhtyd, thanks for this great improvement! Along with @serg38 we've been testing in our env and got some failures on Exception is thrown by In our case it failed for this And Did you have a failure like this? |
Contributor
|
Please ommit my last comment, we were using commons-beanutils version 1.8.3 instead of 1.9.2 LGTM |
Member
|
merging now |
asfgit
pushed a commit
that referenced
this pull request
Apr 6, 2017
CLOUDSTACK-9783: Improve metrics view performanceThis improves the metrics view feature by improving the rendering performance
of metrics view tables, by re-implementing the logic at the backend and data
served via APIs. In large environments, the older implementation would
make several API calls that increases both network and database load.
List of APIs introduced for improving the performance that re-implement the frontend logic at backend:
listClustersMetrics
listHostsMetrics
listInfrastructure
listStoragePoolsMetrics
listVMsMetrics
listVolumesMetrics
listZonesMetrics
Pinging for review - @abhinandanprateek @DaanHoogland @borisstoyanov @karuturi @rashmidixit
Marvin test results:
=== TestName: test_list_clusters_metrics | Status : SUCCESS ===
=== TestName: test_list_hosts_metrics | Status : SUCCESS ===
=== TestName: test_list_infrastructure_metrics | Status : SUCCESS ===
=== TestName: test_list_pstorage_metrics | Status : SUCCESS ===
=== TestName: test_list_vms_metrics | Status : SUCCESS ===
=== TestName: test_list_volumes_metrics | Status : SUCCESS ===
=== TestName: test_list_zones_metrics | Status : SUCCESS ===
* pr/1944:
CLOUDSTACK-9783: Improve metrics view performance
Signed-off-by: Rajani Karuturi <rajani.karuturi@accelerite.com>
Member
Author
Member
|
master is broken after the merge. Pom version changes |
asfgit
pushed a commit
that referenced
this pull request
Apr 7, 2017
CLOUDSTACK-9783: corrected the version number in metrics pommaster is broken after fwd-merging metrics PR #1944 from 4.9 This is due to the incorrect parent version number in the metrics pom. Corrected the same to reflect master version number. * pr/2032: CLOUDSTACK-9783: corrected the version number in metrics pom Signed-off-by: Rajani Karuturi <rajani.karuturi@accelerite.com>
Member
Author
|
@karuturi thanks for fixing it |
abhinandanprateek
pushed a commit
to shapeblue/cloudstack
that referenced
this pull request
Apr 26, 2017
master is broken after fwd-merging metrics PR apache#1944 from 4.9 This is due to the incorrect parent version number in the metrics pom. Corrected the same to reflect master version number.
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This improves the metrics view feature by improving the rendering performance
of metrics view tables, by re-implementing the logic at the backend and data
served via APIs. In large environments, the older implementation would
make several API calls that increases both network and database load.
List of APIs introduced for improving the performance that re-implement the frontend logic at backend:
Pinging for review - @abhinandanprateek @DaanHoogland @borisstoyanov @karuturi @rashmidixit
Marvin test results:
=== TestName: test_list_clusters_metrics | Status : SUCCESS ===
=== TestName: test_list_hosts_metrics | Status : SUCCESS ===
=== TestName: test_list_infrastructure_metrics | Status : SUCCESS ===
=== TestName: test_list_pstorage_metrics | Status : SUCCESS ===
=== TestName: test_list_vms_metrics | Status : SUCCESS ===
=== TestName: test_list_volumes_metrics | Status : SUCCESS ===
=== TestName: test_list_zones_metrics | Status : SUCCESS ===