Use gRPC for distributor <-> ingester rpcs. #144

tomwilkie · 2016-11-18T12:53:08Z

Fixes #105

Don't commit generated code, teach make how to build it
Don't duplicate remote.proto from promtheus, import it.
Make ingester and distributor expose grpc services
Move http server code into the ingester/distributor packages (some duplication, but will go away after we migrate)
Ingesters still expose http interface for migration purposes.

TODO:

Instrument grpc https://github.com/grpc-ecosystem/go-grpc-prometheus

rade · 2016-11-18T14:09:23Z

From #105:

Should remove some boilerplate and hopefully reduce latency.

Any measurements?

tomwilkie · 2016-11-18T14:16:10Z

Nope, not tested it yet.

rade · 2016-11-18T14:22:16Z

I was actually more thinking of the boilerplate reduction than the latency. Because of vendoring it's hard to tell the actual difference at a glance.

tomwilkie · 2016-11-18T14:35:39Z

Take a look at the second commit. We currently haven;t removed the origin code http (as its needed for migration), but will do. I just tidied it up for now.

tomwilkie · 2016-11-18T14:37:01Z

Had to update built-tools and fix the generation of bindata.go to get lint to pass.

tomwilkie · 2016-11-18T18:45:51Z

@jml this is good to go now. Anecdotally query performance is much improved (locally).

- Don't commit generated code, teach make how to build it - Don't duplicate remote.proto from promtheus, import it. - Make ingester and distributor expose grpc services - Move http server code into the ingester/distributor packages (some duplication, but will go away after we migrate) - Start a gRPC server for ingester (along side http server) - Plumb through optionally creating grpc ingester client - Try and propagate user id from distributor to ingester - gRPC middleware package; add logging, auth and instrumentation middleware

jml

Mostly looks good, and I really like the refactoring. Would like another chance to go over it after you've replied to the review, since it really is quite big (even without the vendoring), and I want to make sure I grok it all.

jml · 2016-11-21T11:39:05Z

cmd/cortex/main.go

 	flag.StringVar(&cfg.rulerConfig.ConfigsAPIURL, "ruler.configs.url", "", "URL of configs API server.")
 	flag.StringVar(&cfg.rulerConfig.UserID, "ruler.userID", "", "Weave Cloud org to run rules for")
 	flag.DurationVar(&cfg.rulerConfig.EvaluationInterval, "ruler.evaluation-interval", 15*time.Second, "How frequently to evaluate rules")
-	flag.BoolVar(&cfg.logSuccess, "log.success", false, "Log successful requests")
-	flag.BoolVar(&cfg.watchDynamo, "watch-dynamo", false, "Periodically collect DynamoDB provisioned throughput.")
+


What's up with all this shuffling? It doesn't matter much, just curious.

For my own sanity, I grouped the flags by common, ingester, distributor etc

jml · 2016-11-21T11:41:13Z

cmd/cortex/main.go

+		)
+		cortex.RegisterIngesterServer(grpcServer, ing)
+		go grpcServer.Serve(lis)
+		defer grpcServer.Stop()


Maybe this function (main) is getting too big?

Yeah. I think we'll end up breaking this into a bunch of binaries / images next.

jml · 2016-11-21T11:47:41Z

distributor/distributor.go

 	}
-	d.clients[hostname] = client
+
+	d.clients[ingester.Hostname] = client


Will it matter if some of the ingester clients are HTTP and others are grpc?

It will not. It has to support that for the deployment case.

jml · 2016-11-21T12:01:13Z

distributor/http_ingester_client.go

+	"github.com/weaveworks/cortex/user"
+)
+
+// IngesterClient is a client library for the ingester


Should say httpIngesterClient.

jml · 2016-11-21T12:15:42Z

distributor/distributor.go

@@ -158,13 +173,14 @@ type sampleTracker struct {
 	succeeded  int32
 }

-// Append implements SampleAppender.
-func (d *Distributor) Append(ctx context.Context, samples []*model.Sample) error {
+// Push Implements cortex.IngesterServer


Should be lower-case implements.

jml · 2016-11-21T12:43:28Z

util/compat.go

+		m = append(m, &ss)
+	}
+
+	return m


Please add some tests for each of these To/From pairs that demonstrate that they roundtrip correctly. (i.e. that ToFoo(FromFoo(x)) == x)

jml · 2016-11-21T12:44:47Z

ruler/ruler.go

