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

allow metrics with dashes when mapping #24

Merged
merged 1 commit into from
Oct 12, 2015

Conversation

macb
Copy link
Contributor

@macb macb commented Oct 8, 2015

It wasn't entirely clear why this wasn't allowed. We have some systems that submit metrics with their hostname in the metric. Something in the form of test-myhost01. This allows the mapper to match those.

Actually doesn't fully meet the use case where the name may be test-my-host01 or something. Still working.

@@ -24,7 +24,7 @@ import (
)

var (
identifierRE = `[a-zA-Z_][a-zA-Z0-9_]+`
identifierRE = `[a-zA-Z_]+-?[a-zA-Z0-9_]+`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

identifierRE is used not only in the regex that matches statsd metric lines, but also for the other two ones labelLineRE and metricNameRE. Those are for matching Prometheus label names and Prometheus metric names, both of which may not contain dashes.

If you want to allow dashes in statsd metric names, you'll have to isolate that change to just the regex that matches those.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

@macb macb force-pushed the allow_hyphen_hostname branch from 19a7f72 to c52befb Compare October 9, 2015 22:03
@macb
Copy link
Contributor Author

macb commented Oct 9, 2015

ok @juliusv, teased apart the statsd metric line so that it could be changed independent of the labels regex.

I also added a test case for label names with - to ensure they are still marked as bad.

@macb macb force-pushed the allow_hyphen_hostname branch from c52befb to 411b071 Compare October 9, 2015 23:38
@juliusv
Copy link
Member

juliusv commented Oct 12, 2015

@macb Cool, thanks!

juliusv added a commit that referenced this pull request Oct 12, 2015
allow metrics with dashes when mapping
@juliusv juliusv merged commit 28d4020 into prometheus:master Oct 12, 2015
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

Successfully merging this pull request may close these issues.

2 participants