Extract numeric time series from string values #212

conr · 2017-08-02T13:18:18Z

Closes #150

brian-brazil · 2017-08-02T13:29:41Z

generator/tree.go

+	// Apply module config overrides to their corresponding metrics.
+	for name, params := range cfg.Overrides {
+		for _, metric := range out.Metrics {
+			if name == metric.Name {


Overrides may be provided by oid or name

brian-brazil · 2017-08-02T13:30:21Z

config/config.go

@@ -256,6 +253,60 @@ func (c *Auth) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	return nil
 }

+type MetricOverrides struct {
+	StringMetrics map[string][]StringMetric `yaml:"string_metrics,omitempty"`


At the collector level there should only be this setting, not the whole MetricOverrides as the rest of those will only exist in the generator.

brian-brazil · 2017-08-02T13:30:54Z

config/config.go

@@ -256,6 +253,60 @@ func (c *Auth) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	return nil
 }

+type MetricOverrides struct {
+	StringMetrics map[string][]StringMetric `yaml:"string_metrics,omitempty"`


I don't think string_metrics quite captures it, we have other string metrics. regexp_extract or similar would be better.

brian-brazil · 2017-08-02T13:31:36Z

collector.go

@@ -154,7 +154,10 @@ PduLoop:
 			}
 			if head.metric != nil {
 				// Found a match.
-				ch <- pduToSample(oidList[i+1:], &pdu, head.metric, oidToPdu)
+				samples := pduToSample(oidList[i+1:], &pdu, head.metric, oidToPdu)


it's really pduToSamples now

brian-brazil · 2017-08-02T13:33:11Z

collector.go

+		if len(strMetricSlice) > 0 {
+			for _, strMetric := range strMetricSlice {
+				value := strMetric.Regex.ReplaceAllString(pduValue, strMetric.Value)
+				v, _ := strconv.ParseFloat(value, 64)


You need to check for an error here

brian-brazil · 2017-08-02T13:33:33Z

collector.go

+			for _, strMetric := range strMetricSlice {
+				value := strMetric.Regex.ReplaceAllString(pduValue, strMetric.Value)
+				v, _ := strconv.ParseFloat(value, 64)
+				newMetric := prometheus.MustNewConstMetric(prometheus.NewDesc(metric.Name+"_"+name, metric.Help, []string{""}, nil),


Don't add an underscore here

brian-brazil · 2017-08-02T13:34:44Z

collector.go

@@ -192,6 +196,9 @@ func pduToSample(indexOids []int, pdu *gosnmp.SnmpPDU, metric *config.Metric, oi
 		t = prometheus.GaugeValue
 		value = 1.0
 		stringType = true
+		if len(metric.Overrides.StringMetrics) > 0 {
+			res = generateOverrideMetrics(metric, pdu.Value.(string))


You still need all the label logic below to apply. I'd reorder the code so this flows more naturally.

brian-brazil · 2017-08-02T13:36:09Z

collector.go

+			for _, strMetric := range strMetricSlice {
+				value := strMetric.Regex.ReplaceAllString(pduValue, strMetric.Value)
+				v, _ := strconv.ParseFloat(value, 64)
+				newMetric := prometheus.MustNewConstMetric(prometheus.NewDesc(metric.Name+"_"+name, metric.Help, []string{""}, nil),


Why do you have an empty labelname?

brian-brazil · 2017-08-02T13:37:56Z

collector.go

+	for name, strMetricSlice := range metric.Overrides.StringMetrics {
+		if len(strMetricSlice) > 0 {
+			for _, strMetric := range strMetricSlice {
+				value := strMetric.Regex.ReplaceAllString(pduValue, strMetric.Value)


We need to check that the regex matches before doing subsitution.

brian-brazil · 2017-08-02T13:38:09Z

collector.go

+func generateOverrideMetrics(metric *config.Metric, pduValue string) []prometheus.Metric {
+	results := []prometheus.Metric{}
+	for name, strMetricSlice := range metric.Overrides.StringMetrics {
+		if len(strMetricSlice) > 0 {


No need for this, the for already handles this correctly

brian-brazil · 2017-08-03T10:49:12Z

collector.go

-	// If the name is already an index, we do not need to set it again.
-	if stringType {
+		// For strings we put the value as a label with the same name as the metric.
+		// If the name is already an index, we do not need to set it again.
 		if _, ok := labels[metric.Name]; !ok {


This code doesn't apply if we're applying the regexes

brian-brazil · 2017-08-03T10:49:23Z

collector.go

 		if _, ok := labels[metric.Name]; !ok {
 			labelnames = append(labelnames, metric.Name)
 			labelvalues = append(labelvalues, pduValueAsString(pdu, metric.Type))
 		}
+		if len(metric.Overrides) > 0 {
+			res = generateOverrideMetrics(metric, pdu.Value.(string), labelnames, labelvalues)


I'd return here

brian-brazil · 2017-08-03T10:50:32Z

config/config.go

+	Help      string                     `yaml:"help"`
+	Indexes   []*Index                   `yaml:"indexes,omitempty"`
+	Lookups   []*Lookup                  `yaml:"lookups,omitempty"`
+	Overrides map[string][]RegexpExtract `yaml:"regexp_extract,omitempty"`


The field should be called RegexpExtracts, not overrides

brian-brazil · 2017-08-03T10:51:27Z

config/config.go

+}
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (re *Regexp) UnmarshalYAML(unmarshal func(interface{}) error) error {


Make sure to add an unmarshalling test for a config that's missing the regex, we've had issues there in the past

brian-brazil · 2017-08-03T10:52:07Z

config/config.go

+	if err := unmarshal((*plain)(c)); err != nil {
+		return err
+	}
+	if err := CheckOverflow(c.XXX, "module"); err != nil {


regexp_extract, not module

brian-brazil · 2017-08-03T10:52:44Z

generator/config.go

+	if err := unmarshal((*plain)(c)); err != nil {
+		return err
+	}
+	if err := config.CheckOverflow(c.XXX, "module"); err != nil {


brian-brazil · 2017-08-03T10:55:44Z

collector.go

+				value := strMetric.Regex.ReplaceAllString(pduValue, strMetric.Value)
+				v, err := strconv.ParseFloat(value, 64)
+				if err != nil {
+					log.Errorf("Error parsing float64 from value: %v", value)


This would be a debug log at most, and you need to continue when it happens and try the next regex

brian-brazil · 2017-08-03T11:00:07Z

collector.go

+	results := []prometheus.Metric{}
+	for name, strMetricSlice := range metric.Overrides {
+		for _, strMetric := range strMetricSlice {
+			if strMetric.Regex.MatchString(pduValue) {


Look at https://github.com/prometheus/prometheus/blob/master/relabel/relabel.go#L57 for how to approach this

brian-brazil · 2017-08-03T11:00:54Z

collector.go

+				}
+				newMetric := prometheus.MustNewConstMetric(prometheus.NewDesc(metric.Name+name, metric.Help, labelnames, nil),
+					prometheus.GaugeValue, v, labelvalues...)
+				results = append(results, newMetric)


If we've gotten this far, we need to break

brian-brazil · 2017-08-04T15:52:39Z

collector.go

-	// If the name is already an index, we do not need to set it again.
-	if stringType {
+		if len(metric.RegexpExtracts) > 0 {
+			return generateOverrideMetrics(metric, pdu.Value.(string), labelnames, labelvalues)


You should have a test where the value is a number rather than a string.

brian-brazil · 2017-08-04T15:57:16Z

collector.go

-	return prometheus.MustNewConstMetric(prometheus.NewDesc(metric.Name, metric.Help, labelnames, nil),
-		t, value, labelvalues...)
+
+	if len(res) == 0 {


You don't need this check

brian-brazil · 2017-08-04T15:57:33Z

collector.go

-		t, value, labelvalues...)
+
+	if len(res) == 0 {
+		res = append(res, prometheus.MustNewConstMetric(prometheus.NewDesc(metric.Name, metric.Help, labelnames, nil),


You know there's only one element in the list, you can create a literal here.

brian-brazil · 2017-08-04T15:57:58Z

collector.go

+	return res
+}
+
+func generateOverrideMetrics(metric *config.Metric, pduValue string, labelnames, labelvalues []string) []prometheus.Metric {


This is applying the regex extracts, not all overrides

brian-brazil · 2017-08-04T15:59:16Z

collector.go

+	for name, strMetricSlice := range metric.RegexpExtracts {
+		for _, strMetric := range strMetricSlice {
+			if strMetric.Regex.MatchString(pduValue) {
+				value := strMetric.Regex.ReplaceAllString(pduValue, strMetric.Value)


This needs to use the same logic as relabelling does so there's full regex functionality.

brian-brazil · 2017-08-04T15:59:51Z

collector.go

+					log.Debugf("Error parsing float64 from value: %v", value)
+					continue
+				}
+				newMetric := prometheus.MustNewConstMetric(prometheus.NewDesc(metric.Name+name, metric.Help, labelnames, nil),


In the Help you can mention that this was regex extracted

brian-brazil · 2017-08-04T16:00:32Z

collector_test.go

+				Name:           "snIfOpticalLaneMonitoringTemperature",
+				Oid:            "1.1.1.1.1",
+				Help:           "HelpText",
+				RegexpExtracts: strMetrics,


No need for an extra variable, you can put this in literally

brian-brazil · 2017-08-04T16:01:56Z

generator/config.go

-	Walk    []string  `yaml:"walk"`
-	Lookups []*Lookup `yaml:"lookups"`
+type MetricOverrides struct {
+	RegexpExtracts map[string][]config.RegexpExtract `yaml:"regexp_extract,omitempty"`


We'd said to remove the p

brian-brazil · 2017-08-08T12:56:51Z

collector.go

-	// If the name is already an index, we do not need to set it again.
-	if stringType {
+		if len(metric.RegexpExtracts) > 0 {
+			if reflect.TypeOf(pdu.Value).Name() != "string" {


You don't need reflection here, you can do

v, ok := pdu.Value.(string)

And then check ok.

brian-brazil · 2017-08-08T13:00:50Z

collector.go

+				continue
+			}
+			res := strMetric.Regex.ExpandString([]byte{}, strMetric.Value, pduValue, indexes)
+			if len(res) == 0 {


You don't need this, the parsing should catch it

brian-brazil · 2017-08-08T13:14:30Z

collector_test.go

 	"github.com/prometheus/snmp_exporter/config"
 )

 func TestPduToSample(t *testing.T) {
+	var regexpMatch, regexpNoMatch config.Regexp
+	regexpMatch.Regexp, _ = regexp.Compile(".*")


Put these inline in the struct, it's easier to understand the tests that way. You can use MustCompile

brian-brazil · 2017-08-09T10:46:45Z

collector.go

+		if len(metric.RegexpExtracts) > 0 {
+			v, ok := pdu.Value.(string)
+			if !ok {
+				log.Errorf("Invalid PDU value type: got %v, want string", reflect.TypeOf(pdu.Value))


You could use %T here which will pull out the type

brian-brazil · 2017-08-09T10:50:37Z

collector.go

+			res := strMetric.Regex.ExpandString([]byte{}, strMetric.Value, pduValue, indexes)
+			v, err := strconv.ParseFloat(string(res), 64)
+			if err != nil {
+				log.Debugf("Error parsing float64 from value: %v", res)


Including the metric name here would be useful, plus other log lines where you can

You missed this comment

brian-brazil · 2017-08-09T10:51:08Z

collector_test.go

+					},
+				},
+			},
+			expectedMetrics: []string{},


Put this on the others too

brian-brazil · 2017-08-09T10:52:08Z

collector_test.go

+					},
+					"Blank": []config.RegexpExtract{
+						{
+							Regex: config.Regexp{


Can you test multiple regexes too? We want to ensure first match wins.

brian-brazil · 2017-08-11T10:28:39Z

collector_test.go

+		metric          *config.Metric
+		oidToPdu        map[string]gosnmp.SnmpPDU
+		expectedMetrics map[string]string
+		// expectedMetricDescs []string


Why is this commented out?

brian-brazil · 2017-08-11T10:30:37Z

collector.go

+			res := strMetric.Regex.ExpandString([]byte{}, strMetric.Value, pduValue, indexes)
+			v, err := strconv.ParseFloat(string(res), 64)
+			if err != nil {
+				log.Debugf("Error parsing float64 from value: %v", res)


You missed this comment

brian-brazil · 2017-08-14T15:14:13Z

collector.go

+		for _, strMetric := range strMetricSlice {
+			indexes := strMetric.Regex.FindStringSubmatchIndex(pduValue)
+			if indexes == nil {
+				log.Debugf("No match found for regexp: %v against value: %v for metric %v", strMetric.Regex.String(), pduValue, name)


name is the name of the extract, not the metric. You want metric.Name.

brian-brazil · 2017-08-14T15:15:55Z

generator/README.md

+       metricName:
+         regexp_extract:
+           Temp: # A new metric will be created appending this to the metricName to become metricNameTemp.
+             - regex: '.*' # Regex to extract a value from the returned SNMP walks's value.


For the example to make sense, this should be (.*) so that there is a $1

Also mention it's a float64 value

brian-brazil · 2017-08-14T15:17:06Z

generator/config.go

-	Walk    []string  `yaml:"walk"`
-	Lookups []*Lookup `yaml:"lookups"`
+type MetricOverrides struct {
+	RegexpExtracts map[string][]config.RegexpExtract `yaml:"regex_extract,omitempty"`


The field name here doesn't match the docs or unittest.

brian-brazil · 2017-08-14T15:18:48Z

Format.md also needs updating.

brian-brazil · 2017-08-14T15:42:45Z

generator/README.md

+         regex_extracts:
+           Temp: # A new metric will be created appending this to the metricName to become metricNameTemp.
+             - regex: '(.*)' # Regex to extract a value from the returned SNMP walks's value.
+               value: '$1' # Defaults to $1, float64.


"the result will be parsed as a float64"

brian-brazil · 2017-08-14T15:43:17Z

generator/FORMAT.md

@@ -45,4 +45,11 @@ module_name:
         oid: 1.3.6.1.2.1.2.2.1.2  # OID to look under.
         labelname: ifDescr        # Output label name.
         type: OctetString         # Type of output object.
+     # Overrides allows for per-module overrides of bits of MIBs


This isn't the format of the exporter's yml, it's just the regex_extracts.

brian-brazil · 2017-08-14T15:43:38Z

collector.go

+			res := strMetric.Regex.ExpandString([]byte{}, strMetric.Value, pduValue, indexes)
+			v, err := strconv.ParseFloat(string(res), 64)
+			if err != nil {
+				log.Debugf("Error parsing float64 from value: %v for metric: %v", res, name)


metric.Name here too

conr · 2017-08-14T16:03:50Z

@brian-brazil

brian-brazil · 2017-08-14T16:15:09Z

Thanks!

brian-brazil reviewed Aug 2, 2017

View reviewed changes

conr force-pushed the regex branch 2 times, most recently from 31d16ed to 2ab6e7c Compare August 2, 2017 15:51

brian-brazil reviewed Aug 3, 2017

View reviewed changes

conr force-pushed the regex branch 4 times, most recently from eb00fc5 to d7eeee1 Compare August 4, 2017 12:57

brian-brazil reviewed Aug 4, 2017

View reviewed changes

conr force-pushed the regex branch 6 times, most recently from 18a7c64 to 9550690 Compare August 8, 2017 12:25

brian-brazil reviewed Aug 8, 2017

View reviewed changes

conr force-pushed the regex branch 2 times, most recently from da44f88 to d48adb7 Compare August 8, 2017 17:13

brian-brazil reviewed Aug 9, 2017

View reviewed changes

conr force-pushed the regex branch 3 times, most recently from db0a174 to aa1bad1 Compare August 9, 2017 14:05

brian-brazil reviewed Aug 11, 2017

View reviewed changes

conr force-pushed the regex branch 3 times, most recently from 0fe2ed5 to 183c424 Compare August 14, 2017 14:17

brian-brazil reviewed Aug 14, 2017

View reviewed changes

conr force-pushed the regex branch 2 times, most recently from 89545a6 to 36f6930 Compare August 14, 2017 15:30

brian-brazil reviewed Aug 14, 2017

View reviewed changes

Extract numeric time series from string values

97adef4

conr force-pushed the regex branch from 36f6930 to 97adef4 Compare August 14, 2017 16:03

brian-brazil merged commit 6ea43b7 into prometheus:master Aug 14, 2017

This was referenced Mar 27, 2018

[Discussion] Appropriate level of normalization in snmp_exporter and the generator #180

Closed

Allow overriding of MIB information #186

Closed

Extract numeric time series from string values #212

Extract numeric time series from string values #212

Conversation

conr commented Aug 2, 2017

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

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

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

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

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

brian-brazil commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conr commented Aug 14, 2017

brian-brazil commented Aug 14, 2017