-
Notifications
You must be signed in to change notification settings - Fork 524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Distributor push wrapper should only receive unforwarded samples. #2980
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to leave it as it is if you disagree with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for addressing my feedback
…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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here: #3462
…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
DistributorPushWrapper
should only receive samples that were not forwarded by distributor. PR #2603 changed this andDistributorPushWrapper
received all samples before any Distributor's middleware could do anything with it. This PR changes the behaviour back so thatDistributorPushWrapper
receives only non-forwarded samples, and changes behaviour such that HA-deduplicated samples or samples outside of limits are also not sent toDistributorPushWrapper
.Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]