Repository: knative/pkg
Pull request #1411 (2020-06-16 11:24:58)

Make Prometheus exporter port configurable via env
## Proposed Changes

- Add support for a new environment variable `METRICS_PROMETHEUS_PORT` that can override the default Prometheus port in the metrics exporter.

Rational: the default port 9090 clashes with Serving's queue proxy when a receive adapter is deployed as a Knative Service. There is currently no way to make that work without explicitly disabling metrics by setting the `K_METRICS_CONFIG` env var to `""`.

Second tentative after closing knative/eventing#3316.

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1411#" title="Author self-approved">antoineco</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **yanweiguo**
You can assign the PR to them by writing `/assign @yanweiguo` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[metrics/OWNERS](https://github.com/knative/pkg/blob/master/metrics/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["yanweiguo"]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[metrics/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1411/pull-knative-pkg-go-coverage/1272852816355921920/artifacts/line-cov.html#file0) | 93.0% | 92.5% | -0.5

slinkydeveloper: /assign
slinkydeveloper: /lgtm

That should solve your use case :wink: 
antoineco: I do hope I'm not the only person who deploys Knative Services that expose metrics via the Kn shared main :)
lionelvillard: We need this for the mt github adapter.
lionelvillard: related to https://github.com/knative/serving/issues/8047
yanweiguo: You can set `PrometheusPort` in the `ExporterOptions`, for example: https://github.com/knative/pkg/blob/099343d49e12915275bcd946fc97716f981aa6e2/metrics/exporter.go#L107

I'm curious why it doesn't work for your case.
antoineco: @yanweiguo Because ConfigMap watchers do not expose the `ExporterOptions`, only the `ConfigMap` field can be modified by changing the actual ConfigMap data. Examples:

* [eventing/pkg/reconciler/source/config_watcher.go](https://github.com/knative/eventing/blob/master/pkg/reconciler/source/config_watcher.go)
* [pkg/injection/sharedmain/main.go](https://github.com/knative/pkg/blob/0d967ff0d8789d19096d03062e8c2fd961c95407/injection/sharedmain/main.go#L367-L379)

If I want to access the `ExporterOptions`, I have to write my own main, which defies the purpose of the shared main, especially when writing event sources.
knative-prow-robot: @antoineco: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
knative-prow-robot: New changes are detected. LGTM label has been removed.
antoineco: @vagababov I addressed both of your comments.
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[metrics/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1411/pull-knative-pkg-go-coverage/1273015890882859008/artifacts/line-cov.html#file0) | 93.0% | 93.6% | 0.6

yanweiguo: /approve
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1411#" title="Author self-approved">antoineco</a>*, *<a href="https://github.com/knative/pkg/pull/1411#issuecomment-645060620" title="Approved">yanweiguo</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[metrics/OWNERS](https://github.com/knative/pkg/blob/master/metrics/OWNERS)~~ [yanweiguo]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/metrics/config.go b/metrics/config.go
index 5b7995795..556001255 100644
--- a/metrics/config.go
+++ b/metrics/config.go
@@ -67,6 +67,7 @@ const (
 	CollectorAddressKey = "metrics.opencensus-address"
 	CollectorSecureKey  = "metrics.opencensus-require-tls"
 
+	prometheusPortEnvName = "METRICS_PROMETHEUS_PORT"
 	defaultPrometheusPort = 9090
 	maxPrometheusPort     = 65535
 	minPrometheusPort     = 1024
@@ -217,11 +218,18 @@ func createMetricsConfig(ops ExporterOptions, logger *zap.SugaredLogger) (*metri
 	if mc.backendDestination == Prometheus {
 		pp := ops.PrometheusPort
 		if pp == 0 {
-			pp = defaultPrometheusPort
+			var err error
+			pp, err = prometheusPort()
+			if err != nil {
+				return nil, fmt.Errorf("failed to determine Prometheus port: %w", err)
+			}
 		}
+
 		if pp < minPrometheusPort || pp > maxPrometheusPort {
-			return nil, fmt.Errorf("invalid port %v, should between %v and %v", pp, minPrometheusPort, maxPrometheusPort)
+			return nil, fmt.Errorf("invalid port %d, should be between %d and %d",
+				pp, minPrometheusPort, maxPrometheusPort)
 		}
+
 		mc.prometheusPort = pp
 	}
 
@@ -327,6 +335,25 @@ import (
 )`, DomainEnv, DomainEnv))
 }
 
+// prometheusPort returns the TCP port number configured via the environment
+// for the Prometheus metrics exporter if it's set, a default value otherwise.
+// No validation is performed on the port value, other than ensuring that value
+// is a valid port number (16-bit unsigned integer).
+func prometheusPort() (int, error) {
+	ppStr := os.Getenv(prometheusPortEnvName)
+	if ppStr == "" {
+		return defaultPrometheusPort, nil
+	}
+
+	pp, err := strconv.ParseUint(ppStr, 10, 16)
+	if err != nil {
+		return -1, fmt.Errorf("the environment variable %q could not be parsed as a port number: %w",
+			prometheusPortEnvName, err)
+	}
+
+	return int(pp), nil
+}
+
 // JsonToMetricsOptions converts a json string of a
 // ExporterOptions. Returns a non-nil ExporterOptions always.
 func JsonToMetricsOptions(jsonOpts string) (*ExporterOptions, error) {


Test patch:
diff --git a/metrics/config_test.go b/metrics/config_test.go
index bba592f14..3831dd66f 100644
--- a/metrics/config_test.go
+++ b/metrics/config_test.go
@@ -17,8 +17,11 @@ package metrics
 
 import (
 	"context"
+	"math"
 	"os"
 	"path"
+	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -135,7 +138,7 @@ var (
 			Component:      testComponent,
 			PrometheusPort: 1023,
 		},
-		expectedErr: "invalid port 1023, should between 1024 and 65535",
+		expectedErr: "invalid port 1023, should be between 1024 and 65535",
 	}, {
 		name: "tooBigPrometheusPort",
 		ops: ExporterOptions{
@@ -146,7 +149,7 @@ var (
 			Component:      testComponent,
 			PrometheusPort: 65536,
 		},
-		expectedErr: "invalid port 65536, should between 1024 and 65535",
+		expectedErr: "invalid port 65536, should be between 1024 and 65535",
 	}}
 
 	successTests = []struct {
@@ -505,13 +508,49 @@ var (
 		},
 		expectedNewExporter: true,
 	}}
+)
+
+func successTestsInit() {
+	SetStackdriverSecretLocation(StackdriverSecretNameDefault, StackdriverSecretNamespaceDefault)
+}
+
+func TestGetMetricsConfig(t *testing.T) {
+	for _, test := range errorTests {
+		t.Run(test.name, func(t *testing.T) {
+			defer ClearAll()
+			_, err := createMetricsConfig(test.ops, TestLogger(t))
+			if err == nil || err.Error() != test.expectedErr {
+				t.Errorf("Wanted err: %v, got: %v", test.expectedErr, err)
+			}
+		})
+	}
 
-	envTests = []struct {
+	successTestsInit()
+	for _, test := range successTests {
+		t.Run(test.name, func(t *testing.T) {
+			defer ClearAll()
+			mc, err := createMetricsConfig(test.ops, TestLogger(t))
+			if err != nil {
+				t.Errorf("Wanted valid config %v, got error %v", test.expectedConfig, err)
+			}
+			if diff := cmp.Diff(test.expectedConfig, *mc, cmp.AllowUnexported(*mc), cmpopts.IgnoreTypes(mc.recorder)); diff != "" {
+				t.Errorf("Invalid config (-want +got):\n%s", diff)
+			}
+		})
+	}
+}
+
+func TestGetMetricsConfig_fromEnv(t *testing.T) {
+	successTests := []struct {
 		name           string
+		varName        string
+		varValue       string
 		ops            ExporterOptions
 		expectedConfig metricsConfig
 	}{{
-		name: "stackdriverFromEnv",
+		name:     "Stackdriver backend from env, no config",
+		varName:  defaultBackendEnvName,
+		varValue: string(Stackdriver),
 		ops: ExporterOptions{
 			ConfigMap: map[string]string{},
 			Domain:    servingDomain,
@@ -527,7 +566,9 @@ var (
 			stackdriverCustomMetricTypePrefix: path.Join(customMetricTypePrefix, defaultCustomMetricSubDomain, testComponent),
 		},
 	}, {
-		name: "validPrometheus",
+		name:     "Stackdriver backend from env, Prometheus backend from config",
+		varName:  defaultBackendEnvName,
+		varValue: string(Stackdriver),
 		ops: ExporterOptions{
 			ConfigMap: map[string]string{BackendDestinationKey: string(Prometheus)},
 			Domain:    servingDomain,
@@ -540,27 +581,47 @@ var (
 			reportingPeriod:    5 * time.Second,
 			prometheusPort:     defaultPrometheusPort,
 		},
+	}, {
+		name:     "PrometheusPort from env",
+		varName:  prometheusPortEnvName,
+		varValue: "9999",
+		ops: ExporterOptions{
+			ConfigMap: map[string]string{},
+			Domain:    servingDomain,
+			Component: testComponent,
+		},
+		expectedConfig: metricsConfig{
+			domain:             servingDomain,
+			component:          testComponent,
+			backendDestination: Prometheus,
+			reportingPeriod:    5 * time.Second,
+			prometheusPort:     9999,
+		},
 	}}
-)
-
-func successTestsInit() {
-	SetStackdriverSecretLocation(StackdriverSecretNameDefault, StackdriverSecretNamespaceDefault)
-}
 
-func TestGetMetricsConfig(t *testing.T) {
-	for _, test := range errorTests {
-		t.Run(test.name, func(t *testing.T) {
-			defer ClearAll()
-			_, err := createMetricsConfig(test.ops, TestLogger(t))
-			if err == nil || err.Error() != test.expectedErr {
-				t.Errorf("Wanted err: %v, got: %v", test.expectedErr, err)
-			}
-		})
-	}
+	failureTests := []struct {
+		name                string
+		varName             string
+		varValue            string
+		ops                 ExporterOptions
+		expectedErrContains string
+	}{{
+		name:     "Invalid PrometheusPort from env",
+		varName:  prometheusPortEnvName,
+		varValue: strconv.Itoa(math.MaxUint16 + 1),
+		ops: ExporterOptions{
+			ConfigMap: map[string]string{},
+			Domain:    servingDomain,
+			Component: testComponent,
+		},
+		expectedErrContains: "value out of range",
+	}}
 
-	successTestsInit()
 	for _, test := range successTests {
 		t.Run(test.name, func(t *testing.T) {
+			os.Setenv(test.varName, test.varValue)
+			defer os.Unsetenv(test.varName)
+
 			defer ClearAll()
 			mc, err := createMetricsConfig(test.ops, TestLogger(t))
 			if err != nil {
@@ -571,23 +632,22 @@ func TestGetMetricsConfig(t *testing.T) {
 			}
 		})
 	}
-}
 
-func TestGetMetricsConfig_fromEnv(t *testing.T) {
-	os.Setenv(defaultBackendEnvName, "stackdriver")
-	for _, test := range envTests {
+	for _, test := range failureTests {
 		t.Run(test.name, func(t *testing.T) {
+			os.Setenv(test.varName, test.varValue)
+			defer os.Unsetenv(test.varName)
+
 			defer ClearAll()
 			mc, err := createMetricsConfig(test.ops, TestLogger(t))
-			if err != nil {
-				t.Errorf("Wanted valid config %v, got error %v", test.expectedConfig, err)
+			if mc != nil {
+				t.Errorf("Wanted no config, got %v", mc)
 			}
-			if diff := cmp.Diff(test.expectedConfig, *mc, cmp.AllowUnexported(*mc), cmpopts.IgnoreTypes(mc.recorder)); diff != "" {
-				t.Errorf("Invalid config (-want +got):\n%s", diff)
+			if err == nil || !strings.Contains(err.Error(), test.expectedErrContains) {
+				t.Errorf("Wanted err to contain: %q, got: %v", test.expectedErrContains, err)
 			}
 		})
 	}
-	os.Unsetenv(defaultBackendEnvName)
 }
 
 func TestIsNewExporterRequiredFromNilConfig(t *testing.T) {