Add file emitter that writes event data file #67

coffeegoddd · 2019-09-03T16:18:50Z

Added file emitter that saves event data to files, and a flush that parses the files and sends them to the grpc server.

coffeegoddd · 2019-09-03T16:26:22Z

go/cmd/dolt/commands/send_metrics.go

+		return 1
+	}
+	return 0
+}


SendMetrics is the commandFunc for the dolt command "dolt send-metrics". Looking for feedback on how to handle the error logging here. This also gets run in a separate process on all other dolt commands using os.exec

coffeegoddd · 2019-09-03T16:32:40Z

go/cmd/dolt/dolt.go

+	// 	conn, _ := dEnv.GrpcConnWithCreds(hostAndPort, insecure, nil)
+
+	// 	return events.NewGrpcEmitter(conn)
+	// }



Commented this code out just during development while working on the FileEmitter.
It seems like this function is used to handle the creation of the default emitter we want, based on the various emitters we've created?
Do we want the FileEmitter to now be the default emitter we instantiate?

coffeegoddd · 2019-09-03T16:35:08Z

go/cmd/dolt/dolt.go

+		}
+	}
+	return nil
+}


Running the hidden command "send-metrics" on a new process for every other dolt command. This block does not run if the user types it in herself, since otherwise it would loop infinitely.

coffeegoddd · 2019-09-03T16:37:18Z

go/go.sum

@@ -195,6 +195,7 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
 github.com/lib/pq v1.1.1 h1:sJZmqHoEaY7f+NPP8pgLB/WxulyR3fewgCM2qaSlBb4=
 github.com/lib/pq v1.1.1/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
+github.com/liquidata-inc/dolt v0.9.8 h1:uup9UA59oSvYsqYd9A3zF0CFXUnF1qI8odE+Cxtn+pg=
 github.com/liquidata-inc/go-mysql-server v0.4.1-0.20190710171053-b2883167103a h1:Z8uOEqaBW1a0vpu+S9MlWwiyIDrjtn9sZ/CCcs32e9s=
 github.com/liquidata-inc/go-mysql-server v0.4.1-0.20190710171053-b2883167103a/go.mod h1:bXirMMsoAKR1OI9GTQJPGSyuwJlOseI9AFNAdpwRY2k=
 github.com/liquidata-inc/ishell v0.0.0-20190514193646-693241f1f2a0 h1:phMgajKClMUiIr+hF2LGt8KRuUa2Vd2GI1sNgHgSXoU=


I'm not to sure how the go.sum works yet, which makes me pretty nervous. I don't touch this file, but it seems to change at times automatically... Planning to look into this.

go.sum works via witchcraft

coffeegoddd · 2019-09-03T16:38:50Z

go/libraries/events/directory_proc.go

+			empty = true
+		}
+	}
+}


Same code from the events server. Just changed the log extension variable to be evtDataExt.
Will add the test file as well.

I think this might be a bit of overkill and could result in a ton of send-metrics processes running when someone is working in offline mode. I think instead of watching the directory and looping as long and there are still files, you should just loop over all files until you have a failure to send, and then exit on first failure.

coffeegoddd · 2019-09-03T16:41:51Z

go/libraries/events/event_flush.go

+		// should panic?
+		panic(ErrEventsDataDir)
+	}
+


Not sure if we should panic here? This panic would occur in the separate process.

It's probably fine.

bheni

Took a first pass on this review. Will dig a little deeper later today.

bheni · 2019-09-03T18:26:16Z

go/cmd/dolt/dolt.go

@@ -128,11 +129,19 @@ func runMain() int {

 	dEnv := env.Load(context.TODO(), env.GetCurrentUserHomeDir, filesys.LocalFS, doltdb.LocalDirDoltDB)

+	if err := processEventsDir(args, dEnv); err != nil {
+		// log err
+		log.Fatal(err)


We shouldn't ever let metrics failures fail dolt commands.

bheni · 2019-09-03T18:29:11Z

go/go.sum

@@ -195,6 +195,7 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
 github.com/lib/pq v1.1.1 h1:sJZmqHoEaY7f+NPP8pgLB/WxulyR3fewgCM2qaSlBb4=
 github.com/lib/pq v1.1.1/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
+github.com/liquidata-inc/dolt v0.9.8 h1:uup9UA59oSvYsqYd9A3zF0CFXUnF1qI8odE+Cxtn+pg=
 github.com/liquidata-inc/go-mysql-server v0.4.1-0.20190710171053-b2883167103a h1:Z8uOEqaBW1a0vpu+S9MlWwiyIDrjtn9sZ/CCcs32e9s=
 github.com/liquidata-inc/go-mysql-server v0.4.1-0.20190710171053-b2883167103a/go.mod h1:bXirMMsoAKR1OI9GTQJPGSyuwJlOseI9AFNAdpwRY2k=
 github.com/liquidata-inc/ishell v0.0.0-20190514193646-693241f1f2a0 h1:phMgajKClMUiIr+hF2LGt8KRuUa2Vd2GI1sNgHgSXoU=


go.sum works via witchcraft

bheni · 2019-09-03T18:32:05Z

go/libraries/events/directory_proc.go

+			empty = true
+		}
+	}
+}


