Enable wrapping of distributor push.

[jeschkies]

Summary:
Introduces PushFunc and PushWrapper types which are used to defined a
configurable wrapper around the push function of the distributor. It is
similar to an HTTP middleware but receives the already deserialized
write request.

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[pstibrany]

Would it make sense to reuse recently introduced PushOnlyIngesterServer instead?

[pracucci]

LGTM to me. Just left minor comments.

Would it make sense to reuse recently introduced PushOnlyIngesterServer instead?

@pstibrany I'm not sure. I talked to Karsten offline and the use case here is to run the distributor as is but being able, in a downstream project, to access the WriteRequest before (or after) it has been handled by the Distributor.Push(). I think a wrapper here is what we want. WDYT?

[pstibrany]

@pstibrany I'm not sure. I talked to Karsten offline and the use case here is to run the distributor as is but being able, in a downstream project, to access the WriteRequest before (or after) it has been handled by the Distributor.Push(). I think a wrapper here is what we want. WDYT?

What I meant was to modify util/push.Handler like this:

func Handler(cfg distributor.Config int, sourceIPs *middleware.SourceIPExtractor, push client.PushOnlyIngesterServer) http.Handler

Which would work and wouldn't introduce new dependencies, but it doesn't really solve the problem this PR is solving.

While working on this, we I'd suggest we reduce the dependency from util/push to distributor package, by

1. moving DistributorPushFunc to util/push (interfaces are typically declared at place where they are "consumed", ie. called)
2. applying the push wrapper before calling util/push.Handler, and
3. passing cfg.MaxRecvMsgSize as separate parameter to Handler.

WDYT?

[jeschkies]

@pracucci the linter complains

pkg/distributor/distributor.go:399:6: type name will be used as distributor.DistributorPushFunc by other packages, and that stutters; consider calling this PushFunc (golint)
type DistributorPushFunc func(context.Context, *ingester_client.WriteRequest) (*ingester_client.WriteResponse, error)
     ^

Should I ignore it?

[jeschkies]

moving DistributorPushFunc to util/push (interfaces are typically declared at place where they are "consumed", ie. called)

@pstibrany, I've tried that but it introduces a circular dependency since the config in distrobutor.go must know the type DistributorPushWrapper.

applying the push wrapper before calling util/push.Handler, and

I was thinking about this. However, the wrapper is defined in the config. Since the config is passed to the handler why not evaluate it there?

passing cfg.MaxRecvMsgSize as separate parameter to Handler.

Ah, I start to understand. You don't want the dependency on config and thus avoid the circular dependency. I'm open to that. @pracucci, what's your take?

[pracucci]

Ah, I start to understand. You don't want the dependency on config and thus avoid the circular dependency. I'm open to that. @pracucci, what's your take?

@jeschkies I'm fine with Peter's suggestion 👍

[pracucci]

type DistributorPushFunc func(context.Context, *ingester_client.WriteRequest) (*ingester_client.WriteResponse, error)
Should I ignore it?

Once you will move it to pkg/util you can just name it PushFunc because it will be a generic utility there with no "knowledge" of the distributor.

[jeschkies]

Once you will move it to pkg/util you can just name it PushFunc because it will be a generic utility there with no "knowledge" of the distributor.

I called it Func because the linter would complain about stuttering. Unfortunately my tests don't make any sense anymore. How can I test the logic in the RegisterDistributor method?

[pstibrany]

Thanks for addressing our feedback!

[pracucci]

LGTM (modulo a couple of nits). Thanks!