Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

Dynamic metrics for cpu and net statistics #34

Merged
merged 2 commits into from
Sep 27, 2016

Conversation

marcin-krolik
Copy link
Collaborator

Introduces dynamic metrics to cpu and net statistics

Summary of changes:

  • cpu ids are now presented as dynamic metric
  • interface ids are now presented as dynamic metric
  • calls to gopsutil functions done only once per collect (not for each requested metric)
  • updated docs

How to verify it:

  • execute example task

Testing done:

  • units
  • manual

@intelsdi-x/plugin-maintainers

@kindermoumoute
Copy link
Contributor

kindermoumoute commented Sep 21, 2016

Large tests fail after running make test-large.

psutil_large_test | 2016-09-21 17:02:48 [ INFO ] Downloading http://snap.ci.snap-telemetry.io/snap/master/latest/snapd to /usr/local/bin
psutil_large_test | 2016-09-21 17:02:57 [ INFO ] Downloading http://snap.ci.snap-telemetry.io/snap/master/latest/snapctl to /usr/local/bin
psutil_large_test | 2016-09-21 17:03:02 [ INFO ] Downloading http://snap.ci.snap-telemetry.io/plugins/snap-plugin-collector-psutil/latest/linux/x86_64/snap-plugin-collector-psutil to /etc/snap/plugins
psutil_large_test | 2016-09-21 17:03:08 [ INFO ] Downloading http://snap.ci.snap-telemetry.io/snap/master/latest/snap-plugin-processor-passthru to /etc/snap/plugins
psutil_large_test | 2016-09-21 17:03:14 [ INFO ] Downloading http://snap.ci.snap-telemetry.io/snap/master/latest/snap-plugin-publisher-mock-file to /etc/snap/plugins
psutil_large_test | 2016-09-21 17:03:20 [ INFO ] starting snapd
psutil_large_test | 2016-09-21 17:03:20 [ INFO ] snapctl plugin load /etc/snap/plugins/snap-plugin-collector-psutil
psutil_large_test | 2016-09-21 17:03:24 [ INFO ] snapctl plugin load /etc/snap/plugins/snap-plugin-processor-passthru
psutil_large_test | 2016-09-21 17:03:25 [ INFO ] snapctl plugin load /etc/snap/plugins/snap-plugin-publisher-mock-file
psutil_large_test | 2016-09-21 17:03:28 [ INFO ] snapctl task create -t /snap-plugin-collector-psutil/examples/tasks/task-psutil.json
psutil_large_test | F2016-09-21 17:03:38 [ INFO ] stopping snapd
psutil_large_test |
psutil_large_test | ======================================================================
psutil_large_test | FAIL: test_psutil_collector_plugin (__main__.PsutilCollectorLargeTest)
psutil_large_test | ----------------------------------------------------------------------
psutil_large_test | Traceback (most recent call last):
psutil_large_test |   File "/snap-plugin-collector-psutil/scripts/test/large.py", line 86, in test_psutil_collector_plugin
psutil_large_test |     self.assertEqual(len(tasks), 1, "Tasks available {} expected {}".format(len(tasks), 1))
psutil_large_test | AssertionError: Tasks available 0 expected 1
psutil_large_test |
psutil_large_test | ----------------------------------------------------------------------
psutil_large_test | Ran 1 test in 49.870s
psutil_large_test |
psutil_large_test | FAILED (failures=1)
psutil_large_test exited with code 1
exit code from large_jenkins 1

When I run the example:

2016-09-21 17:12:55 UTC [     info] creating and starting a task
Using task manifest to create task
Error creating task:Metric not found: /intel/psutil/cpu/*/user

EDIT: I figured out we need this to be merged before.

@marcin-krolik
Copy link
Collaborator Author

marcin-krolik commented Sep 22, 2016

@kindermoumoute The reason behind failing large test was old plugin version 6 executed with task manifest which introduced new, dynamic metrics which could not be handled by version 6. Until this PR is merged version 7 will not be available on S3. I changed example task manifest to query only for compatible metrics with version 6. Following PR #36 will introduce update to task manifest and version of plugin used in large tests

Nevertheless large test will fail with latest snap build. Reason behind it is this bug

Copy link
Contributor

@kindermoumoute kindermoumoute left a comment

Choose a reason for hiding this comment

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

Functionality LGTM. As this is also a really good code clean up I made comments to improve the code.

metrics := make([]plugin.MetricType, len(mts))
loadre := regexp.MustCompile(`^/intel/psutil/load/load[1,5,15]`)
cpure := regexp.MustCompile(`^/intel/psutil/cpu.*/.*`)
loadre := regexp.MustCompile(`^/intel/psutil/load/.*`)
Copy link
Contributor

Choose a reason for hiding this comment

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

With the same idea than in cpu.go, and net.go, wouldn't it make sense to remove those regex and use condition on namespaces instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regex removed

results := make([]plugin.MetricType, len(nss))

for i, ns := range nss {

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't change performances, but with the same idea to make the code clear, it might be more readable to set Data_ and Unit_ in variables before creating the metric:

        var data interface{}
        unit := "B"

        switch ns.Element(len(ns) - 1).Value {
        case "total":
            data = mem.Total
        // ...
        case "used_percent":
            data = mem.UsedPercent
            unit = ""
        // ...
        default:
            return nil, fmt.Errorf("Requested memory statistic %s is not found", ns.String())
        }

        results[i] = plugin.MetricType{
            Namespace_: ns,
            Data_:     data,
            Unit_:      unit,
        }

Copy link
Contributor

@kindermoumoute kindermoumoute left a comment

Choose a reason for hiding this comment

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

LGTM. This PR should fix #26 in the same time.

@kindermoumoute kindermoumoute merged commit d442d4c into intelsdi-x:master Sep 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants