This repository was archived by the owner on Aug 23, 2023. It is now read-only.
Implement series lookup and filtering by meta tag#1423
Merged
Conversation
f16544f to
036518a
Compare
Contributor
Author
|
rebased onto master |
Contributor
Author
Contributor
robert-milan
left a comment
There was a problem hiding this comment.
Other than some minor variable naming changes it looks pretty solid. Let's discuss it later today.
idx/memory/tag_query.go
Outdated
|
|
||
| index TagIndex // the tag index, hierarchy of tags & values, set by Run()/RunGetTags() | ||
| byId map[schema.MKey]*idx.Archive // the metric index by ID, set by Run()/RunGetTags() | ||
| mti metaTagIndex // the meta tag index |
Contributor
There was a problem hiding this comment.
Suggested change
| mti metaTagIndex // the meta tag index | |
| metaIndex metaTagIndex // the meta tag index |
There are a few other places this appears where it should be changed as well.
Contributor
Author
There was a problem hiding this comment.
i'll make the naming more consistent.
many properties are called "queries", but in the context of all other packages tag deal with tag query expressions those properties are usually called "expressions", so this fixes that inconsistency
this makes sense for 2 reasons: 1) tagquery.Expression is an interface type, which the json marshaller can't marshall properly automatically 2) when calling /metaTags, the format of the returned meta tags now matches the format that's expected when upserting meta tag records via /metaTags/upsert
this doesn't actually change the tag query evaluation logic, but it should make the code much easier to understand for whoever needs to read it. it also makes it easier to write unit tests for this part of the code
- make id selector a member of tag query context - avoid unnecessarily passing index reference around
this doesn't change the evaluation logic, but it should make the code more readabale and more testable
because I think that way it's easier to understand what this is supposed to be
also deduplicates results now when selecting by meta tag
when receiving a new meta tag record upsert request we shouldn't only validate the given query expressions, but also that the given set of expressions can be used to instantiate a query. if that's not the case, then the given meta tag record should be considered invalid and should get rejected.
978287a to
ea822b5
Compare
Contributor
Author
|
rebased onto master |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

This PR implements the step 3 as described here: #660 (comment)
It is based on the branch of this PR:
#1373
I already commented a lot and added many unit tests, so I hope that helps in making it more understandable. I'll post a flow chart tomorrow, trying to illustrate how the different parts interact with each other.
This implements everything that's required to query by meta tags (or querying by mixed combinations of meta- and metric tags).