Limit series from clusterByFind operation #1021

shanson7 · 2018-08-28T14:16:06Z

Looking through the node::Post code, it seems like we could save quite a lot by marshaling directly from the response body into the msgp generated type. My C++ brain wants to use templates to accomplish it, so I'm not sure the cleanest method to use in go without generics. For that reason, I punted on that for now.

woodsaj · 2018-08-28T15:34:35Z

api/graphite.go

+
+ // 0 disables the check, so only check if maxSeriesPerReq > 0
+ if maxSeriesPerReq > 0 && len(resp.Metrics)+len(allSeries) > maxSeries {
+ return nil,


Shouldn't you cancel ctx so outstanding peer queries are aborted?

Technically, the query will be Doned when the error propagates up and a response is returned. But canceling it here could do it a little bit earlier.

I'm also not entirely sure how to cancel this context here. I suppose we would need to make a new cancelable context here.

that makes sense I think. Since we only get the ctx passed in, we don't know where what the cancelfunc was.
However, if you do something like newCtx, cancel := context.WithCancel(ctx) and pass newCtx into peerQuerySpeculativeChan, then you can cancel it.

shanson7 · 2018-08-28T23:30:15Z

I plan on following up this change with a version that decodes directly into a msgp.Decodable type. As part of this change, it might be easy to support local processing as well.

shanson7 · 2018-08-30T17:44:36Z

See master...bloomberg:directDecode for a version of this change that includes local request processing and decoding straight from the http.Response into a msgp.Decodable type. This should reduce the temporary memory usage from reading into a []byte and then unmarshalling.

Dieterbe · 2018-09-17T07:56:58Z

api/cluster.go

@@ -294,7 +294,7 @@ func (s *Server) peerQuery(ctx context.Context, data cluster.Traceable, name, pa
 result := make(map[string]PeerResponse)
 for resp := range responses {
 if resp.err != nil {
- return nil, err
+ return nil, resp.err


Dieterbe · 2018-09-17T08:16:47Z

api/cluster.go

- log.Debug("HTTP Render querying %s%s", peer.GetName(), path)
- buf, err := peer.Post(reqCtx, name, path, data)
+// peerQuerySpeculativeChan takes a request and the path to request it on, then fans it out
+// across the cluster. If any peer fails requests to other peers are aborted. If enough


I notice peerQuerySpeculative description includes "...except to the local peer...", whereas this one does not.
i think the former is wrong

Dieterbe · 2018-09-17T08:22:00Z

api/graphite.go

+ // 0 disables the check, so only check if maxSeriesPerReq > 0
+ if maxSeriesPerReq > 0 && len(resp.Metrics)+len(allSeries) > maxSeries {
+ return nil,
+ response.NewError(413, fmt.Sprintf("Request exceeds max-series-per-req limit (%d). Reduce the number of targets or ask your admin to increase the limit.", maxSeriesPerReq))


please use the http.StatusXxx constants
in this case http.StatusRequestEntityTooLarge

Dieterbe · 2018-09-17T08:28:47Z

api/cluster.go

- }
+ go func() {
+ defer close(errorChan)
+ defer close(resultChan)


minor thought: would it be cleaner/simpler to just have 1 return channel? we could return PeerResponse along with its error

Possibly. I like the 2 channels, because you can just loop over responses until it is closed, then quickly try to read into an err var for the return value. See this usage

fair enough

Dieterbe · 2018-09-17T08:35:10Z

See master...bloomberg:directDecode for a version of this change that includes local request processing and decoding straight from the http.Response into a msgp.Decodable type. This should reduce the temporary memory usage from reading into a []byte and then unmarshalling.

sounds great. but I suggest we first merge this, and then new PR for that work.

Do you have interesting observations/stats you can share with us?
I assume "decrease in OOM's" would be one, but anything else? decreased heap usage? reduced allocation rate? I guess it's hard to separate the effects from the new limit parameter (which causes MT to do less work) vs the new channeling approach (which should make it work more efficiently)
Is this, in your experience, safe to go to prod?

Dieterbe

looks pretty good. few minor tweaks needed

shanson7 · 2018-09-24T15:28:34Z

I definitely saw a reduction in short-term heap spikes, but overall heap usage stayed about the same. We've been running this in prod for about 2 months now, without any issues.

Dieterbe · 2018-10-10T12:22:05Z

@shanson7 @woodsaj what do you think of the cancellation i added in the last commit?
I think this was the last item to do before we can merge this.

if looks good, can you rebase on master (I didn't want to force push into your branch, not sure if i even can). then i'll merge.

shanson7 · 2018-10-10T13:43:11Z

api/graphite.go


 var allSeries []Series

 for r := range responseChan {
 resp := models.IndexFindByTagResp{}
 _, err := resp.UnmarshalMsg(r.buf)
 if err != nil {
+ cancel()


Would it make sense to just defer cancel() at the top like the peerQuery functions do?

Also, how much does this canceling really buy us if the top-level ctx will be Done when the error is returned?

not sure what you mean with a ctx "being Done".
ctx.Done() just returns a channel that signals cancellation (by getting closed), but you still need to call a corresponding cancel function for the cancellation to happen.

I see 3 callers of this function:

/render -> renderMetrics -> executePlan

/tags/findSeries (graphiteTagFindSeries)

querier.Select

we would need all 3 to call a cancelfunc for my change to be unnecessary.
but from what i can tell, it looks like neither of them, or at least not the first two, do this. (unless macron automatically calls cancel on our behalf?)

but regardless, it shouldn't take much code spelunking for reader to assert whether we cancel a context when we should. so i rather call cancel wherever it seems sensible, even if we call it multiple times (which is harmless).
I think @woodsaj was trying to establish this convention when he first added cancellations throughout this code base.

as for using defer vs just putting the code at the return sites, for short functions it's really debatable and depends more on taste. so unless you have a strong argument, let's just stick with it.

Re: Done

I mean that peerQuerySpeculativelyChan checks if the context is Done() https://github.com/grafana/metrictank/pull/1021/files#diff-bc8a656be21edce0cf2a74adf23d7aeaR401

Done isn't just done on cancel, but also on a final response being returned. So, as soon as the error propagates up and a response is delivered, peerQuerySpeculativelyChan will break its loop and call cancel(). This is done specifically so callers don't need to implement cancellation themselves. Granted, doing it directly will cancel a few milliseconds earlier, so might be marginally cheaper.

the cancels that i added in clusterFindByTag target cases that peerQuerySpeculativeChan cannot detect itself. these are :

unmarshalling error

exceeding maxSeriesPerReq

the cancels triggered in peerQuerySpeculativeChan (upon erroring peer.Post and upon function return) would be triggered not all, or much later, respectively.

the cancels that i added in clusterFindByTag target cases that peerQuerySpeculativeChan cannot detect itself.

True, but as long as these failures cause an error response to the HTTP request, the parent context's Done channel will be closed and peerQuerySpeculativeChan will short-circuit and will call cancel on the context it created.

This is the case in the 3 examples you posted, but I guess it's too much for clusterFindByTag to assume?

the parent context's Done channel will be closed

where does this happen?

TBH, I didn't come to this conclusion by code inspection, but rather by logging when early responses happened. It definitely was how it behaved, but I couldn't tell you if it's by contract or a symptom of how I was querying

I see, well. this reinforces my point that these matters should be made much more obvious. hence rather a call to cancel more rather than too few.
good to merge?

Yeah, let me rebase

shanson7 · 2018-10-10T21:44:47Z

LGTM. I see what looks like an unrelated failure in the tests.

Dieterbe · 2018-10-11T09:39:45Z

yes, this failure is due to the flakey TimeLimiter tests. see above PR.

Dieterbe · 2018-10-11T09:54:06Z

nice work sean!

woodsaj reviewed Aug 28, 2018

View reviewed changes

Dieterbe reviewed Sep 17, 2018

View reviewed changes

Dieterbe suggested changes Sep 17, 2018

View reviewed changes

shanson7 commented Oct 10, 2018

View reviewed changes

shanson7 and others added 11 commits October 10, 2018 13:40

Limit the number of series pulled in from peers

c4d89f6

Add new param to config files

82ddd58

Update default

44d9fa5

Use cleaner async paradigm

451add4

Clean up error check of buffered channel

6b2d9d2

Put defers in the right spots

e8abe52

Slightly cheaper method for clearing out response buffer, plus rationale

dbdaab1

Use config-to-doc.sh

b9a040a

Use http constants, fix speculative docs

6bc0d04

typo

b606b16

cancel() when it makes sense

fa4cc1e

shanson7 force-pushed the limitSeriesChan branch from 80afcab to fa4cc1e Compare October 10, 2018 17:52

Fix log statements

02204ee

Dieterbe mentioned this pull request Oct 11, 2018

simplify TimeLimiter, fixing its tests #1088

Merged

Dieterbe approved these changes Oct 11, 2018

View reviewed changes

Dieterbe merged commit 8194bd7 into grafana:master Oct 11, 2018

Dieterbe mentioned this pull request Oct 11, 2018

streaming processing and series limiting on other cluster calls #1092

Closed

shanson7 deleted the limitSeriesChan branch March 6, 2019 14:27

shanson7 restored the limitSeriesChan branch March 6, 2019 14:27

shanson7 deleted the limitSeriesChan branch December 24, 2019 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit series from clusterByFind operation #1021

Limit series from clusterByFind operation #1021

shanson7 commented Aug 28, 2018

woodsaj Aug 28, 2018

shanson7 Aug 28, 2018 •

edited

shanson7 Aug 28, 2018

Dieterbe Sep 17, 2018

shanson7 commented Aug 28, 2018

shanson7 commented Aug 30, 2018

Dieterbe Sep 17, 2018

Dieterbe Sep 17, 2018

Dieterbe Sep 17, 2018

Dieterbe Sep 17, 2018

shanson7 Sep 24, 2018

Dieterbe Oct 10, 2018

Dieterbe commented Sep 17, 2018 •

edited

Dieterbe left a comment

shanson7 commented Sep 24, 2018

Dieterbe commented Oct 10, 2018 •

edited

shanson7 Oct 10, 2018

Dieterbe Oct 10, 2018 •

edited

shanson7 Oct 10, 2018

Dieterbe Oct 10, 2018

shanson7 Oct 10, 2018

Dieterbe Oct 10, 2018

shanson7 Oct 10, 2018

Dieterbe Oct 10, 2018

shanson7 Oct 10, 2018

shanson7 commented Oct 10, 2018

Dieterbe commented Oct 11, 2018

Dieterbe commented Oct 11, 2018

Limit series from clusterByFind operation #1021

Limit series from clusterByFind operation #1021

Conversation

shanson7 commented Aug 28, 2018

Choose a reason for hiding this comment

shanson7 Aug 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanson7 commented Aug 28, 2018

shanson7 commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dieterbe commented Sep 17, 2018 • edited

Dieterbe left a comment

Choose a reason for hiding this comment

shanson7 commented Sep 24, 2018

Dieterbe commented Oct 10, 2018 • edited

Choose a reason for hiding this comment

Dieterbe Oct 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanson7 commented Oct 10, 2018

Dieterbe commented Oct 11, 2018

Dieterbe commented Oct 11, 2018

shanson7 Aug 28, 2018 •

edited

Dieterbe commented Sep 17, 2018 •

edited

Dieterbe commented Oct 10, 2018 •

edited

Dieterbe Oct 10, 2018 •

edited