CLOUDSTACK-9699: Add global setting for enable/disable Metrics feature #1884

rashmidixit · 2017-01-02T09:28:25Z

The Metrics view for each type of entity basically fires APIs and calculates required values on the client end. For e.g. to display memory usage etc at the zone level, it will fetch all zones. For each zone it will fetch pods->cluster->host->VMs
For a very large Cloudstack installation this will have a major impact on the performance.
Ideally, there should be an API which calculates all metrics in the backend and the UI should simply show the values. However, for the time, introduce a global setting called enable.metrics which will be set to false. This will cause the metrics button not to be shown on any of the pages.
If the Admin changes this to true, then the button will be visible and Metrics functionality will work as usual.

In this pull request, have also added JUnit test case for listCapabilities API.

Additionally added a test case in marvin called test_global_settings as part of the smoke tests which test the output of the listCapabilities API and checks if this setting is present or not.

Refer to CLOUDSTACK-9699 for more details.

Reviewed-By: Radhika Grover & Koushik Das Added a new global setting called enable.metrics.ui (default false) which allows the metrics button to be enabled/disabled in the UI. Note that when false, the metrics button is not visible at all. This feature has been introduced because metrics feature is not optimized and will cause performance issues in large deployments. Conflicts: api/src/org/apache/cloudstack/api/command/user/config/ListCapabilitiesCmd.java api/src/org/apache/cloudstack/api/response/CapabilitiesResponse.java server/src/com/cloud/server/ManagementServerImpl.java ui/scripts/cloudStack.js ui/scripts/instances.js ui/scripts/sharedFunctions.js ui/scripts/storage.js ui/scripts/system.js

…ies API and whether it includes enableMetrics parameter. Reviewed-By Koushik Das, Sanjeev N Added junit test case and marvin test case for testing updated listCapabilities API. The junit test case changes involved updating ResponseGenerator and ApiResponseHelper to correctly build the response. Conflicts: api/src/org/apache/cloudstack/api/command/user/config/ListCapabilitiesCmd.java server/src/com/cloud/api/ApiResponseHelper.java

Reviewed-By: Radhika Grover & Koushik Das Added a new global setting called enable.metrics.ui (default false) which allows the metrics button to be enabled/disabled in the UI. Note that when false, the metrics button is not visible at all. This feature has been introduced because metrics feature is not optimized and will cause performance issues in large deployments. Conflicts: api/src/org/apache/cloudstack/api/command/user/config/ListCapabilitiesCmd.java api/src/org/apache/cloudstack/api/response/CapabilitiesResponse.java server/src/com/cloud/server/ManagementServerImpl.java ui/scripts/cloudStack.js ui/scripts/instances.js ui/scripts/sharedFunctions.js ui/scripts/storage.js ui/scripts/system.js

…stack into CLOUDSTACK-9699 Conflicts: api/src/org/apache/cloudstack/api/response/CapabilitiesResponse.java server/src/com/cloud/api/ApiResponseHelper.java test/integration/smoke/test_global_settings.py ui/scripts/cloudStack.js ui/scripts/sharedFunctions.js

…ded.

jburwell

In addition to the code feedback, please squash the commits for this PR.

jburwell · 2017-01-06T22:30:37Z

api/test/org/apache/cloudstack/api/command/test/ListCapabilitiesCmdTest.java

+            Mockito.when(mgr.listCapabilities(listCapabilitiesCmd)).thenReturn(result);
+        } catch (Exception e) {
+            Assert.fail("Received exception when success expected " + e.getMessage());
+        }


Why not perform lines 58-67 in in setUp? Not only would this approach focus this method on the test operations, but remove the need for unnecessary class-level attributes.

Also, catching exceptions is not required because JUnit will automatically fail when exceptions are thrown. If there are checked exceptions, add them to the throws of the test method to keep test methods as succinct as possible.

jburwell · 2017-01-06T22:35:05Z

api/test/org/apache/cloudstack/api/command/test/ListCapabilitiesCmdTest.java

+        listCapabilitiesCmd._mgr = mgr;
+        listCapabilitiesCmd._responseGenerator = responseGenerator;
+
+        Map<String, Object> result = new HashMap<String, Object>();


Please consider using Collections.emptyMap() instead of allocating a new Map to improve clarity of intention.

jburwell · 2017-01-06T22:36:22Z

test/integration/smoke/test_global_settings.py

+        self.assertIsNotNone(
+                listCapabilities.enablemetricsui,
+                "Failed to fetch enable.metrics.ui"
+        )


Please add tests to verify that the value is set as expected.

