This repository was archived by the owner on Aug 23, 2023. It is now read-only.
Move tag query evaluation logic into tag expressions #1373
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
changed the title
replay
force-pushed
the
move_evaluation_logic_into_tag_expressions
branch
12 times, most recently
from
3e06b88 to
ea77afc
Compare
replay
force-pushed
the
move_evaluation_logic_into_tag_expressions
branch
6 times, most recently
from
1ccbefd to
cda3515
Compare
replay
changed the title
replay
force-pushed
the
move_evaluation_logic_into_tag_expressions
branch
3 times, most recently
from
2eacdba to
13609e1
Compare
|
Just FYI, those are the current benchmark results compared between master and this branch: Most of them are looking good, but two show a significant slow-down. I'll investigate where that slow down comes from and try to fix it. |
replay
force-pushed
the
move_evaluation_logic_into_tag_expressions
branch
from
13609e1 to
3738209
Compare
|
The good news is that I now know why some benchmarks are performing worse, the bad news is that I have no good idea how to improve that.
The difference in these benchmarks comes from the additional looping over the |
|
I think to solve this above described issue with the performance regression I'll add another option similar to |
|
With the latest 3 commits the performance has gotten better, but not as good as master yet: |
This should be really fast. Do we do more than merely iterate and check for equality ? Any parsing or more logic going on ? |
|
@Dieterbe no, it just used to compare the strings for equality, while now it is doing a lookup by id from a map: 39e35e2#diff-cb68716cfded2a2336a3778f548d7a17R41 |
this allows us to minimize the number of ids that need to get filtered by the filters, while at the same time still taking the execution cost of each filter into account. f.e. if we have the choice between first calling a filter which uses regex or first calling a filter that doesn't use regex, we'd first want to call the one that's not using regex and let it reduce the size of the potential result set, that way the regex-using filter would later only get applied on a smaller set of potential results.
also fix some confusing terminology in variable names
this makes HAS_TAG correctly indicate that its value matches exactly, and it also uses the optimized lookup when doing the lookup of the initial ID set in getInitialByTag()
this renames some methods to make it clearer what they do. it also adds another explanator comment.
this changes the sortByCost logic so it puts more weight on the expression type, and only takes cardinality into account when it has to sort two expressions of the same operator cost. in the benchmarks this seems to lead to better results.
replay
force-pushed
the
move_evaluation_logic_into_tag_expressions
branch
from
fe39ea0 to
142a893
Compare
robert-milan
reviewed
Aug 8, 2019
idx/memory/memory.go
Outdated
| @@ -84,6 +82,8 @@ func ConfigSetup() { | |||
| memoryIdx.DurationVar(&findCacheBackoffTime, "find-cache-backoff-time", time.Minute, "amount of time to disable the findCache when the invalidate queue fills up.") | |||
| memoryIdx.StringVar(&indexRulesFile, "rules-file", "/etc/metrictank/index-rules.conf", "path to index-rules.conf file") | |||
| memoryIdx.StringVar(&maxPruneLockTimeStr, "max-prune-lock-time", "100ms", "Maximum duration each second a prune job can lock the index.") | |||
| memoryIdx.IntVar(&tagquery.MatchCacheSize, "match-cache-size", 1000, "size of regular expression cache in tag query evaluation") | |||
There was a problem hiding this comment.
I don't think we should be loading variables into another package from here. Can we move the variable initialization code into tagquery? Same would apply to tagquery.MetaTagSupport.
There was a problem hiding this comment.
actually that's what i've done at first, then @Dieterbe asked me to keep the settings in the memory-idx (which i think makes sense). It's just unfortunate that now we have to set this setting across packages
#1373 (comment)
There was a problem hiding this comment.
Is there something else we can do here then? Like keep them completely contained within the index and then after they are loaded initialize them into tagquery with its own variables to hold them? Although then if we ever want to change them at runtime it would be a bit trickier.
It just feels like we are creating a lot of codependency here.
There was a problem hiding this comment.
Unfortunately the package tagquery can't import the package memory, this would result in a cycle. We could initialize a setting in memory, and then in ConfigProcess() just copy that value into tagquery.MetaTagSupport / tagquery.MatchCacheSize. Then both of these two packages would hold a copy of the setting.
There was a problem hiding this comment.
Yeah something like that.
Co-Authored-By: Robert Milan <42070645+robert-milan@users.noreply.github.com>
replay
force-pushed
the
move_evaluation_logic_into_tag_expressions
branch
from
345ffae to
e565d55
Compare
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.
Moves the tag query expression evaluation logic into the expression types. This provides us more flexibility to combine expressions from meta records and tag queries when we have to take both indexes into account.
One notable change to the logic is that MetricDefinition filters now assume that the tag index they're currently looking at isn't necessarily the only tag index.
F.e. if an expression says
tag1!=abcand we see thatmetric1does not havetag1defined via its metric (intrinsic) tags, then we can't be sure yet whether another index (f.e. meta tag index) does assigntag1=abctometric1. So in this case the filter would return aNonedecision, which means that then other index(es) would need to get checked for whether this expression is satisfied or not.On the other hand if the metric (intrinsic) tags would assign
tag1=abctometric1, then it would directly return a conclusive decisionFailand sub-subsequent index(es) can be ignored. If the metric (intrinsic) tags would assigntag1=otherValuetometric1, then it would directly return a conclusive decisionPassbecause the indexes are checked in order of their priority of conflict resolution.Currently the lookups in the meta tag index are not implemented yet, this will be coming in the next PR.
The current status of this PR is that it works fine in my manual tests and the unit tests pass. I'm going to add more benchmarks and potentially add some performance improvements, but I'm not expecting any major changes.
This implements step
2)of this list: #660 (comment)UPDATE, trying to explain the above stuff a bit better:
Filter Functions (Matchers):
The given query expressions, once parsed into the types implementing
tagquery.Expression, now provide a method calledGetMetricDefinitionFilter(). This method returns a filter function (in other contexts this is also called a "matcher") which can be passed a MetricDefinition and it will return a decision which is eitherPass,FailorNone(inconclusive). This allows us to build a chain of filter functions, first from the given expressions, but also from the query expressions associated with meta records which match the given expressions.So let's say we have an expression
tag=~val.*and want to apply it as a filter (not to build the initial result set). Once it's parsed into the accordingtagquery.Expressionwe callGetMetricDefinitionFilter()inTagQueryContext.prepareExpressions()to obtain a filter function which we'll use to filter down the result set.Here comes the important part ->
Additionally, once we want to use the meta tag index, we can use the same
tagquery.Expressioninstance to lookup meta tag records (tagquery.MetaTagRecord) which match the expressiontag=~val.*from the meta tag index (UnpartitionedMemoryIdx.metaTags). When we do that lookup from the meta tag index we use thetagquery.Expressioninstance not as a filter, but to build a set of meta tag records which match the expressiontag=~val.*. We do that via its methodsGetKey()andValuePasses()(because the=~operator matches against the tag value of a key).The resulting meta tag records come with one or multiple sets of query expressions (
MetaTagRecord.Expressions). If any of these sets of query expressions match aMetricDefinitionthis means that the meta tags associated with the meta tag record get assigned to that Metric and we already know that one of the meta tags matches the expressiontag=~val.*, which means that the expressiontag=~val.*matches this metric (via the meta tag that gets assigned to it).Concretely, to implement this,
prepareExpressions()will lookup those meta tag records as described and build a set of filter chains from them (because there are multiple sets of expressions associated with each meta tag record, and one expression can result in multiple meta tag records). These filter chains can then be used bytestByAllExpressions(). If a MetricDefinition passes any one of the meta tag record based filter chains then it should be part of the result set (so there's a logical OR there).There will probably be some tuning regarding which filter chain is cheaper to evaluate and which is more expensive, we should try to filter the result set down using the cheap filters first before applying the expensive ones, that's why
tagquery.Expressionhas aGetCostMultiplier()method, to estimate cost of evaluation.Filter Decisions:
The metric definition filters (
tagquery.MetricDefinitionFilter) return a filter decision (tagquery.FilterDecision) which can be one ofFail,PassorNone. TheFailandPassvalue simply mean that a filter has been able to conclusively decide that a givenMetricDefinitionhas either passed the filter or has been disqualified by it. TheNonecase is necessary because in certain cases a filter cannot make this decision without also looking at the other indexes that need to be taken into account. Let's take the exampletag!=value:Once we use the meta tag index, the function
testByAllExpressions()will first apply a filter function that's been generated from the expressiontag!=valueto eachMetricDefinitionof the result set. If aMetricDefinitioncomes with a tagtag=value, the filter can directly returnFail. If theMetricDefinitioncomes with a tagtag=otherValue, the filter can directly returnPass. But if theMetricDefinitiondoes not have the tagtagdefined at all, then this filter can't make a final decision because it is possible that one of the meta tag records assigns the tagtag=valueto the givenMetricDefinition, so it returnsNone. In the case ofNone,testByAllExpressions()will have to run the givenMetricDefinitionthrough the filter chains that have been generated from the meta tag records assigning the tagtag=value(as described above). If one of them matches the givenMetricDefinition, then it can be disqualified at that point and we can interrupt the filter chains. If none of them match the givenMetricDefinition, thetagquery.Expressionobject'sGetDefaultDecision()is used to make the final decision whether theMetricDefinitionmatches this expression or not. In the case of the typetagquery.expressionNotEqual(representing!=) the return value ofGetDefaultDecision()would bePass. In the case of other expression types, like for exampletagquery.expressionEqual(=) the return value ofGetDefaultDecision()would beFail. Because if a tag does not get assigned to a metric and we're evaluating a=expression, then the decision should beFail. If a tag does not get assigned to a metric and we're evaluating a!=expression, then the result should bePass.