Repository: knative/pkg
Pull request #1499 (2020-07-15 15:29:28)

Remove unused connTimeout parameter
The connTimeout parameter in newHTTPTransport isn't actually respected, better to just remove it (the actual connection timeout [happens via DialWithBackoff](https://github.com/knative/pkg/blob/6826f2137c31cbf0f03e6d7988985b353d17d883/network/transports.go#L83))

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1499#" title="Author self-approved">julz</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **tcnghia**
You can assign the PR to them by writing `/assign @tcnghia` 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:

- **[network/OWNERS](https://github.com/knative/pkg/blob/master/network/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":["tcnghia"]} -->
knative-prow-robot: @julz: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-build-tests | 496419731b87837beaef6fbdc58e95c9877333bc | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1499/pull-knative-pkg-build-tests/1283423578359861248) | `/test pull-knative-pkg-build-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1499). [Your PR dashboard](https://gubernator.knative.dev/pr/julz).

<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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @julz: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-build-tests | 496419731b87837beaef6fbdc58e95c9877333bc | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1499/pull-knative-pkg-build-tests/1283423578359861248) | `/test pull-knative-pkg-build-tests`
pull-knative-pkg-integration-tests | 496419731b87837beaef6fbdc58e95c9877333bc | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1499/pull-knative-pkg-integration-tests/1283423578359861250) | `/test pull-knative-pkg-integration-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1499). [Your PR dashboard](https://gubernator.knative.dev/pr/julz).

<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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @julz: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-go-coverage | 496419731b87837beaef6fbdc58e95c9877333bc | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1499/pull-knative-pkg-go-coverage/1283423578355666944) | `/test pull-knative-pkg-go-coverage`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1499). [Your PR dashboard](https://gubernator.knative.dev/pr/julz).

<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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @julz: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-build-tests | 496419731b87837beaef6fbdc58e95c9877333bc | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1499/pull-knative-pkg-build-tests/1283423578359861248) | `/test pull-knative-pkg-build-tests`
pull-knative-pkg-integration-tests | 496419731b87837beaef6fbdc58e95c9877333bc | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1499/pull-knative-pkg-integration-tests/1283423578359861250) | `/test pull-knative-pkg-integration-tests`
pull-knative-pkg-go-coverage | 496419731b87837beaef6fbdc58e95c9877333bc | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1499/pull-knative-pkg-go-coverage/1283423578355666944) | `/test pull-knative-pkg-go-coverage`
pull-knative-pkg-unit-tests | 496419731b87837beaef6fbdc58e95c9877333bc | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1499/pull-knative-pkg-unit-tests/1283423578359861249) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1499). [Your PR dashboard](https://gubernator.knative.dev/pr/julz).

<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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1499#" title="Author self-approved">julz</a>*, *<a href="https://github.com/knative/pkg/pull/1499#pullrequestreview-449142761" title="Approved">vagababov</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:

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

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/stackdriver_exporter.go b/metrics/stackdriver_exporter.go
index 1e1a85ae2..96d72dd24 100644
--- a/metrics/stackdriver_exporter.go
+++ b/metrics/stackdriver_exporter.go
@@ -206,76 +206,71 @@ func sdCustomMetricsRecorder(mc metricsConfig, allowCustomMetrics bool) func(con
 		metricskey.LabelClusterName: gm.cluster,
 	}
 	return func(ctx context.Context, mss []stats.Measurement, ros ...stats.Options) error {
-		i := 0
-		var templ *resourceTemplate
-		templateFound := false
+		// Some metrics may be promoted to known Stackdriver schemas, so we may
+		// end up multiple Resources recorded for a single `RecordBatch` call.
+		metricsByResource := map[*resourceTemplate][]stats.Measurement{}
 
-		// This loop serves two purpose, filtering out custom metrics when allowCustomMetrics set to false,
-		// and find the template for the system metrics.
 		for _, m := range mss {
 			metricType := path.Join(mc.stackdriverMetricTypePrefix, m.Measure().Name())
 			t, ok := metricToResourceLabels[metricType]
 			if ok || allowCustomMetrics {
-				mss[i] = m
-				i++
-				if templateFound && templ != t {
-					return fmt.Errorf("mixed resource type measurements in one report: %v", mss)
+				if metricsByResource[t] == nil {
+					metricsByResource[t] = make([]stats.Measurement, 0, len(mss))
 				}
-				templ = t
-				templateFound = true
+				metricsByResource[t] = append(metricsByResource[t], m)
 			}
 		}
-		// Trim the array
-		mss = mss[:i]
-		if i == 0 {
-			return nil
-		}
 
+		baseLabels := map[string]string{}
 		baseResource := metricskey.GetResource(ctx)
-		// Extract resource, if possible
-		if templ != nil {
-			tagMap := tag.FromContext(ctx)
-			r := resource.Resource{
-				Type:   templ.Type,
-				Labels: map[string]string{},
-			}
-			tagMutations := make([]tag.Mutator, 0, len(templ.LabelKeys))
-			if baseResource == nil {
-				baseResource = &resource.Resource{}
-			}
-			for k := range templ.LabelKeys {
-				if v, ok := baseResource.Labels[k]; ok {
-					r.Labels[k] = v
-					continue
+		if baseResource != nil {
+			baseLabels = baseResource.Labels
+		}
+		tagMap := tag.FromContext(ctx)
+		for templ, ms := range metricsByResource {
+			sdResource := baseResource
+			sdCtx := ctx
+			if templ != nil {
+				sdResource = &resource.Resource{
+					Type:   templ.Type,
+					Labels: map[string]string{},
 				}
-				tagKey := tag.MustNewKey(k)
-				if v, ok := tagMap.Value(tagKey); ok {
-					r.Labels[k] = v
-					tagMutations = append(tagMutations, tag.Delete(tagKey))
-					continue
+				tagMutations := make([]tag.Mutator, 0, len(templ.LabelKeys))
+				for k := range templ.LabelKeys {
+					if v, ok := baseLabels[k]; ok {
+						sdResource.Labels[k] = v
+						continue
+					}
+					tagKey := tag.MustNewKey(k)
+					if v, ok := tagMap.Value(tagKey); ok {
+						sdResource.Labels[k] = v
+						tagMutations = append(tagMutations, tag.Delete(tagKey))
+						continue
+					}
+					if v, ok := metadataMap[k]; ok {
+						sdResource.Labels[k] = v
+						continue
+					}
+					sdResource.Labels[k] = metricskey.ValueUnknown
 				}
-				if v, ok := metadataMap[k]; ok {
-					r.Labels[k] = v
-					continue
+				var err error
+				sdCtx, err = tag.New(metricskey.WithResource(ctx, *sdResource), tagMutations...)
+				if err != nil {
+					return err
 				}
-				r.Labels[k] = metricskey.ValueUnknown
 			}
-			var err error
-			ctx, err = tag.New(metricskey.WithResource(ctx, r), tagMutations...)
-			if err != nil {
-				return err
+			if sdResource != nil {
+				opt, err := optionForResource(sdResource)
+				if err != nil {
+					return err
+				}
+				ros = append(ros, opt)
 			}
-			baseResource = &r
-		}
-		if baseResource != nil {
-			opt, err := optionForResource(baseResource)
-			if err != nil {
+			if err := stats.RecordWithOptions(sdCtx, append(ros, stats.WithMeasurements(ms...))...); err != nil {
 				return err
 			}
-			ros = append(ros, opt)
 		}
-
-		return stats.RecordWithOptions(ctx, append(ros, stats.WithMeasurements(mss...))...)
+		return nil
 	}
 }
 
diff --git a/network/transports.go b/network/transports.go
index dcb07a175..c50276259 100644
--- a/network/transports.go
+++ b/network/transports.go
@@ -92,7 +92,7 @@ func dialBackOffHelper(ctx context.Context, network, address string, bo wait.Bac
 	return nil, fmt.Errorf("timed out dialing after %.2fs", elapsed.Seconds())
 }
 
-func newHTTPTransport(connTimeout time.Duration, disableKeepAlives bool) http.RoundTripper {
+func newHTTPTransport(disableKeepAlives bool) http.RoundTripper {
 	return &http.Transport{
 		// Those match net/http/transport.go
 		Proxy:                 http.ProxyFromEnvironment,
@@ -112,7 +112,7 @@ func newHTTPTransport(connTimeout time.Duration, disableKeepAlives bool) http.Ro
 // since it will not cache connections.
 func NewProberTransport() http.RoundTripper {
 	return newAutoTransport(
-		newHTTPTransport(DefaultConnTimeout, true /*disable keep-alives*/),
+		newHTTPTransport(true /*disable keep-alives*/),
 		NewH2CTransport())
 }
 
@@ -120,7 +120,7 @@ func NewProberTransport() http.RoundTripper {
 // based on the request's HTTP version.
 func NewAutoTransport() http.RoundTripper {
 	return newAutoTransport(
-		newHTTPTransport(DefaultConnTimeout, false /*disable keep-alives*/),
+		newHTTPTransport(false /*disable keep-alives*/),
 		NewH2CTransport())
 }
 


Test patch:
diff --git a/metrics/resource_view_test.go b/metrics/resource_view_test.go
index e8c5262a1..76b9e23c7 100644
--- a/metrics/resource_view_test.go
+++ b/metrics/resource_view_test.go
@@ -25,6 +25,7 @@ import (
 	"net/http"
 	"os"
 	"sort"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -450,8 +451,11 @@ func TestStackDriverExports(t *testing.T) {
 		"revision_name":      "revision2",
 		"service_name":       "service2",
 	}
-	label3 := map[string]string{
-		"key": "value",
+	batchLabels := map[string]string{
+		"namespace_name":     "ns2",
+		"configuration_name": "config2",
+		"revision_name":      "revision2",
+		"service_name":       "service2",
 	}
 	harness := []struct {
 		name               string
@@ -472,8 +476,8 @@ func TestStackDriverExports(t *testing.T) {
 				2,
 			},
 			{
-				"custom.googleapis.com/knative.dev/autoscaler/testing/value",
-				label3,
+				"custom.googleapis.com/knative.dev/autoscaler/not_ready_pods",
+				batchLabels,
 				3,
 			},
 		},
@@ -516,14 +520,13 @@ func TestStackDriverExports(t *testing.T) {
 				Measure:     desiredPodCountM,
 				Aggregation: view.LastValue(),
 			}
-			customMeasurement := stats.Int64(
-				"testing/value",
-				"Stored value",
+			notReadyPodCountM := stats.Int64(
+				"not_ready_pods",
+				"Number of pods that are not ready",
 				stats.UnitDimensionless)
 			customView := &view.View{
-				Name:        "testing/value",
-				Description: "Test value",
-				Measure:     customMeasurement,
+				Description: "non-knative-revision metric per KnativeRevisionMetrics",
+				Measure:     notReadyPodCountM,
 				Aggregation: view.LastValue(),
 			}
 
@@ -551,23 +554,13 @@ func TestStackDriverExports(t *testing.T) {
 			Record(ctx, actualPodCountM.M(int64(1)))
 
 			r := resource.Resource{
-				Type: "knative_revision",
-				Labels: map[string]string{
-					metricskey.LabelNamespaceName:     "ns2",
-					metricskey.LabelServiceName:       "service2",
-					metricskey.LabelConfigurationName: "config2",
-					metricskey.LabelRevisionName:      "revision2",
-				},
+				Type:   "testing",
+				Labels: batchLabels,
 			}
-			Record(metricskey.WithResource(context.Background(), r), desiredPodCountM.M(int64(2)))
-
-			r = resource.Resource{
-				Type: "custom_resource",
-				Labels: map[string]string{
-					"key": "value",
-				},
-			}
-			Record(metricskey.WithResource(context.Background(), r), customMeasurement.M(int64(3)))
+			RecordBatch(
+				metricskey.WithResource(context.Background(), r),
+				desiredPodCountM.M(int64(2)),
+				notReadyPodCountM.M(int64(3)))
 
 			records := []metricExtract{}
 			for record := range sdFake.published {
@@ -577,6 +570,11 @@ func TestStackDriverExports(t *testing.T) {
 						Labels: ts.Resource.Labels,
 						Value:  ts.Points[0].Value.GetInt64Value(),
 					})
+					if strings.HasPrefix(ts.Metric.Type, "knative.dev/") {
+						if diff := cmp.Diff(ts.Resource.Type, metricskey.ResourceTypeKnativeRevision); diff != "" {
+							t.Errorf("Incorrect resource type for %q: (-want +got): %s", ts.Metric.Type, diff)
+						}
+					}
 				}
 				if len(records) >= 2 {
 					// There's no way to synchronize on the internal timer used
diff --git a/metrics/workqueue_test.go b/metrics/workqueue_test.go
index c08523965..e45359e14 100644
--- a/metrics/workqueue_test.go
+++ b/metrics/workqueue_test.go
@@ -35,6 +35,20 @@ func newFloat64(name string) *stats.Float64Measure {
 	return stats.Float64(name, "bar", "wtfs/s")
 }
 
+type FixedRateLimiter struct {
+	delay time.Duration
+}
+
+func (f *FixedRateLimiter) When(item interface{}) time.Duration {
+	return f.delay
+}
+
+func (f *FixedRateLimiter) Forget(item interface{}) {}
+
+func (f *FixedRateLimiter) NumRequeues(item interface{}) int {
+	return 0
+}
+
 func TestWorkqueueMetrics(t *testing.T) {
 	wp := &WorkqueueProvider{
 		Adds:                           newInt64("adds"),
@@ -60,10 +74,8 @@ func TestWorkqueueMetrics(t *testing.T) {
 	defer view.Unregister(views...)
 
 	queueName := t.Name()
-	wq := workqueue.NewNamedRateLimitingQueue(
-		workqueue.DefaultControllerRateLimiter(),
-		queueName,
-	)
+	limiter := &FixedRateLimiter{delay: 200 * time.Millisecond}
+	wq := workqueue.NewNamedRateLimitingQueue(limiter, queueName)
 
 	metricstest.CheckStatsNotReported(t, "adds", "depth", "latency", "retries", "work_duration",
 		"unfinished_work_seconds", "longest_running_processor_seconds")
@@ -114,7 +126,7 @@ func TestWorkqueueMetrics(t *testing.T) {
 	metricstest.CheckCountData(t, "adds", map[string]string{"name": queueName}, 2)
 
 	// It doesn't show up as an "add" until the rate limit has elapsed.
-	time.Sleep(1 * time.Second)
+	time.Sleep(2 * limiter.delay)
 	metricstest.CheckCountData(t, "adds", map[string]string{"name": queueName}, 3)
 
 	wq.ShutDown()