Repository: knative/pkg
Pull request #2054 (2021-03-10 16:46:36)

Implement getter machinery to generically fetch all injected clients
<!-- Thanks for sending a pull request! -->

# Changes

This is a stepping stone towards moer unit test stability around informer usage. See https://github.com/knative/serving/pull/10930 for an end to end example.

In a nutshell, we'd like to be able to fetch all injected clients generically to be able to muck with them and make sure everything is synced when we think it is.

/assign @n3wscott 

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

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **markusthoemmes** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @markusthoemmes` 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:

- **[OWNERS](https://github.com/knative/pkg/blob/main/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":["markusthoemmes"]} -->

Patch:
diff --git a/client/injection/apiextensions/client/client.go b/client/injection/apiextensions/client/client.go
index 3edca7e9e..e8695e03f 100644
--- a/client/injection/apiextensions/client/client.go
+++ b/client/injection/apiextensions/client/client.go
@@ -29,6 +29,9 @@ import (
 
 func init() {
 	injection.Default.RegisterClient(withClient)
+	injection.Default.RegisterClientFetcher(func(ctx context.Context) interface{} {
+		return Get(ctx)
+	})
 }
 
 // Key is used as the key for associating information with a context.Context.
diff --git a/client/injection/apiextensions/client/fake/fake.go b/client/injection/apiextensions/client/fake/fake.go
index 1ed129871..6b47e9de3 100644
--- a/client/injection/apiextensions/client/fake/fake.go
+++ b/client/injection/apiextensions/client/fake/fake.go
@@ -31,6 +31,9 @@ import (
 
 func init() {
 	injection.Fake.RegisterClient(withClient)
+	injection.Fake.RegisterClientFetcher(func(ctx context.Context) interface{} {
+		return Get(ctx)
+	})
 }
 
 func withClient(ctx context.Context, cfg *rest.Config) context.Context {
diff --git a/client/injection/kube/client/client.go b/client/injection/kube/client/client.go
index 00591ff57..b7994cb72 100644
--- a/client/injection/kube/client/client.go
+++ b/client/injection/kube/client/client.go
@@ -29,6 +29,9 @@ import (
 
 func init() {
 	injection.Default.RegisterClient(withClient)
+	injection.Default.RegisterClientFetcher(func(ctx context.Context) interface{} {
+		return Get(ctx)
+	})
 }
 
 // Key is used as the key for associating information with a context.Context.
diff --git a/client/injection/kube/client/fake/fake.go b/client/injection/kube/client/fake/fake.go
index 022ae42f0..e16622294 100644
--- a/client/injection/kube/client/fake/fake.go
+++ b/client/injection/kube/client/fake/fake.go
@@ -31,6 +31,9 @@ import (
 
 func init() {
 	injection.Fake.RegisterClient(withClient)
+	injection.Fake.RegisterClientFetcher(func(ctx context.Context) interface{} {
+		return Get(ctx)
+	})
 }
 
 func withClient(ctx context.Context, cfg *rest.Config) context.Context {
diff --git a/codegen/cmd/injection-gen/generators/client.go b/codegen/cmd/injection-gen/generators/client.go
index 317e12a44..d7cb717ed 100644
--- a/codegen/cmd/injection-gen/generators/client.go
+++ b/codegen/cmd/injection-gen/generators/client.go
@@ -66,7 +66,11 @@ func (g *clientGenerator) GenerateType(c *generator.Context, t *types.Type, w io
 		"clientSetNewForConfigOrDie": c.Universe.Function(types.Name{Package: g.clientSetPackage, Name: "NewForConfigOrDie"}),
 		"clientSetInterface":         c.Universe.Type(types.Name{Package: g.clientSetPackage, Name: "Interface"}),
 		"injectionRegisterClient":    c.Universe.Function(types.Name{Package: "knative.dev/pkg/injection", Name: "Default.RegisterClient"}),
-		"restConfig":                 c.Universe.Type(types.Name{Package: "k8s.io/client-go/rest", Name: "Config"}),
+		"injectionRegisterClientFetcher": c.Universe.Function(types.Name{
+			Package: "knative.dev/pkg/injection",
+			Name:    "Default.RegisterClientFetcher",
+		}),
+		"restConfig": c.Universe.Type(types.Name{Package: "k8s.io/client-go/rest", Name: "Config"}),
 		"loggingFromContext": c.Universe.Function(types.Name{
 			Package: "knative.dev/pkg/logging",
 			Name:    "FromContext",
@@ -85,6 +89,9 @@ func (g *clientGenerator) GenerateType(c *generator.Context, t *types.Type, w io
 var injectionClient = `
 func init() {
 	{{.injectionRegisterClient|raw}}(withClient)
+	{{.injectionRegisterClientFetcher|raw}}(func(ctx context.Context) interface{} {
+		return Get(ctx)
+	})
 }
 
 // Key is used as the key for associating information with a context.Context.
