[query] Add config for timeseries limit returned by single DB node #1644

robskillington · 2019-05-17T21:08:05Z

What this PR does / why we need it:

Adds a config and a sane default for setting a max timeseries limit returned by a single DB node to m3query.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

…le DB node for M3 query queries

benraskin92 · 2019-05-17T21:15:03Z

src/cmd/services/m3query/config/config.go

+		return handler.FetchOptionsBuilderOptions{
+			Limit: defaultStorageQueryLimit,
+		}
+	}


nit: newline

benraskin92 · 2019-05-17T21:19:01Z

src/cmd/services/m3query/config/config.go

 	MaxFetchedDatapoints int64 `yaml:"maxFetchedDatapoints"`
+
+	// MaxTimeseries limits the number of time series returned by a storage node.
+	MaxTimeseries int64 `yaml:"maxTimeseries"`


maxFetchedTimeseries instead to be consistent with maxFetchedDatapoints?

benraskin92 · 2019-05-17T21:21:13Z

src/query/api/v1/handler/fetch_options.go

+			return nil, xhttp.NewParseError(err, http.StatusBadRequest)
+		}
+		fetchOpts.Limit = n
+	}


nit: newline

benraskin92 · 2019-05-17T21:21:34Z

src/query/api/v1/handler/fetch_options.go

