Add endpoint name middleware

[sagikazarmark]

This PR adds a middleware to the endpoint package that attaches an operation name to the context, as well as adds support to the existing tracer implementations to use this name in a backward compatible way.

It's supposed to be a (near) top level middleware in the chain, so subsequent middleware can utilize it:

func MakeEndpoint() endpoint.Endpoint {
	return endpoint.Chain(
		endpoint.EndpointNameMiddleware("endpointName"),
		someTracing.TraceEndpoint(tracer, ""), // do not set an operation name
		LoggingMiddleware(logger),
	)(endpointFunc)
}

The motivation behind this middleware is to avoid duplicating an endpoint name when setting up the chain:

func MakeEndpoint() endpoint.Endpoint {
	return endpoint.Chain(
		someTracing.TraceEndpoint(tracer, "endpointName"),
		LoggingMiddleware(logger, "endpointName"),
		SomeOtherMiddleware("endpointName"),
	)(endpointFunc)
}

The middleware can also help with moving other middleware depending on a name to a global chain:

func MakeEndpoint(globalMWs []endpoint.Middleware) endpoint.Endpoint {
	return endpoint.Chain(
		endpoint.EndpointNameMiddleware("endpointName"),
		globalMWs..., // tracer and logger does not have to be setup per endpoint with a name
	)(endpointFunc)
}

Last, but not least this middleware can easily be used by code generation tools to automatically populate the context with an operation name without manually initializing it.

TODO

• Think about a better name for the middleware (eg. OperationName is used by tracing implementations)
• What should happen when a name is empty? (fallback to a default one, like the tracing implementations do, return the original endpoint directly, or attach an empty name to the context?)

[basvanbeek]

So instead of exporting the context key it is much better to add a method to fetch the endpoint name. The getting and setting of context values should be a concern of the owning package, not the consuming packages

[sagikazarmark]

100% agree, I tried to follow the existing pattern in go-kit. Happy to add a getter instead of the exported key.

[sagikazarmark]

I added a getter for the context key, but left it exported to follow existing go-kit examples. Should I make it unexported?

[peterbourgon]

This way of setting values in the context isn't great, because

1. On the server side, the name is visible to the service layer, but not the transport layer
2. On the client side, the name is visible to the transport layer, but not to the caller

Using the context as a way to accumulate per-request metadata through layers in a single process requires a helper package like ctxdata, installed at the outermost layer. And I'm not sure it makes sense for Go kit to mandate any such package.

[sagikazarmark]

Using the context as a way to accumulate per-request metadata through layers

IMHO context added by this middleware is specific to the endpoint layer. In the service layer you probably already know the operation name, the endpoint layer on the other hand is independent of that.

The transport layer also kinda has it's own operation name (request method, URI, gRPC method name, etc).

So in this case I don't see supporting multiple layer an issue, not to mention that go-kit already has several examples for populating the context with data (eg. auth packages).

[basvanbeek]

Looking at the entire thing I feel similar towards it as Peter. This doesn't bring enough value to warrant the approach.

I think we should allow for adding handler options to each tracing endpoint middleware that a consumer can implement themselves which not only can update span name but also control if endpoint local span creation is needed, add tags, etc.

This way it is not a concern of Go kit but of the implementor and no changes are needed in the endpoint package keeping it nice and tidy.

[sagikazarmark]

Tracing might not be the only use case for using an operation name.

I feel like keeping the endpoint package (and go-kit in general) "pure" at the cost of pushing everything to the implementation side might be too high.

This middleware does not force anyone to use it, the rest of go-kit works fine without it.

On the other hand, when used makes using other middleware easier, common code generation tools for go-kit can use it, so the benefit of having it definitely outweighs the costs in my opinion.

I find the first approach much cleaner:

func MakeEndpoint() endpoint.Endpoint {
	return endpoint.Chain(
		endpoint.EndpointNameMiddleware("endpointName"),
		someTracing.TraceEndpoint(tracer, ""), // do not set an operation name
		LoggingMiddleware(logger),
	)(endpointFunc)
}

vs

func MakeEndpoint() endpoint.Endpoint {
	return endpoint.Chain(
		MyEndpointNameMiddleware("endpointName"),
		someTracing.TraceEndpoint(
			tracer,
			"",
			someTracing.SpanNameHandler(func(ctx context.Context) someTracing.Span {
				operationName := MyEndpointName(ctx)
				return someTracing.NewSpan(operationName)
			}),
		), // do not set an operation name
		MyLoggingMiddleware(logger),
	)(endpointFunc)
}

[peterbourgon]

What are everyone's feelings about generalizing it a bit?

package endpoint

// Name TBD, maybe AnnotationMiddleware, ValueMiddleware, ...?
func WithValueMiddleware(next Endpoint, key, val interface{}) Endpoint {
	return func(ctx context.Context, request interface{}) (response interface{}, err error) {
		return next(ctx.WithValue(key, val), request)
	}
}

[sagikazarmark]

@basvanbeek is this something that you meant by handler options?

https://github.com/census-instrumentation/opencensus-go/blob/master/plugin/ochttp/server.go#L57-L72

@peterbourgon I'm fine with that, but I would probably keep the context key for the endpoint name, so go-kit components can also use it.

Adding a name candidate to the list: ContextValueMiddleware

[hecomp]

How to get ResponseWriter from the endpoint?

[peterbourgon]

@hecomp Please open a new issue.

[hecomp]

@peterbourgon ok will do.

[Reasno]

Naming an endpoint by currying:

func MakeEndpoint(tracer opentracing.Tracer, logger log.Logger) endpoint.Endpoint {
	mw := func(name string) endpoint.Middleware {
		return endpoint.Chain(
			someTracing.TraceEndpoint(tracer, name),
			LoggingMiddleware(logger, name),
                        otherMiddleware(name),
		)
	}
	return mw("endpointName")(endpointFunc)
}