jburwell · 2017-01-06T22:41:53Z

ui/scripts/storage.js

+                            return ['add', 'viewMetrics', 'uploadVolume', 'uploadVolumefromLocal'];
+                        else
+                            return ['add', 'uploadVolume', 'uploadVolumefromLocal'];
+                    },


Please remove the duplication in this function by refactoring as follows:

actionPreFilter: function(args){ actions = ['add', 'uploadVolume', 'uploadVolumefromLocal']; if (g_enablemetricsui) { actions.splice(1, 0, 'viewMetrics'); } return actions; },

jburwell · 2017-01-06T22:44:30Z

ui/scripts/system.js

+                                if (g_enablemetricsui)
+                                    return ['add', 'viewMetrics'];
+                                else
+                                    return ['add'];


Please remove the duplication in this function by refactoring as follows:

actions = ['add']; if (g_enablemetricsui) { actions.append('viewMetrics'); } return actions;

jburwell · 2017-01-06T22:45:14Z

ui/scripts/system.js

+                            return ['add', 'viewMetrics'];
+                        else
+                            return ['add'];
+                    },


Lines 14096-14101 appear to be a duplication of lines 7857-7860. Can this code duplication be removed?

jburwell · 2017-01-06T22:45:33Z

ui/scripts/system.js

+                            return ['add', 'viewMetrics'];
+                        else
+                            return ['add'];
+                    },


Please code duplication comments above.

jburwell · 2017-01-06T22:45:44Z

ui/scripts/system.js

+                            return ['add', 'viewMetrics'];
+                        else
+                            return ['add'];
+                    },


Please code duplication comments above.

jburwell · 2017-01-06T22:47:15Z

ui/scripts/ui/widgets/listView.js

        if (listViewData.actions) {
+            // If a preFilter is set, then get the values. Else set this to null
+            var filteredActions = listViewData.actionPreFilter == undefined ?
+                        null : listViewData.actionPreFilter();


Why not set to an empty list rather than null to avoid a potential null error?

jburwell · 2017-01-06T22:47:39Z

ui/scripts/ui/widgets/listView.js

+                // filter out those actions that shouldn't be shown
+                if (filteredActions != null
+                        && (filteredActions.indexOf(actionName) < 0))
+                    return false;


Per coding standards, please surround all if blocks with curly braces.

rashmidixit · 2017-01-10T03:20:04Z

Thank you @jburwell. I will incorporate the comments in a couple of days and resubmit.

yadvr · 2017-01-11T09:38:53Z

@rashmidixit I'm working on writing separate metrics view apis (backend/apis), they will improve the performance significantly. An explicit global settings is not necessary, pl. hold on this PR.

yadvr · 2017-02-13T06:47:59Z

I'm working on improving the metrics view feature by implementing the logic as backend APIs, will post a PR this/next week. Thanks.

rashmidixit · 2017-02-14T13:00:19Z

@rhtyd Thanks for the update.

yadvr · 2017-02-16T10:30:22Z

This is not needed with PR #1944, though changes around capability refactoring can be separated.

rashmidixit · 2017-03-15T06:45:52Z

Closing this based on the new PR #1944.

Rashmi Dixit added 7 commits December 29, 2016 15:31

CLOUDSTACK-9699: Removed some extra code which was not required.

6e13a96

CLOUDSTACK-9699: Removed an incorrect comment.

f6c7485

CLOUDSTACK-9699: Added missing license header to the new test file ad…

0f2a4fe

…ded.

rashmidixit changed the title ~~Add global setting for enable/disable Metrics feature~~ CLOUDSTACK-9699: Add global setting for enable/disable Metrics feature Jan 3, 2017

jburwell suggested changes Jan 6, 2017

View reviewed changes

rashmidixit mentioned this pull request Feb 16, 2017

CLOUDSTACK-9675: Cloudstack Metrics Miscellaneous bug fixes #1826

Closed

rashmidixit closed this Mar 15, 2017

CLOUDSTACK-9699: Add global setting for enable/disable Metrics feature #1884

CLOUDSTACK-9699: Add global setting for enable/disable Metrics feature #1884

Uh oh!

Conversation

rashmidixit commented Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jburwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rashmidixit commented Jan 10, 2017

Uh oh!

yadvr commented Jan 11, 2017

Uh oh!

yadvr commented Feb 13, 2017

Uh oh!

rashmidixit commented Feb 14, 2017

Uh oh!

yadvr commented Feb 16, 2017

Uh oh!

rashmidixit commented Mar 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rashmidixit commented Jan 2, 2017 •

edited

Loading