From 6b42839b29a14b2048a1995dff7ce74599aed275 Mon Sep 17 00:00:00 2001 From: Jimmy Cassis Date: Fri, 14 Jun 2019 11:23:38 -0700 Subject: [PATCH 1/4] Enhance NodeExporterPath JSON option to allow passing node_exporter CLI configuration --- util/metrics/reporter.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/util/metrics/reporter.go b/util/metrics/reporter.go index 62a6bb26e4..af4c499e6b 100644 --- a/util/metrics/reporter.go +++ b/util/metrics/reporter.go @@ -23,6 +23,7 @@ import ( "net/http" "os" "path/filepath" + "regexp" "strings" "time" // logging imports metrics so that we can have metrics about logging, which is more important than the four Debug lines we had here logging about metrics. TODO: find a more clever cycle resolution @@ -205,10 +206,11 @@ func (reporter *MetricReporter) tryInvokeNodeExporter(ctx context.Context) { os.Stderr} } // prepare the vargs that the new process is going to have. - vargs := []string{ - reporter.serviceConfig.NodeExporterPath, - "--web.listen-address=" + reporter.serviceConfig.NodeExporterListenAddress, - "--web.telemetry-path=" + nodeExporterMetricsPath} + whitespaceRegExp := regexp.MustCompile(`\s+`) + vargs := whitespaceRegExp.Split(reporter.serviceConfig.NodeExporterPath, -1) + vargs = append(vargs, + "--web.listen-address="+reporter.serviceConfig.NodeExporterListenAddress, + "--web.telemetry-path="+nodeExporterMetricsPath) // launch the process proc, err := os.StartProcess(vargs[0], vargs, &neAttributes) if err != nil { From 24d59c3f2b31a04aa2d4e6fd5d6a537737900e71 Mon Sep 17 00:00:00 2001 From: Jimmy Cassis Date: Fri, 14 Jun 2019 18:41:45 -0700 Subject: [PATCH 2/4] Add function and unit test to parse NodeExporterPath configuration option --- util/metrics/reporter.go | 30 +++++++++++++++---- util/metrics/reporter_test.go | 56 +++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 5 deletions(-) create mode 100755 util/metrics/reporter_test.go diff --git a/util/metrics/reporter.go b/util/metrics/reporter.go index af4c499e6b..e522173c68 100644 --- a/util/metrics/reporter.go +++ b/util/metrics/reporter.go @@ -178,6 +178,30 @@ func (reporter *MetricReporter) tryDetachNodeExporter() { } } +// parseNodeExporterArgs parses the NodeExporterPath configuration string to extract node exporter's arguments. +func parseNodeExporterArgs(nodeExporterPath string, nodeExporterListenAddress string, nodeExporterMetricsPath string) []string { + whitespaceRE := regexp.MustCompile(`\s+`) + listenAddressRE := regexp.MustCompile(`--web.listen-address=(.+)`) + telemetryPathRE := regexp.MustCompile(`--web.telemetry-path=(.+)`) + vargs := whitespaceRE.Split(nodeExporterPath, -1) + temp := vargs[:0] + for _, varg := range vargs { + if listenAddressRE.MatchString(varg) { + nodeExporterListenAddress = listenAddressRE.FindStringSubmatch(varg)[1] + } else if telemetryPathRE.MatchString(varg) { + nodeExporterMetricsPath = telemetryPathRE.FindStringSubmatch(varg)[1] + } else if varg == "" { + continue + } else { + temp = append(temp, varg) + } + } + vargs = append(vargs[:len(temp)], + "--web.listen-address="+nodeExporterListenAddress, + "--web.telemetry-path="+nodeExporterMetricsPath) + return vargs +} + func (reporter *MetricReporter) tryInvokeNodeExporter(ctx context.Context) { var err error if nil == reporter.neSync { @@ -206,11 +230,7 @@ func (reporter *MetricReporter) tryInvokeNodeExporter(ctx context.Context) { os.Stderr} } // prepare the vargs that the new process is going to have. - whitespaceRegExp := regexp.MustCompile(`\s+`) - vargs := whitespaceRegExp.Split(reporter.serviceConfig.NodeExporterPath, -1) - vargs = append(vargs, - "--web.listen-address="+reporter.serviceConfig.NodeExporterListenAddress, - "--web.telemetry-path="+nodeExporterMetricsPath) + vargs := parseNodeExporterArgs(reporter.serviceConfig.NodeExporterPath, reporter.serviceConfig.NodeExporterListenAddress, nodeExporterMetricsPath) // launch the process proc, err := os.StartProcess(vargs[0], vargs, &neAttributes) if err != nil { diff --git a/util/metrics/reporter_test.go b/util/metrics/reporter_test.go new file mode 100755 index 0000000000..0fe0ed6883 --- /dev/null +++ b/util/metrics/reporter_test.go @@ -0,0 +1,56 @@ +// Copyright (C) 2019 Algorand, Inc. +// This file is part of go-algorand +// +// go-algorand is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as +// published by the Free Software Foundation, either version 3 of the +// License, or (at your option) any later version. +// +// go-algorand is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with go-algorand. If not, see . + +package metrics + +import ( + "reflect" + "testing" +) + +var passTestcases = map[string][]string{ + "./node_exporter": []string{"./node_exporter", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // simple case + "./node_exporter --collector.systemd": []string{"./node_exporter", "--collector.systemd", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // extended case with one argument + "./node_exporter random --collector.systemd": []string{"./node_exporter", "random", "--collector.systemd", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // extended case multiple arguments + "/usr/bin/local/node_exporter --collector.systemd random": []string{"/usr/bin/local/node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // other executable path + " /usr/bin/local/node_exporter --collector.systemd random": []string{"/usr/bin/local/node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // space at beginning of option + "./node_exporter --web.telemetry-path=/foobar --web.listen-address=:9090 ": []string{"./node_exporter", "--web.listen-address=:9090", "--web.telemetry-path=/foobar"}, // overriding defaults + "./node_exporter --web.listen-address=:8080 --web.telemetry-path=/barfoo": []string{"./node_exporter", "--web.listen-address=:8080", "--web.telemetry-path=/barfoo"}, // overriding defaults different order and multiple spaces + "./node_exporter --web.listen-address=:9090 --collector.proc --web.telemetry-path=/foobar": []string{"./node_exporter", "--collector.proc", "--web.listen-address=:9090", "--web.telemetry-path=/foobar"}, // argument in between the persistent ones + "./node_exporter --web.listen-address=:9090 --collector.test --collector.systemd ": []string{"./node_exporter", "--collector.test", "--collector.systemd", "--web.listen-address=:9090", "--web.telemetry-path=/metrics"}, // argument after persistent one +} + +var failTestcases = map[string][]string{ + "./node_exporter": []string{"./node_exporter", "--web.listen-address=:9090", "--web.telemetry-path=/foobar"}, // default arguments not being passed + "./node_exporter --collector.systemd": []string{"./node_exporter", "--web.listen-address=:9100", "--web.telemetry-path=/metrics", "--collector.systemd"}, // incorrect order of persistent and added options + "./node_exporter random --collector.systemd": []string{"./node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // reversed order of persistent options + " /usr/bin/local/node_exporter --collector.systemd random": []string{" /usr/bin/local/node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // space at beginning of option preserved +} + +func TestParseNodeExporterArgs(t *testing.T) { + for test, expected := range passTestcases { + vargs := parseNodeExporterArgs(test, ":9100", "/metrics") + if !reflect.DeepEqual(vargs, expected) { + t.Errorf("Argument parsing did not result in expected value for: %v, got: %v, want: %v.", test, vargs, expected) + } + } + for test, notexpected := range failTestcases { + vargs := parseNodeExporterArgs(test, ":9100", "/metrics") + if reflect.DeepEqual(vargs, notexpected) { + t.Errorf("Argument parsing did result in expected value for: %v, got: %v, want: %v.", test, vargs, notexpected) + } + } +} From 932278c51c13e175351e03275df80cfd05bea7c3 Mon Sep 17 00:00:00 2001 From: Jimmy Cassis Date: Fri, 14 Jun 2019 21:48:11 -0700 Subject: [PATCH 3/4] Encapsulate testcase variables inside TestParseNodeExporterArgs --- util/metrics/reporter_test.go | 37 +++++++++++++++++------------------ 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/util/metrics/reporter_test.go b/util/metrics/reporter_test.go index 0fe0ed6883..4749b23b04 100755 --- a/util/metrics/reporter_test.go +++ b/util/metrics/reporter_test.go @@ -21,32 +21,31 @@ import ( "testing" ) -var passTestcases = map[string][]string{ - "./node_exporter": []string{"./node_exporter", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // simple case - "./node_exporter --collector.systemd": []string{"./node_exporter", "--collector.systemd", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // extended case with one argument - "./node_exporter random --collector.systemd": []string{"./node_exporter", "random", "--collector.systemd", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // extended case multiple arguments - "/usr/bin/local/node_exporter --collector.systemd random": []string{"/usr/bin/local/node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // other executable path - " /usr/bin/local/node_exporter --collector.systemd random": []string{"/usr/bin/local/node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // space at beginning of option - "./node_exporter --web.telemetry-path=/foobar --web.listen-address=:9090 ": []string{"./node_exporter", "--web.listen-address=:9090", "--web.telemetry-path=/foobar"}, // overriding defaults - "./node_exporter --web.listen-address=:8080 --web.telemetry-path=/barfoo": []string{"./node_exporter", "--web.listen-address=:8080", "--web.telemetry-path=/barfoo"}, // overriding defaults different order and multiple spaces - "./node_exporter --web.listen-address=:9090 --collector.proc --web.telemetry-path=/foobar": []string{"./node_exporter", "--collector.proc", "--web.listen-address=:9090", "--web.telemetry-path=/foobar"}, // argument in between the persistent ones - "./node_exporter --web.listen-address=:9090 --collector.test --collector.systemd ": []string{"./node_exporter", "--collector.test", "--collector.systemd", "--web.listen-address=:9090", "--web.telemetry-path=/metrics"}, // argument after persistent one -} - -var failTestcases = map[string][]string{ - "./node_exporter": []string{"./node_exporter", "--web.listen-address=:9090", "--web.telemetry-path=/foobar"}, // default arguments not being passed - "./node_exporter --collector.systemd": []string{"./node_exporter", "--web.listen-address=:9100", "--web.telemetry-path=/metrics", "--collector.systemd"}, // incorrect order of persistent and added options - "./node_exporter random --collector.systemd": []string{"./node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // reversed order of persistent options - " /usr/bin/local/node_exporter --collector.systemd random": []string{" /usr/bin/local/node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // space at beginning of option preserved -} - func TestParseNodeExporterArgs(t *testing.T) { + passTestcases := map[string][]string{ + "./node_exporter": []string{"./node_exporter", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // simple case + "./node_exporter --collector.systemd": []string{"./node_exporter", "--collector.systemd", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // extended case with one argument + "./node_exporter random --collector.systemd": []string{"./node_exporter", "random", "--collector.systemd", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // extended case multiple arguments + "/usr/bin/local/node_exporter --collector.systemd random": []string{"/usr/bin/local/node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // other executable path + " /usr/bin/local/node_exporter --collector.systemd random": []string{"/usr/bin/local/node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // space at beginning of option + "./node_exporter --web.telemetry-path=/foobar --web.listen-address=:9090 ": []string{"./node_exporter", "--web.listen-address=:9090", "--web.telemetry-path=/foobar"}, // overriding defaults + "./node_exporter --web.listen-address=:8080 --web.telemetry-path=/barfoo": []string{"./node_exporter", "--web.listen-address=:8080", "--web.telemetry-path=/barfoo"}, // overriding defaults different order and multiple spaces + "./node_exporter --web.listen-address=:9090 --collector.proc --web.telemetry-path=/foobar": []string{"./node_exporter", "--collector.proc", "--web.listen-address=:9090", "--web.telemetry-path=/foobar"}, // argument in between the persistent ones + "./node_exporter --web.listen-address=:9090 --collector.test --collector.systemd ": []string{"./node_exporter", "--collector.test", "--collector.systemd", "--web.listen-address=:9090", "--web.telemetry-path=/metrics"}, // argument after persistent one + } for test, expected := range passTestcases { vargs := parseNodeExporterArgs(test, ":9100", "/metrics") if !reflect.DeepEqual(vargs, expected) { t.Errorf("Argument parsing did not result in expected value for: %v, got: %v, want: %v.", test, vargs, expected) } } + + failTestcases := map[string][]string{ + "./node_exporter": []string{"./node_exporter", "--web.listen-address=:9090", "--web.telemetry-path=/foobar"}, // default arguments not being passed + "./node_exporter --collector.systemd": []string{"./node_exporter", "--web.listen-address=:9100", "--web.telemetry-path=/metrics", "--collector.systemd"}, // incorrect order of persistent and added options + "./node_exporter random --collector.systemd": []string{"./node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // reversed order of persistent options + " /usr/bin/local/node_exporter --collector.systemd random": []string{" /usr/bin/local/node_exporter", "--collector.systemd", "random", "--web.listen-address=:9100", "--web.telemetry-path=/metrics"}, // space at beginning of option preserved + } for test, notexpected := range failTestcases { vargs := parseNodeExporterArgs(test, ":9100", "/metrics") if reflect.DeepEqual(vargs, notexpected) { From 3095187912881f13b384c1592062596c2509d970 Mon Sep 17 00:00:00 2001 From: Jimmy Cassis Date: Mon, 17 Jun 2019 10:27:11 -0700 Subject: [PATCH 4/4] Use Testify's `require` package for TestParseNodeExporterArgs --- util/metrics/reporter.go | 2 +- util/metrics/reporter_test.go | 11 ++++------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/util/metrics/reporter.go b/util/metrics/reporter.go index e522173c68..78d9a67649 100644 --- a/util/metrics/reporter.go +++ b/util/metrics/reporter.go @@ -178,7 +178,7 @@ func (reporter *MetricReporter) tryDetachNodeExporter() { } } -// parseNodeExporterArgs parses the NodeExporterPath configuration string to extract node exporter's arguments. +// parseNodeExporterArgs parses the NodeExporterPath configuration string to extract Node Exporter's arguments. func parseNodeExporterArgs(nodeExporterPath string, nodeExporterListenAddress string, nodeExporterMetricsPath string) []string { whitespaceRE := regexp.MustCompile(`\s+`) listenAddressRE := regexp.MustCompile(`--web.listen-address=(.+)`) diff --git a/util/metrics/reporter_test.go b/util/metrics/reporter_test.go index 4749b23b04..5e5031a8ed 100755 --- a/util/metrics/reporter_test.go +++ b/util/metrics/reporter_test.go @@ -17,8 +17,9 @@ package metrics import ( - "reflect" "testing" + + "github.com/stretchr/testify/require" ) func TestParseNodeExporterArgs(t *testing.T) { @@ -35,9 +36,7 @@ func TestParseNodeExporterArgs(t *testing.T) { } for test, expected := range passTestcases { vargs := parseNodeExporterArgs(test, ":9100", "/metrics") - if !reflect.DeepEqual(vargs, expected) { - t.Errorf("Argument parsing did not result in expected value for: %v, got: %v, want: %v.", test, vargs, expected) - } + require.Equalf(t, vargs, expected, "Argument parsing did not result in expected value for: %v, got: %v, want: %v.", test, vargs, expected) } failTestcases := map[string][]string{ @@ -48,8 +47,6 @@ func TestParseNodeExporterArgs(t *testing.T) { } for test, notexpected := range failTestcases { vargs := parseNodeExporterArgs(test, ":9100", "/metrics") - if reflect.DeepEqual(vargs, notexpected) { - t.Errorf("Argument parsing did result in expected value for: %v, got: %v, want: %v.", test, vargs, notexpected) - } + require.NotEqualf(t, vargs, notexpected, "Argument parsing did result in expected value for: %v, got: %v, want: %v.", test, vargs, notexpected) } }