Distributor push wrapper should only receive unforwarded samples. #2980
Conversation
There was a problem hiding this comment.
The change to apply the DistributorPushWrapper before the other middlewares so that it only receives samples that haven't been forwarded makes sense.
But this change also makes the DistributorPushWrapper not receive samples which for example have been de-duplicated by HA-dedupe and which have been rejected by limits, so I'm wondering whether the users of DistributorPushWrapper would prefer to get these samples or not... @jeschkies I've seen that you made some commits related to the DistributorPushWrapper, do you maybe know what the expectation is here?
|
The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase |
pstibrany
force-pushed
the
fix-initialization-of-distributor-wrapper
branch
from
7d32819
to
326448e
Compare
|
Thanks for review. I've updated the PR to address @replay's feedback, and to rebase it onto the latest I've also pinged @jeschkies privately. Until we hear opposite, I'd suggest to go ahead with this. (It also helps to reduce memory usage in case of we reject request due to limits. Previously distributorWrapper would always parse the request before we even check for limits, while now it will only parse the request after limits check) |
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
pstibrany
force-pushed
the
fix-initialization-of-distributor-wrapper
branch
from
326448e
to
dec0a03
Compare
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
| // GetPushFunc returns push.Func that can be used by push handler. | ||
| // Wrapper, if not nil, is added to the list of distributor middlewares. | ||
| func (d *Distributor) GetPushFunc(distributorWrapper func(next push.Func) push.Func) push.Func { | ||
| return d.wrapPushWithMiddlewares(distributorWrapper, d.push) |
There was a problem hiding this comment.
Another non-blocking nit:
Am I understanding it correctly that we only need GetPushFunc() to be able to mock d.push in tests?
Usually when creating these wrappers that are only necessary for mocking I'd give them the same name as the function that they wrap, with the only difference being that they're exported while the wrapped function remains unexported.
That way it's clear that they're basically the same with the only difference being that one is meant to be used from inside the package and the other from outside.
There was a problem hiding this comment.
Feel free to leave it as it is if you disagree with me.
There was a problem hiding this comment.
I personally find it confusing to have two methods with the same name (one exported, one not), but different signature. I thought GetPushFunc would better describe what we're doing here. The fact that we're wrapping with middlewares is internal detail of distributor, not something we need to be public about.
…les. (#2980)" This reverts commit 3d14b39. See grafana/mimir-squad#973
* Revert "Distributor push wrapper should only receive unforwarded samples. (#2980)" This reverts commit 3d14b39. See grafana/mimir-squad#973 * Revert "move validation in distributor into separate middleware (#3386)" This reverts commit 5356edd. See grafana/mimir-squad#973 * Pin doc-validator version Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com> Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
| @@ -1119,7 +1119,14 @@ func (d *Distributor) forwardSamples(ctx context.Context, userID string, ts []mi | |||
| func (d *Distributor) Push(ctx context.Context, req *mimirpb.WriteRequest) (*mimirpb.WriteResponse, error) { | |||
| pushReq := push.NewParsedRequest(req) | |||
| pushReq.AddCleanup(func() { mimirpb.ReuseSlice(req.Timeseries) }) | |||
| return d.PushWithMiddlewares(ctx, pushReq) | |||
|
|
|||
| return d.GetPushFunc(nil)(ctx, pushReq) | |||
There was a problem hiding this comment.
This rewrap the middlewares for each Push() request (previously we were not doing it). Distributor.Push() is called by the ruler. We should fix it.
…afana#2980) * Distributor push wrapper should only receive unforwarded samples. Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
What this PR does
Which issue(s) this PR fixes or relates to
According to documentation
DistributorPushWrappershould only receive samples that were not forwarded by distributor. PR #2603 changed this andDistributorPushWrapperreceived all samples before any Distributor's middleware could do anything with it. This PR changes the behaviour back so thatDistributorPushWrapperreceives only non-forwarded samples, and changes behaviour such that HA-deduplicated samples or samples outside of limits are also not sent toDistributorPushWrapper.Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]