I think this might be a bit of overkill and could result in a ton of send-metrics processes running when someone is working in offline mode. I think instead of watching the directory and looping as long and there are still files, you should just loop over all files until you have a failure to send, and then exit on first failure.

bheni · 2019-09-03T18:35:07Z

go/libraries/events/event_flush.go

+	// conn, _ := dEnv.GrpcConnWithCreds(hostAndPort, insecure, nil)
+
+	// local grpc for dev
+	hostAndPort := fmt.Sprintf("%s:%d", "localhost", 50051)


you can connect locally without having to hard code this. You can run these commands:

dolt config --global --add metrics.host localhost dolt config --global --add metrics.port 50051 dolt config --global --add metrics.insecure true

in order to point your dolt cli at your local metrics server.

bheni · 2019-09-03T18:35:52Z

go/libraries/events/event_flush.go

+		// should panic?
+		panic(ErrEventsDataDir)
+	}
+


It's probably fine.

coffeegoddd · 2019-09-04T00:29:45Z

go/cmd/dolt/dolt.go

+
+		if err := processEventsDir(args, dEnv); err != nil {
+			log.Print(err)
+		}
 	}()


Update: This is no longer a log.Fatal

coffeegoddd · 2019-09-04T00:34:50Z

go/libraries/events/event_flush.go

+	}
+
+	return nil
+}


Much simpler implementation now using fs.Iter() and the flush() method.

bheni

Looking pretty good. Couple more comments.

bheni · 2019-09-04T05:11:42Z

go/cmd/dolt/dolt.go

+				cmd := exec.Command("dolt", commands.SendMetricsCommand)
+
+				if err := cmd.Start(); err != nil {
+					log.Print(err)


I think we should fail silently until a later date when we support a verbose logging mode.

bheni · 2019-09-04T05:15:32Z

go/libraries/events/event_flush.go

+		return false
+	}
+
+	isFileValid, err := checkFilenameAndExt(data, path)


if isFileValid is false, and err is nil that means the file is corrupted. Retries on this file won't fix it, so we should go ahead and delete it.

bheni · 2019-09-04T05:18:40Z

go/libraries/events/file_backed_proc.go

