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

Backtracking causes wrong mapping of labels #168

Closed
pnyheim opened this issue Dec 5, 2018 · 5 comments · Fixed by #169
Closed

Backtracking causes wrong mapping of labels #168

pnyheim opened this issue Dec 5, 2018 · 5 comments · Fixed by #169
Labels

Comments

@pnyheim
Copy link

pnyheim commented Dec 5, 2018

When I have the following mappings defined.

---
mappings:
- match: '*.dummy.*'
  name: metric_one
  labels:
    system: $1
    attribute: $2
- match: 'full.name.*'
  name: metric_two
  labels:
    system: static
    attribute: $1

And then send in the following 2 metrics

whatever.dummy.test
full.name.anothertest

I expect the following result

metric_one{attribute="test",system="whatever"}
metric_two{attribute="anothertest",system="static"} 

But instead I get

metric_one{attribute="test",system="whatever"}
metric_two{attribute="full",system="static"} 

It's as if after backtracking, it remembers that $1 mached the first part, full - but when going down the second path, the first part is not a *, and should not match it - but rather match on the 3rd part anothertest - which is now dropped entirely from the result.

I was able to verify this in the mapper_test.go by adding the following scenario.

		//Config with backtracking, starting star
		{
			config: `
defaults:
  glob_disable_ordering: false
mappings:
- match: '*.dummy.*'
  name: metric_one
  labels:
    system: $1
    attribute: $2
- match: 'full.name.*'
  name: metric_two
  labels:
    system: static
    attribute: $1
  `,
			mappings: mappings{
				"whatever.dummy.test": {
					name: "metric_one",
					labels: map[string]string{
						"system": "whatever",
                        "attribute": "test",
					},
				},
				"full.name.anothertest": {
					name: "metric_two",
					labels: map[string]string{
						"system": "static",
                        "attribute": "anothertest",
					},
				},
			},
		},

Also, I think there is something wrong with the scenario starting with the comment

//Config with backtracking

Where the label of the second metric should either have produced an error or be the literal ${1}_foo because there is no *-parts to the metric.

A workaround for me is to set glob_disable_ordering to true, but I'm not sure if that has any other implications on our mappings - as we have quite a lot of them.

@matthiasr
Copy link
Contributor

@fffonion could you take a look at this?

@matthiasr matthiasr added the bug label Dec 5, 2018
@fffonion
Copy link
Contributor

fffonion commented Dec 5, 2018

Thanks for reporting this. Looking.

@fffonion
Copy link
Contributor

fffonion commented Dec 5, 2018

@matthiasr I've created a PR addressing this bug. Could you take a look at #169 ?

@pnyheim The test case in //Config with backtracking is correct because backtrack.justatest.bbb will match the first rule, which has a capture. To make this more clear I also added another metric backtrack.justatest.aaa that will match the second rule and produced the effect you're expecting.
I also added your sample test case with this bug to mapping_test.

@pnyheim
Copy link
Author

pnyheim commented Dec 5, 2018

@fffonion Thanks for the quick fix.
And yes - I see now that I had been too quick in my reasoning about the //Config with backtracking testcase. But the addition makes it a lot clearer. Excellent work.

@matthiasr The thing is, we actually use graphite_exporter, so what is the process for getting this fix in there once it makes it into statsd_exporter? Do I create an Issue over there?

@matthiasr
Copy link
Contributor

No need to open an issue, I'll update the vendoring and make a release for both.

matthiasr pushed a commit to prometheus/graphite_exporter that referenced this issue Dec 5, 2018
this brings in prometheus/statsd_exporter#169 which fixes
prometheus/statsd_exporter#168: clobbering of captures when
unsuccessfully backtracking

Signed-off-by: Matthias Rampke <[email protected]>
matthiasr pushed a commit to prometheus/graphite_exporter that referenced this issue Dec 5, 2018
this brings in prometheus/statsd_exporter#169 which fixes
prometheus/statsd_exporter#168: clobbering of captures when
unsuccessfully backtracking

Signed-off-by: Matthias Rampke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants