First example using the pretty context builder. (#1403)

n3wscott · Ville Aikas · commit 33f2b04fd6da · 2017-10-18T11:31:33.000-07:00
* First example using the log context builder.

* No format.

* Remote controller logging class, not needed yet.

* Fixing cr date.

* Adding function documentation.

* Kind is always type Kind.

* Add comment for type.

* I feel so pretty. Oh so pretty. Pretty log lines.

* Renaming files, fixing receiver names.

* Finish controller.go.
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -41,6 +41,7 @@ import (
 	servicecatalogclientset "github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/clientset/typed/servicecatalog/v1beta1"
 	informers "github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/externalversions/servicecatalog/v1beta1"
 	listers "github.com/kubernetes-incubator/service-catalog/pkg/client/listers_generated/servicecatalog/v1beta1"
+	pretty "github.com/kubernetes-incubator/service-catalog/pkg/pretty"
 )
 
 const (
@@ -247,16 +248,14 @@ func worker(queue workqueue.RateLimitingInterface, resourceType string, maxRetri
 // a brokerClient to use for that method given an ServiceInstance.
 // Sets ClusterServiceClassRef and/or ClusterServicePlanRef if they haven't been already set.
 func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance *v1beta1.ServiceInstance) (*v1beta1.ClusterServiceClass, *v1beta1.ClusterServicePlan, string, osb.Client, error) {
+	pcb := pretty.NewContextBuilder(pretty.ServiceInstance, instance.Namespace, instance.Name)
 	serviceClass, err := c.serviceClassLister.Get(instance.Spec.ClusterServiceClassRef.Name)
 	if err != nil {
 		s := fmt.Sprintf(
 			"References a non-existent ClusterServiceClass (K8S: %q ExternalName: %q)",
 			instance.Spec.ClusterServiceClassRef.Name, instance.Spec.ExternalClusterServiceClassName,
 		)
-		glog.Infof(
-			`ServiceInstance "%v/%v": %s`,
-			instance.Namespace, instance.Name, s,
-		)
+		glog.Info(pcb.Message(s))
 		c.updateServiceInstanceCondition(
 			instance,
 			v1beta1.ServiceInstanceConditionReady,
@@ -274,10 +273,7 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance
 			"References a non-existent ClusterServicePlan (K8S: %q ExternalName: %q) on ClusterServiceClass (K8S: %q ExternalName: %q)",
 			instance.Spec.ClusterServicePlanName, instance.Spec.ExternalClusterServicePlanName, serviceClass.Name, serviceClass.Spec.ExternalName,
 		)
-		glog.Warningf(
-			`ServiceInstance "%s/%s": %s`,
-			instance.Namespace, instance.Name, s,
-		)
+		glog.Warning(pcb.Message(s))
 		c.updateServiceInstanceCondition(
 			instance,
 			v1beta1.ServiceInstanceConditionReady,
@@ -292,10 +288,7 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance
 	broker, err := c.brokerLister.Get(serviceClass.Spec.ClusterServiceBrokerName)
 	if err != nil {
 		s := fmt.Sprintf("References a non-existent broker %q", serviceClass.Spec.ClusterServiceBrokerName)
-		glog.Warningf(
-			`ServiceInstance "%s/%s": %s`,
-			instance.Namespace, instance.Name, s,
-		)
+		glog.Warning(pcb.Message(s))
 		c.updateServiceInstanceCondition(
 			instance,
 			v1beta1.ServiceInstanceConditionReady,
@@ -310,10 +303,7 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance
 	authConfig, err := getAuthCredentialsFromClusterServiceBroker(c.kubeClient, broker)
 	if err != nil {
 		s := fmt.Sprintf("Error getting broker auth credentials for broker %q: %s", broker.Name, err)
-		glog.Infof(
-			`ServiceInstance "%v/%v": %s`,
-			instance.Namespace, instance.Name, s,
-		)
+		glog.Info(pcb.Message(s))
 		c.updateServiceInstanceCondition(
 			instance,
 			v1beta1.ServiceInstanceConditionReady,
@@ -327,7 +317,8 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance
 
 	clientConfig := NewClientConfigurationForBroker(broker, authConfig)
 
-	glog.V(4).Infof("Creating client for ClusterServiceBroker %v, URL: %v", broker.Name, broker.Spec.URL)
+	s := fmt.Sprintf("Creating client for ClusterServiceBroker %v, URL: %v", broker.Name, broker.Spec.URL)
+	glog.V(4).Info(pcb.Message(s))
 	brokerClient, err := c.brokerClientCreateFunc(clientConfig)
 	if err != nil {
 		return nil, nil, "", nil, err
@@ -341,16 +332,14 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance
 // a brokerclient to use for a given ServiceInstance.
 // Sets ClusterServiceClassRef and/or ClusterServicePlanRef if they haven't been already set.
 func (c *controller) getClusterServiceClassPlanAndClusterServiceBrokerForServiceBinding(instance *v1beta1.ServiceInstance, binding *v1beta1.ServiceBinding) (*v1beta1.ClusterServiceClass, *v1beta1.ClusterServicePlan, string, osb.Client, error) {
+	pcb := pretty.NewContextBuilder(pretty.ServiceInstance, instance.Namespace, instance.Name)
 	serviceClass, err := c.serviceClassLister.Get(instance.Spec.ClusterServiceClassRef.Name)
 	if err != nil {
 		s := fmt.Sprintf(
 			"References a non-existent ClusterServiceClass (K8S: %q ExternalName: %q)",
 			instance.Spec.ClusterServiceClassRef.Name, instance.Spec.ExternalClusterServiceClassName,
 		)
-		glog.Warningf(
-			`ServiceBinding "%s/%s": %s`,
-			instance.Namespace, instance.Name, s,
-		)
+		glog.Warning(pcb.Message(s))
 		c.updateServiceBindingCondition(
 			binding,
 			v1beta1.ServiceBindingConditionReady,
@@ -368,10 +357,7 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBrokerForService
 			"References a non-existent ClusterServicePlan (K8S: %q ExternalName: %q) on ClusterServiceClass (K8S: %q ExternalName: %q)",
 			instance.Spec.ClusterServicePlanName, instance.Spec.ExternalClusterServicePlanName, serviceClass.Name, serviceClass.Spec.ExternalName,
 		)
-		glog.Warningf(
-			`ServiceBinding "%s/%s": %s`,
-			instance.Namespace, instance.Name, s,
-		)
+		glog.Warning(pcb.Message(s))
 		c.updateServiceBindingCondition(
 			binding,
 			v1beta1.ServiceBindingConditionReady,
@@ -386,10 +372,7 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBrokerForService
 	broker, err := c.brokerLister.Get(serviceClass.Spec.ClusterServiceBrokerName)
 	if err != nil {
 		s := fmt.Sprintf("References a non-existent ClusterServiceBroker %q", serviceClass.Spec.ClusterServiceBrokerName)
-		glog.Warningf(
-			`ServiceBinding "%s/%s": %s`,
-			instance.Namespace, instance.Name, s,
-		)
+		glog.Warning(pcb.Message(s))
 		c.updateServiceBindingCondition(
 			binding,
 			v1beta1.ServiceBindingConditionReady,
@@ -404,10 +387,7 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBrokerForService
 	authConfig, err := getAuthCredentialsFromClusterServiceBroker(c.kubeClient, broker)
 	if err != nil {
 		s := fmt.Sprintf("Error getting broker auth credentials for broker %q: %s", broker.Name, err)
-		glog.Warningf(
-			`ServiceBinding "%s/%s": %s`,
-			instance.Namespace, instance.Name, s,
-		)
+		glog.Warning(pcb.Message(s))
 		c.updateServiceBindingCondition(
 			binding,
 			v1beta1.ServiceBindingConditionReady,
diff --git a/pkg/pretty/context_builder.go b/pkg/pretty/context_builder.go
@@ -0,0 +1,104 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package pretty
+
+import (
+	"fmt"
+)
+
+// Kind is used for the enum of the Type of object we are building context for.
+type Kind int
+
+// Names of Types to use when creating pretty messages.
+const (
+	ServiceInstance Kind = 1
+)
+
+func (k Kind) String() string {
+	switch k {
+	case ServiceInstance:
+		return "ServiceInstance"
+	default:
+		return ""
+	}
+}
+
+// ContextBuilder allows building up pretty message lines with context
+// that is important for debugging and tracing. This class helps create log
+// line formatting consistency. Pretty lines should be in the form:
+// <Kind> "<Namespace>/<Name>": <message>
+type ContextBuilder struct {
+	Kind      Kind
+	Namespace string
+	Name      string
+}
+
+// NewContextBuilder returns a new ContextBuilder that can be used to format messages in the
+// form `<Kind> "<Namespace>/<Name>": <message>`.
+// kind,  namespace, name are all optional.
+func NewContextBuilder(kind Kind, namespace string, name string) *ContextBuilder {
+	lb := new(ContextBuilder)
+	lb.Kind = kind
+	lb.Namespace = namespace
+	lb.Name = name
+	return lb
+}
+
+// SetKind sets the kind to use in the source context for messages.
+func (pcb *ContextBuilder) SetKind(k Kind) *ContextBuilder {
+	pcb.Kind = k
+	return pcb
+}
+
+// SetNamespace sets the namespace to use in the source context for messages.
+func (pcb *ContextBuilder) SetNamespace(n string) *ContextBuilder {
+	pcb.Namespace = n
+	return pcb
+}
+
+// SetName sets the name to use in the source context for messages.
+func (pcb *ContextBuilder) SetName(n string) *ContextBuilder {
+	pcb.Name = n
+	return pcb
+}
+
+// Message returns a string with message prepended with the current source context.
+func (pcb *ContextBuilder) Message(msg string) string {
+	if pcb.Kind > 0 || pcb.Namespace != "" || pcb.Name != "" {
+		return fmt.Sprintf(`%s: %s`, pcb, msg)
+	}
+	return msg
+}
+
+// TODO(n3wscott): Support <type> (K8S: <K8S-Type-Name> ExternalName: <External-Type-Name>)
+
+func (pcb ContextBuilder) String() string {
+	s := ""
+	space := ""
+	if pcb.Kind > 0 {
+		s += fmt.Sprintf("%s", pcb.Kind)
+		space = " "
+	}
+	if pcb.Namespace != "" && pcb.Name != "" {
+		s += fmt.Sprintf(`%s"%s/%s"`, space, pcb.Namespace, pcb.Name)
+	} else if pcb.Namespace != "" {
+		s += fmt.Sprintf(`%s"%s"`, space, pcb.Namespace)
+	} else if pcb.Name != "" {
+		s += fmt.Sprintf(`%s"%s"`, space, pcb.Name)
+	}
+	return s
+}
diff --git a/pkg/pretty/context_builder_test.go b/pkg/pretty/context_builder_test.go
@@ -0,0 +1,115 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package pretty
+
+import (
+	"testing"
+)
+
+func TestPrettyContextBuilderKind(t *testing.T) {
+	pcb := ContextBuilder{}
+
+	pcb.SetKind(ServiceInstance)
+
+	e := "ServiceInstance"
+	g := pcb.String()
+	if g != e {
+		t.Fatalf("Unexpected value of ContextBuilder String; expected %v, got %v", e, g)
+	}
+}
+
+func TestPrettyContextBuilderNamespace(t *testing.T) {
+	pcb := ContextBuilder{}
+
+	pcb.SetNamespace("Namespace")
+
+	e := `"Namespace"`
+	g := pcb.String()
+	if g != e {
+		t.Fatalf("Unexpected value of ContextBuilder String; expected %v, got %v", e, g)
+	}
+}
+
+func TestPrettyContextBuilderName(t *testing.T) {
+	pcb := ContextBuilder{}
+
+	pcb.SetName("Name")
+
+	e := `"Name"`
+	g := pcb.String()
+	if g != e {
+		t.Fatalf("Unexpected value of ContextBuilder String; expected %v, got %v", e, g)
+	}
+}
+
+func TestPrettyContextBuilderKindAndNamespace(t *testing.T) {
+	pcb := ContextBuilder{}
+
+	pcb.SetKind(ServiceInstance).SetNamespace("Namespace")
+
+	e := `ServiceInstance "Namespace"`
+	g := pcb.String()
+	if g != e {
+		t.Fatalf("Unexpected value of ContextBuilder String; expected %v, got %v", e, g)
+	}
+}
+
+func TestPrettyContextBuilderKindAndName(t *testing.T) {
+	pcb := ContextBuilder{}
+
+	pcb.SetKind(ServiceInstance).SetName("Name")
+
+	e := `ServiceInstance "Name"`
+	g := pcb.String()
+	if g != e {
+		t.Fatalf("Unexpected value of ContextBuilder String; expected %v, got %v", e, g)
+	}
+}
+
+func TestPrettyContextBuilderKindNamespaceName(t *testing.T) {
+	pcb := ContextBuilder{}
+
+	pcb.SetKind(ServiceInstance).SetNamespace("Namespace").SetName("Name")
+
+	e := `ServiceInstance "Namespace/Name"`
+	g := pcb.String()
+	if g != e {
+		t.Fatalf("Unexpected value of ContextBuilder String; expected %v, got %v", e, g)
+	}
+}
+
+func TestPrettyContextBuilderMsg(t *testing.T) {
+	pcb := ContextBuilder{}
+
+	e := `Msg`
+	g := pcb.Message("Msg")
+	if g != e {
+		t.Fatalf("Unexpected value of ContextBuilder String; expected %v, got %v", e, g)
+	}
+}
+
+func TestPrettyContextBuilderContextAndMsg(t *testing.T) {
+	pcb := ContextBuilder{}
+
+	pcb.SetKind(ServiceInstance).SetNamespace("Namespace").SetName("Name")
+
+	e := `ServiceInstance "Namespace/Name": Msg`
+	g := pcb.Message("Msg")
+	if g != e {
+		t.Fatalf("Unexpected value of ContextBuilder String; expected %v, got %v", e, g)
+	}
+}
