Review 1 - Add descriptions of where column definitions come from

Add haproxy rtime (average response latency for http in ms over the last
1024 requests) as a metric.

Author: smarterclayton

pkg/cmd/admin/router/router.go

@@ -666,7 +666,7 @@ func RunCmdRouter(f *clientcmd.Factory, cmd *cobra.Command, out, errout io.Write
 		}
 		env["ROUTER_CANONICAL_HOSTNAME"] = cfg.RouterCanonicalHostname
 	}
-	// automatically start the internal metrics agent if the type has metrics
+	// automatically start the internal metrics agent if we are handling a known type
 	if cfg.Type == "haproxy-router" {
 		env["ROUTER_LISTEN_ADDR"] = fmt.Sprintf("0.0.0.0:%d", defaultStatsPort-1)
 		env["ROUTER_METRICS_TYPE"] = "haproxy"

pkg/cmd/infra/router/template.go

@@ -14,6 +14,7 @@ import (
 
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	ktypes "k8s.io/kubernetes/pkg/types"
+	"k8s.io/kubernetes/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/util/validation"
 
 	ocmd "github.com/openshift/origin/pkg/cmd/cli/cmd"
@@ -182,9 +183,12 @@ func (o *TemplateRouterOptions) Complete() error {
 	return o.RouterSelection.Complete()
 }
 
+// supportedMetricsTypes is the set of supported metrics arguments
+var supportedMetricsTypes = sets.NewString("haproxy")
+
 func (o *TemplateRouterOptions) Validate() error {
-	if len(o.MetricsType) > 0 && o.MetricsType != "haproxy" {
-		return errors.New("supported metrics types are: 'haproxy'")
+	if len(o.MetricsType) > 0 && !supportedMetricsTypes.Has(o.MetricsType) {
+		return fmt.Errorf("supported metrics types are: %s", strings.Join(supportedMetricsTypes.List(), ", "))
 	}
 	if len(o.RouterName) == 0 {
 		return errors.New("router must have a name to identify itself in route status")
@@ -204,6 +208,10 @@ func (o *TemplateRouterOptions) Run() error {
 	var reloadCallbacks []func()
 	switch {
 	case o.MetricsType == "haproxy" && len(o.ListenAddr) > 0:
+		if len(o.StatsUsername) == 0 || len(o.StatsPassword) == 0 {
+			glog.Warningf("Metrics were requested but no username or password has been provided - the metrics endpoint will not be accessible to prevent accidental security breaches")
+		}
+		// Exposed to allow tuning in production if this becomes an issue
 		var timeout time.Duration
 		if t := util.Env("ROUTER_METRICS_HAPROXY_TIMEOUT", ""); len(t) > 0 {
 			d, err := time.ParseDuration(t)
@@ -212,6 +220,7 @@ func (o *TemplateRouterOptions) Run() error {
 			}
 			timeout = d
 		}
+		// Exposed to allow tuning in production if this becomes an issue
 		var baseScrapeInterval time.Duration
 		if t := util.Env("ROUTER_METRICS_HAPROXY_BASE_SCRAPE_INTERVAL", ""); len(t) > 0 {
 			d, err := time.ParseDuration(t)
@@ -220,6 +229,7 @@ func (o *TemplateRouterOptions) Run() error {
 			}
 			baseScrapeInterval = d
 		}
+		// Exposed to allow tuning in production if this becomes an issue
 		var serverThreshold int
 		if t := util.Env("ROUTER_METRICS_HAPROXY_SERVER_THRESHOLD", ""); len(t) > 0 {
 			i, err := strconv.Atoi(t)
@@ -228,18 +238,21 @@ func (o *TemplateRouterOptions) Run() error {
 			}
 			serverThreshold = i
 		}
+		// Exposed to allow tuning in production if this becomes an issue
 		var exported []int
 		if t := util.Env("ROUTER_METRICS_HAPROXY_EXPORTED", ""); len(t) > 0 {
 			for _, s := range strings.Split(t, ",") {
 				i, err := strconv.Atoi(s)
 				if err != nil {
-					return errors.New("ROUTER_METRICS_HAPROXY_EXPORTED must be a comma delimited list of non-negative integers")
+					return errors.New("ROUTER_METRICS_HAPROXY_EXPORTED must be a comma delimited list of column numbers to extract from the HAProxy configuration")
 				}
 				exported = append(exported, i)
 			}
 		}
 		_, err := haproxy.NewPrometheusCollector(haproxy.PrometheusOptions{
-			ScrapeURI:          util.Env("ROUTER_METRICS_HAPROXY_SCRAPE_URI", ""),
+			// Only template router customizers who alter the image should need this
+			ScrapeURI: util.Env("ROUTER_METRICS_HAPROXY_SCRAPE_URI", ""),
+			// Only template router customizers who alter the image should need this
 			PidFile:            util.Env("ROUTER_METRICS_HAPROXY_PID_FILE", ""),
 			Timeout:            timeout,
 			ServerThreshold:    serverThreshold,

pkg/router/metrics/haproxy/haproxy.go

@@ -28,7 +28,8 @@ const (
 	// HAProxy 1.4
 	// # pxname,svname,qcur,qmax,scur,smax,slim,stot,bin,bout,dreq,dresp,ereq,econ,eresp,wretr,wredis,status,weight,act,bck,chkfail,chkdown,lastchg,downtime,qlimit,pid,iid,sid,throttle,lbtot,tracked,type,rate,rate_lim,rate_max,check_status,check_code,check_duration,hrsp_1xx,hrsp_2xx,hrsp_3xx,hrsp_4xx,hrsp_5xx,hrsp_other,hanafail,req_rate,req_rate_max,req_tot,cli_abrt,srv_abrt,
 	// HAProxy 1.5
-	// pxname,svname,qcur,qmax,scur,smax,slim,stot,bin,bout,dreq,dresp,ereq,econ,eresp,wretr,wredis,status,weight,act,bck,chkfail,chkdown,lastchg,downtime,qlimit,pid,iid,sid,throttle,lbtot,tracked,type,rate,rate_lim,rate_max,check_status,check_code,check_duration,hrsp_1xx,hrsp_2xx,hrsp_3xx,hrsp_4xx,hrsp_5xx,hrsp_other,hanafail,req_rate,req_rate_max,req_tot,cli_abrt,srv_abrt,comp_in,comp_out,comp_byp,comp_rsp,lastsess,
+	// These columns are part of the stable API for HAProxy and are documented here: https://cbonte.github.io/haproxy-dconv/1.5/configuration.html#9.1
+	// pxname,svname,qcur,qmax,scur,smax,slim,stot,bin,bout,dreq,dresp,ereq,econ,eresp,wretr,wredis,status,weight,act,bck,chkfail,chkdown,lastchg,downtime,qlimit,pid,iid,sid,throttle,lbtot,tracked,type,rate,rate_lim,rate_max,check_status,check_code,check_duration,hrsp_1xx,hrsp_2xx,hrsp_3xx,hrsp_4xx,hrsp_5xx,hrsp_other,hanafail,req_rate,req_rate_max,req_tot,cli_abrt,srv_abrt,comp_in,comp_out,comp_byp,comp_rsp,lastsess,last_chk,last_agt,qtime,ctime,rtime,ttime
 	expectedCsvFieldCount = 52
 	statusField           = 17
 
@@ -91,8 +92,9 @@ func (m metrics) Names() []int {
 	return keys
 }
 
-// defaultSelectedMetrics is the list of metrics included by default
-var defaultSelectedMetrics = []int{2, 4, 5, 7, 8, 9, 13, 14, 17, 21, 24, 33, 35, 40, 43}
+// defaultSelectedMetrics is the list of metrics included by default. These metrics are a subset
+// of the metrics exposed by haproxy_exporter by default for performance reasons.
+var defaultSelectedMetrics = []int{2, 4, 5, 7, 8, 9, 13, 14, 17, 21, 24, 33, 35, 40, 43, 60}
 
 // Exporter collects HAProxy stats from the given URI and exports them using
 // the prometheus metrics package.
@@ -194,6 +196,7 @@ func NewExporter(opts PrometheusOptions) (*Exporter, error) {
 			43: newFrontendMetric("http_responses_total", "Total of HTTP responses.", prometheus.Labels{"code": "5xx"}),
 			44: newFrontendMetric("http_responses_total", "Total of HTTP responses.", prometheus.Labels{"code": "other"}),
 			48: newFrontendMetric("http_requests_total", "Total HTTP requests.", nil),
+			60: newFrontendMetric("http_average_response_latency_milliseconds", "Average response latency of the last 1024 requests in milliseconds.", nil),
 		}),
 		reducedBackendExports: map[int]struct{}{2: {}, 3: {}, 7: {}, 17: {}},
 		backendMetrics: filterMetrics(opts.ExportedMetrics, metrics{
@@ -219,6 +222,7 @@ func NewExporter(opts PrometheusOptions) (*Exporter, error) {
 			42: newBackendMetric("http_responses_total", "Total of HTTP responses.", prometheus.Labels{"code": "4xx"}),
 			43: newBackendMetric("http_responses_total", "Total of HTTP responses.", prometheus.Labels{"code": "5xx"}),
 			44: newBackendMetric("http_responses_total", "Total of HTTP responses.", prometheus.Labels{"code": "other"}),
+			60: newBackendMetric("http_average_response_latency_milliseconds", "Average response latency of the last 1024 requests in milliseconds.", nil),
 		}),
 		serverMetrics: filterMetrics(opts.ExportedMetrics, metrics{
 			2:  newServerMetric("current_queue", "Current number of queued requests assigned to this server.", nil),
@@ -246,6 +250,7 @@ func NewExporter(opts PrometheusOptions) (*Exporter, error) {
 			42: newServerMetric("http_responses_total", "Total of HTTP responses.", prometheus.Labels{"code": "4xx"}),
 			43: newServerMetric("http_responses_total", "Total of HTTP responses.", prometheus.Labels{"code": "5xx"}),
 			44: newServerMetric("http_responses_total", "Total of HTTP responses.", prometheus.Labels{"code": "other"}),
+			60: newServerMetric("http_average_response_latency_milliseconds", "Average response latency of the last 1024 requests in milliseconds.", nil),
 		}),
 	}, nil
 }