Minor refactoring of cgroup telemetry #1607

narrieta · 2019-08-08T18:57:59Z

While reviewing the code for cpu computation I did some refactoring to make it clearer. I'm adding comments in the PR to point our what changed.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes in the description.
New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines

I have read the contribution guidelines.

This change is

narrieta · 2019-08-08T19:00:26Z

azurelinuxagent/common/cgroup.py

@@ -184,15 +181,14 @@ def _get_cpu_percent(self):

        return round(float(cpu_delta * self._osutil.get_processor_cores() * 100) / float(system_delta), 3)

-    def collect(self):
+    def get_cpu_usage(self):


The implementation of collect returned an array of different length with different items for each of the controllers. This makes it difficult to use and error prone. I removed it and instead added explicit methods to get each metric.

narrieta · 2019-08-08T19:01:28Z

azurelinuxagent/common/cgroup.py

@@ -231,7 +227,7 @@ def _get_memory_usage(self):
            usage = "0"
        return int(usage)

-    def _get_memory_max_usage(self):
+    def get_max_memory_usage(self):


made it public + rename

narrieta · 2019-08-08T19:01:28Z

azurelinuxagent/common/cgroup.py

@@ -209,7 +205,7 @@ def __str__(self):
            self.name, self.path, self.controller
        )

-    def _get_memory_usage(self):
+    def get_memory_usage(self):


made it public

narrieta · 2019-08-08T19:01:43Z

azurelinuxagent/common/cgroup.py

-                CollectedMetrics("memory", "Max Memory Usage", max_usage)]
-
-
-class CollectedMetrics(object):


this was not needed

narrieta · 2019-08-08T19:02:34Z

azurelinuxagent/common/cgroupstelemetry.py

@@ -118,19 +120,7 @@ def poll_all_tracked():
                if cgroup.name not in CGroupsTelemetry._cgroup_metrics:
                    CGroupsTelemetry._cgroup_metrics[cgroup.name] = CgroupMetrics()

-                metric = None


moved the exception handling to collect_data

narrieta · 2019-08-08T19:03:30Z

azurelinuxagent/common/cgroupstelemetry.py

-        self._current_memory_usage = Metric()
-        self._max_memory_levels = Metric()
-        self._current_cpu_usage = Metric()
+        self._memory_usage = Metric()


renamed those members to have consistent names with the rest of the code

narrieta · 2019-08-08T19:03:53Z

azurelinuxagent/common/cgroupstelemetry.py

-        self._current_memory_usage.clear()
-        self._max_memory_levels.clear()
-        self._current_cpu_usage.clear()
+        self._memory_usage.clear()


renamed for consistency

narrieta · 2019-08-08T19:04:32Z

azurelinuxagent/common/cgroupstelemetry.py

@@ -188,12 +182,12 @@ def __init__(self):
        self._first_poll_time = None
        self._last_poll_time = None

-    def append(self, metric):
+    def append(self, data):


'metric' was confusing since we are not adding a Metric, but a number

narrieta · 2019-08-08T19:05:28Z

tests/common/test_cgroupstelemetry.py

-                                self.assertListEqual(max_memory_levels._data, [current_max_memory] * data_count)
-                                self.assertEqual(len(current_cpu_usage._data), data_count)
-                                self.assertListEqual(current_cpu_usage._data, [current_cpu] * data_count)
+                            self.assertEqual(len(CGroupsTelemetry._cgroup_metrics), num_extensions)


those checks were repeated over and over so capture them on a helper function _assert_cgroup_metrics_equal

codecov · 2019-08-08T19:05:46Z

Codecov Report

Merging #1607 into develop will decrease coverage by 0.02%.
The diff coverage is 92.68%.

@@             Coverage Diff             @@
##           develop    #1607      +/-   ##
===========================================
- Coverage    66.01%   65.98%   -0.03%     
===========================================
  Files           77       77              
  Lines        11051    11036      -15     
  Branches      1558     1556       -2     
===========================================
- Hits          7295     7282      -13     
+ Misses        3422     3421       -1     
+ Partials       334      333       -1

Impacted Files	Coverage Δ
azurelinuxagent/common/cgroup.py	`90.75% <100%> (-0.09%)`	⬇️
azurelinuxagent/common/cgroupstelemetry.py	`94.73% <91.89%> (+0.61%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5346d4a...ff0933f. Read the comment docs.

narrieta · 2019-08-08T19:08:38Z

azurelinuxagent/common/cgroupstelemetry.py


-    def get_metrics(self):
-        return self._current_memory_usage, self._max_memory_levels, self._current_cpu_usage
+    def get_cpu_usage(self):


get_metrics was returning a list of values, replaced it with explicit methods for each value

pgombar

LGTM. Thanks for the inline comments, made it easier to understand the changes

larohra

LGTM

narrieta added 2 commits August 8, 2019 10:38

Minor refactoring of cgroup telemetry

6c0a505

Update tests

ff0933f

narrieta requested review from vrdmr, pgombar and larohra August 8, 2019 18:57

narrieta commented Aug 8, 2019

View reviewed changes

pgombar approved these changes Aug 8, 2019

View reviewed changes

larohra approved these changes Aug 8, 2019

View reviewed changes

narrieta merged commit fce905a into Azure:develop Aug 8, 2019

narrieta deleted the cgroup-telemetry branch August 8, 2019 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor refactoring of cgroup telemetry #1607

Minor refactoring of cgroup telemetry #1607

narrieta commented Aug 8, 2019 •

edited by vrdmr

Loading

narrieta Aug 8, 2019

narrieta Aug 8, 2019

narrieta Aug 8, 2019

narrieta Aug 8, 2019

narrieta Aug 8, 2019

narrieta Aug 8, 2019

narrieta Aug 8, 2019

narrieta Aug 8, 2019

narrieta Aug 8, 2019

codecov bot commented Aug 8, 2019

narrieta Aug 8, 2019

pgombar left a comment

larohra left a comment

		CollectedMetrics("memory", "Max Memory Usage", max_usage)]


		class CollectedMetrics(object):

Minor refactoring of cgroup telemetry #1607

Minor refactoring of cgroup telemetry #1607

Conversation

narrieta commented Aug 8, 2019 • edited by vrdmr Loading

PR information

Quality of Code and Contribution Guidelines

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2019

Codecov Report

Choose a reason for hiding this comment

pgombar left a comment

Choose a reason for hiding this comment

larohra left a comment

Choose a reason for hiding this comment

narrieta commented Aug 8, 2019 •

edited by vrdmr

Loading