@@ -151,12 +151,14 @@ func (r *Ruler) getConfig(userID string) (*config.Config, error) {

 // appenderAdapter adapts cortex.SampleAppender to prometheus.SampleAppender
 type appenderAdapter struct {
-	appender cortex.SampleAppender
-	ctx      context.Context
+	distributor *distributor.Distributor


That said, I think this should probably be an IngesterServer (if it is indeed correct that the distributor implements the IngesterServer interface).

Not fully - its only implements the Push method. I thought the coupling was a bit strong here, but there is no other interface just for Push, unless we invent one.

Something we can save for a future refactoring then.

jml · 2016-11-21T12:45:00Z

ingester/ingester.go

-// Append implements storage.SampleAppender.
-func (i *Ingester) Append(ctx context.Context, samples []*model.Sample) error {
-	for _, sample := range samples {
+// Push Implements cortex.IngesterServer


Lower-case "implements"

jml · 2016-11-21T12:55:50Z

distributor/http.go

@@ -0,0 +1,56 @@
+package distributor


How would you feel about renaming this file to http_server.go or http_handlers.go or something? Likewise for its ingester equivalent.

Don't mind either way. The ingester_client.go and ingester/http.go are going to be deleted in the next PR.

jml · 2016-11-21T13:07:20Z

distributor/http.go

+	_, err := d.Push(ctx, &req)
+	if err != nil {
+		switch e := err.(type) {
+		case IngesterError:


AIUI, this is only going to be relevant if we're also using the HTTP ingester client, and that with gRPC, we will no longer be able to distinguish between user errors and server errors. Is there a way of addressing this? It's a bit of a regression.

I'm sure there is with gRPC, but I'll have to investigate.

Can you please file a bug for follow-up?

tomwilkie · 2016-11-21T14:09:28Z

@jml PTAL

jml · 2016-11-21T14:14:17Z

util/middleware/logging.go

@@ -15,8 +15,10 @@ func ServerLoggingInterceptor(logSuccess bool) grpc.UnaryServerInterceptor {
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 		begin := time.Now()
 		resp, err := handler(ctx, req)
-		if logSuccess || err != nil {
+		if logSuccess && err != nil {


That should be err == nil, I think. Probably overall clearer to do:

if err != nil { ... } else if logSuccess { ... }

jml · 2016-11-21T14:14:40Z

Thanks. Minor tweaks.

tomwilkie changed the title ~~Use gRPC for distributor <-> ingester rpcs.~~ [WIP] Use gRPC for distributor <-> ingester rpcs. Nov 18, 2016

tomwilkie self-assigned this Nov 18, 2016

tomwilkie force-pushed the grpc branch from 6ce379a to 2f9826d Compare November 18, 2016 14:07

tomwilkie force-pushed the grpc branch 2 times, most recently from 06a2986 to f68f206 Compare November 18, 2016 18:21

Update grpc vendor

3b6843e

tomwilkie force-pushed the grpc branch from f68f206 to 3466e25 Compare November 18, 2016 18:26

tomwilkie changed the title ~~[WIP] Use gRPC for distributor <-> ingester rpcs.~~ Use gRPC for distributor <-> ingester rpcs. Nov 18, 2016

tomwilkie assigned jml and unassigned tomwilkie Nov 18, 2016

tomwilkie force-pushed the grpc branch from e38c2e3 to c5eece9 Compare November 19, 2016 11:01

jml reviewed Nov 21, 2016

View reviewed changes

Review feedback

23f4fcf

jml reviewed Nov 21, 2016

View reviewed changes

jml approved these changes Nov 21, 2016

View reviewed changes

Review feedback

ab8a67f

tomwilkie merged commit edcc768 into master Nov 22, 2016

tomwilkie deleted the grpc branch November 22, 2016 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use gRPC for distributor <-> ingester rpcs. #144

Use gRPC for distributor <-> ingester rpcs. #144

tomwilkie commented Nov 18, 2016 •

edited

Loading

rade commented Nov 18, 2016

tomwilkie commented Nov 18, 2016

rade commented Nov 18, 2016

tomwilkie commented Nov 18, 2016

tomwilkie commented Nov 18, 2016

tomwilkie commented Nov 18, 2016

jml left a comment

jml Nov 21, 2016

tomwilkie Nov 21, 2016

jml Nov 21, 2016

tomwilkie Nov 21, 2016

jml Nov 21, 2016

tomwilkie Nov 21, 2016 •

edited

Loading

jml Nov 21, 2016

jml Nov 21, 2016

tomwilkie Nov 21, 2016

jml Nov 21, 2016

tomwilkie Nov 21, 2016

jml Nov 21, 2016

tomwilkie Nov 21, 2016

jml Nov 21, 2016

jml Nov 21, 2016

jml Nov 21, 2016

tomwilkie Nov 21, 2016

jml Nov 21, 2016

tomwilkie Nov 21, 2016

jml Nov 21, 2016

tomwilkie commented Nov 21, 2016

jml Nov 21, 2016

jml commented Nov 21, 2016

Use gRPC for distributor <-> ingester rpcs. #144

Use gRPC for distributor <-> ingester rpcs. #144

Conversation

tomwilkie commented Nov 18, 2016 • edited Loading

rade commented Nov 18, 2016

tomwilkie commented Nov 18, 2016

rade commented Nov 18, 2016

tomwilkie commented Nov 18, 2016

tomwilkie commented Nov 18, 2016

tomwilkie commented Nov 18, 2016

jml 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

tomwilkie Nov 21, 2016 • edited Loading

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

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

tomwilkie commented Nov 21, 2016

Choose a reason for hiding this comment

jml commented Nov 21, 2016

tomwilkie commented Nov 18, 2016 •

edited

Loading

tomwilkie Nov 21, 2016 •

edited

Loading