From b207b1bc0394b5424b2b1ee8a61b7bdf8251b672 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Mon, 4 Dec 2023 17:03:24 -0800 Subject: [PATCH] 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 | 52 ++++++++++++------------ go/libraries/events/event_flush.go | 31 +++++++------- integration-tests/bats/send-metrics.bats | 29 +++++++++++++ 3 files changed, 71 insertions(+), 41 deletions(-) diff --git a/go/cmd/dolt/commands/send_metrics.go b/go/cmd/dolt/commands/send_metrics.go index 46fcaae340..e00857c4a6 100644 --- a/go/cmd/dolt/commands/send_metrics.go +++ b/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 diff --git a/go/libraries/events/event_flush.go b/go/libraries/events/event_flush.go index 82961c5afa..e6d0cbcb3a 100644 --- a/go/libraries/events/event_flush.go +++ b/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 } \ No newline at end of file diff --git a/integration-tests/bats/send-metrics.bats b/integration-tests/bats/send-metrics.bats index d561e25ec0..1c8cdb53c0 100644 --- a/integration-tests/bats/send-metrics.bats +++ b/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 +}