diff --git a/codegen/cmd/injection-gen/generators/fake_client.go b/codegen/cmd/injection-gen/generators/fake_client.go
index 3e98ce381..76725f2bd 100644
--- a/codegen/cmd/injection-gen/generators/fake_client.go
+++ b/codegen/cmd/injection-gen/generators/fake_client.go
@@ -71,6 +71,10 @@ func (g *fakeClientGenerator) GenerateType(c *generator.Context, t *types.Type,
 			Package: "knative.dev/pkg/injection",
 			Name:    "Fake.RegisterClient",
 		}),
+		"injectionRegisterClientFetcher": c.Universe.Function(types.Name{
+			Package: "knative.dev/pkg/injection",
+			Name:    "Fake.RegisterClientFetcher",
+		}),
 		"loggingFromContext": c.Universe.Function(types.Name{
 			Package: "knative.dev/pkg/logging",
 			Name:    "FromContext",
@@ -91,6 +95,9 @@ func (g *fakeClientGenerator) GenerateType(c *generator.Context, t *types.Type,
 var injectionFakeClient = `
 func init() {
 	{{.injectionRegisterClient|raw}}(withClient)
+	{{.injectionRegisterClientFetcher|raw}}(func(ctx context.Context) interface{} {
+		return Get(ctx)
+	})
 }
 
 func withClient(ctx {{.contextContext|raw}}, cfg *{{.restConfig|raw}}) {{.contextContext|raw}} {
diff --git a/injection/clients.go b/injection/clients.go
index 5c464924c..b71ef1d94 100644
--- a/injection/clients.go
+++ b/injection/clients.go
@@ -26,6 +26,10 @@ import (
 // client type to a context.
 type ClientInjector func(context.Context, *rest.Config) context.Context
 
+// ClientFetcher holds the type of a callback that returns a particular client type
+// from a context.
+type ClientFetcher func(context.Context) interface{}
+
 func (i *impl) RegisterClient(ci ClientInjector) {
 	i.m.Lock()
 	defer i.m.Unlock()
@@ -40,3 +44,21 @@ func (i *impl) GetClients() []ClientInjector {
 	// Copy the slice before returning.
 	return append(i.clients[:0:0], i.clients...)
 }
+
+func (i *impl) RegisterClientFetcher(f ClientFetcher) {
+	i.m.Lock()
+	defer i.m.Unlock()
+
+	i.clientFetchers = append(i.clientFetchers, f)
+}
+
+func (i *impl) FetchAllClients(ctx context.Context) []interface{} {
+	i.m.RLock()
+	defer i.m.RUnlock()
+
+	clients := make([]interface{}, 0, len(i.clientFetchers))
+	for _, f := range i.clientFetchers {
+		clients = append(clients, f(ctx))
+	}
+	return clients
+}
diff --git a/injection/interface.go b/injection/interface.go
index 7d2764cdc..112a6415f 100644
--- a/injection/interface.go
+++ b/injection/interface.go
@@ -36,6 +36,13 @@ type Interface interface {
 	// GetClients fetches all of the registered client injectors.
 	GetClients() []ClientInjector
 
+	// RegisterClientFetcher registers a new callback that fetches a client from
+	// a given context.
+	RegisterClientFetcher(ClientFetcher)
+
+	// FetchAllClients returns all known clients from the given context.
+	FetchAllClients(context.Context) []interface{}
+
 	// RegisterInformerFactory registers a new injector callback for associating
 	// a new informer factory with a context.
 	RegisterInformerFactory(InformerFactoryInjector)
@@ -92,6 +99,7 @@ type impl struct {
 	m sync.RWMutex
 
 	clients           []ClientInjector
+	clientFetchers    []ClientFetcher
 	factories         []InformerFactoryInjector
 	informers         []InformerInjector
 	filteredInformers []FilteredInformersInjector


Test patch:
diff --git a/injection/clients_test.go b/injection/clients_test.go
index 4e47006a1..d84dbb353 100644
--- a/injection/clients_test.go
+++ b/injection/clients_test.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"k8s.io/client-go/rest"
 )
 
@@ -50,3 +51,35 @@ func TestRegisterClient(t *testing.T) {
 		t.Errorf("GetClients() = %d, wanted %d", want, got)
 	}
 }
+
+type fakeClient struct {
+	Name string
+}
+
+func TestRegisterClientFetcher(t *testing.T) {
+	i := &impl{}
+
+	fakeA := fakeClient{Name: "a"}
+	fetchA := func(ctx context.Context) interface{} {
+		return fakeA
+	}
+
+	fakeB := fakeClient{Name: "b"}
+	fetchB := func(ctx context.Context) interface{} {
+		return fakeB
+	}
+
+	ctx := context.Background()
+	if want, got := 0, len(i.FetchAllClients(ctx)); got != want {
+		t.Errorf("FetchAllClients() = %d, wanted %d", got, want)
+	}
+
+	i.RegisterClientFetcher(fetchA)
+	i.RegisterClientFetcher(fetchB)
+
+	got := i.FetchAllClients(ctx)
+	want := []interface{}{fakeA, fakeB}
+	if !cmp.Equal(got, want) {
+		t.Errorf("FetchAllClients() = %v, wanted %v", got, want)
+	}
+}
diff --git a/metrics/record_test.go b/metrics/record_test.go
index e597926ef..35fb28a07 100644
--- a/metrics/record_test.go
+++ b/metrics/record_test.go
@@ -29,6 +29,7 @@ import (
 	"go.opencensus.io/resource"
 	"go.opencensus.io/stats"
 	"go.opencensus.io/stats/view"
+	"go.opencensus.io/tag"
 )
 
 type cases struct {
@@ -226,3 +227,83 @@ func TestMeter(t *testing.T) {
 		})
 	}
 }
+
+func BenchmarkMetricsRecording(b *testing.B) {
+	requestKey := tag.MustNewKey("request")
+	requestStatus := tag.MustNewKey("requestStatus")
+	requestURL := tag.MustNewKey("requestUrl")
+	tagKeys := []tag.Key{requestKey, requestStatus, requestURL}
+	measure1 := stats.Int64("count1", "First counter", stats.UnitDimensionless)
+	measure2 := stats.Int64("count2", "Second counter", stats.UnitDimensionless)
+	v := []*view.View{{
+		Measure:     measure1,
+		Aggregation: view.LastValue(),
+		TagKeys:     tagKeys,
+	}, {
+		Measure:     measure2,
+		Aggregation: view.LastValue(),
+		TagKeys:     tagKeys,
+	}}
+	err := RegisterResourceView(v...)
+	if err != nil {
+		b.Error("Failed to register resource view")
+	}
+	defer UnregisterResourceView(v...)
+	metricsConfig := &metricsConfig{}
+	measurement1 := measure1.M(1000)
+	measurement2 := measure2.M(1)
+	setCurMetricsConfig(metricsConfig)
+	getTagCtx := func() (context.Context, error) {
+		ctx := context.Background()
+		ctx, err := tag.New(
+			ctx,
+			tag.Insert(requestKey, "login"),
+			tag.Insert(requestStatus, "status_ok"),
+			tag.Insert(requestURL, "localhost"),
+		)
+		return ctx, err
+	}
+	if err != nil {
+		b.Error("Failed to create tags")
+	}
+	b.Run("sequential", func(b *testing.B) {
+		for j := 0; j < b.N; j++ {
+			ctx, err := getTagCtx()
+			if err != nil {
+				b.Error("Failed to get context")
+			}
+			Record(ctx, measurement1)
+		}
+	})
+	b.Run("parallel", func(b *testing.B) {
+		b.RunParallel(func(pb *testing.PB) {
+			for pb.Next() {
+				ctx, err := getTagCtx()
+				if err != nil {
+					b.Error("Failed to get context")
+				}
+				Record(ctx, measurement1)
+			}
+		})
+	})
+	b.Run("sequential-batch", func(b *testing.B) {
+		for j := 0; j < b.N; j++ {
+			ctx, err := getTagCtx()
+			if err != nil {
+				b.Error("Failed to get context")
+			}
+			RecordBatch(ctx, measurement1, measurement2)
+		}
+	})
+	b.Run("parallel-batch", func(b *testing.B) {
+		b.RunParallel(func(pb *testing.PB) {
+			for pb.Next() {
+				ctx, err := getTagCtx()
+				if err != nil {
+					b.Error("Failed to get context")
+				}
+				RecordBatch(ctx, measurement1, measurement2)
+			}
+		})
+	})
+}