+) (*storage.FetchOptions, *xhttp.ParseError) {
+	fetchOpts := storage.NewFetchOptions()
+	fetchOpts.Limit = b.opts.Limit
+	if str := req.Header.Get(LimitMaxTimeseriesHeader); str != "" {


Good call on making this part of the request. Although does it make more sense to have this be part of the url parameters? More of an educational question for me..

It does make more sense to be a URL parameter but I wanted this to be a part of any endpoint. So I didn't want to necessarily have one endpoint using "limit" for something other than what I'm specifying then have them collide.

We can always go add a URL param later if we think we can avoid collisions, this was just a quick way to add it for now.

richardartoul

Stamp to unblock, but a few things in there worth cleaning up.

Also:

I really think we should have a small doc on how to set these if we don't already
Any thoughts on tests for this? Not obvious how to me how you would write good tests for this at the coordinator level so happy to merge for now, but something to think about

richardartoul · 2019-05-17T21:34:57Z

src/query/api/v1/handler/fetch_options.go

+) (*storage.FetchOptions, *xhttp.ParseError) {
+	fetchOpts := storage.NewFetchOptions()
+	fetchOpts.Limit = b.opts.Limit
+	if str := req.Header.Get(LimitMaxTimeseriesHeader); str != "" {


Do we have any documentation right now on setting query limits and such? If not would be nice to add a simple document that outlines the YAML configs you can set as well as this head.

Yeah, let me look around for it. @benraskin92 do you know of any?

richardartoul · 2019-05-17T21:36:49Z

src/query/api/v1/handler/headers.go

 	DeprecatedHeader = "M3-Deprecated"

+	// LimitMaxTimeseriesHeader is the M3 limit timeseries header that limits
+	// the number of time series returned by each storage node.
+	LimitMaxTimeseriesHeader = "M3-Limit-Max-Timeseries"


I think M3-Limit-Max-Series or just M3-Max-Series to be easier to remember

richardartoul · 2019-05-17T21:37:49Z

src/query/api/v1/handler/prometheus/native/list_tags.go

@@ -48,13 +48,15 @@ var (

 // ListTagsHandler represents a handler for list tags endpoint.
 type ListTagsHandler struct {
-	storage storage.Storage
-	nowFn   clock.NowFn
+	storage             storage.Storage


This new limit wont affect aggregate endpoints will it?

It will too yeah.

But will correctly respect the size of the aggregate values, not the amount of documents scanned.

richardartoul · 2019-05-17T21:40:39Z

src/query/api/v1/handler/search.go

+	query, parseBodyErr := h.parseBody(r)
+	opts, parseURLParamsErr := h.parseURLParams(r)
+	// NB(r): Use a loop here to avoid two err handling code paths
+	for _, rErr := range []*xhttp.ParseError{parseBodyErr, parseURLParamsErr} {


Took me longer to digest this than two error handlings paths, and it possibly allocates? Would probably just do the normal thing here personally or at least do

for _, rErr := range [2]*xhttp.ParseError{parseBodyErr, parseURLParamsErr} { ... }

to be sure it does not allocate

Sure, I might actually just do a helper function like I have in xerrors.FirstError (only reason I didn't do that is that it expects func FirstError(err ...error) error but these are typed errors....

richardartoul · 2019-05-17T21:44:08Z

src/query/api/v1/handler/search.go

+	}
+
+	if str := r.URL.Query().Get("limit"); str != "" {
+		if limit, err := strconv.Atoi(str); err == nil {


honestly if they pass the correct query URL name but we can't parse it into a number, it should just return an error

Yeah agreed, will change what this behavior used to do to match that.

richardartoul · 2019-05-17T21:45:43Z

src/query/models/query_context.go

+	Options  QueryContextOptions
+}
+
+// QueryContextOptions is a set of optionally set options for the query


QueryContextOptions contains optional configuration for the query context

richardartoul · 2019-05-17T21:50:04Z

src/query/server/server.go

+		queryContextOptions, poolWrapper)
+
+	var (
+		waitForStart = make(chan struct{}, 1)


Can you just make this a chan error and get rid of the startErr atomic stuff? Seems confusing for no reason.

// StartNewGrpcServer starts server on given address, then notifies channel func StartNewGrpcServer( server *grpc.Server, address string, waitForStart chan<- error{}, ) error { lis, err := net.Listen("tcp", address) if err != nil { waitForStart <-err } waitForStart <- nil return server.Serve(lis) }

The whole thing is kinda messed up, I wanted to introduce least amount of changes possible.

I'll just break it up into two steps and make the grpc server take the listener.

richardartoul · 2019-05-17T21:50:34Z

src/query/server/server.go

+		wg           sync.WaitGroup
+	)
+	startErr.Store(nil)
+	wg.Add(1)


I dont think you actually use this

Ta, I'm going to rip all this stuff out thankfully - good catch though.

codecov · 2019-05-18T02:24:12Z

Codecov Report

Merging #1644 into master will increase coverage by 0.9%.
The diff coverage is 69.2%.

@@           Coverage Diff            @@
##           master   #1644     +/-   ##
========================================
+ Coverage    71.3%   72.2%   +0.9%     
========================================
  Files         962     962             
  Lines       80908   80103    -805     
========================================
+ Hits        57747   57903    +156     
+ Misses      19393   18409    -984     
- Partials     3768    3791     +23

Flag	Coverage Δ
#aggregator	`82.3% <ø> (-0.1%)`	⬇️
#cluster	`85.7% <ø> (ø)`	⬆️
#collector	`63.9% <ø> (ø)`	⬆️
#dbnode	`80.1% <ø> (ø)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.1% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.7% <ø> (-0.2%)`	⬇️
#query	`67.9% <69.2%> (+4.1%)`	⬆️
#x	`86.4% <ø> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ec3b0b...5887d97. Read the comment docs.

…nfig-limit

…n test

Add config and defaults for max timeseries limit returned from a sing…

2d91b35

…le DB node for M3 query queries

benraskin92 reviewed May 17, 2019

View reviewed changes

richardartoul approved these changes May 17, 2019

View reviewed changes

Fix disabled tests

30c9b1d

robskillington added 10 commits May 18, 2019 00:17

Merge branch 'master' into r/add-config-limit

5ce21d1

Add tests for fetchoptionsbuilder

a6a64fd

Merge branch 'r/add-config-limit' of github.com:m3db/m3 into r/add-co…

7722f8a

…nfig-limit

Fix build errors

52f22fc

Fix tests in native package

2028926

Add tests to default query limits to the prometheus docker integratio…

bae3053

…n test

Fix comments

03773a7

Fix simple docker integration test

3aeeaf3

Optionally use max timeout for the docker integration tests

4888588

Bump docker timeouts to avoid early failures

5887d97

robskillington merged commit 9ce4743 into master May 18, 2019

robskillington deleted the r/add-config-limit branch May 18, 2019 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Add config for timeseries limit returned by single DB node #1644

[query] Add config for timeseries limit returned by single DB node #1644

robskillington commented May 17, 2019 •

edited

Loading

benraskin92 May 17, 2019

benraskin92 May 17, 2019

benraskin92 May 17, 2019

benraskin92 May 17, 2019

robskillington May 18, 2019

richardartoul left a comment

richardartoul May 17, 2019

robskillington May 18, 2019

richardartoul May 17, 2019

richardartoul May 17, 2019

robskillington May 18, 2019

robskillington May 18, 2019

richardartoul May 17, 2019

robskillington May 17, 2019

richardartoul May 17, 2019

robskillington May 17, 2019

richardartoul May 17, 2019

richardartoul May 17, 2019

robskillington May 17, 2019

richardartoul May 17, 2019

robskillington May 17, 2019

codecov bot commented May 18, 2019 •

edited

Loading

[query] Add config for timeseries limit returned by single DB node #1644

[query] Add config for timeseries limit returned by single DB node #1644

Conversation

robskillington commented May 17, 2019 • edited Loading

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

richardartoul left a 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

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

Choose a reason for hiding this comment

codecov bot commented May 18, 2019 • edited Loading

Codecov Report

robskillington commented May 17, 2019 •

edited

Loading

codecov bot commented May 18, 2019 •

edited

Loading