Remove *Request to fix streaming client context

[emcfarlane]

Context in client streams doesn't include the span due to the *Request requiring information only included once the connection is created. This PR proposes removing the *Request in favour of simply providing the connect.Spec information. Now we can correctly initialize the span creation before passing the context to the client handler.

Changes

• Removes exported *Request object (breaking change)
• Changes option filters to use connect.Spec instead of *Request (breaking change)

[jhump]

Changes option filters to use connect.Spec instead of *Request (breaking change)

This is actually removing a certain class of capability, right? They previously also had access to the peer and the headers. That seems useful to maintain. Is there some way to use a mutable placeholder for the span in the context (so headers can be added and attributes filtered later?) or perhaps a custom wrapper/implementation of context.Context that allows the span to be inserted later?

[emcfarlane]

@jhump correct, this removes the peer and header attributes limited by the API of connect client interceptors. The span could be accessed by any other interceptor in the call to next(ctx, spec) which returns the peer and header objects required by the filter. To obtain the same filtering one can set values on the context to introspect in the interceptor from any http middleware. Some other implementations don't include these filtering options like otelgrpc but in contrast otelhttp includes the full request.

Happy with the connect.Spec only approach and users requiring the filtering to set context values?

[jhump]

users requiring the filtering to set context values

How do you envision this? What exactly sets context values and how are they used? It sounds, from prior paragraph, like this also requires users to add a custom HTTP middleware that coordinates? How does this filter the attributes? Does it mutate the span in context by removing attributes? If so, can we just take a similar approach inside the library, and defer the call to the filter and use its results to modify an already-created span?

[emcfarlane]

how do you envision this?

@jhump add information to the context (the span isn't present yet) that the filter can use to inform it's decision on whether or not to add metrics and tracing to this request. On the filter output otelconnect then decides whether or not to create the span for the request. As an example we could inject the http.Request object in custom http middleware to access within the filter:

type requestKey struct{}

// injectRequest adds a reference to the original request object for use by otel filters.
func injectRequest(handler http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        r = r.WithContext(context.WithValue(ctx, requestKey, r))
        handler(w, r)
    })
}

// myRequestFilter implements WithFilter func.
func myRequestFilter(ctx context.Context, spec connect.Spec) bool {
    r, _ := context.Value(requestKey)
    return r.Header.Get("SomeHeader") != "Internal" // Filter on "SomeHeader"
}

Saying this, the full *http.Request is probably not useful for filtering. A use case I'd imagine desired would be to filter on authenticated users to avoid logging metrics for internal bots or private access. This would probably already use a context based approach similar to the suggested.

[jhump]

@ed, now I'm really confused. I thought, at the point the filter was called, we haven't yet invoked the HTTP call. Isn't that the entire reason we can no longer directly provide peer and headers to the filter function? We instead have to wait for the first call to stream.Send, after the filter function is already called, and the middleware won't run until then.

[emcfarlane]

@jhump correct for the client, but not for the server.

[jhump]

Okay, I've come around on the API change. The only thing I can think of that would be useful in here related to headers is a way to force a trace to be captured (like even if spans are sampled and sampling would otherwise lead to this request being omitted), but I think that is orthogonal to this attribute filter stuff. For span attributes, it does seem like the spec should suffice.

Mind rebasing to fix merge conflicts? I'll dive in and do a proper review after that.