Smoke test for faulty GRPC config, fixed broken error handling in the case of a bad GRPC client

go/cmd/dolt/commands/send_metrics.go

@@ -94,7 +94,6 @@ func (cmd SendMetricsCmd) Exec(ctx context.Context, commandStr string, args []st
 
 	disabled, err := strconv.ParseBool(metricsDisabled)
 	if err != nil {
-		// log.Print(err)
 		return 1
 	}
 
@@ -103,30 +102,28 @@ func (cmd SendMetricsCmd) Exec(ctx context.Context, commandStr string, args []st
 		return 0
 	}
 
-	if !disabled {
-		ctx, cancel := context.WithTimeout(ctx, time.Minute)
-		defer cancel()
+	ctx, cancel := context.WithTimeout(ctx, time.Minute)
+	defer cancel()
 
-		userHomeDir, err := dEnv.GetUserHomeDir()
-		if err != nil {
-			return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage)
-		}
-		
-		output := apr.GetValueOrDefault(EventsOutputFormat, events.EmitterTypeGrpc)
-		err = FlushLoggedEvents(ctx, dEnv, userHomeDir, output)
-
-		if err != nil {
-			if err == events.ErrFileLocked {
-				return 2
-			}
-
-			return 1
-		}
-
-		return 0
+	userHomeDir, err := dEnv.GetUserHomeDir()
+	if err != nil {
+		return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage)
 	}
 
-	return 1
+	output := apr.GetValueOrDefault(EventsOutputFormat, events.EmitterTypeGrpc)
+	err = FlushLoggedEvents(ctx, dEnv, userHomeDir, output)
+
+	if err != nil {
+		cli.PrintErrf("Error flushing events: %s\n", err.Error())
+
+		if err == events.ErrFileLocked {
+			return 2
+		}
+		
+		return 1
+	}
+
+	return 0
 }
 
 // FlushLoggedEvents flushes any logged events in the directory given to an appropriate event emitter
@@ -149,7 +146,7 @@ func NewEmitter(emitterType string, pro EmitterConfigProvider) (events.Emitter,
 	case events.EmitterTypeStdout:
 		return events.WriterEmitter{Wr: os.Stdout}, nil
 	case events.EmitterTypeGrpc:
-		return GRPCEmitterForConfig(pro), nil
+		return GRPCEmitterForConfig(pro)
 	case events.EmitterTypeFile:
 		homeDir, err := pro.GetUserHomeDir()
 		if err != nil {
@@ -163,17 +160,18 @@ func NewEmitter(emitterType string, pro EmitterConfigProvider) (events.Emitter,
 
 // GRPCEmitterForConfig returns an event emitter for the given environment, or nil if the environment cannot
 // provide one
-func GRPCEmitterForConfig(pro EmitterConfigProvider) *events.GrpcEmitter {
+func GRPCEmitterForConfig(pro EmitterConfigProvider) (*events.GrpcEmitter, error) {
 	cfg, err := GRPCEventRemoteConfig(pro)
 	if err != nil {
-		return nil
+		return nil, err
 	}
 
 	conn, err := grpc.Dial(cfg.Endpoint, cfg.DialOptions...)
 	if err != nil {
-		return nil
+		return nil, err
 	}
-	return events.NewGrpcEmitter(conn)
+	
+	return events.NewGrpcEmitter(conn), nil
 }
 
 // GRPCEventRemoteConfig returns a GRPCRemoteConfig for the given configuration provider

go/libraries/events/event_flush.go

@@ -128,22 +128,25 @@ func lockAndFlush(ctx context.Context, fs filesys.Filesys, dirPath string, lockP
 		return err
 	}
 
-	if isUnlocked && err == nil {
-		err := fs.Iter(dirPath, false, func(path string, size int64, isDir bool) (stop bool) {
-			if err := fcb(ctx, path); err != nil {
-				// log.Print(err)
-				return false
-			}
-
-			return false
-		})
-
-		if err != nil {
-			return err
-		}
-
+	if !isUnlocked {
 		return nil
 	}
 
+	var returnErr error
+	iterErr := fs.Iter(dirPath, false, func(path string, size int64, isDir bool) (stop bool) {
+		if err := fcb(ctx, path); err != nil {
+			returnErr = err
+			return true
+		}
+
+		return false
+	})
+	
+	if iterErr != nil {
+		return iterErr
+	} else if returnErr != nil {
+		return returnErr
+	}
+	
 	return nil
 }

integration-tests/bats/send-metrics.bats

@@ -91,3 +91,32 @@ teardown() {
     [ "$status" -eq 0 ]
     [ "$output" == "" ]
 }
+
+@test "send-metrics: grpc smoke test" {
+    DOLT_DISABLE_EVENT_FLUSH=true dolt sql -q "create table t1 (a int primary key, b int)"
+    DOLT_DISABLE_EVENT_FLUSH=true dolt sql -q "insert into t1 values (1, 2)"
+    DOLT_DISABLE_EVENT_FLUSH=true dolt ls
+    DOLT_DISABLE_EVENT_FLUSH=true dolt status
+
+    # output all the metrics data to stdout for examination
+    dolt config --global --add metrics.host "fake.server"
+    run dolt send-metrics
+    [ "$status" -eq 1 ]
+    [[ "$output" =~ "Error flushing events" ]] || false
+    [[ "$output" =~ "fake.server" ]] || false
+}
+
+# TODO: we need a local metrics server here that we can spin up to verify the send actually works
+@test "send-metrics: sql-server heartbeat" {
+    DOLT_DISABLE_EVENT_FLUSH=true dolt sql -q "create table t1 (a int primary key, b int)"
+    DOLT_DISABLE_EVENT_FLUSH=true dolt sql -q "insert into t1 values (1, 2)"
+    DOLT_DISABLE_EVENT_FLUSH=true dolt ls
+    DOLT_DISABLE_EVENT_FLUSH=true dolt status
+
+    # output all the metrics data to stdout for examination
+    dolt config --global --add metrics.host "fake.server"
+    dolt send-metrics
+    [ "$status" -eq 1 ]
+    [[ "$output" =~ "Error flushing events" ]] || false
+    [[ "$output" =~ "fake.server" ]] || false
+}