[query] Plumb potential tag completion/aggregate through to m3query endpoints #1481

arnikola · 2019-03-20T22:41:16Z

Adds new aggregate endpoint to Query

scripts/development/m3_stack/start_m3.sh

prateek · 2019-03-21T02:57:45Z

@arnikola I have a branch with some of these types defined too. Lemme put it up and we can merge the best features

prateek · 2019-03-21T03:06:16Z

@arnikola take a look at src/dbnode/client/types.go and src/dbnode/storage/index/types.go in #1483

prateek · 2019-03-21T03:06:56Z

src/dbnode/storage/index/types.go

@@ -60,6 +61,13 @@ type Query struct {
 	idx.Query
 }

+type AggregateQueryOptions struct {
+	QueryOptions


better to not embed the type here

prateek · 2019-03-21T03:07:29Z

src/dbnode/storage/index/types.go

+type AggregateQueryOptions struct {
+	QueryOptions
+
+	TagNameFilter      [][]byte


would be more flexible to inject a lambda here

prateek · 2019-03-21T03:08:02Z

src/dbnode/storage/index/types.go

+	QueryOptions
+
+	TagNameFilter      [][]byte
+	AggregateQueryType rpc.AggregateQueryType


Not a fan of using rpc generated types in the exported interface.

Yeah, agreed; was just throwing up something quick and dirty

prateek · 2019-03-21T03:08:40Z

src/dbnode/client/types.go

@@ -78,6 +78,10 @@ type Session interface {
 	// FetchTaggedIDs resolves the provided query to known IDs.
 	FetchTaggedIDs(namespace ident.ID, q index.Query, opts index.QueryOptions) (iter TaggedIDsIterator, exhaustive bool, err error)

+	// FetchTags resolves the optionally provided query to any known tag matchers.
+	// NB: this is a provisional function until the `session` contract is fully defined
+	FetchTags(namespace ident.ID, q index.Query, opts index.AggregateQueryOptions) (tags FetchedTags, exhaustive bool, err error)


How would you feel about Aggregate instead?

also, re: the return type - would be better to use an Iterator - allows us to avoid a bunch of allocs

Sounds good, I like the iterator you added in #1483; re: Aggregate where do you think we change the context of the function from being a generic aggregation vs a tag completion; would that be entirely in query? Maybe meet halfway with AggregateTags 😛 ? We've already got tag specific stuff in this interface so wouldn't be a huge change, right?

benraskin92 · 2019-03-25T22:24:58Z

src/query/api/v1/handler/graphite/find.go

+	// NB: fine to overwrite any tags which were present in the `noChildren` map
+	for _, tag := range withChildren.CompletedTags {
+		for _, value := range tag.Values {
+			tagMap[string(value)] = true


can you just create a set here instead of the true/false thing? I think you can just do something like:

tagMap := make(map[string]struct{}, mapLength)

then just set it to an empty struct when you're looping through.

for _, value := range tag.Values { tagMap[string(value)] = struct{}{} }

No, I need to see if the given tag has children here so need the bool

benraskin92 · 2019-03-25T22:33:57Z

src/query/api/v1/handler/prometheus/common.go

+	for _, result := range results {
+		for _, tags := range result {
+			jw.BeginObject()
+			for _, tag := range tags.Tags.Tags {


tags.Tags.Tags tags on tags on tags - can we possibly come up with a better naming convention?

it's also a list of tags ;)

Agree we could do with a refactor but out of scope here

benraskin92 · 2019-03-25T22:35:57Z

src/query/api/v1/handler/prometheus/common.go

+			}
+			jw.EndObject()
+		}
+


remove newline

benraskin92 · 2019-03-25T22:50:26Z

src/query/api/v1/handler/graphite/find_parser.go

+	// NB: Filter will always be the second last term in the matchers, and the
+	// matchers should always have a length of at least 2 (term + terminator)
+	// so this is a sanity check and unexpected in actual execution.
+	filter := [][]byte{matchers[len(matchers)-2].Name}


Should you check to make sure len(matchers) > 2?

:| yeah so wrote the comment intending to do the check but ended up forgetting to haha

benraskin92 · 2019-03-26T13:27:14Z

src/query/api/v1/handler/graphite/find_parser.go

 	*xhttp.ParseError,
 ) {
 	values := r.URL.Query()
-	now := time.Now()
-	fromString, untilString := r.FormValue("from"), r.FormValue("until")


Why are we getting rid of the from and until? I thought the new index supports that.

Good one, for some reason decided these are always from time.Time{} until time.Now(), rather than by default (I think because we drop em later which was wrong too)

benraskin92 · 2019-03-26T13:34:57Z

src/query/api/v1/handler/prometheus/common.go

 func RenderSeriesMatchResultsJSON(
 	w io.Writer,
-	results []*storage.CompleteTagsResult,
+	results []models.Metrics,
+	dropRole bool,


Where is this coming from? Config?

Just auto-enabled for all prom requests, and auto-disabled for graphite.

Okay cool - maybe add a todo to config it. But at this point maybe removing some config stuff is the way to go..

benraskin92 · 2019-03-26T13:36:47Z

src/query/api/v1/handler/prometheus/remote/match.go

-	if renderErr := prometheus.RenderSeriesMatchResultsJSON(w, results); renderErr != nil {
-		logger.Error("unable to write matched series", zap.Error(renderErr))
-		xhttp.Error(w, renderErr, http.StatusBadRequest)
+	if err := prometheus.RenderSeriesMatchResultsJSON(w, results, true); err != nil {


I don't think we should drop any tags from the results unless that's considered default for Prom. Especially if there's no way for a user to control this.

Yeah, reluctantly agree... It's a weird tag that's tacked on by prom but will keep it around for the time being

benraskin92 · 2019-03-26T13:39:31Z

src/query/storage/index.go

+	// If no matchers provided, explicitly set this to an AllQuery
+	if len(matchers) == 0 {
+		return index.Query{
+			// TODO: change this to an idx.AllQuery: https://github.com/m3db/m3/pull/1478


This returns every id if no matchers are provided? That seems not safe.

It's the expected behaviour here

To expound:

If not providing any queries, we want to return the set of all possible tags, and all values that each of those tags can have. We'll be building functionality for this in the index which should actually end up more performant than the new tag completion endpoint (perhaps counterintuitively, but we can get huge gains here when we're not required to filter down to satisfy an arbitrary query)

Makes sense

benraskin92 · 2019-03-28T13:06:11Z

src/query/api/v1/handler/graphite/find_parser.go

@@ -49,46 +57,64 @@ func parseFindParamsToQuery(r *http.Request) (
 		untilString = "now"
 	}

-	from, err := graphite.ParseTime(
+	end, err := graphite.ParseTime(


Shouldn't this be start? And the other be end?

nit: maybe just leave it as from and until to be consistent with the URL parameter names.

Good catch; added it to the test.

benraskin92

LGTM

prateek · 2019-03-28T18:56:19Z

src/query/storage/m3/storage.go

+		return nil, errNoNamespacesConfigured
+	}
+
+	// TODO: (arnikola): implement a better lifecycle for incoming aggregated tag


do you want to do this in this PR?

super nit: TODO(arnikola): instead of TODO: (arnikola)

I think there's some low hanging fruit gains for query in general, but probably best not to add them here and risk weird pooling issues for now

prateek · 2019-03-28T18:56:32Z

src/query/storage/m3/storage.go

+	var mu sync.Mutex
+	aggIterators := make([]client.AggregatedTagsIterator, 0, initSize)
+	defer func() {
+		for _, it := range aggIterators {


don't you need to lock here?

I do indeed 👍

prateek · 2019-03-28T18:58:36Z

src/query/storage/m3/storage.go

+				name, values := aggTagIter.Current()
+				tagValues := make([][]byte, values.Remaining())
+				for j := 0; values.Next(); j++ {
+					// TODO: (arnikola) fix the lifecycle here so we don't have to clone


this feels like a redundant copy. you're making a local copy into storage.CompletedTag and then convert to models.Metrics above. can you do both in a single pass?

Yeah, looking at the tag aggregator again looks like we cast to string, so the copy is unnecessary. Likewise the lifecycle is a bit less janky then I was expecting; there's definitely work to do on pooling/lifecycle management in m3query but that's pretty universally true rather than just in this case

prateek · 2019-03-28T19:07:44Z

src/query/storage/index.go

+	// If no matchers provided, explicitly set this to an AllQuery
+	if len(matchers) == 0 {
+		return index.Query{
+			// TODO: change this to an idx.AllQuery: https://github.com/m3db/m3/pull/1478


is there something left server side before we can do this here?

Nope, wrote this before AllQuery was pushed and forgot to update it 👍

prateek · 2019-03-28T19:16:42Z

src/query/api/v1/handler/graphite/find_parser.go

 	"github.com/m3db/m3/src/query/storage"
 	"github.com/m3db/m3/src/query/util/json"
 	"github.com/m3db/m3/src/x/net/http"
 )

-func parseFindParamsToQuery(r *http.Request) (
-	*storage.FetchQuery,
+func parseFindParamsToQueries(r *http.Request) (


nit: could you add a comment w/ an example to explain expected input/output

nice, i like it.

prateek · 2019-03-28T19:17:56Z

src/query/api/v1/handler/graphite/find.go

 	if rErr != nil {
 		xhttp.Error(w, rErr.Inner(), rErr.Code())
 		return
 	}

 	opts := storage.NewFetchOptions()
-	result, err := h.storage.FetchTags(ctx, query, opts)
+	noChildrenResult, err := h.storage.CompleteTags(ctx, noChildrenQuery, opts)


sorry, no context - what's the difference between noChildrenResult and childrenResult

Probably easiest with an example:

Query is: query=foo.*

This will actually call two Aggregate queries on the session:
foo.*.*, which is withChildrenResult, and is translated(-ish) to:
[ { __g0__ =~ foo } , { __g1__ =~ .* }, { __g2__ =~ .* } ]

foo.*, which is noChildrenResult, and is translated(-ish) to:
[ { __g0__ =~ foo } , { __g1__ =~ .* }, { __g2__ !=~ .* } ],

Where =~ is the regex operator, and !=~ is the regex exclusion operator.
Then these two are combined to determine which fields have children, and which are leaf nodes.

Hopefully this makes sense, will add a comment to this effect

hm using your example of query=foo.*, why not just translate into a single aggregate query as follows: [ { __g0__ =~ foo } , { __g1__ =~ .* }, { __g2__ =~ .* } ]

it's exactly the same as the union currently done in a single pass afaict

actually, do you even need to use a regex operator for __g0__? that can be a straight up term operator

Good idea; added an exact matcher case for alphanumeric segments but will look into properly detecting regex-able statements

codecov · 2019-03-29T02:51:10Z

Codecov Report

Merging #1481 into master will increase coverage by 19.6%.
The diff coverage is 67.9%.

@@            Coverage Diff            @@
##           master   #1481      +/-   ##
=========================================
+ Coverage    51.4%     71%   +19.6%     
=========================================
  Files         307     852     +545     
  Lines       26019   72877   +46858     
=========================================
+ Hits        13394   51815   +38421     
- Misses      11669   17665    +5996     
- Partials      956    3397    +2441

Flag	Coverage Δ
#aggregator	`82.3% <ø> (+28.8%)`	⬆️
#cluster	`85.7% <ø> (-0.6%)`	⬇️
#collector	`63.7% <ø> (+24.5%)`	⬆️
#dbnode	`80.7% <ø> (+11.5%)`	⬆️
#m3em	`73.2% <ø> (-26.8%)`	⬇️
#m3ninx	`74.3% <ø> (?)`
#m3nsch	`51.1% <ø> (?)`
#metrics	`17.5% <ø> (ø)`	⬆️
#msg	`75% <ø> (?)`
#query	`66.5% <67.9%> (-8.2%)`	⬇️
#x	`76.2% <ø> (+35.2%)`	⬆️

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 2427bbb...8a5c8cc. Read the comment docs.

robskillington · 2019-03-29T02:55:45Z

src/query/api/v1/handler/graphite/find.go

+		var err error
+		terminatedResult, err = h.storage.CompleteTags(ctx, terminatedQuery, opts)
+		if err != nil {
+			multiErr = multiErr.Add(err)


nit: Need to do locking on this multiErr.

Instead perhaps just keep both separate then use a xerrors.FirstError(terminatedErr, childErr) instead of multiErr.FinalError()?

Ah my bad, we have another multiErr type somewhere which is locking; I like the FirstError suggestion more anyway 👍

robskillington · 2019-03-29T03:06:11Z

src/query/graphite/storage/converter.go

+	)
+
+	// NB: small optimization to use exact matcher rather than regex if only
+	// alphanumeric characters present.


Wouldn't it make more sense to do always try to do a glob and make glob return ([]byte, bool) and if it returns false to imply that it didn't need to turn the glob into a regexp then you can use a MatchEqual matcher?

I'm pretty sure that's how we used to do it (and helps in the case you use a non alphanumeric symbol but turns out that it's still not a regexp? (i.e. using the - or _ character).

robskillington · 2019-03-29T03:09:29Z

src/query/storage/index.go

 		return index.Query{Query: val}, nil
 	}

+	cache.RUnlock()


Can you simplify the above to not have to do two RUnlocks?

k := queryKey(matchers) cache.RLock() val, ok := cache.get(k) cache.RUnlock() if ok { return index.Query{Query: val}, nil } //....

Sure, looks better

robskillington · 2019-03-29T03:15:01Z

src/query/storage/m3/storage.go

+	}
+
+	var mu sync.Mutex
+	aggIterators := make([]client.AggregatedTagsIterator, 0, initSize)


Why don't you make this len(namespaces) since you get back one for each namespace?

Nice, good call

robskillington · 2019-03-29T03:15:49Z

src/query/storage/m3/storage.go

+	defer func() {
+		mu.Lock()
+		for _, it := range aggIterators {
+			it.Finalize()


Will this actually finalize all bytes from all the results that came back? If so, we need to copy them all below (right now we're just taking ref's to the bytes).

I initially was cloning the returned bytes, but if you look into how completed tags builder works, it should already copy the bytes when taking a string

It's a bit janky but should hold up

robskillington · 2019-03-29T03:16:19Z

src/query/storage/m3/storage.go

+				name, values := aggTagIter.Current()
+				tagValues := make([][]byte, values.Remaining())
+				for j := 0; values.Next(); j++ {
+					tagValues[j] = values.Current().Bytes()


I think this needs to be a copy of the bytes if we're finalizing these results no?

Same deal here where the accumulator is casting to strings; I'll put up a test to double check though

robskillington · 2019-03-29T03:16:21Z

src/query/storage/m3/storage.go

-				if !found {
-					continue
+				completedTags[i] = storage.CompletedTag{
+					Name:   name.Bytes(),


I think this needs to be a copy of the bytes if we're finalizing these results no?

robskillington · 2019-03-29T16:17:57Z

src/query/api/v1/handler/graphite/find.go

+		terminatedResult *storage.CompleteTagsResult
+		tErr             error
+		childResult      *storage.CompleteTagsResult
+		cErr             error


nit: You could make this (to slightly compact):

var ( terminateResult, childResult *storage.CompleteTagsResult termErr, childErr error // ... )

Actually not a fan of doing it like that, looks messier to me :(

If it's a standard we're using will update though 👍

robskillington · 2019-03-29T16:23:43Z

src/query/api/v1/handler/graphite/find.go

+	}
+
+	mapLength := len(terminatedResult.CompletedTags) + len(childResult.CompletedTags)
+	tagMap := make(map[string]bool, mapLength)


nit: In future we should use an autogen'd map that can use []byte as the key and we could then use SetUnsafe(key, ..., Options{NoCopyKey: true, NoFinalizeKey: true}).

prateek · 2019-03-29T18:07:05Z

src/query/storage/validator/storage.go

 	ctx context.Context,
 	query *storage.FetchQuery,
 	_ *storage.FetchOptions,
 ) (*storage.SearchResults, error) {
-	return nil, errors.New("FetchTags not implemented")
+	return nil, errors.New("SearchSeries not implemented")


what's the deal with this storage type?

It's a weird handrolled mock; we should probably strip it and get a real mock in instead

prateek · 2019-03-29T18:14:48Z

src/query/api/v1/handler/graphite/find_parser.go

+func parseFindParamsToQueries(r *http.Request) (
+	*storage.CompleteTagsQuery,
+	*storage.CompleteTagsQuery,
+	string,


nit: what is this string supposed to be?

i've taken to giving returned variables name for this reason in the method spec (but no naked returns in the code itself). i.e. something like:

func parseFindParamsToQueries(r *http.Request) ( terminatedQuery *storage.CompleteTagsQuery, childQuery *storage.CompleteTagsQuery, ? string, err *xhttp.ParseError, ) { values := r.URL.Query() query := values.Get("query") if query == "" { return nil, nil, "", xhttp.NewParseError(errors.ErrNoQueryFound, http.StatusBadRequest) // notice how this isn't just "return" }

prateek · 2019-03-29T18:23:59Z

src/query/api/v1/handler/prometheus/common.go

 func ParseTagCompletionParamsToQuery(
 	r *http.Request,
 ) (*storage.CompleteTagsQuery, *xhttp.ParseError) {
-	tagQuery := storage.CompleteTagsQuery{}
+	tagQuery := storage.CompleteTagsQuery{
+		Start: time.Time{},


just making sure this is the default that can be overriden by the parameters passed over http

It can not... fixing

prateek · 2019-03-29T18:27:14Z

src/query/api/v1/handler/prometheus/common.go

@@ -191,7 +196,7 @@ func ParseSeriesMatchQuery(
 		return nil, xhttp.NewParseError(err, http.StatusBadRequest)
 	}

-	tagMatchers := make([]models.Matchers, len(matcherValues))
+	queries := make([]*storage.FetchQuery, len(matcherValues))


on line 189 - what's with the magic numbers? (unrelated to this change)

Was defaulting to 40 days as query start for ...reasons; changed to go from 0 until Now by default

prateek · 2019-03-29T18:28:19Z

src/query/api/v1/handler/prometheus/common.go

@@ -52,14 +53,15 @@ const (

 var (
 	matchValues = []byte(".*")
+	roleName    = []byte("role")


hm this feels like a weird thing to modify a metric with. won't it conflict with people's tags?

yeah, I've disabled stripping it for now

Prom does some dumb thing where it adds a "role":"remote" tag to each metric we get from the remote write endpoint which is questionable, but probably safer to leave it in.

yeah, i'd avoid doing anything magicy to the metrics. can add down the road behind config if it's really required

prateek · 2019-03-29T18:31:18Z

src/query/api/v1/handler/prometheus/common.go

 func RenderSeriesMatchResultsJSON(
 	w io.Writer,
-	results []*storage.CompleteTagsResult,
+	results []models.Metrics,


any reason to use custom encoder here? could we get away with just creating the right json object and using the default encoder

Historical reasons, mostly? We use this hand rolled encoder everywhere at the moment but should definitely look into changing this and using something sane instead

arnikola requested review from prateek, robskillington, andrewmains12 and benraskin92 March 20, 2019 22:41

andrewmains12 reviewed Mar 20, 2019

View reviewed changes

scripts/development/m3_stack/start_m3.sh Outdated Show resolved Hide resolved

prateek reviewed Mar 21, 2019

View reviewed changes

arnikola mentioned this pull request Mar 21, 2019

Poor performance for tag completion endpoints #1453

Closed

6 tasks

benraskin92 reviewed Mar 25, 2019

View reviewed changes

benraskin92 reviewed Mar 26, 2019

View reviewed changes

arnikola mentioned this pull request Mar 27, 2019

[index] Standardizes aggregate interfaces #1502

Merged

arnikola force-pushed the arnikola/query-tag-completion branch from 98c0729 to 0c605e6 Compare March 27, 2019 20:13

benraskin92 reviewed Mar 28, 2019

View reviewed changes

arnikola mentioned this pull request Mar 28, 2019

api/v1/series returns wrong series with certain queries #1501

Closed

benraskin92 approved these changes Mar 28, 2019

View reviewed changes

prateek reviewed Mar 28, 2019

View reviewed changes

robskillington reviewed Mar 29, 2019

View reviewed changes

m3db deleted a comment from benraskin92 Mar 29, 2019

m3db deleted a comment from arnikola Mar 29, 2019

m3db deleted a comment from benraskin92 Mar 29, 2019

robskillington reviewed Mar 29, 2019

View reviewed changes

arnikola force-pushed the arnikola/query-tag-completion branch from 8f97936 to 65a5523 Compare March 29, 2019 16:49

arnikola changed the base branch from master to prateek/dbnode/aggregate-rpc March 29, 2019 17:55

arnikola force-pushed the arnikola/query-tag-completion branch from 65a5523 to 74a34b4 Compare March 29, 2019 18:05

prateek reviewed Mar 29, 2019

View reviewed changes

arnikola force-pushed the arnikola/query-tag-completion branch from 74a34b4 to dc84075 Compare March 29, 2019 19:16

arnikola changed the base branch from prateek/dbnode/aggregate-rpc to master March 29, 2019 20:05

[query] Add new aggregate endpoint to query

c535a7d

arnikola force-pushed the arnikola/query-tag-completion branch from dc84075 to c535a7d Compare March 29, 2019 20:14

Emit values in aggregator test until termination of test

8a5c8cc

arnikola merged commit ad005eb into master Mar 29, 2019

arnikola deleted the arnikola/query-tag-completion branch March 29, 2019 20:49

[query] Plumb potential tag completion/aggregate through to m3query endpoints #1481

[query] Plumb potential tag completion/aggregate through to m3query endpoints #1481

Conversation

arnikola commented Mar 20, 2019 • edited Loading

prateek commented Mar 21, 2019

prateek commented Mar 21, 2019

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

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

benraskin92 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

arnikola Mar 28, 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

codecov bot commented Mar 29, 2019 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

arnikola Mar 29, 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

arnikola commented Mar 20, 2019 •

edited

Loading

arnikola Mar 28, 2019 •

edited

Loading

codecov bot commented Mar 29, 2019 •

edited

Loading

arnikola Mar 29, 2019 •

edited

Loading