This repository has been archived by the owner on Aug 23, 2023. It is now read-only.
Implement series lookup and filtering by meta tag #1423
Merged
Conversation
This file contains 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
replay
force-pushed
the
implement_series_lookup_by_meta_tag
branch
2 times, most recently
from
f16544f
to
036518a
Compare
|
rebased onto master |
|
This illustrates how all the different parts interact with each other in a |
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 |
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.
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.
replay
force-pushed
the
implement_series_lookup_by_meta_tag
branch
from
978287a
to
ea822b5
Compare
|
rebased onto master |
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).