Provide default sd.Factory for each transport

[jupp0r]

Working on #89, I think it would actually make sense to provide loadbalancer.Factory functions in each transport package, ie transforming strings into endpoints. According to the godoc in loadbalancer.Factory, each user is supposed to provide their own, but they would probably look quite similar. Any thoughts?

[peterbourgon]

loadbalancer.Factory converts an instance string to an endpoint.Endpoint, but remember one instance of a microservice is almost always going to give multiple methods, so you need one factory per method. For some transports like gRPC this might be generalizable and I'd tentatively support having a factory-factory, if it wasn't too awkward to use. For other transports like HTTP there's not strict rules and I wouldn't want to bless one way of specifying endpoints e.g. with a specific path.

[basvanbeek]

Example of wiring gRPC per method Endpoints with service discovery, load balancing, rate limiting, circuit breaking, etc.

// MakeEndpoint wires up a gRPC endpoint with Go kit tooling
func MakeEndpoint(method string, enc kitgrpc.EncodeRequestFunc, dec kitgrpc.DecodeResponseFunc, reply interface{}, spanFunc zipkin.NewSpanFunc, traceFunc kitgrpc.RequestFunc, config ClientConfig) endpoint.Endpoint {
    // identify correct path for endpoint service discovery
    path := "/myprefix/" + strings.ToLower(config.ServiceName)
​
    // generate a load balancer factory for endpoints
    factoryFunc := func(instance string) (endpoint.Endpoint, io.Closer, error) {
        // Try to establish connection to gRPC Service
        cc, err := stdgrpc.Dial(instance, config.GRPCSettings...)
        if err != nil {
            return nil, nil, err
        }
        // Create Go Kit gRPC Client Endpoint
        var e endpoint.Endpoint
        e = kitgrpc.NewClient(
            cc,
            config.ServiceName,
            method,
            enc,
            dec,
            reply,
            kitgrpc.SetClientBefore(traceFunc),
        ).Endpoint()
        // Wrap endpoint with a Circuit Breaker
        e = circuitbreaker.Gobreaker(
            gobreaker.NewCircuitBreaker(config.BreakerSettings),
        )(e)
        // Wrap endpoint with a Rate Limiter
        e = kitratelimit.NewTokenBucketLimiter(
            jujuratelimit.NewBucketWithRate(
                float64(config.MaxQPS),
                int64(config.MaxQPS),
            ),
        )(e)
        // return Endpoint to Endpoint Cache
        return e, nil, nil
    }
​
    // create the needed publisherFactory (Zookeeper based)
    logger = log.NewContext(logger).With("component", "loadbalancer/zk")
    zkClient, _ := zk.NewClient(zkhosts, logger)
    p, _ := zk.NewPublisher(zkClient, path, factoryFunc, logger)
​
    // create a Round Robin loadbalancer for the discovered endpoints
    lb := loadbalancer.NewRoundRobin(p)
​
    // wrap our loadbalancer with retry and timeout logic
    e := loadbalancer.Retry(config.MaxAttempts, config.MaxTime, lb)
​
    // annotate our endpoint with zipkin tracing
    e = zipkin.AnnotateClient(spanFunc, config.Collector)(e)
​
    // return the fully wrapped & decorated endpoint
    return e
}

which than can be brought into a single Client like this:

// CreateClient returns a client to our Demo service
func CreateClient(ctx context.Context, publisherFactory publisher.Factory, logger log.Logger, zipkinHostPort string, zipkinServiceName string, collector zipkin.Collector, options ...Option) (service.Demo, error) {
    // fill the config with our defaults
    config := newDefaultClientConfig()

    // check and handle the functional options
    for _, option := range options {
        if err := option(config); err != nil {
            return nil, err
        }
    }

    var (
        span  = mw.MakeSpanFuncs(zipkinHostPort, zipkinServiceName)
        trace = mw.MakeClientTraceFuncs(span)
    )

    // create and return the client
    return client{
        Context: ctx,
        Logger:  logger,
        sum: grpc.MakeEndpoint(
            "Sum",
            grpc.EncodeSumRequest,
            grpc.DecodeSumResponse,
            pb.SumReply{},
            span.Sum,
            trace.Sum,
            config,
        ),
        concat: grpc.MakeEndpoint(
            "Concat",
            grpc.EncodeConcatRequest,
            grpc.DecodeConcatResponse,
            pb.ConcatReply{},
            span.Concat,
            trace.Concat,
            config,
        ),
    }, nil
}

[majest]

From slack: it’s a bit confusing to use loadbalancer with the client in the current design. On one side you have a client struct that contains the endpoints (https://github.com/go-kit/kit/blob/master/examples/addsvc/client/grpc/client.go) and on other side there’s a publisher that requires a factory that returns a single endpoint : https://github.com/go-kit/kit/blob/master/loadbalancer/factory.go Is it up to me to tie both up or there’s something I’m missing here?

[peterbourgon]

Potentially related: #167

[peterbourgon]

@majest I believe (hope?) the current iteration of the examples makes things clearer. Please let me know if you disagree :)

—

I took a shot at what a transport/http.NewFactory might look like

func NewFactory(
    method string,
    path string,
    enc EncodeRequestFunc,
    dec DecodeResponseFunc,
    options ...ClientOption,
) sd.Factory {
    return func(instance string) (endpoint.Endpoint, io.Closer, error) {
        if !strings.HasPrefix(instance, "http") {
            instance = "http://" + instance
        }
        tgt, err := url.Parse(instance)
        if err != nil {
            return nil, nil, err
        }
        if path != "" {
            tgt.Path = path
        }
        return NewClient(method, tgt, enc, dec, options...).Endpoint(), nil, nil
    }
}

In the end it's just a wrapper for another constructor, and it necessarily must enforce an opinion about endpoint specification, namely that the user has to specify a path, which is appended to the instance string.

My feeling is that this class of helper function isn't actually so helpful. And I'd expect a gRPC version to be even more opinionated. Unless there are strong opinions otherwise, I'd like to close this as a WONTFIX :)

[basvanbeek]

I agree. The way the client endpoints need to be wired are very specific to the Go kit user and adding more logic decreases flexibility while not decreasing the amount of boilerplate or improving readability significantly. With the improved examples it should also have become clearer how to get started.

If this is not the case we should focus effort on improving the examples and the explanation around them, rather than adding more wrappers.