+		return err
+	}
+
+	if _, err := f.Write(data); err != nil {


it's possible that Write will write less than the entire buffer. iohelp.WriteAll will take care of flushing the entire contents of data.

go/libraries/events/file_backed_proc.go

bheni

Just a couple things to fix before merging. But looks good.

bheni · 2019-09-04T18:49:55Z

go/cmd/dolt/dolt.go

-
-		insecure, err := strconv.ParseBool(*insecureStr)
+			if err := cmd.Start(); err != nil {
+				log.Print(err)


disable logging until we have verbose mode.

bheni · 2019-09-04T18:51:18Z

go/libraries/events/event_flush.go

+	isFileValid, err := egf.fbp.CheckingFunc(data, path)
+
+	if isFileValid && err == nil {
+		ctx, cnclFn := context.WithDeadline(context.Background(), time.Now().Add(time.Second+500*time.Millisecond))


Since we are running in a new process we can up this to time.Minute

bheni · 2019-09-04T18:55:02Z

go/libraries/events/event_flush_test.go

+	"google.golang.org/grpc"
+)
+
+var globalEvents []*eventsapi.ClientEvent


using a global will be problematic if 2 of these tests run concurrently. I'd make this a member of the TestClient

coffeegoddd · 2019-09-04T19:32:31Z

go/libraries/events/event_flush_test.go

+type TestClient struct {
+	CES []*eventsapi.ClientEvent
+}
+


no longer using the global events slice.

coffeegoddd · 2019-09-04T19:34:01Z

go/cmd/dolt/commands/send_metrics.go

+			// log.Print(err)
+			return 1
+		}
+


logging commented out in both dolt.go and send_metrics.go

coffeegoddd · 2019-09-04T19:35:18Z

go/libraries/events/event_flush.go

+	if isFileValid && err == nil {
+		ctx, cnclFn := context.WithDeadline(context.Background(), time.Now().Add(time.Minute))
+		defer cnclFn()
+


changed to time.Minute

…hod, tests in progress.

coffeegoddd · 2019-09-11T19:13:01Z

go/libraries/events/file_backed_proc.go

+
+	return &FileBackedProc{ed: eventsDataDir, namingFunc: nf, CheckingFunc: cf, LockPath: lp}
+}
+


This is the new implementation of the locking system. Instead of locking the entire events directory, I create a dolt.lock file if it does not exist, then lock/unlock this file.

coffeegoddd force-pushed the db/file-emitter branch from 9858e56 to 3671f32 Compare September 3, 2019 16:28

coffeegoddd commented Sep 3, 2019

View reviewed changes

coffeegoddd requested a review from bheni September 3, 2019 16:45

bheni reviewed Sep 3, 2019

View reviewed changes

coffeegoddd force-pushed the db/file-emitter branch 4 times, most recently from 177cfcd to 1090c2e Compare September 4, 2019 00:37

coffeegoddd commented Sep 4, 2019

View reviewed changes

bheni reviewed Sep 4, 2019

View reviewed changes

coffeegoddd force-pushed the db/file-emitter branch 2 times, most recently from 8c6f06c to 0249669 Compare September 4, 2019 17:44

coffeegoddd commented Sep 4, 2019

View reviewed changes

go/libraries/events/file_backed_proc.go Show resolved Hide resolved

coffeegoddd changed the title ~~[WIP] Add file emitter that writes event data file~~ Add file emitter that writes event data file Sep 4, 2019

coffeegoddd requested a review from bheni September 4, 2019 17:46

bheni approved these changes Sep 4, 2019

View reviewed changes

coffeegoddd force-pushed the db/file-emitter branch from 0249669 to 9f7e867 Compare September 4, 2019 19:28

Add file emitter that writes event data file

dba75f6

coffeegoddd force-pushed the db/file-emitter branch from 9f7e867 to dba75f6 Compare September 4, 2019 19:31

coffeegoddd commented Sep 4, 2019

View reviewed changes

coffeegoddd requested a review from bheni September 4, 2019 19:35

coffeegoddd added 9 commits September 5, 2019 15:10

Add locking to event flush

7199bd9

add flockAndFlush function to SendMetrics command

fe69fb7

removed flockAndFlush function, now type switching in FlushEvents met…

e396de4

…hod, tests in progress.

initial implementation of concurrency safe flush events

d34df0d

cleaning up pr

cb41702

implementing the filesyslocker interface for the localfs

bf91fb8

implemented new filesyslock inteface

660eb8d

added localfs test for event flush

753891c

added local fs event flush test

ab81777

coffeegoddd force-pushed the db/file-emitter branch 23 times, most recently from cfeb926 to ecba41d Compare September 11, 2019 18:26

coffeegoddd changed the base branch from bh/client-events to master September 11, 2019 18:43

coffeegoddd force-pushed the db/file-emitter branch from ecba41d to e4bf724 Compare September 11, 2019 18:50

fixing bats test

ce7a338

coffeegoddd force-pushed the db/file-emitter branch from e4bf724 to ce7a338 Compare September 11, 2019 19:00

coffeegoddd commented Sep 11, 2019

View reviewed changes

coffeegoddd merged commit b2f77fa into master Sep 11, 2019

coffeegoddd deleted the db/file-emitter branch September 11, 2019 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file emitter that writes event data file #67

Add file emitter that writes event data file #67

coffeegoddd commented Sep 3, 2019 •

edited

coffeegoddd Sep 3, 2019

coffeegoddd Sep 3, 2019

coffeegoddd Sep 3, 2019

coffeegoddd Sep 3, 2019

bheni Sep 3, 2019

coffeegoddd Sep 3, 2019

bheni Sep 3, 2019

coffeegoddd Sep 3, 2019

bheni Sep 3, 2019

bheni left a comment

bheni Sep 3, 2019

bheni Sep 3, 2019

bheni Sep 3, 2019

bheni Sep 3, 2019

bheni Sep 3, 2019

coffeegoddd Sep 4, 2019

coffeegoddd Sep 4, 2019

bheni left a comment

bheni Sep 4, 2019

bheni Sep 4, 2019

bheni Sep 4, 2019

bheni left a comment

bheni Sep 4, 2019

bheni Sep 4, 2019

bheni Sep 4, 2019

coffeegoddd Sep 4, 2019

coffeegoddd Sep 4, 2019

coffeegoddd Sep 4, 2019

coffeegoddd Sep 11, 2019


		return &FileBackedProc{ed: eventsDataDir, namingFunc: nf, CheckingFunc: cf, LockPath: lp}
		}

Add file emitter that writes event data file #67

Add file emitter that writes event data file #67

Conversation

coffeegoddd commented Sep 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bheni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bheni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bheni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coffeegoddd commented Sep 3, 2019 •

edited