Tag index by replay · Pull Request #729 · grafana/metrictank

replay · 2017-09-14T16:05:00Z

Adds a tag index, together with the functions to query it.
It does not expose any API endpoints yet, those will follow in a separate PR (from branch https://github.com/grafana/metrictank/compare/tag_api_endpoints).

DanCech · 2017-09-14T16:23:35Z

+			tags[tagSplits[0]] = make(map[string][]string)
+		}
+
+		tags[tagSplits[0]][tagSplits[1]] = append(tags[tagSplits[0]][tagSplits[1]], def.Name)


Do we need to init tags[tagSplits[0]][tagSplits[1]] first? this code will be much more readable if you say tagName := tagSplits[0] tagValue := tagSplits[1]

It shouldn't be necessary to init slices i think, only maps. Ok, i'll name them

shanson7 · 2017-09-14T16:38:24Z

+	return m.idsByTagQuery(tree, query)
+}
+
+func (m *MemoryIdx) idsByTagQuery(tags map[string]map[string][]string, query tag.TagQuery) []string {


Is this function supposed to be returning the ids that match ANY expression or ALL expressions? Seems like it's ANY right now. I suppose this function is designed to be composable to support ALLs?

@replay was confused, it should be ALL. He's sorting that out now.

From an end-user perspective, ANY matches can be done via a combination of using regular expression matches and/or using multiple calls to seriesByTag and combining the results with group

DanCech · 2017-09-19T14:05:54Z

+		value := exprSplits[0][3]
+
+		// always anchor all regular expressions at the beginning
+		if operator[len(operator)-1] == byte('~') && len(value) > 0 && value[len(value)-1] != byte('^') {


value[0] != byte('^')

DanCech · 2017-09-19T14:06:23Z

+
+		// special case of empty value
+		if len(value) == 0 {
+		}


this is a no-op

yeah, still going to add that. the idea is that i'll just reverse the operation.

so key!=~ becomes key=~.* and key=~ becomes key!=~.*

then, further down where the matching is done, i could also shortcut it to not even do the matching if the pattern is .* or .+. (for .+ this works assuming that no value that's in the index can be "")

DanCech · 2017-09-19T14:11:52Z

+					return ids, nil
+				}
+				return make(map[string]struct{}), nil
+			})


Can't this just be return idx[key][value], nil?

DanCech · 2017-09-19T14:14:03Z

+					return nil, err
+				}
+
+				for v, ids := range values {


same question, it seems like this could just be for v, ids := range idx[key] {

DanCech · 2017-09-20T21:39:17Z

+			return nil, errInvalidQuery
+		}
+
+		exprSplits := expressionRe.FindAllStringSubmatch(expr, -1)


https://golang.org/pkg/regexp/#Regexp.FindStringSubmatch

DanCech · 2017-09-20T21:52:59Z

+				value = ".+"
+			}
+		}
+


I think we can do better in handling terms that match the empty string, since we have a requirement that there must be a tag expression that doesn't match the empty string. Because of that requirement, we should be able to make a list of candidate series from the expression that requires a match, then use those as the list of "all" series that should be considered to match if they don't have the tags that we're matching the empty string for. I didn't implement that concept in the graphite tagdbs, but I'm going to go back and see whether I can use it.

so i'm not completely sure i understand you right. but actually i think this already does what you describe:

further down queries are divided into "selects" and "filters". any query that's excluding something is a "filter", any query that's selecting by a match is a "select".

a query like key= is translated into key!=.+, this is a filter

the "select" queries are always executed first, building that list of candidates you mention

after the list of candidates is built, the "filters" are applied

we can further speed this up by not even matching the regex .+ because we can't have empty tag values in the index. so every series that has the tag key will match the value .+, so we can shortcut it and skip the regex match.

Dieterbe · 2017-09-30T17:42:16Z

FTR: prior discussion: #532
feel free to link to any other tickets or discussions

Dieterbe · 2017-10-01T12:28:15Z

+		{"key1=~value[0-9]", "key2=~", "key3!=value3"},
+		{"key2=", "key1=value1"},
+	}
+	expecting := []int{3, 1, 4, 1, 0, 1, 2, 0, 1, 2}


let's make 1 slice of cases, where each case shows its expression and which metrics are expected to match.
just checking the number of matches can hide bugs, is hard for readers, and having 2 structures is a bit confusing

Dieterbe · 2017-10-01T12:45:53Z

