ConfigOptions return an error (#657)

* ConfigOptions return an error, so we can react to things failing, rather than only reading it in the logs.

* Add unit tests.

* PR comments.

* PR comment - remove TODO.

tracing/opencensus.go

@@ -2,12 +2,14 @@ package tracing
 
 import (
 	"errors"
+	"fmt"
 	"io"
+	"os"
 	"sync"
 
 	"contrib.go.opencensus.io/exporter/stackdriver"
 	oczipkin "contrib.go.opencensus.io/exporter/zipkin"
-	zipkin "github.com/openzipkin/zipkin-go"
+	"github.com/openzipkin/zipkin-go"
 	httpreporter "github.com/openzipkin/zipkin-go/reporter/http"
 	"go.opencensus.io/trace"
 	"go.uber.org/zap"
@@ -16,7 +18,7 @@ import (
 )
 
 // ConfigOption is the interface for adding additional exporters and configuring opencensus tracing.
-type ConfigOption func(*config.Config)
+type ConfigOption func(*config.Config) error
 
 // OpenCensusTracer is responsible for managing and updating configuration of OpenCensus tracing
 type OpenCensusTracer struct {
@@ -46,14 +48,16 @@ func (oct *OpenCensusTracer) ApplyConfig(cfg *config.Config) error {
 		return err
 	}
 
-	// Short circuit if our config hasnt changed
+	// Short circuit if our config hasn't changed.
 	if oct.curCfg != nil && oct.curCfg.Equals(cfg) {
 		return nil
 	}
 
 	// Apply config options
 	for _, configOpt := range oct.configOptions {
-		configOpt(cfg)
+		if err = configOpt(cfg); err != nil {
+			return err
+		}
 	}
 
 	// Set config
@@ -70,7 +74,9 @@ func (oct *OpenCensusTracer) Finish() error {
 	}
 
 	for _, configOpt := range oct.configOptions {
-		configOpt(nil)
+		if err = configOpt(nil); err != nil {
+			return err
+		}
 	}
 	globalOct = nil
 
@@ -108,7 +114,7 @@ func createOCTConfig(cfg *config.Config) *trace.Config {
 // WithExporter returns a ConfigOption for use with NewOpenCensusTracer that configures
 // it to export traces based on the configuration read from config-tracing.
 func WithExporter(name string, logger *zap.SugaredLogger) ConfigOption {
-	return func(cfg *config.Config) {
+	return func(cfg *config.Config) error {
 		var (
 			exporter trace.Exporter
 			closer   io.Closer
@@ -120,15 +126,25 @@ func WithExporter(name string, logger *zap.SugaredLogger) ConfigOption {
 			})
 			if err != nil {
 				logger.Errorw("error reading project-id from metadata", zap.Error(err))
-				return
+				return err
 			}
 			exporter = exp
 		case config.Zipkin:
+			// If name isn't specified, then zipkin.NewEndpoint will return an error saying that it
+			// can't find the host named ''. So, if not specified, default it to this machine's
+			// hostname.
+			if name == "" {
+				n, err := os.Hostname()
+				if err != nil {
+					return fmt.Errorf("unable to get hostname: %v", err)
+				}
+				name = n
+			}
 			hostPort := name + ":80"
 			zipEP, err := zipkin.NewEndpoint(name, hostPort)
 			if err != nil {
 				logger.Errorw("error building zipkin endpoint", zap.Error(err))
-				return
+				return err
 			}
 			reporter := httpreporter.NewReporter(cfg.ZipkinEndpoint)
 			exporter = oczipkin.NewExporter(reporter, zipEP)
@@ -149,5 +165,7 @@ func WithExporter(name string, logger *zap.SugaredLogger) ConfigOption {
 
 		globalOct.exporter = exporter
 		globalOct.closer = closer
+
+		return nil
 	}
 }

tracing/opencensus_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package tracing_test
 
 import (
+	"errors"
 	"testing"
 
 	. "knative.dev/pkg/tracing"
@@ -47,3 +48,43 @@ func TestOpenCensusTracerGlobalLifecycle(t *testing.T) {
 	}
 	otherOCT.Finish()
 }
+
+func TestOpenCensusTraceApplyConfigFailingConfigOption(t *testing.T) {
+	coErr := errors.New("configOption error")
+	oct := NewOpenCensusTracer(func(c *config.Config) error {
+		if c != nil {
+			return coErr
+		}
+		return nil
+	})
+	if err := oct.ApplyConfig(&config.Config{}); err != coErr {
+		t.Errorf("Expected error not seen. Got %q. Want %q", err, coErr)
+	}
+	if err := oct.Finish(); err != nil {
+		t.Errorf("Unexpected error Finishing: %q", err)
+	}
+}
+
+func TestOpenCensusTraceFinishFailingConfigOption(t *testing.T) {
+	coErr := errors.New("configOption error")
+	errToReturn := coErr
+	oct := NewOpenCensusTracer(func(c *config.Config) error {
+		if c == nil {
+			// We need finish to work on the second try, otherwise we have mutated global state. So,
+			// make sure that next run through, the returned error is nil.
+			e := errToReturn
+			errToReturn = nil
+			return e
+		}
+		return nil
+	})
+	if err := oct.ApplyConfig(&config.Config{}); err != nil {
+		t.Errorf("Unexpected error Applying Config: %q", err)
+	}
+	if err := oct.Finish(); err != coErr {
+		t.Errorf("Expected error not seen. Got %q. Want %q", err, coErr)
+	}
+	if err := oct.Finish(); err != nil {
+		t.Errorf("Unexpected error on second Finish (global state mutated, other tests may fail oddly): %q", err)
+	}
+}

tracing/zipkin.go

@@ -32,7 +32,7 @@ type ZipkinReporterFactory func(*config.Config) (zipkinreporter.Reporter, error)
 //  - WithExporter() in production code
 //  - testing/FakeZipkinExporter() in test code.
 func WithZipkinExporter(reporterFact ZipkinReporterFactory, endpoint *zipkinmodel.Endpoint) ConfigOption {
-	return func(cfg *config.Config) {
+	return func(cfg *config.Config) error {
 		var (
 			reporter zipkinreporter.Reporter
 			exporter trace.Exporter
@@ -43,8 +43,7 @@ func WithZipkinExporter(reporterFact ZipkinReporterFactory, endpoint *zipkinmode
 			// do this before cleanup to minimize time where we have duplicate exporters
 			reporter, err := reporterFact(cfg)
 			if err != nil {
-				// TODO(greghaynes) log this error
-				return
+				return err
 			}
 			exporter := zipkin.NewExporter(reporter, endpoint)
 			trace.RegisterExporter(exporter)
@@ -63,5 +62,7 @@ func WithZipkinExporter(reporterFact ZipkinReporterFactory, endpoint *zipkinmode
 
 		oct.closer = reporter
 		oct.exporter = exporter
+
+		return nil
 	}
 }