add conntrack_count and conntrack_max #59
Conversation
|
Can one of the admins verify this patch? |
gregleroux
force-pushed
the
addConntrack
branch
from
590a379 to
312cdbd
Compare
gregleroux
force-pushed
the
addConntrack
branch
from
312cdbd to
6a3beb4
Compare
| Data_: conntrack[0].ConnTrackMax, | ||
| Timestamp_: time.Now(), | ||
| Unit_: netConntrackCounterLabels[metricName].unit, | ||
| } |
There was a problem hiding this comment.
it seems that you can combine two if statements together as data is the difference.
| @@ -62,6 +62,17 @@ var netIOCounterLabels = map[string]label{ | |||
| }, | |||
| } | |||
|
|
|||
| var netConntrackCounterLabels = map[string]label{ | |||
There was a problem hiding this comment.
what reason you didn't want to combine netConntrackCounterLables initialization with IO but rename the variable?
There was a problem hiding this comment.
Metric returned by GetMetricTypes is in static list (netIOCounterLabels and netConntrackCounterLabels), and in case of netfilter is not loaded in kernel, task with this metric will fail and plugin crash. In the reason why i separate "basic" net metric and netfilter metric.
| @@ -102,7 +121,28 @@ func netIOCounters(nss []core.Namespace) ([]plugin.MetricType, error) { | |||
| } | |||
| results = append(results, metric) | |||
| } | |||
| } else { | |||
| case "netfilter": | |||
There was a problem hiding this comment.
In the case someone try to collect netfilter metrics but an error occurred line 90, it should still throw an error.
There was a problem hiding this comment.
@gregleroux Could you address this comment?
| conntrack, err := net.FilterCounters() | ||
| if err != nil { | ||
| // Conntrack probably not loaded into the kernel. | ||
| fmt.Printf("Requested netfilter statistics %s not found", err) |
There was a problem hiding this comment.
only print error and continue? I just want to make sure the desired behavior. Do you like to return the error here?
There was a problem hiding this comment.
Yes, with this plugin, metric list is "static", snap metric list return conntrack_count while it not available on the server. I think it good, to see this error in snap log and continue to retreive other metric of the task. With return statement, task will failed and no metric will be publish. This is why i remove the return statement.
| if ns[4].Value == "conntrackcount" { | ||
| metric := plugin.MetricType{ | ||
| Namespace_: ns, | ||
| Data_: conntrack[0].ConnTrackCount, |
There was a problem hiding this comment.
What if conntrack is empty?
There was a problem hiding this comment.
if conntrack is empty, it terminate in error i think.
| if ns[4].Value == "conntrackmax" { | ||
| metric := plugin.MetricType{ | ||
| Namespace_: ns, | ||
| Data_: conntrack[0].ConnTrackMax, |
|
@gregleroux Could you extend tests to cover new metrics? |
|
For psutil plugin GetMetricTypes is base on static array of plugin.MetricType. |
Fixes #54
Summary of changes:
add conntrack_count and conntrack
/intel/psutil/netfilter/conntrackcount
/intel/psutil/netfilter/conntrackmax
How to verify it:
load task with this metrics:
"/intel/psutil/net/netfilter/conntrackcount": {},
"/intel/psutil/net/netfilter/conntrackmax": {},
Testing done:
A picture of a snapping turtle (not required but encouraged):