+
+func (c *CasIdx) IdsByTagExpressions(orgId int, expressions []string) ([]string, error) {
+	return c.MemoryIdx.IdsByTagExpressions(orgId, expressions)
+}


these additions should not be necessary. go compiler should automatically wire these through since MemoryIdx is embedded into the CasIdx struct (that's why Find, List, etc work also on this index)

Dieterbe · 2017-10-01T12:48:18Z


-	Enabled bool
+	Enabled    bool
+	TagSupport bool


don't need to export this i think

Dieterbe · 2017-10-01T12:50:17Z

+func (m *MemoryIdx) addTags(def *schema.MetricDefinition) {
+	tags, ok := m.Tags[def.OrgId]
+	if !ok {
+		tags = make(map[string]map[string]map[string]struct{})


can make(TagIndex) here

Dieterbe · 2017-10-01T12:51:38Z

+		tagValue := tagSplits[1]
+
+		if _, ok = tags[tagName]; !ok {
+			tags[tagName] = make(map[string]map[string]struct{})


maybe we should give these substructures a type, so that it's easier/clearer to create them

That might be good, but I can't come up with a good name :)
The top level structure is currently named TagIndex. I suggest the next one could be TagKey and it's nested values would be called TagValue. So each TagValue would then contain a set of IDs

how about:

TagIndex -> TagValues -> MetricDefinitions

Dieterbe · 2017-10-01T12:54:17Z

 	m.Unlock()
 }

+func (m *MemoryIdx) addTags(def *schema.MetricDefinition) {


func name is a bit confusing. we're indexing tags (or "adding tags to the index") for a metricdefinition that already has tags. so maybe call this indexTags or at least at a function comment explaining what this does. thanks :)

Dieterbe · 2017-10-01T12:55:51Z

+	}
+}
+
+func (m *MemoryIdx) delTags(def *schema.MetricDefinition) {


same here as above. clarify a bit via function comment, thanks (could be confused for "delete tags from a metric")

Dieterbe · 2017-10-01T14:25:03Z

my main concern here is the amount of strings used. every string has a pointer internally, and GC workload is directly proportional to amount of live pointers on the heap. and we already have a too-high number of strings and pointers due to legacy reasons (eg all the AggMetric stuff, and other stuff in the current index), which results in a high GC workload, which combined with golang/go#14812 results occassionally in poor latencies.

so, I think we need a way to verify if this concern holds any ground, with minimal time investment.

should be pretty trivial to spin up a docker stack, use fakemetrics for ingest workload, and vegeta for query workload, and compare both cases. I can help with the specifics, i've done this a bunch of times before. see also #440 and https://github.com/grafana/metrictank/blob/7a508e43e9b48af9ecdf1723c91dd75e1b4ca212/docker/docker-cluster/load.sh

Dieterbe · 2017-10-01T14:28:03Z

 	Prune(int, time.Time) ([]Archive, error)
+	TagList(int, uint32) []string
+	Tag(int, string) map[string]uint32
+	IdsByTagExpressions(int, []string) ([]string, error)


these 3 functions are not called by anything, it's not really clear what they are for

adding comments

Dieterbe · 2017-10-12T22:22:08Z

 	memoryIdx := flag.NewFlagSet("memory-idx", flag.ExitOnError)
 	memoryIdx.BoolVar(&Enabled, "enabled", false, "")
+	memoryIdx.BoolVar(&tagSupport, "tag-support", false, "")
+	memoryIdx.IntVar(&matchCacheSize, "match-cache-size", 1000, "")


please fill in help message, and add these settings, along with the same help message and default value to metrictank-sample.ini, then run the sync-configs script to apply it to the other files as well. and config-to-doc.sh

no need to loop twice no need to do map lookups if key doesn't even match only need to track values, not entire k=v pair

* add a corruption case metric+log * standardize the format of all recoverable internal errors makes a simpler, cleaner namespace, on which we can easily alert. * standardize terms: - corrupt for internal datastructures - invalid for user data * no need to export vars

shanson7 · 2017-10-13T21:37:33Z

@@ -328,11 +318,19 @@ func (q *TagQuery) filterByMatch(resultSet TagIDs, byId map[string]*idx.Archive,
 					continue
 				}


Should this just break? There shouldn't be another tag value for this tag key, right?

hm github is not showing anything besides the continue line. are you talking about the case where we found an entry in notMatchingTags ?

@shanson7 you're right. each def should only have one value per key

i gave the inner loop a name for consistency with the outer one: 6fb73bf

Dieterbe · 2017-10-13T21:44:38Z

@shanson7 this will be merged very soon now, unless you have any more feedback :)
main thing missing is we need to pull in the tag input validation raintank/schema#11

…tion

DanCech reviewed Sep 14, 2017

View reviewed changes

shanson7 reviewed Sep 14, 2017

View reviewed changes

replay force-pushed the tag_index branch 3 times, most recently from d7dcf96 to d375fb9 Compare September 15, 2017 10:14

DanCech reviewed Sep 19, 2017

View reviewed changes

DanCech reviewed Sep 20, 2017

View reviewed changes

replay force-pushed the tag_index branch 2 times, most recently from ebc48cf to e30aee4 Compare September 29, 2017 15:11

Dieterbe mentioned this pull request Sep 30, 2017

index lastUpdated timestamps are only stored on leaf nodes #714

Open

replay force-pushed the tag_index branch from e30aee4 to a321a31 Compare October 1, 2017 10:37

replay changed the title ~~[WIP] (do not merge yet) Tag index~~ Tag index Oct 1, 2017

replay requested review from Dieterbe and woodsaj October 1, 2017 10:38

replay force-pushed the tag_index branch 3 times, most recently from 7402db6 to a4ee0e2 Compare October 1, 2017 10:45

Dieterbe suggested changes Oct 1, 2017

View reviewed changes

Dieterbe reviewed Oct 1, 2017

View reviewed changes

replay force-pushed the tag_index branch from 5cbf900 to aa3749f Compare October 1, 2017 14:23

Dieterbe reviewed Oct 1, 2017

View reviewed changes

replay added 13 commits October 11, 2017 10:01

rename IdsByTagExpression to FindByTag

0adb706

dont export IdsByTagQuery

b5b54bf

build resultset build preallocating slice of strings

5b4811e

add tag support check

3fcbf64

reorganize expression parsing

1e2fe06

use goish error in return from parseExpression()

92a850e

do not check length of value twice

e38b582

do error checking in NewTagQuery, not when running query

54ee08d

compile patterns in NewTagQuery()

ecaa3fb

minor refactor

6316319

comment

d5322eb

go through permutations of query in tag query benchmark

04769a7

sort all expressions by cost

862144c

Dieterbe force-pushed the tag_index branch from 008d98b to 862144c Compare October 12, 2017 22:11

Dieterbe reviewed Oct 12, 2017

View reviewed changes

Dieterbe and others added 7 commits October 12, 2017 19:23

simplify

4ac71dd

fix

4a47d8a

only cache regex matching of value and nothing more

34f5323

help messages

21165db

example configs

78a31d2

simplify filterByMatch

ae9e5d7

no need to loop twice no need to do map lookups if key doesn't even match only need to track values, not entire k=v pair

shanson7 reviewed Oct 13, 2017

View reviewed changes

replay added 2 commits October 13, 2017 18:48

update schema

84f8128

minor optimization to not unnecessarily loop in tag expression evalua…

6fb73bf

…tion

Dieterbe approved these changes Oct 13, 2017

View reviewed changes

Dieterbe merged commit fa147d6 into master Oct 13, 2017

shanson7 mentioned this pull request Feb 8, 2018

Use strings as keys until both non-string ids are used everywhere bloomberg/metrictank#19

Merged

Dieterbe deleted the tag_index branch September 18, 2018 09:09

		@@ -328,11 +318,19 @@ func (q TagQuery) filterByMatch(resultSet TagIDs, byId map[string]idx.Archive,
		continue
		}

Conversation

replay commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

replay Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanCech Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

replay Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

replay Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

replay Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

replay Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

replay Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dieterbe commented Sep 30, 2017

Uh oh!

Dieterbe Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dieterbe Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

replay Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dieterbe Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dieterbe commented Oct 1, 2017

Uh oh!

replay commented Sep 14, 2017 •

edited

Loading

replay Sep 14, 2017 •

edited

Loading

DanCech Sep 14, 2017 •

edited

Loading

replay Sep 15, 2017 •

edited

Loading

replay Sep 19, 2017 •

edited

Loading

replay Sep 19, 2017 •

edited

Loading

replay Sep 19, 2017 •

edited

Loading

replay Sep 21, 2017 •

edited

Loading

Dieterbe Oct 1, 2017 •

edited

Loading

Dieterbe Oct 1, 2017 •

edited

Loading

replay Oct 1, 2017 •

edited

Loading

Dieterbe Oct 1, 2017 •

edited

Loading

replay Oct 13, 2017 •

edited

Loading