Remove last remaining use of LabelNameRE #66

beorn7 · 2016-11-11T15:51:09Z

The IsValid method is much faster.

Same as MetricNameRE, the regexp is mostly there for documentation
reasons.

This commit also clarifies that in the doc comments and encourages
using the IsValid method (or IsValidMatricName function) for
performance reasons.

@grobie @stuartnelson3

The IsValid method is much faster. Same as MetricNameRE, the regexp is mostly there for documentation reasons. This commit also clarifies that in the doc comments and encourages using the IsValid method (or IsValidMatricName function) for performance reasons.

beorn7 · 2016-11-11T15:57:25Z

Also added test for colons in metric names and label names, as that triggered this whole revisit..

brian-brazil · 2016-11-11T15:56:00Z

model/labels.go

 var LabelNameRE = regexp.MustCompile("^[a-zA-Z_][a-zA-Z0-9_]*$")

 // A LabelName is a key for a LabelSet or Metric.  It has a value associated
 // therewith.
 type LabelName string

-// IsValid is true iff the label name matches the pattern of LabelNameRE.
+// IsValid is true iff the label name matches the pattern of LabelNameRE. ;This


The ; is meant to be a newline I guess?

Just spurious. Removed the ';'.
(Newline doesn't really help here, would be ignored by godoc anyway.)

brian-brazil · 2016-11-11T15:56:26Z

model/metric.go

+	separator = []byte{0}
+	// MetricNameRE is a regular expression matching valid metric
+	// names. Note that the IsValidMetricName function performs the same
+	// check but faster than a metch with this regular expression.


brian-brazil · 2016-11-11T15:57:05Z

model/metric_test.go

+		{
+			mn:    "colon:in:the:middle",
+			valid: true,
+		},
 	}

 	for _, s := range scenarios {


Should we also check the regex here?

+1 for testing both implementations in tests.

All was a mess, we had duplicates of the REs for label name and metric names here, and we sometimes used them, sometimes we used those from common/model. Now we are consistently using the fast checking functions from common/model. (Tests for leading colons are included there, see prometheus/common#66 .)

grobie · 2016-11-11T17:07:30Z

model/labels.go

@@ -80,14 +80,18 @@ const (
 	QuantileLabel = "quantile"
 )

-// LabelNameRE is a regular expression matching valid label names.
+// LabelNameRE is a regular expression matching valid label names. Note that the
+// IsValid method of LabelName performs the same check but faster than a metch


grobie · 2016-11-11T17:08:54Z

model/metric_test.go

+		{
+			mn:    "colon:in:the:middle",
+			valid: true,
+		},
 	}

 	for _, s := range scenarios {


+1 for testing both implementations in tests.

beorn7 · 2016-11-12T14:34:32Z

Added tests for the regexps, too.

stuartnelson3 · 2016-11-14T08:57:51Z

model/labels.go

 var LabelNameRE = regexp.MustCompile("^[a-zA-Z_][a-zA-Z0-9_]*$")

 // A LabelName is a key for a LabelSet or Metric.  It has a value associated
 // therewith.
 type LabelName string

-// IsValid is true iff the label name matches the pattern of LabelNameRE.
+// IsValid is true iff the label name matches the pattern of LabelNameRE. This


I'm assuming iff is for "if and only if", but I personally find it unexpected when scanning programming documentation and jumps out to me as a typo. I would consider writing out the whole statement for clarity, but if this is an established pattern that's fine by me.

Agreed, in one of my first PRs I tried fixing it 😄

iff is pretty standard.

I went through the same evolution of "typo" → "pretty standard". I always blamed myself not being a native speaker. Now Stuart fell into the typo trap, too. But I concur that it is used quite commonly. Once you start looking for it, it's everywhere.

stuartnelson3

Looks good to me. Since you mention several times that the isValid implementation is faster than a regexp match I'm guessing you tested this, in which case either posting the benchmark here for posterity or including it in the PR might be nice.

beorn7 · 2016-11-14T13:47:35Z

The hand-coded functions have been introduced ages ago, with benchmarks and everything. This PR does not touch any of this, just removes left-over uses of the regexp. No need to justify the function again.

03cc598 Don't lint generated protobuf code. 2b55c2d Merge pull request prometheus#66 from weaveworks/reduce-test-timeout d4e163c Make timeout a flag 49a8609 Reduce test timeout 8fa15cb Merge pull request prometheus#63 from weaveworks/test-defaults b783528 Tweak test script so it can be run on a mca a3b18bf Merge pull request prometheus#65 from weaveworks/fix-integration-tests ecb5602 Fix integration tests f9dcbf6 ... without tab (clearly not my day) a6215c3 Add break I forgot 0e6832d Remove incorrectly added tab eb26c68 Merge pull request prometheus#64 from weaveworks/remove-test-package-linting f088e83 Review feedback 2c6e83e Remove test package linting 2b3a1bb Merge pull request prometheus#62 from weaveworks/revert-61-test-defaults 8c3883a Revert "Make no-go-get the default, and don't assume -tags netgo" e75c226 Fix bug in GC of firewall rules. e49754e Merge pull request prometheus#51 from weaveworks/gc-firewall-rules 191f487 Add flag to enale/disable firewall rules' GC. 567905c Add GC of firewall rules for weave-net-tests to scheduler. 03119e1 Fix typo in GC of firewall rules. bbe3844 Fix regular expression for firewall rules. c5c23ce Pre-change refactoring: splitted gc_project function into smaller methods for better readability. ed5529f GC firewall rules ed8e757 Merge pull request prometheus#61 from weaveworks/test-defaults 57856e6 Merge pull request prometheus#56 from weaveworks/remove-wcloud dd5f3e6 Add -p flag to test, run test in parallel 62f6f94 Make no-go-get the default, and don't assume -tags netgo 8946588 Merge pull request prometheus#60 from weaveworks/2647-gc-weave-net-tests 4085df9 Scheduler now also garbage-collects VMs from weave-net-tests. 4b7d5c6 Merge pull request prometheus#59 from weaveworks/57-fix-lint-properly b7f0e69 Merge pull request prometheus#58 from weaveworks/fix-lint 794702c Pin version of shfmt ab1b11d Fix lint d1a5e46 Remove wcloud cli tool 81d80f3 Merge pull request prometheus#55 from weaveworks/lint-tf 05ad5f2 Review feedback 4c0d046 Use hclfmt to lint terraform. git-subtree-dir: tools git-subtree-split: 03cc5989769d93aa03f8aed3784ddfdb1fffc1c6

All was a mess, we had duplicates of the REs for label name and metric names here, and we sometimes used them, sometimes we used those from common/model. Now we are consistently using the fast checking functions from common/model. (Tests for leading colons are included there, see prometheus/common#66 .)

beorn7 added 3 commits November 11, 2016 16:47

Add tests for colons in metric names

52fe451

Add tests for colons in label names

28397f8

brian-brazil reviewed Nov 11, 2016

View reviewed changes

beorn7 mentioned this pull request Nov 11, 2016

Remove local REs for label and metric names and use fast checks prometheus/client_golang#256

Merged

grobie reviewed Nov 11, 2016

View reviewed changes

Added tests for metric and label name regexp

567b4bb

stuartnelson3 reviewed Nov 14, 2016

View reviewed changes

stuartnelson3 approved these changes Nov 14, 2016

View reviewed changes

beorn7 merged commit 0d5de9d into master Nov 14, 2016

beorn7 deleted the beorn7/tweaks branch November 14, 2016 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove last remaining use of LabelNameRE #66

Remove last remaining use of LabelNameRE #66

beorn7 commented Nov 11, 2016

beorn7 commented Nov 11, 2016

brian-brazil Nov 11, 2016

beorn7 Nov 12, 2016

brian-brazil Nov 11, 2016

beorn7 Nov 12, 2016

brian-brazil Nov 11, 2016

grobie Nov 11, 2016

grobie Nov 11, 2016

beorn7 Nov 12, 2016

grobie Nov 11, 2016

beorn7 commented Nov 12, 2016

stuartnelson3 Nov 14, 2016

brancz Nov 14, 2016

brian-brazil Nov 14, 2016

beorn7 Nov 14, 2016

stuartnelson3 left a comment

beorn7 commented Nov 14, 2016

Remove last remaining use of LabelNameRE #66

Remove last remaining use of LabelNameRE #66

Conversation

beorn7 commented Nov 11, 2016

beorn7 commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Nov 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartnelson3 left a comment

Choose a reason for hiding this comment

beorn7 commented Nov 14, 2016