search

intelsdi-x / snap-plugin-collector-disk

Collects Linux disk metrics from /proc/diskstats
http://snap-telemetry.io/
Apache License 2.0
4 stars 23 forks source link

proc_path not presented as a "Rule" #8

Closed elemoine closed 8 years ago

elemoine commented 8 years ago

I used this in my workflow definition:

  "workflow": {
    "collect": {
      "config": {
        "/intel/procfs": {
          "proc_path": "/host-proc"
        }
      },
      "metrics": {

When I do snapctl metric get -m /intel/procfs/disk/loop0/io_time I get this:

$ snapctl metric get -m /intel/procfs/disk/loop0/io_time       
NAMESPACE                                VERSION         UNIT    LAST ADVERTISED TIME            DESCRIPTION
/intel/procfs/disk/loop0/io_time         3                       Fri, 17 Jun 2016 11:31:23 UTC   

  Rules for collecting /intel/procfs/disk/loop0/io_time:

       NAME      TYPE    DEFAULT         REQUIRED        MINIMUM         MAXIMUM

The issue is that there is no proc_path rule!

The "swap" plugin, for example, does not have the same issue:

$ snapctl metric get -m /intel/procfs/swap/io/out_pages_per_sec
NAMESPACE                                        VERSION         UNIT    LAST ADVERTISED TIME            DESCRIPTION
/intel/procfs/swap/io/out_pages_per_sec          3                       Fri, 17 Jun 2016 11:31:25 UTC   

  Rules for collecting /intel/procfs/swap/io/out_pages_per_sec:

       NAME              TYPE            DEFAULT         REQUIRED        MINIMUM         MAXIMUM
       proc_path         string          /proc           false

I guess this has to do with the rule created by the "swap" plugin in the GetConfigPolicy function.

tiffanyfay commented 8 years ago

Yes, that is why. I had made a comment about the config policy rule on PR #7, but I guess it was not added.

@marcin-krolik

marcin-krolik commented 8 years ago

@tiffanyfj @elemoine Indeed, it's my fault. I forgot about it somehow. I will fix that.

tiffanyfay commented 8 years ago

No worries. Lots of comments to keep track of. Thanks @marcin-krolik !

elemoine commented 8 years ago

@marcin-krolik Thanks. Please coordinate with @obourdon, who might have started on that.

obourdon commented 8 years ago

@marcin-krolik in fact I have started working on uniformizing the way plugins get their config starting with CPU: see one of my local commits and was planning to get the same code across all plugins in turn

marcin-krolik commented 8 years ago

@obourdon thanks! It looks good, go ahead with it.

obourdon commented 8 years ago

This issue is fixed in this PR which has been merged

obourdon commented 8 years ago

@marcin-krolik @elemoine : don't know who can close this issue has the corresponding PR has been merged

Oops sorry I thought it was merged but seems like it is still pending :-(, seems kind of weird to me because merge commit is effectively in master. I do not understand why we are in this situation.