fix(prometheus) Metric name should not be set twice #1330
Conversation
amolnayak311
requested review from
vishramachandran,
tjackpaul,
sherali42 and
alextheimer
| parseError("foo{__name__=\"foo\"}") | ||
| parseError("foo{_metric_=\"bar\"}") | ||
| parseError("foo{_metric_=\"foo\"}") |
There was a problem hiding this comment.
Does something like {_metric_="foo"} still parse successfully? I'm not 100% sure if metricName accounts for both foo{~} and {_metric_="foo"}
There was a problem hiding this comment.
I'll add tests for those too but yes it parses. I'm not sure is {_metric_="foo"} is equivalent to foo{~} as _metric_ is more internal to FiloDB
There was a problem hiding this comment.
Pushed more tests
There was a problem hiding this comment.
What do you think should happen for {__name__="foo", _metric_="foo"} ? This is expected to fail?
There was a problem hiding this comment.
Ack, lgtm. Good point about _metric_-- I'm also not sure.
There was a problem hiding this comment.
@vishramachandran @tjackpaul @sherali42 Can you please share your view on this?
There was a problem hiding this comment.
{__name__="foo", _metric_="foo"} should fail.
Pull Request checklist
Current behavior : Following prom expressions are valid
foo{_metric_="foo"}butfoo{__name__="foo"}will fail to parseNew behavior :
both
foo{_metric_="foo"}andfoo{__name__="foo"}will fail to parse