Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Router optimise doesn't work with regex grouping. #184

Closed
iain-buclaw-sociomantic opened this issue Jun 9, 2016 · 3 comments
Closed

Router optimise doesn't work with regex grouping. #184

iain-buclaw-sociomantic opened this issue Jun 9, 2016 · 3 comments

Comments

@iain-buclaw-sociomantic
Copy link
Contributor

Now that the number of rules has grown to a big enough size to trigger router_optimise to walk through all patterns. I've noticed that some aggregations have stopped, where a rule like so exists.

aggregate
    (foo|bar)\.(.+)
  # ...
  ;

What happens is that a bar group is created, and all rules that match bar go into it. But when a metric foo comes in, it never hits the rule.

@grobian
Copy link
Owner

grobian commented Jun 9, 2016

Ok, it's a matter of removing the optimiser code, or making it deal with simple expressions only. (I guess its heuristics still work out well for a pattern like '.*foo.*bar.*')

@iain-buclaw-sociomantic
Copy link
Contributor Author

iain-buclaw-sociomantic commented Jun 9, 2016

Yes. Although that's not to say that being aware of regex groups is not a bad thing, where in the above example, two groups are created with the same rules inside.

However for simplicity, something like the following works well enough.

/* strip off chars that won't belong to a block */
int ingroup = 0;
while (
                p > rwalk->pattern &&
                ingroup > 0 ||
                (*p < 'a' || *p > 'z') &&
                (*p < 'A' || *p > 'Z') &&
                *p != '_'
          ) {
        if (*p == ')')
                ingroup++;
        else if (*p == '(' && ingroup > 0)
                ingroup--;
        p--;
}

So a pattern like foo\.(bar|baz) will go inside a group for foo

@grobian
Copy link
Owner

grobian commented Jun 9, 2016

That works too. The code was originally setup to be very rule-of-thumb-like (WorkingForUs(tm)) and quite naive, but worked wonders back then. Perhaps skipping the complex part would be smartest thing to do here indeed.

grobian added a commit that referenced this issue Jun 16, 2016
If we match strings inside (conditional) groups, we're possibly exluding
matches.  This could use some refinement, such that (foo) or (foo)? is
no problem.  This commit makes sure that (bar|foo) doesn't result in a
group for foo.
@grobian grobian closed this as completed Jun 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants