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

desc: Allow initial colon in metric name #255

Closed
wants to merge 1 commit into from

Conversation

kamalmarhubi added a commit to kamalmarhubi/rust-prometheus that referenced this pull request Nov 6, 2016
This allows dropping the dependency on regex, making the instrumentation
library much more lightweight.

This also fixes a bug with metric name validation, where an initial
colon was incorrectly disallowed. See data model for correct format:
  https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels

This may be because the library closely follows the official Go library,
which also had this issue.

Refs tikv#85
Refs prometheus/client_golang#255
@brian-brazil
Copy link
Contributor

Can you add a unittest please?

Note that direct instrumentation should never have colons in it, and having a colon first is not in line with any naming practice.

@beorn7
Copy link
Member

beorn7 commented Nov 6, 2016

There is something fishy here.
https://github.com/prometheus/common/blob/master/model/metric.go#L25 does not allow leading colons.
In https://github.com/prometheus/common/blob/master/model/metric.go#L93, which is the validation we actually use, it is allowed.

Before we patch all inconsistent places into allowing leading colons, can we first clarify what the original intention was and then fix everything the right way?

@brian-brazil
Copy link
Contributor

My expectation is that colons are allowed anywhere in a metric name, it's only digits that we restrict to not be first for parsing reasons.

I wouldn't be against prohibiting colons completely in direct instrumentation, the rare cases where it's okay to have them would very likely come from custom collectors.

@kamalmarhubi
Copy link
Author

@brian-brazil

Note that direct instrumentation should never have colons in it, and having a colon first is not in line with any naming practice.

I wouldn't be against prohibiting colons completely in direct instrumentation, the rare cases where it's okay to have them would very likely come from custom collectors.

This should probably be reflected in the docs. Since I'm not super acquainted, could you or @beorn7 add something there to clarify and maybe give an example where colons are used?

If it's all right, I'll hold off the unit test / getting this merged till after that.

@beorn7
Copy link
Member

beorn7 commented Nov 8, 2016

OK, this all sounds like the following course of action:

  • Allow colon as the 1st character (i.e. declare the current docs and the actual behavior as correct).
  • Clarify in the docs when to use colons.
  • Fix the MetricNameRE to reflect reality.

I'll create a PR for the latter. @brian-brazil could you look into the doc clarification? @kamalmarhubi feel free to add unit tests to this here to fix client_golang. For other clients, respective maintainers should check the behavior (cc @grobie for Ruby).

@beorn7
Copy link
Member

beorn7 commented Nov 8, 2016

See prometheus/common#65

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

var (
metricNameRE = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_:]*$`)
metricNameRE = regexp.MustCompile(`^[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.

We should just use https://github.com/prometheus/common/blob/master/model/metric.go#L25 , now that it is fixed (i.e. don't set a variable here but just use model.MetricNameRE below).

Same for next line, use model.LabelNameRE, now that we are on it.

@beorn7
Copy link
Member

beorn7 commented Nov 11, 2016

To get things moving here, I'll create a new PR on my own. Closing this in lieu.

@beorn7 beorn7 closed this Nov 11, 2016
@kamalmarhubi
Copy link
Author

@beorn7 apologies for the non-response, and thanks for picking it up!

@kamalmarhubi kamalmarhubi deleted the patch-1 branch November 11, 2016 16:01
@beorn7
Copy link
Member

beorn7 commented Nov 11, 2016

See #256
Thanks for spotting the issue in the first place.

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.

None yet

3 participants